Rewrite AndroidSyncSettings to be significantly simpler.

Rewrite AndroidSyncSettings to be significantly simpler.

The public methods no longer take an Account as a parameter.
Instead, when the account changes, updateAccount() is called
by ChromeSigninController. This removes the need for all of
the "didUpdate" logic.

Instead, updateCachedSettings() is called only when the
Android settings are changed or the account is changed.

CachedAccountSettings has been removed entirely. A separate
class is not necessary to cache three boolean values (which
it was already only handling two of!)

Committed: https://crrev.com/2a1dd1c63c6e26f53120b4362dcaa8b8a9e830ae
Cr-Commit-Position: refs/heads/master@{#318196}

[maxbogue, 2015-01-30 20:53:53]

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

[maxbogue, 2015-01-30 22:02:48]

Apparently adding a reviewer doesn't "mail" a CL. Good to know.

[nyquist, 2015-02-06 03:03:57]

I think the new approach looks sane and way simpler to reason about, but I have
a few concerns about the @ThreadSafe annotation (which is a great thing to
have!).

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:143:
setChromeSyncEnabled(true);
This class is marked as @ThreadSafe. Is it still safe to call makeSyncable and
setChromeSyncEnabled in two different calls?

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:174:
mObservers.addObserver(observer);
I believe ObserverList is marked @NotThreadSafe, but this class is marked
@ThreadSafe, so I think you need to wrap this using synchronized (and for
unregister and iterations probably).

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:184: private
synchronized void setChromeSyncEnabled(boolean value) {
This synchronizes on the instance of this object, which we don't have control
over. Other users of this object could use the lock for their own purposes.
Create an internal lock that you fully control instead. Same for the methods
below.

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:269: for
(AndroidSyncSettingsObserver observer : mObservers) {
I don't think it's thread safe to iterate over observers, but this class is
marked as @ThreadSafe. Could you make this threadsafe?
Also, take care not to hold the mutex while you are calling the observer, as it
is hard to reason about whether that code will later call something that will
call back into this, waiting for the mutex we are holding (through thread
hopping).

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
mAndroidSyncSettings.updateAccount(getSignedInUser());
Could this be done by SigninManager instead? I think we should merge
ChromeSigninController and SigninManager into one class. It's not for this CL
though. Only tests and the SigninManager currently calls this method, so it
should be OK to move this to SigninManager later.
We just need to take care about whether native has been loaded or not.
InvalidationControllerTest and AndroidSyncSettingsTest currently don't load
native.

Could you add a TODO or file a bug for that?

[maxbogue, 2015-02-10 09:20:01]

PTAL!

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:143:
setChromeSyncEnabled(true);
On 2015/02/06 03:03:56, nyquist wrote:
> This class is marked as @ThreadSafe. Is it still safe to call makeSyncable and
> setChromeSyncEnabled in two different calls?

makeSyncable is now ensureSyncable and is called from within
setChromeSyncEnabled().

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:174:
mObservers.addObserver(observer);
On 2015/02/06 03:03:56, nyquist wrote:
> I believe ObserverList is marked @NotThreadSafe, but this class is marked
> @ThreadSafe, so I think you need to wrap this using synchronized (and for
> unregister and iterations probably).

Done for register and unregister.

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:184: private
synchronized void setChromeSyncEnabled(boolean value) {
On 2015/02/06 03:03:56, nyquist wrote:
> This synchronizes on the instance of this object, which we don't have control
> over. Other users of this object could use the lock for their own purposes.
> Create an internal lock that you fully control instead. Same for the methods
> below.

Done.

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:269: for
(AndroidSyncSettingsObserver observer : mObservers) {
On 2015/02/06 03:03:56, nyquist wrote:
> I don't think it's thread safe to iterate over observers, but this class is
> marked as @ThreadSafe. Could you make this threadsafe?
> Also, take care not to hold the mutex while you are calling the observer, as
it
> is hard to reason about whether that code will later call something that will
> call back into this, waiting for the mutex we are holding (through thread
> hopping).

From ObserverList.java: This container can be modified during iteration without
invalidating the iterator.

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/60001/sync/android/java/src/or...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
mAndroidSyncSettings.updateAccount(getSignedInUser());
On 2015/02/06 03:03:57, nyquist wrote:
> Could this be done by SigninManager instead? I think we should merge
> ChromeSigninController and SigninManager into one class. It's not for this CL
> though. Only tests and the SigninManager currently calls this method, so it
> should be OK to move this to SigninManager later.
> We just need to take care about whether native has been loaded or not.
> InvalidationControllerTest and AndroidSyncSettingsTest currently don't load
> native.
> 
> Could you add a TODO or file a bug for that?

Added a TODO for this line. I can file a bug for merging the whole class if
you'd like.

[maxbogue, 2015-02-18 01:50:52]

Ping! PTAL

[pval...(no longer on Chromium), 2015-02-18 19:13:15]

pvalenzuela@chromium.org changed reviewers:
+ pvalenzuela@chromium.org

[pval...(no longer on Chromium), 2015-02-18 19:13:16]

drive-by review: this looks really nice from an API standpoint. nice work.

I'll let Tommy handle the real review.

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:286:
@Deprecated
when we can we delete these deprecated methods?

[maxbogue, 2015-02-18 19:54:20]

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:286:
@Deprecated
On 2015/02/18 19:13:16, pvalenzuela wrote:
> when we can we delete these deprecated methods?

When they're no longer used downstream.

[nyquist, 2015-02-19 21:34:17]

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

[nyquist, 2015-02-19 21:34:18]

I think this is starting to lg.

Adding yfriedman since he wrote this cache initially.

yfriedman: PTAL

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:208: * This
function must be called within a synchronized block.
You could possibly add the synchronized block instead inside of this method,
which would just make it work, but keeping this comment instead is fine.

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
// TODO(maxbogue): Move this to AccountManager.
I don't think we want this in the AccountManager, but the SigninManager.

[maxbogue, 2015-02-20 20:22:46]

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:208: * This
function must be called within a synchronized block.
On 2015/02/19 21:34:17, nyquist wrote:
> You could possibly add the synchronized block instead inside of this method,
> which would just make it work, but keeping this comment instead is fine.

Acknowledged.

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/100001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
// TODO(maxbogue): Move this to AccountManager.
On 2015/02/19 21:34:17, nyquist wrote:
> I don't think we want this in the AccountManager, but the SigninManager.

Whoops, fixed.

[Yaron, 2015-02-24 22:26:36]

Cool! I think this is a lot simpler and seems to make sense to me

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:31: // Lock for
ensuring singleton instantiation across threads.
This is more useful as javadoc than a one-line comment

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:126: * @param
account the account to check if Chrome sync is enabled on.
remove

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
// TODO(maxbogue): Move this to SigninManager.
Rather than adding a strong-ref to AndroidSyncSettings how about adding another
notification and making AndroidSyncSettings an observer

https://codereview.chromium.org/879533004/diff/120001/sync/android/javatests/...
File sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java
(left):

https://codereview.chromium.org/879533004/diff/120001/sync/android/javatests/...
sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java:325:
assertTrue("Update sync settings failed to exit early",
mCachedAccountSyncSettings
This removal is probably for the best, heh

[nyquist, 2015-02-25 18:52:43]

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
// TODO(maxbogue): Move this to SigninManager.
On 2015/02/24 22:26:36, Yaron wrote:
> Rather than adding a strong-ref to AndroidSyncSettings how about adding
another
> notification and making AndroidSyncSettings an observer

Either that or possibly listening to the pref (
http://developer.android.com/reference/android/content/SharedPreferences.html...
).

I think a real notification might be better though.

[maxbogue, 2015-02-25 21:40:41]

PTAL!

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java (right):

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:31: // Lock for
ensuring singleton instantiation across threads.
On 2015/02/24 22:26:36, Yaron wrote:
> This is more useful as javadoc than a one-line comment 

Done.

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:126: * @param
account the account to check if Chrome sync is enabled on.
On 2015/02/24 22:26:36, Yaron wrote:
> remove

Done.

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
// TODO(maxbogue): Move this to SigninManager.
On 2015/02/25 18:52:43, nyquist wrote:
> On 2015/02/24 22:26:36, Yaron wrote:
> > Rather than adding a strong-ref to AndroidSyncSettings how about adding
> another
> > notification and making AndroidSyncSettings an observer
> 
> Either that or possibly listening to the pref (
>
http://developer.android.com/reference/android/content/SharedPreferences.html...
> ).
> 
> I think a real notification might be better though.

AndroidSyncSettings (//sync) doesn't know about SigninManager
(//chrome/browser/signin), and my understanding is that we basically want to
remove ChromeSigninController. That's why I haven't done that. Also if you look
in SigninManager, it already references AndroidSyncSettings directly, and I
believe this call needs to happen before the other one, which is before the
observers are notified:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

https://codereview.chromium.org/879533004/diff/120001/sync/android/javatests/...
File sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java
(left):

https://codereview.chromium.org/879533004/diff/120001/sync/android/javatests/...
sync/android/javatests/src/org/chromium/sync/AndroidSyncSettingsTest.java:325:
assertTrue("Update sync settings failed to exit early",
mCachedAccountSyncSettings
On 2015/02/24 22:26:36, Yaron wrote:
> This removal is probably for the best, heh

:)

[Yaron, 2015-02-25 22:39:36]

lgtm

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
File sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java
(right):

https://codereview.chromium.org/879533004/diff/120001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:87:
// TODO(maxbogue): Move this to SigninManager.
On 2015/02/25 21:40:41, maxbogue wrote:
> On 2015/02/25 18:52:43, nyquist wrote:
> > On 2015/02/24 22:26:36, Yaron wrote:
> > > Rather than adding a strong-ref to AndroidSyncSettings how about adding
> > another
> > > notification and making AndroidSyncSettings an observer
> > 
> > Either that or possibly listening to the pref (
> >
>
http://developer.android.com/reference/android/content/SharedPreferences.html...
> > ).
> > 
> > I think a real notification might be better though.
> 
> AndroidSyncSettings (//sync) doesn't know about SigninManager
> (//chrome/browser/signin), and my understanding is that we basically want to
> remove ChromeSigninController. That's why I haven't done that. Also if you
look
> in SigninManager, it already references AndroidSyncSettings directly, and I
> believe this call needs to happen before the other one, which is before the
> observers are notified:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

ok.

[maxbogue, 2015-02-26 01:47:02]

The CQ bit was checked by maxbogue@chromium.org

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

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

[commit-bot: I haz the power, 2015-02-26 02:11:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-26 02:11:12]

Failed to commit the patch.

[commit-bot: I haz the power, 2015-02-26 02:11:43]

The CQ bit was unchecked by commit-bot@chromium.org

[maxbogue, 2015-02-26 06:09:37]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2015-02-26 06:10:09]

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

[commit-bot: I haz the power, 2015-02-26 06:11:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-26 06:11:58]

Patchset 8 (id:??) landed as
https://crrev.com/2a1dd1c63c6e26f53120b4362dcaa8b8a9e830ae
Cr-Commit-Position: refs/heads/master@{#318196}