🌅 Refactor SigninHelper.

Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806, 599284
Committed: https://crrev.com/53511f941065102f8c8c5a476c5e4cc3770726ee
Cr-Commit-Position: refs/heads/master@{#387905}

[Peter Wen, 2016-04-05 21:17:19]

Description was changed from

==========
[Signin] Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806,599284
==========

to

==========
Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806,599284
==========

[Peter Wen, 2016-04-05 21:19:20]

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

[Peter Wen, 2016-04-05 21:19:21]

Hi Max,

This is a refactoring in order to fix http://crbug.com/599284, let me know
whether you think SigninHelper should be merged into SigninManager like I
suggested in http://crbug.com/600806.

Thanks, 🍄

Peter

[maxbogue, 2016-04-06 01:44:25]

On 2016/04/05 21:19:21, Peter Wen wrote:
> Hi Max,
> 
> This is a refactoring in order to fix http://crbug.com/599284, let me know
> whether you think SigninHelper should be merged into SigninManager like I
> suggested in http://crbug.com/600806.
> 
> Thanks, 🍄
> 
> Peter

This is mostly fine but looks like you need to update SyncTest.java.

I agree that SigninHelper and GoogleServicesManager should not exist, but I do
not want them wholesale merged into SigninManager. I've avoided doing that
because the SIgninHelper code needs to be refactored and cleaned up
significantly first. It is very old and not very comprehensible.

[maxbogue, 2016-04-06 01:44:39]

https://codereview.chromium.org/1860143003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/services/GoogleServicesManager.java
(right):

https://codereview.chromium.org/1860143003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/GoogleServicesManager.java:43:
protected final Context mAppContext;
I don't really think mAppContext is necessary vs mContext; almost every
singleton stores the app context as mContext. If that's explicitly the issue
you're working on though this is fine.

[Peter Wen, 2016-04-06 12:42:22]

Thanks for the quick review! I'll leave the merging of these helpers to you
folks then and just refactor enough to fix the SharedPrefs issue.

https://codereview.chromium.org/1860143003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/services/GoogleServicesManager.java
(right):

https://codereview.chromium.org/1860143003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/services/GoogleServicesManager.java:43:
protected final Context mAppContext;
On 2016/04/06 01:44:39, maxbogue wrote:
> I don't really think mAppContext is necessary vs mContext; almost every
> singleton stores the app context as mContext. If that's explicitly the issue
> you're working on though this is fine.

Agreed, I'll be making this distinction throughout our codebase as Android is
changing things so that the difference between knowing it's an app context vs
it's possibly an activity means I/O jank or not. Unfortunate, but as a result
we'll also be certain when it's our app context. :)

[Peter Wen, 2016-04-07 19:06:02]

Friendly ping. 🍕

[maxbogue, 2016-04-07 19:15:32]

lgtm

[Peter Wen, 2016-04-07 19:24:31]

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

[Peter Wen, 2016-04-07 19:24:31]

Thanks Max!

+nyquist@ for chrome/android OWNERS.

[Peter Wen, 2016-04-07 19:24:58]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-07 19:25:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860143003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860143003/40001

[commit-bot: I haz the power, 2016-04-07 21:00:28]

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

[commit-bot: I haz the power, 2016-04-07 21:00:29]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-04-12 18:03:23]

Friendly ping. 🌅

[nyquist, 2016-04-16 00:32:02]

lgtm

[Peter Wen, 2016-04-18 14:15:44]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2016-04-18 14:15:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1860143003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1860143003/40001

[commit-bot: I haz the power, 2016-04-18 14:53:18]

Description was changed from

==========
Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806,599284
==========

to

==========
Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806,599284
==========

[commit-bot: I haz the power, 2016-04-18 14:53:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-18 14:54:39]

Description was changed from

==========
Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806,599284
==========

to

==========
Refactor SigninHelper.

Rather than expose static methods and passing in the context of the
caller, since SigninHelper is designed to be used application-wide,
save the application context as a member variable instead.

Save the default shared preferences as a member variable, reducing
potential burden in the future.

BUG=600806,599284

Committed: https://crrev.com/53511f941065102f8c8c5a476c5e4cc3770726ee
Cr-Commit-Position: refs/heads/master@{#387905}
==========

[commit-bot: I haz the power, 2016-04-18 14:54:40]

Patchset 3 (id:??) landed as
https://crrev.com/53511f941065102f8c8c5a476c5e4cc3770726ee
Cr-Commit-Position: refs/heads/master@{#387905}

[Peter Wen, 2016-04-27 15:06:21]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1926703003/ by wnwen@chromium.org.

The reason for reverting is: Some static methods in SigninHelper are used when
Chrome is not started, so native is not loaded. See crbug.com/605832 for
context..