Only set precache.last_time when precache ran.

Only set precache.last_time when precache ran.

Modify the prefetch run to pass a precacheStarted boolean from the C++ side to
the Java side. If precacheStarted, the precache.last_time pref is set and an
alarm is created for WAIT_UNTIL_NEXT_PRECACHE_MS later. Otherwise, the pref is
not updated, and an alarm is set for INTERACTIVE_STATE_POLLING_PERIOD_MS later.

This should reduce the possibility that a user has to wait an additional 4
hours for the prefetch run, because some precondition for prefetching wasn't
yet met.

BUG=309216
Committed: https://crrev.com/0100d1eef69974b4826959acf0f82cfe48f9e9e2
Cr-Commit-Position: refs/heads/master@{#342833}

[twifkak, 2015-08-04 03:59:39]

twifkak@chromium.org changed reviewers:
+ bengr@chromium.org, yfriedman@chromium.org

[twifkak, 2015-08-04 03:59:39]

bengr@chromium.org: Please review all changes. (Approval needed for
components/precache/ specifically.)

yfriedman@chromium.org: Please review changes in chrome/android/ and
chrome/browser/android/.

This CL depends on https://codereview.chromium.org/1266243003.

[bengr, 2015-08-05 23:25:35]

I started commenting. See below.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:50:
protected abstract void onPrecacheCompleted(boolean precacheStarted);
Describe the parameter in javadoc.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:57:
private void onPrecacheCompletedCallback(boolean precacheStarted) {
Why is this precacheStarted and not precacheCompleted? Add javadoc.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:83:
PrecacheServiceLauncher.setIsPrecachingEnabled(context.getApplicationContext(),
Please update comment to explain why getApplicationContext() is needed.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java:82:
protected void onPrecacheCompleted(boolean precacheStarted) {
Why would onPrecachCompleted be called if precacheStarted is false?

https://codereview.chromium.org/1272443002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheServiceLauncherTest.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheServiceLauncherTest.java:105:
launcher.checkExpectations(false, false, true);
What changed?

[twifkak, 2015-08-06 00:09:46]

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:50:
protected abstract void onPrecacheCompleted(boolean precacheStarted);
On 2015/08/05 23:25:35, bengr wrote:
> Describe the parameter in javadoc.

Done.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:57:
private void onPrecacheCompletedCallback(boolean precacheStarted) {
On 2015/08/05 23:25:35, bengr wrote:
> Why is this precacheStarted and not precacheCompleted? Add javadoc.

Done.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:83:
PrecacheServiceLauncher.setIsPrecachingEnabled(context.getApplicationContext(),
On 2015/08/05 23:25:35, bengr wrote:
> Please update comment to explain why getApplicationContext() is needed.

Added comment to PrecacheServiceLauncher.

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java:82:
protected void onPrecacheCompleted(boolean precacheStarted) {
On 2015/08/05 23:25:35, bengr wrote:
> Why would onPrecachCompleted be called if precacheStarted is false?

There are two cases:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...

This should be obvious: precache_manager is not there for some reason.

https://code.google.com/p/chromium/codesearch#chromium/src/components/precach...

The latter is a little less obvious, but you can see at line 102 that
is_precaching_ is set only if ShouldRun(). So not having sync tabs or not being
in the field trial would cause this to call onPrecacheCompleted(false).

Is it okay that this runs (and does nothing) every 15 minutes for people not in
the field trial or for people opted out of sync?

https://codereview.chromium.org/1272443002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheServiceLauncherTest.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheServiceLauncherTest.java:105:
launcher.checkExpectations(false, false, true);
On 2015/08/05 23:25:35, bengr wrote:
> What changed?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
was moved to
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav....

[bengr, 2015-08-07 18:41:57]

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java:82:
protected void onPrecacheCompleted(boolean precacheStarted) {
On 2015/08/06 00:09:46, twifkak wrote:
> On 2015/08/05 23:25:35, bengr wrote:
> > Why would onPrecachCompleted be called if precacheStarted is false?
> 
> There are two cases:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
> 
> This should be obvious: precache_manager is not there for some reason.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/precach...
> 
> The latter is a little less obvious, but you can see at line 102 that
> is_precaching_ is set only if ShouldRun(). So not having sync tabs or not
being
> in the field trial would cause this to call onPrecacheCompleted(false).
> 
> Is it okay that this runs (and does nothing) every 15 minutes for people not
in
> the field trial or for people opted out of sync?

It's not ideal. What would it take to prevent it?

[twifkak, 2015-08-07 21:48:15]

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java
(right):

https://codereview.chromium.org/1272443002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java:82:
protected void onPrecacheCompleted(boolean precacheStarted) {
On 2015/08/07 18:41:57, bengr wrote:
> On 2015/08/06 00:09:46, twifkak wrote:
> > On 2015/08/05 23:25:35, bengr wrote:
> > > Why would onPrecachCompleted be called if precacheStarted is false?
> > 
> > There are two cases:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
> > 
> > This should be obvious: precache_manager is not there for some reason.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/precach...
> > 
> > The latter is a little less obvious, but you can see at line 102 that
> > is_precaching_ is set only if ShouldRun(). So not having sync tabs or not
> being
> > in the field trial would cause this to call onPrecacheCompleted(false).
> > 
> > Is it okay that this runs (and does nothing) every 15 minutes for people not
> in
> > the field trial or for people opted out of sync?
> 
> It's not ideal. What would it take to prevent it?

Done. I renamed the parameter since it's a misnomer now.

[bengr, 2015-08-10 21:48:34]

lgtm

[twifkak, 2015-08-11 01:18:28]

Yaron: Ping.

[Yaron, 2015-08-11 13:59:29]

lgtm

https://codereview.chromium.org/1272443002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheServiceLauncher.java
(right):

https://codereview.chromium.org/1272443002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheServiceLauncher.java:66:
// |context| needs to be the application context so that the following call gets
a
It's also generally preferred in non-ui code to avoid leaking an activity.

[twifkak, 2015-08-11 16:25:34]

The CQ bit was checked by twifkak@chromium.org

[commit-bot: I haz the power, 2015-08-11 16:25:50]

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

[commit-bot: I haz the power, 2015-08-11 17:13:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-11 17:14:34]

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