Tweaks to the precache triggering code.

Tweaks to the precache triggering code.

This commit encompasses a few related changes:

1. Enforce that IsPrecachingAllowed is called before IsPrecachingEnabled. This
is so that users are only selected into the field trial if it would have an
effect on them. Calling IsPrecachingEnabled triggers the field trial group
selection, and doing so for users for whom !IsPrecachingAllowed means that our
Enabled population consists of many users for whom precaching won't ultimately
run.

2. Renamed the externally facing methods to ShouldRun and WouldRun, so I could
lean on the compiler to ensure that I addressed all clients, and because
PrecacheManager and PrecacheLauncher were exposing two different notions of
"enabled", which I found confusing.

3. Make updatePrecachingEnabled wait, asynchronously, for the sync backend to
be initialized, before determining what value to pass to setIsPrecachingEnabled.
Previously, DeferredStartupHandler would call
PrecacheLauncher.updatePrecachingEnabled before the sync backend was enabled,
so the is_precaching_enabled pref would be set to false until either
PrivacyPreferences or ConnectionChangeReceiver called updatePrecachingEnabled.
This change should increase the likelihood that precaching will run when the
conditions are right, and decrease the time until that happens.

4. Get rid of the redundant PrivacyPreferencesManager parameter, to simplify
the interface and thus make the implementation more flexible.

5. Change from android.util.Log to org.chromium.base.Log, per presubmit
warning.

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

[twifkak, 2015-08-01 05:20:04]

twifkak@chromium.org changed reviewers:
+ battre@chromium.org, bengr@chromium.org, miguelg@chromium.org

[twifkak, 2015-08-01 05:20:05]

battre@chromium.org: Please review changes in chrome/browser/net.

bengr@chromium.org: Please review changes in *precache*. (Your approval is
needed for components/precache in particular.)

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

[twifkak, 2015-08-01 05:39:13]

twifkak@chromium.org changed reviewers:
+ zea@chromium.org

[twifkak, 2015-08-01 05:44:37]

