Precache uses GcmNetworkManager for task scheduling

Precache uses GcmNetworkManager for task scheduling

Precaching now uses GcmNetworkManager to be notified of good
times to precache, but still uses it's own logic to determine
when conditions have turned bad during a precache.

In addition, this change renames PrecacheService as
PrecacheController, and makes it a singleton.

BUG=542862
Committed: https://crrev.com/eb63156da4337e66b5c4df3ef54b62ea4d4ad4be
Cr-Commit-Position: refs/heads/master@{#383973}

[bengr, 2016-03-01 22:15:51]

Patchset #1 (id:1) has been deleted

[bengr, 2016-03-01 22:16:03]

Patchset #1 (id:20001) has been deleted

[bengr, 2016-03-01 22:19:26]

bengr@chromium.org changed reviewers:
+ nyquist@chromium.org, sclittle@chromium.org

[bengr, 2016-03-01 22:19:26]

sclittle: components/precache
nyquist: the rest

[sclittle, 2016-03-10 00:56:25]

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheBootReceiver.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheBootReceiver.java:14:
* Receives a boot signal to reschedule precaching.
Is a boot signal enough to ensure that precaching is scheduled? What if Chrome
is upgraded? IIRC the old broadcast receiver setup (i.e. the removed
PrecacheServiceLauncher) would get notified whenever the network changes or
charging status changes - maybe this BootReceiver be hooked up to those as well?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:73:
private static PrecacheController sInstance;
Why does this need to be a singleton? Could you document that somewhere?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:154:
/** True if a precache session is in progress. */
Could you mention here that this is protected by a lock?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:161:
private Context mContext;
Having a static object hold on to the application context looks like it would
prevent a great deal of objects from being garbage collected. Is there some way
that the context could just be passed through in arguments, or some way that the
PrecacheController doesn't have to be a static object?

https://codereview.chromium.org/1751183002/diff/40001/components/precache/and...
File
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/components/precache/and...
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java:63:
public boolean isWifiAvailable(Context context) {
Is this method necessary anymore?

[nyquist, 2016-03-10 19:30:36]

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:52:
handlePrecache(context, tag);
I don't understand how this compiles. Where does |tag| come from? Did you mean
params.getTag() ?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:166:
mContext = context;
I believe you specifically want this to be the application context, so just
store it as: mContext = context.getApplicationContext();

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:205:
isPrecaching = mIsPrecaching;
Can you just return mIsPrecaching inside this synchronized-block?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:233:
private final BroadcastReceiver mDeviceStateReceiver = new BroadcastReceiver() {
These object members seems very misplaced. Move to top?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:291:
// TODO(bengr): Determine if we need an onStartCommand that returns
START_STICKY.
START_STICKY for what service?

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:359:
/** Acquires the precaching {@link WakeLock}. */
Can you add a comment about which thread this must be called on? Same below with
the release-call. Just to make it clear for new readers that you thought about
concurrency when you wrote this and so they don't have to necessarily check the
code for correctness, etc.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/MockPrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/MockPrecacheController.java:24:
acquiredLockCnt++;
Nit: Do we use ++acquiredLockCnt in Java too?

[bengr, 2016-03-17 00:46:14]

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:52:
handlePrecache(context, tag);
On 2016/03/10 19:30:36, nyquist wrote:
> I don't understand how this compiles. Where does |tag| come from? Did you mean
> params.getTag() ?

Ha, sorry it doesn't of course. I made a last minute change before uploading and
forgot to fix that up. Sorry. Done.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheBootReceiver.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheBootReceiver.java:14:
* Receives a boot signal to reschedule precaching.
On 2016/03/10 00:56:25, sclittle wrote:
> Is a boot signal enough to ensure that precaching is scheduled? What if Chrome
> is upgraded? IIRC the old broadcast receiver setup (i.e. the removed
> PrecacheServiceLauncher) would get notified whenever the network changes or
> charging status changes - maybe this BootReceiver be hooked up to those as
well?

I'm not entirely sure what you're suggesting, but handling upgrades is certainly
an issue, so I added code for that. GMS should hold onto a scheduled task
indefinitely, except for on upgrades, so theoretically speaking it is fine to
schedule the periodic task just once. I didn't trust it, so I reschedule on each
boot. If I did trust it, I would probably still just reschedule on boot because
otherwise I'd have to schedule somewhere in Chrome startup logic and set some
pref to remember that I scheduled, which seems messy.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:73:
private static PrecacheController sInstance;
On 2016/03/10 00:56:25, sclittle wrote:
> Why does this need to be a singleton? Could you document that somewhere?

Done.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:154:
/** True if a precache session is in progress. */
On 2016/03/10 00:56:25, sclittle wrote:
> Could you mention here that this is protected by a lock?

Done.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:161:
private Context mContext;
On 2016/03/10 00:56:25, sclittle wrote:
> Having a static object hold on to the application context looks like it would
> prevent a great deal of objects from being garbage collected. Is there some
way
> that the context could just be passed through in arguments, or some way that
the
> PrecacheController doesn't have to be a static object?

We hold onto the application context, which is around for the lifetime of the
application, so it isn't preventing anything from being garbage collected.
Passing context around is more error prone, because we should always be using
the application context and might get some other context (and would need to call
getApplicationContext() everywhere.)

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:166:
mContext = context;
On 2016/03/10 19:30:36, nyquist wrote:
> I believe you specifically want this to be the application context, so just
> store it as: mContext = context.getApplicationContext();

That's what the singleton getter does.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:205:
isPrecaching = mIsPrecaching;
On 2016/03/10 19:30:36, nyquist wrote:
> Can you just return mIsPrecaching inside this synchronized-block?

Java is awesome. Done.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:233:
private final BroadcastReceiver mDeviceStateReceiver = new BroadcastReceiver() {
On 2016/03/10 19:30:36, nyquist wrote:
> These object members seems very misplaced. Move to top?

The style guide doesn't require an ordering and suggests only that we be
consistent, but I imposed an ordering anyway. Done.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:291:
// TODO(bengr): Determine if we need an onStartCommand that returns
START_STICKY.
On 2016/03/10 19:30:36, nyquist wrote:
> START_STICKY for what service?

Legacy comment. Removed.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:359:
/** Acquires the precaching {@link WakeLock}. */
On 2016/03/10 19:30:36, nyquist wrote:
> Can you add a comment about which thread this must be called on? Same below
with
> the release-call. Just to make it clear for new readers that you thought about
> concurrency when you wrote this and so they don't have to necessarily check
the
> code for correctness, etc.

Done.

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/MockPrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/MockPrecacheController.java:24:
acquiredLockCnt++;
On 2016/03/10 19:30:36, nyquist - OOO back 3-29 wrote:
> Nit: Do we use ++acquiredLockCnt in Java too?

We don't typically. Loops use pre-increment more often than not in Chromium, but
not other counters. I think the bytecode is identical either way.

https://codereview.chromium.org/1751183002/diff/40001/components/precache/and...
File
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/components/precache/and...
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java:63:
public boolean isWifiAvailable(Context context) {
On 2016/03/10 00:56:25, sclittle wrote:
> Is this method necessary anymore?

Done.

[nyquist, 2016-03-17 05:04:43]

lgtm

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:166:
mContext = context;
On 2016/03/17 00:46:13, bengr wrote:
> On 2016/03/10 19:30:36, nyquist wrote:
> > I believe you specifically want this to be the application context, so just
> > store it as: mContext = context.getApplicationContext();
> 
> That's what the singleton getter does.

Right, but this constructor is package protected, which means it technically
could be invoked directly. Anyway, you moved it here now.

https://codereview.chromium.org/1751183002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:88:
private Context mContext;
Nit: Rename to mAppContext for clarity.

https://codereview.chromium.org/1751183002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:191:
public static void rescheduleTasksOnUpgrade(final Context context) {
Nit: Unnecessary final.

[bengr, 2016-03-17 22:34:14]

https://codereview.chromium.org/1751183002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/1751183002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:88:
private Context mContext;
On 2016/03/17 05:04:43, nyquist - OOO back 3-29 wrote:
> Nit: Rename to mAppContext for clarity.

Done.

https://codereview.chromium.org/1751183002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:191:
public static void rescheduleTasksOnUpgrade(final Context context) {
On 2016/03/17 05:04:43, nyquist - OOO back 3-29 wrote:
> Nit: Unnecessary final.

Done.

[sclittle, 2016-03-18 19:52:25]

lgtm

[bengr, 2016-03-22 18:02:55]

Patchset #4 (id:100001) has been deleted

[bengr, 2016-03-22 18:31:52]

Patchset #4 (id:120001) has been deleted

[bengr, 2016-03-22 19:00:42]

Patchset #4 (id:140001) has been deleted

[bengr, 2016-03-22 21:18:08]

Patchset #4 (id:160001) has been deleted

[bengr, 2016-03-23 20:00:43]

Patchset #4 (id:180001) has been deleted

[bengr, 2016-03-23 20:41:22]

The CQ bit was checked by bengr@chromium.org

[bengr, 2016-03-23 20:41:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1751183002/#ps200001
(title: "updated thread safety")

[commit-bot: I haz the power, 2016-03-23 20:41:37]

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

[commit-bot: I haz the power, 2016-03-23 20:57:52]

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

[commit-bot: I haz the power, 2016-03-23 20:57:53]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[bengr, 2016-03-25 17:45:07]

Patchset #5 (id:220001) has been deleted

[bengr, 2016-03-25 17:45:26]

Patchset #4 (id:200001) has been deleted

[bengr, 2016-03-30 02:24:05]

Patchset #7 (id:300001) has been deleted

[bengr, 2016-03-30 02:24:19]

Patchset #6 (id:280001) has been deleted

[bengr, 2016-03-30 02:24:39]

Patchset #5 (id:260001) has been deleted

[bengr, 2016-03-30 02:25:01]

Patchset #4 (id:240001) has been deleted

[bengr, 2016-03-30 02:25:37]

Patchset #3 (id:80001) has been deleted

[bengr, 2016-03-30 03:39:50]

The CQ bit was checked by bengr@chromium.org

[bengr, 2016-03-30 03:39:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1751183002/#ps320001
(title: "Addressed comments and fixed tests")

[commit-bot: I haz the power, 2016-03-30 03:40:25]

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

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

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

[commit-bot: I haz the power, 2016-03-30 09:41:31]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[bengr, 2016-03-30 13:32:27]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2016-03-30 13:32:46]

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

[commit-bot: I haz the power, 2016-03-30 14:19:36]

Committed patchset #3 (id:320001)

[commit-bot: I haz the power, 2016-03-30 14:20:55]

Description was changed from

==========
Precache uses GcmNetworkManager for task scheduling

Precaching now uses GcmNetworkManager to be notified of good
times to precache, but still uses it's own logic to determine
when conditions have turned bad during a precache.

In addition, this change renames PrecacheService as
PrecacheController, and makes it a singleton.

BUG=542862
==========

to

==========
Precache uses GcmNetworkManager for task scheduling

Precaching now uses GcmNetworkManager to be notified of good
times to precache, but still uses it's own logic to determine
when conditions have turned bad during a precache.

In addition, this change renames PrecacheService as
PrecacheController, and makes it a singleton.

BUG=542862

Committed: https://crrev.com/eb63156da4337e66b5c4df3ef54b62ea4d4ad4be
Cr-Commit-Position: refs/heads/master@{#383973}
==========

[commit-bot: I haz the power, 2016-03-30 14:20:56]

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