New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 718462 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression

Blocking:
issue 770742



Sign in to add a comment

"Enable Sync" is displayed in Other Devices on every cold start.

Project Member Reported by srikanthg@chromium.org, May 4 2017

Issue description

App Version: 60.0.3089.0 canary
iOS Version: 10.3.2, 10.1.1
Device: iPhone, iPad
URL: na

Steps to reproduce:
1. Launch Google Chrome
2. Sing in to Chrome
3. Tap Menu → Recent Tabs
4. Background the app
5. Force quit the by swiping up from App Switcher
6. Launch the app

Observed results: "Enable Sync" button is displayed in Other Devices.

Expected results: Loading Status should be displayed in 'Other Devices' while sync is in progress.

Good Version 58.0.3029.66 #f0ce1a9
Bad Version: 58.0.3029.67 #2fe3f21

Number of times you were able to reproduce: 5/5
Bug reproducible after clean install: Yes
Bug reproducible after clearing cache and cookies: Yes
Bug reproducible on Chrome Mobile on Android: NA
Bug reproducible on Dolphin/Safari/Atomic: Dolphin: NA, Safari: NA
Bug reproducible on current stable build (App Version, iOS Version): M58 Yes, Working fine in M57. 
Bug reproducible on the current beta channel build (App Version, iOS Version): M59 Yes

Link to video/image: https://drive.google.com/file/d/0B-xmXLQhjeKuZW43QzdHVnlVSFk/view 
 
Labels: -Restrict-View-Google
Labels: M-60
Owner: jif@chromium.org
Status: Assigned (was: Untriaged)
Any updates on this older regression?
Components: -UI>Browser>NewTabPage UI>Browser>History
Ping ? Jif@ who should be assigned to this? 

Punctuation matters.
Jif@, who should be assigned to this?  :)
Cc: noyau@chromium.org
Labels: -Pri-2 Pri-1
Hi Jif@: Could you please assign this to someone who would be best suited to work on it ? Thanks.

Comment 8 by jif@chromium.org, Oct 10 2017

Owner: gambard@chromium.org
Cc: linds...@chromium.org pinkerton@chromium.org
Hmm. Gauthier seems out until next Monday. Is there anyone else who can work on this? 
Blocking: 770742

Comment 11 by pkl@chromium.org, Oct 10 2017

This was introduced in M58. The user experience is very poor. Add M62 milestone for evaluation in the event of a refresh release. But for sure we should work on this for M63.

Comment 12 by pkl@chromium.org, Oct 10 2017

Cc: gch...@chromium.org
Can you take a look since gambard is out till next week?

Comment 13 by noyau@chromium.org, Oct 10 2017

Owner: olivierrobin@chromium.org
Cc: ramyasharma@chromium.org
May be related to this CL, which touched iOS Sync in the revision range: https://codereview.chromium.org/2790483002/
The CL above changed the determination of whether a Sync data type is enabled. Before, preferred types were considered enabled, but now, only active types are. The data type for recent tabs (syncer::PROXY_TABS) is considered preferred, but not active (until the sync completes).

So, until the sync is completed, the UI shows an activate sync button, since it thinks the data type is not enabled. 

Is this type supposed to be active if the user enabled it?
Cc: zea@chromium.org
Wow. Good catch Olivier. I am surprised this flew under the radar for 4 releases and we're shipping the 5th one now with this bug in it. There were not unit tests to catch this ?
Labels: M-62
Unittesting is not easy as it requires a slow syncing (slow server or a lot of sync data).
I will try to do something.
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4159d97a69759595319b3a48e9b93e0fec9b9eb

commit e4159d97a69759595319b3a48e9b93e0fec9b9eb
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Wed Oct 11 17:53:28 2017

Introduce SyncSetupService::IsDataTypeActive

https://codereview.chromium.org/2790483002/ changed the logic of 
IsDataTypeEnabled, which broke both settings and recent tabs.
Fork the IsDataTypeEnabled and introduce IsDataTypeActive
- IsDataTypePreferred returns true if the user enabled the settings.
- IsDataTypeActive returns true if IsDataTypeEnabled and the syncing setup is complete.

Bug:  718462 
Change-Id: I09d744725361989a9e165d29158f9f699bc579a2
Reviewed-on: https://chromium-review.googlesource.com/712038
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508027}
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/sync/sync_setup_service.cc
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/sync/sync_setup_service.h
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/sync/sync_setup_service_mock.h
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/ui/history/history_collection_view_controller.mm
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/ui/history/history_collection_view_controller_unittest.mm
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_table_coordinator_unittest.mm
[modify] https://crrev.com/e4159d97a69759595319b3a48e9b93e0fec9b9eb/ios/chrome/browser/ui/settings/sync_settings_collection_view_controller.mm

Status: Fixed (was: Assigned)
Merge status TBD
Status: Verified (was: Fixed)
Verified on chrome canary version 63.0.3238.0 on iPhone 6s plus and iPad Air with iOS 10.3.3, iPad Air with iOS 11.0.3, iPhone 8 with iOS 11.1 beta 2, following the steps mentioned in comment #0.  After force quitting and relaunching chrome app, other devices under recent tab section is displayed in loading state. Looks good.
Labels: ReleaseBlock-Stable Merge-Request-62
Project Member

Comment 25 by sheriffbot@chromium.org, Oct 12 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: Less than 1 days to go before AppStore submit on M62
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Review -Merge-Review-62 Merge-Approved-62
Project Member

Comment 27 by bugdroid1@chromium.org, Oct 12 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a58092d07f7730c92c166f176b312f441073155

commit 4a58092d07f7730c92c166f176b312f441073155
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Thu Oct 12 19:33:23 2017

Introduce SyncSetupService::IsDataTypeActive

https://codereview.chromium.org/2790483002/ changed the logic of 
IsDataTypeEnabled, which broke both settings and recent tabs.
Fork the IsDataTypeEnabled and introduce IsDataTypeActive
- IsDataTypePreferred returns true if the user enabled the settings.
- IsDataTypeActive returns true if IsDataTypeEnabled and the syncing setup is complete.

Bug:  718462 
Change-Id: I09d744725361989a9e165d29158f9f699bc579a2
Reviewed-on: https://chromium-review.googlesource.com/712038
Commit-Queue: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#508027}(cherry picked from commit e4159d97a69759595319b3a48e9b93e0fec9b9eb)
Reviewed-on: https://chromium-review.googlesource.com/716382
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#667}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/sync/sync_setup_service.cc
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/sync/sync_setup_service.h
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/sync/sync_setup_service_mock.h
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/ui/history/history_collection_view_controller.mm
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/ui/history/history_collection_view_controller_unittest.mm
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_table_coordinator_unittest.mm
[modify] https://crrev.com/4a58092d07f7730c92c166f176b312f441073155/ios/chrome/browser/ui/settings/sync_settings_collection_view_controller.mm

Verified on 62.0.3202.55 Beta in iPad Air(iOS 10.3.3), iPhone 6s(10.3.3), iPad Air (iOS 11.0) and iPad 8Plus(iOS 11.1 beta)
Followed the steps mentioned in comment#0, after force quit and relaunch the recent tab is displaying a loading indicator until the Other devices records are fetched. Working as expected.
Issue 770742 has been merged into this issue.

Sign in to add a comment