+zea: This change affects a client of the sync service, so you might be
interested or helpful here. (You may know me from such earlier CLs as
https://codereview.chromium.org/1176253002/.) In particular, look at
PrecacheLauncher.java and PrecacheLauncherTest.java for the sync-related code.
Thanks!

[battre, 2015-08-03 09:43:42]

chrome/browser/net LGTM

[Nicolas Zea, 2015-08-03 18:14:07]

sync usage LGTM

[twifkak, 2015-08-03 22:27:33]

miguelg + bengr: I made a tweak to PrecacheLauncher::Start to call
OnPrecacheComplete on early return (as it does pre- this CL). This is necessary
to release the wakelock.

[twifkak, 2015-08-04 03:54:28]

twifkak@chromium.org changed reviewers:
+ yfriedman@chromium.org
- miguelg@chromium.org

[twifkak, 2015-08-04 03:54:29]

-miguelg, +yfriedman for chrome/android and chrome/browser/android. Originally,
I just picked the person with the highest Review Score, but it appears Yaron
does more reviews for this section of code. Apologies if I got that wrong.

[twifkak, 2015-08-04 05:17:43]

Also, any clues on the linux_android_rel_ng error? This seems completely
unrelated to my change, but also seems consistent (it occurs for my other CL
https://codereview.chromium.org/1272443002/, but other builds at the same base
revision are passing, e.g.
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...).

The error seems to be a difference in the non-obfuscated and obfuscated versions
of ThreadUtils:

$ diff <(javap -classpath out/Release/lib.java/base_java.jar
org.chromium.base.ThreadUtils) <(javap -classpath
out/Release/chrome_public_apk/obfuscated.jar org.chromium.base.ThreadUtils)
5d4
<   public static void setWillOverrideUiThread();
8,11c7,8
<   public static <T extends java/lang/Object> T
runOnUiThreadBlockingNoException(java.util.concurrent.Callable<T>);
<   public static <T extends java/lang/Object> T
runOnUiThreadBlocking(java.util.concurrent.Callable<T>) throws
java.util.concurrent.ExecutionException;
<   public static <T extends java/lang/Object>
java.util.concurrent.FutureTask<T>
runOnUiThread(java.util.concurrent.FutureTask<T>);
<   public static <T extends java/lang/Object>
java.util.concurrent.FutureTask<T>
runOnUiThread(java.util.concurrent.Callable<T>);
---
>   public static java.util.concurrent.FutureTask
runOnUiThread(java.util.concurrent.FutureTask);
>   public static java.util.concurrent.FutureTask
runOnUiThread(java.util.concurrent.Callable);
13c10
<   public static <T extends java/lang/Object>
java.util.concurrent.FutureTask<T>
postOnUiThread(java.util.concurrent.FutureTask<T>);
---
>   public static java.util.concurrent.FutureTask
postOnUiThread(java.util.concurrent.FutureTask);

This error occurs when trying to build chrome_public_test_apk.

[twifkak, 2015-08-04 05:20:49]

On 2015/08/04 05:17:43, twifkak wrote:
> <   public static void setWillOverrideUiThread();
> <   public static <T extends java/lang/Object> T
> runOnUiThreadBlockingNoException(java.util.concurrent.Callable<T>);
> <   public static <T extends java/lang/Object> T
> runOnUiThreadBlocking(java.util.concurrent.Callable<T>) throws

These are the relevant lines from the diff. The others are just generics being
erased.

[Yaron, 2015-08-04 21:07:13]

https://codereview.chromium.org/1266243003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
(right):

https://codereview.chromium.org/1266243003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:79:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
I think you need to add a @VisibleForTesting to the declaration of this so that
proguard doesn't strip it in release.

[twifkak, 2015-08-04 23:17:15]

https://codereview.chromium.org/1266243003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
(right):

https://codereview.chromium.org/1266243003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:79:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
On 2015/08/04 21:07:13, Yaron wrote:
> I think you need to add a @VisibleForTesting to the declaration of this so
that
> proguard doesn't strip it in release.

Thanks. This fixed it. That's annoying that you can break a bunch of unrelated
tests just by removing the only production call to a method. (The call in
PrecacheServiceLauncher:68 going away is what triggered this;
PrecacheLauncherTest is just one of the many tests that failed as a result of
ProGuard.)

[Yaron, 2015-08-05 15:57:04]

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

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:62:
* Updates the PrecacheServiceLauncher with whether conditions are right for
precaching. All of
PrecacheLauncher

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:99:
public void run() {
@Override

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:105:
if (sync.isSyncInitialized()) {
sync can't get uninitialized, right? just disabled?

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:131:
private static final PrecacheLauncher sInstance = new PrecacheLauncher() {
I'm confused. Why do you have this no-op PrecacheLauncher? This adds a bit of
overhead but more importantly - where is sInstance ever set?

[twifkak, 2015-08-05 17:44:23]

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

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:62:
* Updates the PrecacheServiceLauncher with whether conditions are right for
precaching. All of
On 2015/08/05 15:57:04, Yaron wrote:
> PrecacheLauncher

No, it calls PrecacheServiceLauncher.setIsPrecachingEnabled().
PrecacheServiceLauncher is actually the launching point for the precache
component, and thus the one that keeps track of the enabled pref. This method
lives in PrecacheLauncher only to call nativeShouldRun().

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:99:
public void run() {
On 2015/08/05 15:57:04, Yaron wrote:
> @Override

Done.

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:105:
if (sync.isSyncInitialized()) {
On 2015/08/05 15:57:04, Yaron wrote:
> sync can't get uninitialized, right? just disabled?

I'm not sure if it can get uninitialized, but it starts off uninitialized when
the browser process is first launched, i.e. when updatePrecachingEnabled is
called by DeferredStartupHandler. This is the issue I'm trying to solve with
this listener. (#3 in the description.)

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:131:
private static final PrecacheLauncher sInstance = new PrecacheLauncher() {
On 2015/08/05 15:57:04, Yaron wrote:
> I'm confused. Why do you have this no-op PrecacheLauncher? This adds a bit of
> overhead but more importantly - where is sInstance ever set?

sInstance is set right on this line. It's a private singleton instance of
PrecacheLauncher so that I can override nativeShouldRun in test.

[Yaron, 2015-08-05 21:43:47]

lgtm with a possible suggestion for improvement

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

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:62:
* Updates the PrecacheServiceLauncher with whether conditions are right for
precaching. All of
On 2015/08/05 17:44:22, twifkak wrote:
> On 2015/08/05 15:57:04, Yaron wrote:
> > PrecacheLauncher
> 
> No, it calls PrecacheServiceLauncher.setIsPrecachingEnabled().
> PrecacheServiceLauncher is actually the launching point for the precache
> component, and thus the one that keeps track of the enabled pref. This method
> lives in PrecacheLauncher only to call nativeShouldRun().

ah, got it, thanks

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:131:
private static final PrecacheLauncher sInstance = new PrecacheLauncher() {
On 2015/08/05 17:44:22, twifkak wrote:
> On 2015/08/05 15:57:04, Yaron wrote:
> > I'm confused. Why do you have this no-op PrecacheLauncher? This adds a bit
of
> > overhead but more importantly - where is sInstance ever set?
> 
> sInstance is set right on this line. It's a private singleton instance of
> PrecacheLauncher so that I can override nativeShouldRun in test.

Right, of course it's set here. :) My confusion was b/c it's a little strange to
have a singleton like this and also to have the only overridden (abstract)
function be an empty function. It might be cleaner if this whole thing wasn't
static. It looks like the staticness comes in from updatePrecachingEnabled,
perhaps it can be moved out?

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
(right):

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:25:
private class PrecacheLauncherUnderTest extends PrecacheLauncher {
nit: make static and move member field to top of class

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:154:
private Context getContext() {
Nit: rename getTargetContext. since instrumentation has a context and
targetContext, this initially threw me off cause it's intentionally not
returning context

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:159:
private void waitUntilUiThreadIdle() {
Typically use insrumentation.waitForIdleSync() for this.

[twifkak, 2015-08-05 22:31:30]

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

https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:131:
private static final PrecacheLauncher sInstance = new PrecacheLauncher() {
On 2015/08/05 21:43:47, Yaron wrote:
> On 2015/08/05 17:44:22, twifkak wrote:
> > On 2015/08/05 15:57:04, Yaron wrote:
> > > I'm confused. Why do you have this no-op PrecacheLauncher? This adds a bit
> of
> > > overhead but more importantly - where is sInstance ever set?
> > 
> > sInstance is set right on this line. It's a private singleton instance of
> > PrecacheLauncher so that I can override nativeShouldRun in test.
> 
> Right, of course it's set here. :) My confusion was b/c it's a little strange
to
> have a singleton like this and also to have the only overridden (abstract)
> function be an empty function. It might be cleaner if this whole thing wasn't
> static. It looks like the staticness comes in from updatePrecachingEnabled,
> perhaps it can be moved out?

Ah, yes, this is largely a design in transition. The abstract
onPrecacheCompleted is used as a listener mechanism -- PrecacheServiceLauncher
instantiates an anonymous inner subclass of PrecacheLauncher, providing a
definition that calls into its enclosing scope. Here I provide an empty body
because I'm forced to provide one, and the contents of onPrecacheCompleted are
not relevant to my interests.

The typical means of establishing a listener is with an interface. Doing so
would eliminate the need to subclass PrecacheLauncher and the need for any
abstract methods. I would like to make that change, but perhaps it would be
better in a future CL. For now, this is a "surgical" approach that changes as
little as possible.

As for the static keyword, I think that's here to stay. Static seems like the
right API given its clients, short of having a dependency injector in chromium.
I could move updatePrecachingEnabled (and the PrecacheLauncher instance) into
PrecacheServiceLauncher. That seems like a good idea, but I'd prefer to delay
that to a future cleanup CL.

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
(right):

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:25:
private class PrecacheLauncherUnderTest extends PrecacheLauncher {
On 2015/08/05 21:43:47, Yaron wrote:
> nit: make static and move member field to top of class

Half-done. Non-static is needed for notifySyncChanged().

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:154:
private Context getContext() {
On 2015/08/05 21:43:47, Yaron wrote:
> Nit: rename getTargetContext. since instrumentation has a context and
> targetContext, this initially threw me off cause it's intentionally not
> returning context

Done.

https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:159:
private void waitUntilUiThreadIdle() {
On 2015/08/05 21:43:47, Yaron wrote:
> Typically use insrumentation.waitForIdleSync() for this.

Ah, thanks! I was looking for this.

[bengr, 2015-08-06 00:01:01]

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

https://codereview.chromium.org/1266243003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java:33:
private static final String TAG = "cr.Precache";
Why are this and the import needed?

https://codereview.chromium.org/1266243003/diff/80001/chrome/browser/android/...
File chrome/browser/android/precache/precache_launcher.cc (right):

https://codereview.chromium.org/1266243003/diff/80001/chrome/browser/android/...
chrome/browser/android/precache/precache_launcher.cc:65:
history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
nit: No need to get the hs if precache_manager is null.

https://codereview.chromium.org/1266243003/diff/80001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1266243003/diff/80001/components/precache/con...
components/precache/content/precache_manager.cc:103: if (ShouldRun()) {
slightly prefer:

if (!ShouldRun()) {
  OnDone();
  return;
}

...

[twifkak, 2015-08-06 00:18:17]

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

https://codereview.chromium.org/1266243003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheService.java:33:
private static final String TAG = "cr.Precache";
On 2015/08/06 00:01:01, bengr wrote:
> Why are this and the import needed?

#5 in the CL description.

https://codereview.chromium.org/1266243003/diff/80001/chrome/browser/android/...
File chrome/browser/android/precache/precache_launcher.cc (right):

https://codereview.chromium.org/1266243003/diff/80001/chrome/browser/android/...
chrome/browser/android/precache/precache_launcher.cc:65:
history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
On 2015/08/06 00:01:01, bengr wrote:
> nit: No need to get the hs if precache_manager is null.

A future CL (not yet sent) moves the hs out of here entirely, so I'm not
inclined to worry about it.

https://codereview.chromium.org/1266243003/diff/80001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1266243003/diff/80001/components/precache/con...
components/precache/content/precache_manager.cc:103: if (ShouldRun()) {
On 2015/08/06 00:01:01, bengr wrote:
> slightly prefer:
> 
> if (!ShouldRun()) {
>   OnDone();
>   return;
> }
> 
> ...

I strongly prefer not to, actually. Early returns are tricky when there's
cleanup to do (as in this case). Personally, it's easier to wrap my head around
the necessary invariant in this format (e.g. that OnDone() should be called in
all branches). I realize I'm a minority in this regard, so if your preference is
more than slight, I'm flexible.

[Yaron, 2015-08-06 15:17:36]

On 2015/08/05 22:31:30, twifkak wrote:
>
https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java
> (right):
> 
>
https://codereview.chromium.org/1266243003/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheLauncher.java:131:
> private static final PrecacheLauncher sInstance = new PrecacheLauncher() {
> On 2015/08/05 21:43:47, Yaron wrote:
> > On 2015/08/05 17:44:22, twifkak wrote:
> > > On 2015/08/05 15:57:04, Yaron wrote:
> > > > I'm confused. Why do you have this no-op PrecacheLauncher? This adds a
bit
> > of
> > > > overhead but more importantly - where is sInstance ever set?
> > > 
> > > sInstance is set right on this line. It's a private singleton instance of
> > > PrecacheLauncher so that I can override nativeShouldRun in test.
> > 
> > Right, of course it's set here. :) My confusion was b/c it's a little
strange
> to
> > have a singleton like this and also to have the only overridden (abstract)
> > function be an empty function. It might be cleaner if this whole thing
wasn't
> > static. It looks like the staticness comes in from updatePrecachingEnabled,
> > perhaps it can be moved out?
> 
> Ah, yes, this is largely a design in transition. The abstract
> onPrecacheCompleted is used as a listener mechanism -- PrecacheServiceLauncher
> instantiates an anonymous inner subclass of PrecacheLauncher, providing a
> definition that calls into its enclosing scope. Here I provide an empty body
> because I'm forced to provide one, and the contents of onPrecacheCompleted are
> not relevant to my interests.
> 
> The typical means of establishing a listener is with an interface. Doing so
> would eliminate the need to subclass PrecacheLauncher and the need for any
> abstract methods. I would like to make that change, but perhaps it would be
> better in a future CL. For now, this is a "surgical" approach that changes as
> little as possible.
> 
> As for the static keyword, I think that's here to stay. Static seems like the
> right API given its clients, short of having a dependency injector in
chromium.
> I could move updatePrecachingEnabled (and the PrecacheLauncher instance) into
> PrecacheServiceLauncher. That seems like a good idea, but I'd prefer to delay
> that to a future cleanup CL.

Both sgtm and I'm fine with that in a follow-up
> 
>
https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java
> (right):
> 
>
https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:25:
> private class PrecacheLauncherUnderTest extends PrecacheLauncher {
> On 2015/08/05 21:43:47, Yaron wrote:
> > nit: make static and move member field to top of class
> 
> Half-done. Non-static is needed for notifySyncChanged().
> 
>
https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:154:
> private Context getContext() {
> On 2015/08/05 21:43:47, Yaron wrote:
> > Nit: rename getTargetContext. since instrumentation has a context and
> > targetContext, this initially threw me off cause it's intentionally not
> > returning context
> 
> Done.
> 
>
https://codereview.chromium.org/1266243003/diff/60001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/precache/PrecacheLauncherTest.java:159:
> private void waitUntilUiThreadIdle() {
> On 2015/08/05 21:43:47, Yaron wrote:
> > Typically use insrumentation.waitForIdleSync() for this.
> 
> Ah, thanks! I was looking for this.

[bengr, 2015-08-07 17:21:46]

components/precache lgtm with nits.

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
components/precache/content/precache_manager.cc:103: if (ShouldRun()) {
Maybe do:

if (!ShouldRun()) {
  OnDone();
  return;
}

BrowserThread::PostTask(...

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
components/precache/content/precache_manager.h:66: virtual bool ShouldRun()
const;
I'm not a huge fan of these names, but am ok with the change.

[twifkak, 2015-08-07 17:57:08]

Thanks for the lg.

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
components/precache/content/precache_manager.cc:103: if (ShouldRun()) {
On 2015/08/07 17:21:45, bengr wrote:
> Maybe do:
> 
> if (!ShouldRun()) {
>   OnDone();
>   return;
> }
> 
> BrowserThread::PostTask(...

I'm not a fan of it (see
https://codereview.chromium.org/1266243003/diff/80001/components/precache/con...).
If you feel strongly, say so. Note that this if-else clause will become an
if-elseif-else clause when I introduce a Control group in a bit.

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/1266243003/diff/100001/components/precache/co...
components/precache/content/precache_manager.h:66: virtual bool ShouldRun()
const;
On 2015/08/07 17:21:45, bengr wrote:
> I'm not a huge fan of these names, but am ok with the change.

I'm not a huge fan of them either. When I introduce the control group, I'm
thinking of renaming them to InExperimentGroup and InControlGroup. (Sorry for
the naming churn. I can't do it on this CL because "control group" is a bit of a
misnomer - gives the wrong impression that it only happens for a field trial
group.)

[twifkak, 2015-08-07 17:57:53]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2015-08-07 17:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
battre@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1266243003/#ps100001
(title: "Rebase.")

[twifkak, 2015-08-07 17:58:03]

The CQ bit was checked by twifkak@chromium.org

[commit-bot: I haz the power, 2015-08-07 17:58:49]

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

[commit-bot: I haz the power, 2015-08-07 19:18:10]

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

[commit-bot: I haz the power, 2015-08-07 19:18:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[twifkak, 2015-08-07 19:19:15]

On 2015/08/07 19:18:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Nvidia test is flake. Trying again.

[twifkak, 2015-08-07 19:19:20]

The CQ bit was checked by twifkak@chromium.org

[commit-bot: I haz the power, 2015-08-07 19:20:10]

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

[commit-bot: I haz the power, 2015-08-07 20:19:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-07 20:19:54]

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