Remove the context argument from all WebappRegistry/WebappDataStorage methods.

Remove the context argument from all WebappRegistry/WebappDataStorage methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry and
WebappDataStorage methods. There is only one application context within
a process, so this should not change any behaviour.

This simplifies the interfaces, and will prepare for:
 * pre-warming the SharedPreferences on startup
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being synchronous and directly callable on the main thread instead
   of needing a callback

BUG=633791
Committed: https://crrev.com/54870dc572cb26a1c92b4ac5484d64d15f51be86
Cr-Commit-Position: refs/heads/master@{#421428}

[dominickn, 2016-09-20 03:04:08]

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

[commit-bot: I haz the power, 2016-09-20 03:04:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 03:37:17]

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

[commit-bot: I haz the power, 2016-09-20 03:37:18]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-20 04:05:26]

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

[commit-bot: I haz the power, 2016-09-20 04:05:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 04:34:20]

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

[commit-bot: I haz the power, 2016-09-20 04:34:20]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-20 08:38:48]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[dominickn, 2016-09-20 08:38:49]

WDYT? I *think* this is fine, since there's only one ApplicationContext per
process. It's the initial step to making most methods in WebappRegistry and
WebappDataStorage runnable on the main thread by pre-warming the
SharedPreferences.

[dominickn, 2016-09-20 08:39:47]

Description was changed from

==========
Remove the context argument from all WebappRegistry methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry methods.
There is only one application context within a process, so this should
not change any behaviour.

This simplifies the WebappRegistry interface and will prepare for:
 * pre-warming the WebappRegistry SharedPreferences on startup, allowing
   certain WebappRegistry methods to be called on the main thread
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.

BUG=633791
==========

to

==========
Remove the context argument from all WebappRegistry methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry methods.
There is only one application context within a process, so this should
not change any behaviour.

This simplifies the WebappRegistry interface and will prepare for:
 * pre-warming the WebappRegistry SharedPreferences on startup, allowing
   certain WebappRegistry methods to be called on the main thread
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being directly callable instead of needing a callback

BUG=633791
==========

[gone, 2016-09-20 19:52:07]

At first blush it seems fine, but the right person to ping is wnwen@.  He did
changes for both the SharedPrefs and the Application Context.

[dominickn, 2016-09-20 22:17:40]

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

[dominickn, 2016-09-20 22:17:40]

+ wnwen: WDYT?

