Prepare SyncController to replace a downstream component.

Prepare SyncController to replace a downstream component.

SyncController handles the coordination of sync state
between the invalidation controller, the Android sync
settings, and the native sync code. Sync state can be
changed from four places:

- The Chrome UI, which will call SyncController directly.
- Native sync, which can disable it via a dashboard stop
  and clear.
- Android's Chrome sync setting.
- Android's master sync setting.

SyncController implements listeners for the last three
cases. When master sync is disabled, we are careful to not
change the Android Chrome sync setting so we know whether
to turn sync back on when it is re-enabled.

BUG=428882
Committed: https://crrev.com/0a04eaa8dcaaad67cbcfa41d40bc994cbc89ce0b
Cr-Commit-Position: refs/heads/master@{#312533}

[maxbogue, 2015-01-13 19:07:05]

maxbogue@chromium.org changed reviewers:
+ nyquist@chromium.org

[maxbogue, 2015-01-13 19:07:05]

PTAL!

[nyquist, 2015-01-16 17:42:22]

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:116:
Log.w(TAG, "updateSyncStateFromAndroid, master = " + masterSyncEnabled);
Nit: This should not be warning. At most it should be debug or verbose.

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:124:
public void start() {
Add javadoc here describing the three things it does (sync, android state and
invalidation)

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:136:
* Stops Sync if a user is currently signed in.
Mention that this also updates Android and stops invalidation.

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:142:
Account account = mChromeSigninController.getSignedInUser();
Should the ordering of the next three lines be opposite of start()?

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:158:
boolean isSyncActive = !mProfileSyncService.isStartSuppressed();
Does it happen during sign-in or startup that we get events where start is
temporarily suppressed (say, waiting for user to finish signing in or enabling
sync) where this method now would lead to disabling sync again?

[maxbogue, 2015-01-16 23:08:45]

PTAL!

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:116:
Log.w(TAG, "updateSyncStateFromAndroid, master = " + masterSyncEnabled);
On 2015/01/16 17:42:22, nyquist wrote:
> Nit: This should not be warning. At most it should be debug or verbose.

Didn't mean to leave that line in at all.

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:124:
public void start() {
On 2015/01/16 17:42:22, nyquist wrote:
> Add javadoc here describing the three things it does (sync, android state and
> invalidation)

Done.

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:136:
* Stops Sync if a user is currently signed in.
On 2015/01/16 17:42:22, nyquist wrote:
> Mention that this also updates Android and stops invalidation.

Done.

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:142:
Account account = mChromeSigninController.getSignedInUser();
On 2015/01/16 17:42:22, nyquist wrote:
> Should the ordering of the next three lines be opposite of start()?

They're in this order in the downstream class so I figure it doesn't matter
much.

https://codereview.chromium.org/852473002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:158:
boolean isSyncActive = !mProfileSyncService.isStartSuppressed();
On 2015/01/16 17:42:22, nyquist wrote:
> Does it happen during sign-in or startup that we get events where start is
> temporarily suppressed (say, waiting for user to finish signing in or enabling
> sync) where this method now would lead to disabling sync again?

No, StartSuppressed is only set when stopping or signing out of sync.

[nyquist, 2015-01-17 01:14:11]

https://codereview.chromium.org/852473002/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:100:
private void setupSessionSyncId() {
I don't think this should be here. This is specific to Chrome Shell.

https://codereview.chromium.org/852473002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:152:
if (mSyncStatusHelper.isMasterSyncAutomaticallyEnabled()) {
Could you do a sanity check for this and see if this always yields the same
results as the sync state calculator from downstream?

[maxbogue, 2015-01-20 21:50:56]

https://codereview.chromium.org/852473002/diff/120001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:100:
private void setupSessionSyncId() {
On 2015/01/17 01:14:11, nyquist wrote:
> I don't think this should be here. This is specific to Chrome Shell.

Done.

https://codereview.chromium.org/852473002/diff/120001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:152:
if (mSyncStatusHelper.isMasterSyncAutomaticallyEnabled()) {
On 2015/01/17 01:14:11, nyquist wrote:
> Could you do a sanity check for this and see if this always yields the same
> results as the sync state calculator from downstream?

I honestly can't follow the logic of the sync state calculator well. I can tell
you that I've tested the new code for all the scenarios I can think of.
Disabling sync from all four avenues (native, UI, Android Chrome and Android
master) and re-enabling via all but native (which there's no way to do ATM).

The master sync setting was still not working until the most recent patch on
this CL; I can now confirm that if you toggle master off and back on, sync will
come back up. The Android chrome sync setting acts as the "wanted" state in that
case.

[nyquist, 2015-01-21 22:53:04]

https://codereview.chromium.org/852473002/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:44:
mChromeSigninController.ensureGcmIsInitialized();
Could we move this to SigninManager and have it linked to the sign-in state?

https://codereview.chromium.org/852473002/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:143:
* From {@link ProfileSyncService.SyncStateChangedListener}.
Could you add a comment about what this affects like the ones above?

[maxbogue, 2015-01-21 23:37:56]

https://codereview.chromium.org/852473002/diff/160001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:44:
mChromeSigninController.ensureGcmIsInitialized();
Leaving as is per offline discussion.

https://codereview.chromium.org/852473002/diff/160001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:143:
* From {@link ProfileSyncService.SyncStateChangedListener}.
On 2015/01/21 22:53:04, nyquist wrote:
> Could you add a comment about what this affects like the ones above?

Done.

[nyquist, 2015-01-21 23:39:15]

Please write a longer CL description (similar to what's downstream, but with the
context of this CL naturally).

Otherwise lgtm

[nyquist, 2015-01-22 01:07:18]

lgtm % the last comment thing

https://codereview.chromium.org/852473002/diff/200001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/200001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:21:
* A helper class for managing sync state.
I think this line can go away now, since the rest of the comment is so helpful.

[maxbogue, 2015-01-22 01:12:34]

https://codereview.chromium.org/852473002/diff/200001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
(right):

https://codereview.chromium.org/852473002/diff/200001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java:21:
* A helper class for managing sync state.
On 2015/01/22 01:07:18, nyquist wrote:
> I think this line can go away now, since the rest of the comment is so
helpful.

Done.

[maxbogue, 2015-01-22 01:13:25]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2015-01-22 01:14:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/852473002/220001

[commit-bot: I haz the power, 2015-01-22 02:26:09]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-01-22 02:27:48]

Patchset 12 (id:??) landed as
https://crrev.com/0a04eaa8dcaaad67cbcfa41d40bc994cbc89ce0b
Cr-Commit-Position: refs/heads/master@{#312533}