Spinner to show progress during sync after signing in |
|||||||
Issue descriptionBefore the user completes the initial sync, the Bookmarks panel is empty. Initial sync on iOS usually takes a few seconds, but the 90th percentile is as high as 11 seconds. This may give them the impression that we deleted all their bookmarks. The Sync client API does not expose the necessary information to simply add a spinner. The transition from not signed in to signed in happens outside the normal flow of loading the bookmarks model. Watching for the proper notifications is non-trivial and naive solutions end up coupling together systems that should be independent.
,
Jul 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/927b8799d354775dd8735ac760c66c423c82059b commit 927b8799d354775dd8735ac760c66c423c82059b Author: Marti Wong <martiw@chromium.org> Date: Thu Jul 13 02:02:11 2017 Move synced_sessions_bridge.mm from ui/ntp/recent_tabs/ to ui/sync/ So that this class could be reused by bookmark_collection_view for keeping track of the syncing status. (and in later CL, it will show a loading spinner when bookmark is syncing.) Bug: 696893 Change-Id: I370c91ad1f9815c6d3c70021939140478beaebcc Reviewed-on: https://chromium-review.googlesource.com/567786 Commit-Queue: Marti Wong <martiw@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Jean-François Geyelin <jif@chromium.org> Cr-Commit-Position: refs/heads/master@{#486218} [modify] https://crrev.com/927b8799d354775dd8735ac760c66c423c82059b/ios/chrome/browser/ui/ntp/recent_tabs/BUILD.gn [modify] https://crrev.com/927b8799d354775dd8735ac760c66c423c82059b/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_panel_controller.mm [modify] https://crrev.com/927b8799d354775dd8735ac760c66c423c82059b/ios/chrome/browser/ui/sync/BUILD.gn [rename] https://crrev.com/927b8799d354775dd8735ac760c66c423c82059b/ios/chrome/browser/ui/sync/synced_sessions_bridge.h [rename] https://crrev.com/927b8799d354775dd8735ac760c66c423c82059b/ios/chrome/browser/ui/sync/synced_sessions_bridge.mm [modify] https://crrev.com/927b8799d354775dd8735ac760c66c423c82059b/ios/chrome/browser/ui/tab_switcher/tab_switcher_model.h
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cbd742dd1e63e553a4a33b428b81620f7fa7cedb commit cbd742dd1e63e553a4a33b428b81620f7fa7cedb Author: Marti Wong <martiw@chromium.org> Date: Tue Jul 18 03:37:31 2017 Show loading spinner on bookmark panel when syncing (iOS) [Demo videos] - account with bookmarks: https://drive.google.com/open?id=0B1O0Z7eoZMuGeTBjVXdla0RPeFE - account with no bookmarks: https://drive.google.com/open?id=0B1O0Z7eoZMuGR0pGb2tYUHcyOWM TEST=Make sure it works when signed in user has no bookmarks or there is any sync error... Bug: 696893 Change-Id: I30a29eed423253651ff2466943c8ecdd6974e0b0 Reviewed-on: https://chromium-review.googlesource.com/569538 Commit-Queue: Marti Wong <martiw@chromium.org> Reviewed-by: Jean-François Geyelin <jif@chromium.org> Reviewed-by: Eric Noyau <noyau@chromium.org> Cr-Commit-Position: refs/heads/master@{#487374} [modify] https://crrev.com/cbd742dd1e63e553a4a33b428b81620f7fa7cedb/ios/chrome/browser/ui/bookmarks/BUILD.gn [modify] https://crrev.com/cbd742dd1e63e553a4a33b428b81620f7fa7cedb/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm [modify] https://crrev.com/cbd742dd1e63e553a4a33b428b81620f7fa7cedb/ios/chrome/browser/ui/ntp/recent_tabs/BUILD.gn [modify] https://crrev.com/cbd742dd1e63e553a4a33b428b81620f7fa7cedb/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_panel_controller.mm [modify] https://crrev.com/cbd742dd1e63e553a4a33b428b81620f7fa7cedb/ios/chrome/browser/ui/sync/synced_sessions_bridge.h [modify] https://crrev.com/cbd742dd1e63e553a4a33b428b81620f7fa7cedb/ios/chrome/browser/ui/sync/synced_sessions_bridge.mm
,
Jul 19 2017
martiw@: Could you please confirm if this is iOS or Mac related?
,
Jul 19 2017
This should be an iOS bug.
,
Jul 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b1ba8966a0aa6b724232517a8566cda4c2bd11de commit b1ba8966a0aa6b724232517a8566cda4c2bd11de Author: Olivier Robin <olivierrobin@chromium.org> Date: Wed Jul 19 13:15:45 2017 Revert "Show loading spinner on bookmark panel when syncing (iOS)" This reverts commit cbd742dd1e63e553a4a33b428b81620f7fa7cedb. Reason for revert: Eg tests time out waiting for this animation. Testing the EGtests bots without this CL. Original change's description: > Show loading spinner on bookmark panel when syncing (iOS) > > [Demo videos] > - account with bookmarks: > https://drive.google.com/open?id=0B1O0Z7eoZMuGeTBjVXdla0RPeFE > - account with no bookmarks: > https://drive.google.com/open?id=0B1O0Z7eoZMuGR0pGb2tYUHcyOWM > > TEST=Make sure it works when signed in user has no bookmarks or there is any sync error... > > Bug: 696893 > Change-Id: I30a29eed423253651ff2466943c8ecdd6974e0b0 > Reviewed-on: https://chromium-review.googlesource.com/569538 > Commit-Queue: Marti Wong <martiw@chromium.org> > Reviewed-by: Jean-François Geyelin <jif@chromium.org> > Reviewed-by: Eric Noyau <noyau@chromium.org> > Cr-Commit-Position: refs/heads/master@{#487374} TBR=jif@chromium.org,noyau@chromium.org,martiw@google.com,ramyasharma@chromium.org,martiw@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 696893 Change-Id: I89336f1cb8f0ed6213fe09d344aba37529cf7dab Reviewed-on: https://chromium-review.googlesource.com/577547 Reviewed-by: Olivier Robin <olivierrobin@chromium.org> Reviewed-by: Eric Noyau <noyau@chromium.org> Reviewed-by: Jean-François Geyelin <jif@chromium.org> Commit-Queue: Olivier Robin <olivierrobin@chromium.org> Cr-Commit-Position: refs/heads/master@{#487839} [modify] https://crrev.com/b1ba8966a0aa6b724232517a8566cda4c2bd11de/ios/chrome/browser/ui/bookmarks/BUILD.gn [modify] https://crrev.com/b1ba8966a0aa6b724232517a8566cda4c2bd11de/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm [modify] https://crrev.com/b1ba8966a0aa6b724232517a8566cda4c2bd11de/ios/chrome/browser/ui/ntp/recent_tabs/BUILD.gn [modify] https://crrev.com/b1ba8966a0aa6b724232517a8566cda4c2bd11de/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_panel_controller.mm [modify] https://crrev.com/b1ba8966a0aa6b724232517a8566cda4c2bd11de/ios/chrome/browser/ui/sync/synced_sessions_bridge.h [modify] https://crrev.com/b1ba8966a0aa6b724232517a8566cda4c2bd11de/ios/chrome/browser/ui/sync/synced_sessions_bridge.mm
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34cf3f816d0d4c4986116a63405cdc34f30a07dc commit 34cf3f816d0d4c4986116a63405cdc34f30a07dc Author: Marti Wong <martiw@chromium.org> Date: Wed Aug 02 01:42:14 2017 Show loading spinner on bookmark panel when syncing (iOS) This CL is the followup fix for CL/569538/ which was reverted because it caused timeouts of egtests. This CL made some change on a egtest to prevent the timeout. Other than that, this CL is identical to CL/569538. [Patch set 1] is identical to CL/569538. [Patch set 2] added the fix to prevent egtest timeout. [Demo videos] - account with bookmarks: https://drive.google.com/open?id=0B1O0Z7eoZMuGeTBjVXdla0RPeFE - account with no bookmarks: https://drive.google.com/open?id=0B1O0Z7eoZMuGR0pGb2tYUHcyOWM TEST=Make sure it works when signed in user has no bookmarks or there is any sync error... Bug: 696893 Change-Id: I92af0f4a1c9f88f6595f8a851ff92f0b5215132b Reviewed-on: https://chromium-review.googlesource.com/587517 Commit-Queue: Marti Wong <martiw@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Jean-François Geyelin <jif@chromium.org> Reviewed-by: Eric Noyau <noyau@chromium.org> Cr-Commit-Position: refs/heads/master@{#491181} [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/authentication/BUILD.gn [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/bookmarks/BUILD.gn [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/ntp/recent_tabs/BUILD.gn [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/ntp/recent_tabs/recent_tabs_panel_controller.mm [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/sync/synced_sessions_bridge.h [modify] https://crrev.com/34cf3f816d0d4c4986116a63405cdc34f30a07dc/ios/chrome/browser/ui/sync/synced_sessions_bridge.mm
,
Aug 3 2017
,
Aug 8 2017
Verified in 62.0.3179.0 canary, iPhone 6 iOS 10.3.3, iPhone 7 iOS11, iPad Pro iOS11 Loading spinner on bookmark panel is shown when syncing. Looks good.
,
Oct 15
Sorry to reopen this, please consider filing a dedicated bug instead, if my observation is accurate. I ran into this code recently and realized SyncedSessionsObserverBridge (which is about sync datatype SESSIONS) is being used to influence a UI about bookmarks. This is wrong, since bookmark sync is independent of session sync: they can be enabled-disabled independently, and their runtime state is independent too. I don't know much about iOS, but as an example, I suppose you can turn off session sync (that is, history sync and tab sync) and then I'd expect the spinner to stay forever. The simplest solution would be to at least fork SyncedSessionsObserverBridge and do the analogous for bookmarks.
,
Oct 15
I started a patch here: https://chromium-review.googlesource.com/c/chromium/src/+/1280548
,
Oct 16
Assigned to gambard@ because I left the Bling team for a while. gambard@, I wasn't sure who to send this to, so feel free to assign to someone else if you like.
,
Oct 16
Assigning to sczs@ as bookmark owner.
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf commit 84c9d2e1a5b5248917f1d783629eb1143b3c3fdf Author: Mikel Astiz <mastiz@chromium.org> Date: Tue Oct 23 16:16:05 2018 Fix bookmarks home UI depending on SESSIONS sync state Instead of reusing SyncedSessionsObserverBridge, which is about sync datatype SESSIONS (aka tab&history sync), the bookmarks UI should care about whether BOOKMARKS have synced, and in particular SyncedSessionsObserverBridge::CheckIfFirstSyncIsCompleted() is not reusable. We fork the class into an analogous observer for BOOKMARKS, together with some obvious simplifications. Future patches may do further cleanup, because it does feel like the current implementations are overly complex. Bug: 696893 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: I0ee639b33ce07101c6e467ed2d84631c40b578c5 Reviewed-on: https://chromium-review.googlesource.com/c/1280548 Reviewed-by: Olivier Robin <olivierrobin@chromium.org> Reviewed-by: Sergio Collazos <sczs@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#601976} [modify] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/sync/BUILD.gn [modify] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/sync/sync_observer_bridge.h [delete] https://crrev.com/502529c19a3023f02e6a34a72c2584981796fe53/ios/chrome/browser/sync/synced_sessions_bridge.mm [modify] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/bookmarks/BUILD.gn [modify] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/bookmarks/bookmark_home_mediator.mm [add] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/bookmarks/synced_bookmarks_bridge.h [add] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/bookmarks/synced_bookmarks_bridge.mm [modify] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/recent_tabs/BUILD.gn [modify] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.h [rename] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.h [add] https://crrev.com/84c9d2e1a5b5248917f1d783629eb1143b3c3fdf/ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.mm |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by martiw@chromium.org
, Jul 7 2017Owner: martiw@chromium.org
Status: Started (was: Assigned)