🍰 Migrate app shared preferences to ContextUtils.

Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284
Committed: https://crrev.com/2d287cf0ce7e5393d5f2d7a826c5b285768e1192
Cr-Commit-Position: refs/heads/master@{#392355}

[Peter Wen, 2016-04-11 19:38:42]

Description was changed from

==========
Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284
==========

to

==========
Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284
==========

[Peter Wen, 2016-04-11 21:03:24]

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

[Peter Wen, 2016-04-11 21:03:25]

🍔

[Torne, 2016-04-12 10:05:56]

m

https://codereview.chromium.org/1874423002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/ContextUtils.java (right):

https://codereview.chromium.org/1874423002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/ContextUtils.java:60: public static
SharedPreferences getAppSharedPreferences(Context context) {
Is it absolutely necessary for this to take a context as a parameter? Can't the
callers just initialise the app context before using this? It looks really
confusing having a method that takes a context when its whole purpose is to make
sure we always use the same context, especially if the method is in the class
whose current purpose is to make sure we always have a global app context.

If we really need to pass the context then it seems like it would be less weird
to actually *require* the context to be passed in all cases, and have the sole
purpose of this function to be to call getApplicationContext on the passed
context, but I'd *much* prefer to have this method take no parameters.

[Peter Wen, 2016-04-28 18:50:19]

Should be ready for a more thorough review now. :)

https://codereview.chromium.org/1874423002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/ContextUtils.java (right):

https://codereview.chromium.org/1874423002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/ContextUtils.java:60: public static
SharedPreferences getAppSharedPreferences(Context context) {
On 2016/04/12 10:05:56, Torne wrote:
> Is it absolutely necessary for this to take a context as a parameter? Can't
the
> callers just initialise the app context before using this? It looks really
> confusing having a method that takes a context when its whole purpose is to
make
> sure we always use the same context, especially if the method is in the class
> whose current purpose is to make sure we always have a global app context.
> 
> If we really need to pass the context then it seems like it would be less
weird
> to actually *require* the context to be passed in all cases, and have the sole
> purpose of this function to be to call getApplicationContext on the passed
> context, but I'd *much* prefer to have this method take no parameters.

Done.

[Torne, 2016-04-29 12:10:45]

https://codereview.chromium.org/1874423002/diff/120001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContextUtils.java (right):

https://codereview.chromium.org/1874423002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContextUtils.java:73: // No need for
synchronization since sApplicationContext never changes.
Nope, this isn't safe - there's no memory ordering guarantee and so a thread can
observe sSharedPreferences as non-null when the object is not fully initialised.

Just put this in a static holder class to initialise it on first use?

[Peter Wen, 2016-05-02 22:05:52]

PTAL. :)

https://codereview.chromium.org/1874423002/diff/120001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContextUtils.java (right):

https://codereview.chromium.org/1874423002/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContextUtils.java:73: // No need for
synchronization since sApplicationContext never changes.
On 2016/04/29 12:10:45, Torne wrote:
> Nope, this isn't safe - there's no memory ordering guarantee and so a thread
can
> observe sSharedPreferences as non-null when the object is not fully
initialised.
> 
> Just put this in a static holder class to initialise it on first use?

Ah, nice catch! Put this in a holder class now.

[Torne, 2016-05-03 12:46:41]

https://codereview.chromium.org/1874423002/diff/140001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContextUtils.java (right):

https://codereview.chromium.org/1874423002/diff/140001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContextUtils.java:102:
Holder.sSharedPreferences = fetchAppSharedPreferences();
By having this backdoor for tests to update the variable you're re-opening
possible thread safety issues. Is it always guaranteed that every call to
getAppSharedPreferences in a test will definitely come at a time that's strictly
ordered after this call with respect to memory? (e.g. because this is called
before threads are created would be one reason)

[Peter Wen, 2016-05-03 12:58:14]

https://codereview.chromium.org/1874423002/diff/140001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ContextUtils.java (right):

https://codereview.chromium.org/1874423002/diff/140001/base/android/java/src/...
base/android/java/src/org/chromium/base/ContextUtils.java:102:
Holder.sSharedPreferences = fetchAppSharedPreferences();
On 2016/05/03 12:46:41, Torne wrote:
> By having this backdoor for tests to update the variable you're re-opening
> possible thread safety issues. Is it always guaranteed that every call to
> getAppSharedPreferences in a test will definitely come at a time that's
strictly
> ordered after this call with respect to memory? (e.g. because this is called
> before threads are created would be one reason)

Tests should be calling initApplicationContextForTests in setUp, so yes. The one
exception is PrivacyPreferencesManagerTest which does its own context magic, in
which all the tests are run sequentially and the context/sharedpreference usage
is also sequential. I think if tests are using this function then they are
intentionally doing some context mucking and should be responsible themselves
for thread safety.

[Torne, 2016-05-03 13:11:17]

LGTM then; it's just unfortunate to have it not be final.

[Peter Wen, 2016-05-03 13:16:44]

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

[Peter Wen, 2016-05-03 13:16:45]

+caitkp for components/ OWNERS.
+nyquist for chrome/android and chrome/test/android OWNERS.

[Cait (Slow), 2016-05-04 18:36:33]

components/ lgtm

[nyquist, 2016-05-05 17:21:40]

chrome lgtm, and bonus points for including //blimp in this :-)

One question; is there anything we can do with our build to ensure we don't
regress? Some findbugs or checkstyle trick we can use? I'm worried developers
would be looking in the Android documentation and finding PreferencesManager
when trying to get prefs.

If not, or possibly in addition, should we document this somewhere in our style
guide?

[Torne, 2016-05-05 17:28:55]

On 2016/05/05 17:21:40, nyquist wrote:
> chrome lgtm, and bonus points for including //blimp in this :-)
> 
> One question; is there anything we can do with our build to ensure we don't
> regress? Some findbugs or checkstyle trick we can use? I'm worried developers
> would be looking in the Android documentation and finding PreferencesManager
> when trying to get prefs.
> 
> If not, or possibly in addition, should we document this somewhere in our
style
> guide?

We should be enforcing this in presubmit; I think Peter plans to do this?

[Peter Wen, 2016-05-05 17:36:42]

On 2016/05/05 17:28:55, Torne wrote:
> On 2016/05/05 17:21:40, nyquist wrote:
> > chrome lgtm, and bonus points for including //blimp in this :-)
> > 
> > One question; is there anything we can do with our build to ensure we don't
> > regress? Some findbugs or checkstyle trick we can use? I'm worried
developers
> > would be looking in the Android documentation and finding PreferencesManager
> > when trying to get prefs.
> > 
> > If not, or possibly in addition, should we document this somewhere in our
> style
> > guide?
> 
> We should be enforcing this in presubmit; I think Peter plans to do this?

Yep, //blimp and others will come soon! I just wanted to get chrome/ over with
to reduce number of reverts, also paves way for internal CL. :)

