[Sync] Make it impossible to get a reference to AndroidSyncSettings.

[Sync] Make it impossible to get a reference to AndroidSyncSettings.

This change is motivated by some complex test flakiness issues that were
discovered in http://crrev.com/1118833002. Making it impossible to store
a reference means that if we overwrite it for tests, we know everyone is
then using the overwritten version.

The approach here is to make every public method static and take the
context as an argument, so it can initialize the inner object if
necessary.

This CL is part 1/3 and leaves in deprecated versions of all the methods.
In part 2/3 the downstream uses of AndroidSyncSettings will be changed,
and in part 3/3 the deprecated methods will be removed upstream.

BUG=480604
Committed: https://crrev.com/80344ba46a6299d852d068ebaf26d658847fc49d
Cr-Commit-Position: refs/heads/master@{#330570}

[maxbogue, 2015-05-15 17:44:03]

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

[maxbogue, 2015-05-15 17:44:04]

Hey Tommy and Nick, this is the change we discussed to resolve the flakiness
issues from the other CL. PTAL!

[maniscalco, 2015-05-15 23:17:53]

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java
(right):

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java:63:
if (!AndroidSyncSettings.isSyncEnabled(mApplicationContext)) {
Prior to this change, we were using the context passed into the ctor
("context"). Now we are using the one returned by
context.getApplicationContext(). Is the difference important? Which is
preferred?

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

https://codereview.chromium.org/1138013008/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:24: * To
retrieve an instance of this class, call AndroidSyncSettings.get(someContext).
Please update this javadoc and be sure to explain why AndroidSyncSettings is a
collection of static methods (testing, so you can't cache it, etc.).

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

https://codereview.chromium.org/1138013008/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:86:
AndroidSyncSettings.updateAccount(mApplicationContext, getSignedInUser());
Similar to the comment I made in SyncNotificationController.java: context vs
getApplicationContext ?

[maxbogue, 2015-05-18 17:33:01]

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java
(right):

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java:63:
if (!AndroidSyncSettings.isSyncEnabled(mApplicationContext)) {
On 2015/05/15 23:17:53, maniscalco wrote:
> Prior to this change, we were using the context passed into the ctor
> ("context"). Now we are using the one returned by
> context.getApplicationContext(). Is the difference important? Which is
> preferred?

AndroidSyncSettings only uses the application context anyways (see its
constructor), so it makes no difference.

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

https://codereview.chromium.org/1138013008/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:24: * To
retrieve an instance of this class, call AndroidSyncSettings.get(someContext).
On 2015/05/15 23:17:53, maniscalco wrote:
> Please update this javadoc and be sure to explain why AndroidSyncSettings is a
> collection of static methods (testing, so you can't cache it, etc.).

Done. Good catch, thanks!

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

https://codereview.chromium.org/1138013008/diff/40001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/signin/ChromeSigninController.java:86:
AndroidSyncSettings.updateAccount(mApplicationContext, getSignedInUser());
On 2015/05/15 23:17:53, maniscalco wrote:
> Similar to the comment I made in SyncNotificationController.java: context vs
> getApplicationContext ?
> 

Ack; doesn't matter. See my reply above.

[maxbogue, 2015-05-18 18:07:27]

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java
(right):

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java:63:
if (!AndroidSyncSettings.isSyncEnabled(mApplicationContext)) {
On 2015/05/18 17:33:00, maxbogue wrote:
> On 2015/05/15 23:17:53, maniscalco wrote:
> > Prior to this change, we were using the context passed into the ctor
> > ("context"). Now we are using the one returned by
> > context.getApplicationContext(). Is the difference important? Which is
> > preferred?
> 
> AndroidSyncSettings only uses the application context anyways (see its
> constructor), so it makes no difference.

To follow up from an offline conversation with Nick: it doesn't matter what
context we pass in to AndroidSyncSettings, because part of its contract is that
it will extract the ApplicationContext and use that. I've updated the
AndroidSyncSettings constructor comment to express that, since before it was in
the .get() comment.

[maniscalco, 2015-05-18 18:22:35]

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java
(right):

https://codereview.chromium.org/1138013008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java:63:
if (!AndroidSyncSettings.isSyncEnabled(mApplicationContext)) {
On 2015/05/18 18:07:26, maxbogue wrote:
> On 2015/05/18 17:33:00, maxbogue wrote:
> > On 2015/05/15 23:17:53, maniscalco wrote:
> > > Prior to this change, we were using the context passed into the ctor
> > > ("context"). Now we are using the one returned by
> > > context.getApplicationContext(). Is the difference important? Which is
> > > preferred?
> > 
> > AndroidSyncSettings only uses the application context anyways (see its
> > constructor), so it makes no difference.
> 
> To follow up from an offline conversation with Nick: it doesn't matter what
> context we pass in to AndroidSyncSettings, because part of its contract is
that
> it will extract the ApplicationContext and use that. I've updated the
> AndroidSyncSettings constructor comment to express that, since before it was
in
> the .get() comment.

Thanks.  I found these two links which seem relevant.  It sounds like when
storing a Context in a member variable, it's important to decide which one to
store (the Activity Context or the Application Context) as it can affect garbage
collection of activity resources.

For the case of AndroidSyncSettings, it seems like storing an application
context (the one returned by Context.getApplicationContext) is indeed the way to
go because it effectively protects against the caller passing an Activity
Context which might result in unnecessarily retaining resources.

http://stackoverflow.com/questions/7298731/when-to-call-activity-context-or-a...

http://android-developers.blogspot.com/2009/01/avoiding-memory-leaks.html

nyquist@, did I interpret this correctly?

[maniscalco, 2015-05-18 23:55:03]

lgtm

[nyquist, 2015-05-19 06:51:01]

great! thanks! lgtm

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

https://codereview.chromium.org/1138013008/diff/80001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:25: * stored.
This is important because in tests we need to be able to overwrite the object
Nit: We usually don't use pronouns in comments in the chromium code base. How
about just changing "in tests we need to" to "tests need to"?

[maxbogue, 2015-05-19 17:58:23]

Thanks for the speedy review, both of you!

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

https://codereview.chromium.org/1138013008/diff/80001/sync/android/java/src/o...
sync/android/java/src/org/chromium/sync/AndroidSyncSettings.java:25: * stored.
This is important because in tests we need to be able to overwrite the object
On 2015/05/19 06:51:01, nyquist wrote:
> Nit: We usually don't use pronouns in comments in the chromium code base. How
> about just changing "in tests we need to" to "tests need to"?

Done.

[maxbogue, 2015-05-19 17:58:28]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-05-19 17:58:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, maniscalco@chromium.org
Link to the patchset: https://codereview.chromium.org/1138013008/#ps100001
(title: "Rebase and address nit.")

[commit-bot: I haz the power, 2015-05-19 17:59:36]

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

[commit-bot: I haz the power, 2015-05-19 18:36:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-19 18:38:19]

Patchset 6 (id:??) landed as
https://crrev.com/80344ba46a6299d852d068ebaf26d658847fc49d
Cr-Commit-Position: refs/heads/master@{#330570}