Provide a way to specify in Finch trials to disable prerender local

Provide a way to specify in Finch trials to disable prerender local
predictor, based on the following two options:
- not on cellular
- predictive actions are disabled for the current network selection
This allows to only gather stats for a the subset of people we are interested
in (eg only people on cellular, or only people who have prefetch enabled for
the current network condition)
R=jkarlin@chromium.org
TBR=bnc@chromium.org, zhenw@chromium.org

Committed: https://crrev.com/d5cb0cb06c1c7d735b330c6fb4f42914e9f522ee
Cr-Commit-Position: refs/heads/master@{#296730}

[tburkard, 2014-09-23 14:19:28]

Hi,

could you please review:
zhenw: Please take a look at logic
jkarlin: overall (since you are an OWNER)
bnc: please review the preference/network state logic as dicussed.

Thanks.

[jkarlin, 2014-09-23 14:54:03]

Please add unit tests to verify that finch will behave the expected way under
various network circumstances.  We have similar tests in prefetch_browsertest.cc

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:339: bool
DisableLocalPredictorBasedOnSyncAndConfiguration(Profile* profile) {
This should also be "ShouldDisableLocalPredictor..."

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:354: kEnabledGroup &&
!on_cellular) {
Put !on_cellular first in the conditional since it's faster.

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:368: int
network_prediction_preference =
Can you replace lines 368 and below with:

return !chrome_browser_net::CanPrefetchAndPrerenderUI(profile->GetPrefs());

?

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_field_trial.h (right):

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.h:39: bool
TemporarilyDisableLocalPredictorBasedOnPreferencesAndNetwork(
TemporarilyDisable makes it sound like the function actually makes some sort of
change, but it doesn't.  Please change to
'ShouldDisableLocalPredictorDueToPreferencesAndNetwork'

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:620: if (!profile ||
DisableLocalPredictorBasedOnSyncAndConfiguration(profile))
I expected to see something call your new TemporarilyDisable... function?

[tburkard, 2014-09-24 15:15:34]

Great feedback, fixed.

Will add test with another update. Meanwhile, PTAL.

Thanks.

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:339: bool
DisableLocalPredictorBasedOnSyncAndConfiguration(Profile* profile) {
On 2014/09/23 14:54:03, jkarlin wrote:
> This should also be "ShouldDisableLocalPredictor..."

Done.

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:354: kEnabledGroup &&
!on_cellular) {
On 2014/09/23 14:54:03, jkarlin wrote:
> Put !on_cellular first in the conditional since it's faster.

Done.

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.cc:368: int
network_prediction_preference =
On 2014/09/23 14:54:03, jkarlin wrote:
> Can you replace lines 368 and below with:
> 
> return !chrome_browser_net::CanPrefetchAndPrerenderUI(profile->GetPrefs());
> 
> ?

Done.

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_field_trial.h (right):

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_field_trial.h:39: bool
TemporarilyDisableLocalPredictorBasedOnPreferencesAndNetwork(
On 2014/09/23 14:54:03, jkarlin wrote:
> TemporarilyDisable makes it sound like the function actually makes some sort
of
> change, but it doesn't.  Please change to
> 'ShouldDisableLocalPredictorDueToPreferencesAndNetwork'

Done.

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/591313005/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:620: if (!profile ||
DisableLocalPredictorBasedOnSyncAndConfiguration(profile))
On 2014/09/23 14:54:03, jkarlin wrote:
> I expected to see something call your new TemporarilyDisable... function?
> 

Done.

[Zhen Wang, 2014-09-24 16:33:58]

https://codereview.chromium.org/591313005/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/591313005/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_field_trial.cc:368: return
!chrome_browser_net::CanPrefetchAndPrerenderUI(profile->GetPrefs());
Should we check CanPrefetchAndPrerenderUI() first?

Say the following case:
- CanPrefetchAndPrerenderUI() is false
- kLocalPredictorNetworkPredictionEnabledOnly != enabled

The function should return true. But the check on
kLocalPredictorNetworkPredictionEnabledOnly will return false beforehand.

I don't seen any other function using CanPrefetchAndPrerenderUI(). Should we
actually check this for all prefetch/prerender experiments?

[tburkard, 2014-09-25 14:12:28]

add test

[tburkard, 2014-09-25 14:21:37]

Hi Josh,

I added the test that you requested, it was a great exercise to go through this,
figure out how to formulate it in the test, and verify that everything works
correctly in each possible scenario.

Thanks.

https://codereview.chromium.org/591313005/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/591313005/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_field_trial.cc:368: return
!chrome_browser_net::CanPrefetchAndPrerenderUI(profile->GetPrefs());
On 2014/09/24 16:33:58, Zhen Wang wrote:
> Should we check CanPrefetchAndPrerenderUI() first?
> 
> Say the following case:
> - CanPrefetchAndPrerenderUI() is false
> - kLocalPredictorNetworkPredictionEnabledOnly != enabled
> 
> The function should return true. But the check on
> kLocalPredictorNetworkPredictionEnabledOnly will return false beforehand.

Actually, the idea is that it should return false in this case. If
kLocalPredictorNetworkPredictionEnabledOnly != enabled, you don't care whether
it is enabled or not.

So you don't need to disable the local predictor.

Basically, the two options, NPEO (network prediction enabled only) and OCO (on
cellular only) can be combined as follows:

- neither: always proceed and issue local predictor pre*
- OCO: only issue local predictor pre* if we are on cellular, but irrespective
of whether the actual pre* action will succeed or not
- NPEO: only issue the local predictor pre* if it will actually succeed, but
irrespective if we are on cellular or not.
- OCO+NPEO: only issue local predictor pre* if it will actually succeed, AND if
we are on cellular

This allows us to do three things:
If we don't care about the actual issuance (eg to collect stats of how often it
would trigger etc), use neither.
If we want stats without caring about the actual issuance, but cellular only,
use OCO.
If we want stats on the benefits of when it was actually issued, for both wifi
and cellular, use NPEO
If we want stats on the benefits of when it wsa actually issued, but limited to
cellular (so that we can present the benefit we bring to cellar users), use
OCO+NPEO

Does this make sense?

If you see any missing scenarios not covered by this, please let me know!

> I don't seen any other function using CanPrefetchAndPrerenderUI(). Should we
> actually check this for all prefetch/prerender experiments?

No, because in tests, the default is that it's enabled. Only actual users
disable it sometimes, which is what we are testing here.

[tburkard, 2014-09-25 15:55:39]

fix test

[tburkard, 2014-09-25 15:56:45]

The new test fails in debug. I fixed is by splitting it up into separate test
cases, which also makes it easier to read and easier to debug test failures.

Please review today, since I want to land this before the branch point.

Thanks.

[jkarlin, 2014-09-25 16:09:35]

Thanks for adding the test. lgtm

[tburkard, 2014-09-25 17:04:19]

The CQ bit was checked by tburkard@chromium.org

[commit-bot: I haz the power, 2014-09-25 17:04:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/591313005/60001

[commit-bot: I haz the power, 2014-09-25 17:17:25]

Committed patchset #4 (id:60001) as 92b1f6b90d31bae828e76b7d913ceeb1023a8872

[commit-bot: I haz the power, 2014-09-25 17:17:53]

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