Thanks for the reviews, yes, presubmit is planned!

[Peter Wen, 2016-05-05 17:39:35]

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

[commit-bot: I haz the power, 2016-05-05 17:40:16]

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

[commit-bot: I haz the power, 2016-05-05 17:44:15]

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

[commit-bot: I haz the power, 2016-05-05 17:44:16]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Wen, 2016-05-09 15:41:46]

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

[commit-bot: I haz the power, 2016-05-09 15:42:04]

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

[commit-bot: I haz the power, 2016-05-09 16:49:46]

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

[commit-bot: I haz the power, 2016-05-09 16:49:47]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-05-09 17:14:10]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2016-05-09 17:14:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, caitkp@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1874423002/#ps180001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-05-09 17:14:29]

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

[Peter Wen, 2016-05-09 17:14:30]

The CQ bit was unchecked by wnwen@chromium.org

[Peter Wen, 2016-05-09 17:40:07]

The CQ bit was checked by wnwen@chromium.org

[commit-bot: I haz the power, 2016-05-09 17:40:46]

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

[commit-bot: I haz the power, 2016-05-09 17:46:42]

Description was changed from

==========
Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284
==========

to

==========
Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284
==========

[commit-bot: I haz the power, 2016-05-09 17:46:43]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-05-09 17:48:11]

Description was changed from

==========
Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284
==========

to

==========
Migrate app shared preferences to ContextUtils.

Calls to PreferenceManager.getDefaultSharedPreferences will start to
cause disk I/O in android N if called with different context objects,
even if the context objects (activity, service, application) are all
part of Chrome, which results in the same SharedPreferences object.

Unifying these callers to use ContextUtils.getAppSharedPreferences
makes it clear that the intention is to get the application-wide
shared preferences. This will be the de facto way of accessing shared
preferences in the future.

BUG=599284

Committed: https://crrev.com/2d287cf0ce7e5393d5f2d7a826c5b285768e1192
Cr-Commit-Position: refs/heads/master@{#392355}
==========

[commit-bot: I haz the power, 2016-05-09 17:48:13]

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