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

Issue 860482 link

Starred by 2 users
Status: WontFix
Owner:
treib@chromium.org
Closed: Oct 24
Cc:
mastiz@chromium.org
ios-bugs@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

Consider getting rid of SyncService::RequestStop(KEEP_DATA)

Project Member Reported by treib@chromium.org, Jul 5 
Currently, SyncService::RequestStop can be called with one of two params:
- KEEP_DATA, which means keeping the Sync directory files and metadata around.
- CLEAR_DATA, which means to wipe the above.

In both cases, IsSyncRequested is set to false, so Sync won't start up again until explicitly requested via RequestStart.

From the comment on RequestStop: KEEP_DATA is used when the user just stops sync, and CLEAR_DATA is used when they sign out of the profile entirely.

RequestStop(KEEP_DATA) has very few callers outside of tests (two on Android [1,2] and one on iOS [3]). Can we maybe get rid of those? I don't see why it would even make sense to keep Sync data around if Sync is disabled. I guess it makes things a bit more efficient if Sync gets subsequently enabled again?

[1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java?rcl=b02a244eab56a93c957bb2aea8ce6749d876bba3&l=163
[2] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java?rcl=b02a244eab56a93c957bb2aea8ce6749d876bba3&l=730
[3] https://cs.chromium.org/chromium/src/ios/chrome/browser/sync/sync_setup_service.cc?rcl=bb6a9c2cf2fdbd724973ebc2128227376bd7cce6&l=203
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 23 

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

commit 4520dc4bc55ea43e33e8631b7c1cd808b4c1ef0b
Author: Marc Treib <treib@chromium.org>
Date: Mon Jul 23 10:51:36 2018

about:sync-internals: Use RequestStop(CLEAR_DATA) instead of KEEP_DATA

CLEAR_DATA is what's typically used in practice, and KEEP_DATA is a
weird concept anyway.

Bug:  860482 
Change-Id: Ia3d15463a03f947db19d08ebfd035299ee566eaf
Reviewed-on: https://chromium-review.googlesource.com/1146572
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577142}
[modify] https://crrev.com/4520dc4bc55ea43e33e8631b7c1cd808b4c1ef0b/chrome/browser/ui/webui/sync_internals_message_handler.cc

Comment 2 by treib@chromium.org, Jul 26 

It turns out that we can't fully get rid of KEEP_DATA: On Android, users often turn off Sync at the OS level, to save bandwidth. When they later turn it back on again, it would be bad if we had to re-download everything.

So, maybe the proper API is:
- SetSyncRequested(bool), mapping to today's RequestStart() and RequestStop(KEEP_DATA).
- DisableSync() or ClearSyncData() mapping to today's RequestStop(CLEAR_DATA).
  It looks like this is mostly used for the case of signout, which PSS already listens for anyway, so no external call should be necessary. It's also used if first-time setup is canceled, not sure about that case.

Comment 3 by mastiz@chromium.org, Oct 24

Owner: treib@chromium.org
Status: Assigned (was: Available)
treib@: can you please provide some status update? did anything change here?

Comment 4 by treib@chromium.org, Oct 24

Status: WontFix (was: Assigned)
#2 is still up-to-date; I was planning to address this in the course of bug 884159. I guess there's no point in keeping this one open too, so closing as WontFix.
►

Sign in to add a comment