[Peter Wen, 2016-09-21 14:58:42]

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:109:
long lastUsed = new WebappDataStorage(context, webappId).getLastUsedTime();
No need to pass in context to the constructor.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:218:
private static SharedPreferences openSharedPreferences(Context context, String
webappId) {
No need for context param.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:219:
return context.getSharedPreferences(
ContextUtils.getApplicationContext().getSharedPreferences(

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:74:
SharedPreferences preferences = openSharedPreferences(context);
Same as above, openSharedPreferences doesn't need a context.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:86:
WebappDataStorage storage = new WebappDataStorage(context, webappId);
Since WebappDataStorage doesn't need context, no need for this local context
variable. Also, preference should be given to doing:

Context appContext = ContextUtils.getApplicationContext();

As it helps make the distinction clearer.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:116:
WebappDataStorage.getLastUsedTime(ContextUtils.getApplicationContext(), "test",
Don't need any of these ContextUtils calls.

[dominickn, 2016-09-21 21:19:11]

On 2016/09/21 14:58:42, Peter Wen wrote:
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
> (right):
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:109:
> long lastUsed = new WebappDataStorage(context, webappId).getLastUsedTime();
> No need to pass in context to the constructor.
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:218:
> private static SharedPreferences openSharedPreferences(Context context, String
> webappId) {
> No need for context param.
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:219:
> return context.getSharedPreferences(
> ContextUtils.getApplicationContext().getSharedPreferences(
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
> (right):
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:74:
> SharedPreferences preferences = openSharedPreferences(context);
> Same as above, openSharedPreferences doesn't need a context.
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:86:
> WebappDataStorage storage = new WebappDataStorage(context, webappId);
> Since WebappDataStorage doesn't need context, no need for this local context
> variable. Also, preference should be given to doing:
> 
> Context appContext = ContextUtils.getApplicationContext();
> 
> As it helps make the distinction clearer.
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
> File
>
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
> (right):
> 
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
>
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:116:
> WebappDataStorage.getLastUsedTime(ContextUtils.getApplicationContext(),
"test",
> Don't need any of these ContextUtils calls. thanks 

Thanks Peter. The reason I kept WebappDataStorage like this is because it's not
meant to be used (aside from tests) independently of WebappRegistry, and I want
that class calling getApplicationContext and passing it. The follow up patch
(https://codereview.chromium.org/2351113005/) makes WebappRegistry hold a member
context variable so ContextUtils is called once only, and the resulting variable
is passed to WebappDataStorage. WDYT?

[Peter Wen, 2016-09-21 23:03:24]

On 2016/09/21 21:19:11, dominickn wrote:
> On 2016/09/21 14:58:42, Peter Wen wrote:
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:109:
> > long lastUsed = new WebappDataStorage(context, webappId).getLastUsedTime();
> > No need to pass in context to the constructor.
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:218:
> > private static SharedPreferences openSharedPreferences(Context context,
String
> > webappId) {
> > No need for context param.
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:219:
> > return context.getSharedPreferences(
> > ContextUtils.getApplicationContext().getSharedPreferences(
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:74:
> > SharedPreferences preferences = openSharedPreferences(context);
> > Same as above, openSharedPreferences doesn't need a context.
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:86:
> > WebappDataStorage storage = new WebappDataStorage(context, webappId);
> > Since WebappDataStorage doesn't need context, no need for this local context
> > variable. Also, preference should be given to doing:
> > 
> > Context appContext = ContextUtils.getApplicationContext();
> > 
> > As it helps make the distinction clearer.
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
> > File
> >
>
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
> >
>
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:116:
> > WebappDataStorage.getLastUsedTime(ContextUtils.getApplicationContext(),
> "test",
> > Don't need any of these ContextUtils calls. thanks 
> 
> Thanks Peter. The reason I kept WebappDataStorage like this is because it's
not
> meant to be used (aside from tests) independently of WebappRegistry, and I
want
> that class calling getApplicationContext and passing it. The follow up patch
> (https://codereview.chromium.org/2351113005/) makes WebappRegistry hold a
member
> context variable so ContextUtils is called once only, and the resulting
variable
> is passed to WebappDataStorage. WDYT?

ContextUtils.getApplicationContext() is a very light getter, and ProGuard
probably inlines it, so no performance impact.

The reason I don't think it's necessary is because that application context is
only used by WebappDataStorage#openSharedPreferences, and that function requires
the application context. So openSharedPreferences no longer needs a parameter,
it can just call ContextUtils.getApplicationContext(), and this means all its
callers no longer need the context either. I prefer calling
ContextUtils.getApplicationContext() at the local scope as much as possible,
since relying on naming conventions and callers to ensure it's the application
context is more flaky. How does that sound?

[dominickn, 2016-09-22 03:12:34]

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

[commit-bot: I haz the power, 2016-09-22 03:12:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-09-22 03:14:51]

Description was changed from

==========
Remove the context argument from all WebappRegistry methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry methods.
There is only one application context within a process, so this should
not change any behaviour.

This simplifies the WebappRegistry interface and will prepare for:
 * pre-warming the WebappRegistry SharedPreferences on startup, allowing
   certain WebappRegistry methods to be called on the main thread
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being directly callable instead of needing a callback

BUG=633791
==========

to

==========
Remove the context argument from all WebappRegistry/WebappDataStorage methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry and
WebappDataStorage methods. There is only one application context within
a process, so this should not change any behaviour.

This simplifies the interfaces, and will prepare for:
 * pre-warming the SharedPreferences on startup
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being synchronous and directly callable on the main thread instead
   of needing a callback

BUG=633791
==========

[commit-bot: I haz the power, 2016-09-22 03:41:59]

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

[commit-bot: I haz the power, 2016-09-22 03:41:59]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-09-22 03:47:00]

Thanks for the comments, it all makes sense. PTAL. :)

One more question: just to verify, we should be accessing exactly the same
SharedPreference files after this CL as before, right?

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:109:
long lastUsed = new WebappDataStorage(context, webappId).getLastUsedTime();
On 2016/09/21 14:58:41, Peter Wen wrote:
> No need to pass in context to the constructor.

Done.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:218:
private static SharedPreferences openSharedPreferences(Context context, String
webappId) {
On 2016/09/21 14:58:41, Peter Wen wrote:
> No need for context param.

Done.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:219:
return context.getSharedPreferences(
On 2016/09/21 14:58:41, Peter Wen wrote:
> ContextUtils.getApplicationContext().getSharedPreferences(

Done.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
(right):

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:74:
SharedPreferences preferences = openSharedPreferences(context);
On 2016/09/21 14:58:41, Peter Wen wrote:
> Same as above, openSharedPreferences doesn't need a context.

Done.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java:86:
WebappDataStorage storage = new WebappDataStorage(context, webappId);
On 2016/09/21 14:58:41, Peter Wen wrote:
> Since WebappDataStorage doesn't need context, no need for this local context
> variable. Also, preference should be given to doing:
> 
> Context appContext = ContextUtils.getApplicationContext();
> 
> As it helps make the distinction clearer.

Done.

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java
(right):

https://codereview.chromium.org/2352943003/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java:116:
WebappDataStorage.getLastUsedTime(ContextUtils.getApplicationContext(), "test",
On 2016/09/21 14:58:41, Peter Wen wrote:
> Don't need any of these ContextUtils calls.

Done.

[Peter Wen, 2016-09-22 19:45:31]

lgtm

https://codereview.chromium.org/2352943003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
(right):

https://codereview.chromium.org/2352943003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:211:
return ContextUtils.getApplicationContext().getSharedPreferences(
Yes, context.getApplicationContext() is the exact same Application object as
ContextUtils.getApplicationContext().

[dominickn, 2016-09-26 23:52:55]

On 2016/09/22 19:45:31, Peter Wen wrote:
> lgtm
> 
>
https://codereview.chromium.org/2352943003/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java
> (right):
> 
>
https://codereview.chromium.org/2352943003/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java:211:
> return ContextUtils.getApplicationContext().getSharedPreferences(
> Yes, context.getApplicationContext() is the exact same Application object as
> ContextUtils.getApplicationContext().

Thanks! Waiting on dfalcantara to get back for the final review.

[gone, 2016-09-27 17:42:12]

lgtm

[dominickn, 2016-09-28 01:52:56]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-09-28 01:53:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 02:22:29]

Description was changed from

==========
Remove the context argument from all WebappRegistry/WebappDataStorage methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry and
WebappDataStorage methods. There is only one application context within
a process, so this should not change any behaviour.

This simplifies the interfaces, and will prepare for:
 * pre-warming the SharedPreferences on startup
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being synchronous and directly callable on the main thread instead
   of needing a callback

BUG=633791
==========

to

==========
Remove the context argument from all WebappRegistry/WebappDataStorage methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry and
WebappDataStorage methods. There is only one application context within
a process, so this should not change any behaviour.

This simplifies the interfaces, and will prepare for:
 * pre-warming the SharedPreferences on startup
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being synchronous and directly callable on the main thread instead
   of needing a callback

BUG=633791
==========

[commit-bot: I haz the power, 2016-09-28 02:22:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-28 02:24:03]

Description was changed from

==========
Remove the context argument from all WebappRegistry/WebappDataStorage methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry and
WebappDataStorage methods. There is only one application context within
a process, so this should not change any behaviour.

This simplifies the interfaces, and will prepare for:
 * pre-warming the SharedPreferences on startup
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being synchronous and directly callable on the main thread instead
   of needing a callback

BUG=633791
==========

to

==========
Remove the context argument from all WebappRegistry/WebappDataStorage methods.

This CL replaces the context argument with calls to
ContextUtils.getApplicationContext() within WebappRegistry and
WebappDataStorage methods. There is only one application context within
a process, so this should not change any behaviour.

This simplifies the interfaces, and will prepare for:
 * pre-warming the SharedPreferences on startup
 * calling SharedPreferences.editor.apply() exclusively on the main
   thread in all webapp code.
 * moving most WebappDataStorage and WebappRegistry methods to
   being synchronous and directly callable on the main thread instead
   of needing a callback

BUG=633791

Committed: https://crrev.com/54870dc572cb26a1c92b4ac5484d64d15f51be86
Cr-Commit-Position: refs/heads/master@{#421428}
==========

[commit-bot: I haz the power, 2016-09-28 02:24:04]

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