Update prerender policy for custom tabs

Update prerender policy for custom tabs

- Explicitly early return if the prerender privacy setting is set to off
- If the setting is on then prerendering on mobile is off by default
- Add calls and session params to override the above default if needed

BUG=592635
Committed: https://crrev.com/425b45e3bcf31dff702e3f59fc8bc180977f5730
Cr-Commit-Position: refs/heads/master@{#382875}

[Yusuf, 2016-03-07 22:06:58]

yusufo@chromium.org changed reviewers:
+ lizeb@chromium.org, pasko@chromium.org

[Benoit L, 2016-03-08 13:03:29]

On 2016/03/07 22:06:59, Yusuf wrote:

Should we just follow the current policy of Chrome in general w/ respect to
prerendering, instead of being stricter by default? That wouldn't break the API.

[pasko, 2016-03-08 14:28:18]

On 2016/03/08 13:03:29, Benoit L wrote:
> On 2016/03/07 22:06:59, Yusuf wrote:
> 
> Should we just follow the current policy of Chrome in general w/ respect to
> prerendering, instead of being stricter by default? That wouldn't break the
API.

+1 for consistency in prerendering policy

I do not see why we should early return.

Also, it seems we cannot provide a private API that would guarantee prerendering
unless we cancel out all other concurrent prerenders (from omnibox, potentially
the offline-prerenders, etc). Let's continue this discussion on the meeting
today or in the bug.

[Benoit L, 2016-03-08 18:37:26]

On 2016/03/08 14:28:18, pasko wrote:
> On 2016/03/08 13:03:29, Benoit L wrote:
> > On 2016/03/07 22:06:59, Yusuf wrote:
> > 
> > Should we just follow the current policy of Chrome in general w/ respect to
> > prerendering, instead of being stricter by default? That wouldn't break the
> API.
> 
> +1 for consistency in prerendering policy
> 
> I do not see why we should early return.
> 
> Also, it seems we cannot provide a private API that would guarantee
prerendering
> unless we cancel out all other concurrent prerenders (from omnibox,
potentially
> the offline-prerenders, etc). Let's continue this discussion on the meeting
> today or in the bug.

lgtm

[pasko, 2016-03-08 18:55:22]

In chrome/browser/net/prediction_options.cc the function
CanPrefetchAndPrerender() would disallow prerendering even if the session allows
it.

[Yusuf, 2016-03-09 22:23:10]

yusufo@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mmenke@chromium.org

[Yusuf, 2016-03-09 22:23:10]

Thanks Egor! Added the bits about prerender_manager

mmenke@ for prerender/
dfalcantara for plumbing in ExternalPrerenderHandler and PrerenderTestHelper.

[gone, 2016-03-09 23:02:14]

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(right):

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:290:
if (params != null) params.mShouldPrerenderOnMobile = prerender;
should setIgnoreFragmentsForSession also check if params != null?

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
File chrome/browser/prerender/prerender_manager.cc (right):

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
chrome/browser/prerender/prerender_manager.cc:304: return AddPrerender(origin,
url, referrer, size,
nit: does this fit on one line now?

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
chrome/browser/prerender/prerender_manager.cc:941:
GetPredictionStatusForOrigin(origin);
nit: should still fit on one line?

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
File chrome/browser/prerender/prerender_origin.h (right):

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
chrome/browser/prerender/prerender_origin.h:24: ORIGIN_EXTERNAL_REQUEST_MOBILE =
11,
Don't do this; you're messing up the histograms.  See the comment above.

[Yusuf, 2016-03-09 23:22:49]

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java
(right):

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:290:
if (params != null) params.mShouldPrerenderOnMobile = prerender;
On 2016/03/09 23:02:14, dfalcantara wrote:
> should setIgnoreFragmentsForSession also check if params != null?

Done.

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
File chrome/browser/prerender/prerender_manager.cc (right):

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
chrome/browser/prerender/prerender_manager.cc:304: return AddPrerender(origin,
url, referrer, size,
On 2016/03/09 23:02:14, dfalcantara wrote:
> nit: does this fit on one line now?

Done.

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
chrome/browser/prerender/prerender_manager.cc:941:
GetPredictionStatusForOrigin(origin);
On 2016/03/09 23:02:14, dfalcantara wrote:
> nit: should still fit on one line?

Nope.

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
File chrome/browser/prerender/prerender_origin.h (right):

https://chromiumcodereview.appspot.com/1767243002/diff/40001/chrome/browser/p...
chrome/browser/prerender/prerender_origin.h:24: ORIGIN_EXTERNAL_REQUEST_MOBILE =
11,
On 2016/03/09 23:02:14, dfalcantara wrote:
> Don't do this; you're messing up the histograms.  See the comment above.

Done.

[gone, 2016-03-09 23:24:33]

lgtm

[Benoit L, 2016-03-10 13:58:30]

Thanks for that!
Only minor nits, and a request to keep the code consistent. Sorry about that, I
know that this is kind of urgent. :-/

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.cc:61: bool
prerender_on_mobile) {
nit: jboolean.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.h (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.h:39: bool
prerender_on_mobile);
nit: jboolean.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.cc (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.cc:29: "External Request Mobile",
If you update the name, don't forget to update the description here as well. :-)

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.h (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.h:26: ORIGIN_EXTERNAL_REQUEST_MOBILE =
13,
I guess that you need to update places where this enum is used. At least for the
histograms:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...

Fortunately, it doesn't seem to be used too widely, that is mainly for
histograms (we may want to conflate this with EXTERNAL_REQUEST) and throttling
(ditto?).


Also, should the name reflect that we force the prerender even though we're on a
mobile network?

Maybe ORIGIN_EXTERNAL_REQUEST_FORCED_ON_MOBILE?

[pasko, 2016-03-10 14:21:37]

overall looking good, agreed to lizeb@ on his main suggestions

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1293: NetworkPredictionStatus
prerender_status = GetPredictionStatus();
nit: s/prerender_status/prediction_status/

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.h (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.h:26: ORIGIN_EXTERNAL_REQUEST_MOBILE =
13,
+1, also would need a new suffix in histograms.xml

Maybe ORIGIN_EXTERNAL_REQUEST_FORCED, since the mechanics of PrerenderManager is
not mobile-specific. On the other hand, it is important to emphasize in the
origin what kind of hack it is. Maybe then
RIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS is even more descriptive. The offline
thing will probably use the same mechanics, but we would like the origin to be
different for UMA accounting reasons.

[Yusuf, 2016-03-10 18:55:54]

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.cc (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.cc:61: bool
prerender_on_mobile) {
On 2016/03/10 13:58:30, Benoit L wrote:
> nit: jboolean.

Done.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/external_prerender_handler_android.h (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/external_prerender_handler_android.h:39: bool
prerender_on_mobile);
On 2016/03/10 13:58:30, Benoit L wrote:
> nit: jboolean.

Done.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1293: NetworkPredictionStatus
prerender_status = GetPredictionStatus();
On 2016/03/10 14:21:37, pasko wrote:
> nit: s/prerender_status/prediction_status/

Done.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.cc (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.cc:29: "External Request Mobile",
On 2016/03/10 13:58:30, Benoit L wrote:
> If you update the name, don't forget to update the description here as well.
:-)

Done.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.h (right):

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.h:26: ORIGIN_EXTERNAL_REQUEST_MOBILE =
13,
On 2016/03/10 14:21:37, pasko wrote:
> +1, also would need a new suffix in histograms.xml
> 
> Maybe ORIGIN_EXTERNAL_REQUEST_FORCED, since the mechanics of PrerenderManager
is
> not mobile-specific. On the other hand, it is important to emphasize in the
> origin what kind of hack it is. Maybe then
> RIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS is even more descriptive. The offline
> thing will probably use the same mechanics, but we would like the origin to be
> different for UMA accounting reasons.
> 

Done.

https://codereview.chromium.org/1767243002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.h:26: ORIGIN_EXTERNAL_REQUEST_MOBILE =
13,
On 2016/03/10 14:21:37, pasko wrote:
> +1, also would need a new suffix in histograms.xml
> 
> Maybe ORIGIN_EXTERNAL_REQUEST_FORCED, since the mechanics of PrerenderManager
is
> not mobile-specific. On the other hand, it is important to emphasize in the
> origin what kind of hack it is. Maybe then
> RIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS is even more descriptive. The offline
> thing will probably use the same mechanics, but we would like the origin to be
> different for UMA accounting reasons.
> 

Done.

[Yusuf, 2016-03-10 19:02:19]

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

[commit-bot: I haz the power, 2016-03-10 19:02:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1767243002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1767243002/80001

[pasko, 2016-03-10 19:19:25]

lgtm, thank you!

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.cc (right):

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.cc:29: "External Request Forced Custom
Tabs",
nit: it would be more human readable this way: "Forced External Request by
Custom Tabs"

[commit-bot: I haz the power, 2016-03-10 19:42:33]

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

[commit-bot: I haz the power, 2016-03-10 19:42:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pasko, 2016-03-11 11:00:59]

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

[commit-bot: I haz the power, 2016-03-11 11:01:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1767243002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1767243002/80001

[commit-bot: I haz the power, 2016-03-11 12:09:21]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yusuf, 2016-03-14 16:56:34]

ping

[mmenke, 2016-03-14 16:58:14]

On 2016/03/14 16:56:34, Yusuf wrote:
> ping

Sorry, forgot about this.  I'll try to get to it later today, but may not be
able to get to it until tomorrow.

[mmenke, 2016-03-14 20:38:00]

mmenke@chromium.org changed reviewers:
+ cbentzel@chromium.org

[mmenke, 2016-03-14 20:38:01]

[+cbentzel]:  Any thoughts on whether we should push back on prerendering on
cellular?  It doesn't seem like a great idea to me.  I'm not sure we have any
clear police here.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:301: bool prerender_on_mobile) {
This is weird, for a couple reasons:

* This method is only used on mobile.
* Presumably we know at build time if we're on mobile.
* It can be false on mobile.

I also am not a fan of bool arguments passed in by value - "false" just sin't
too meaningful.

I suggest either we make this an enum class within PrerenderManager (And rename
it - prerender_on_cellular?) or make a new method
AddPrerenderOnCellularFromExternalRequest (Or somesuch)

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:303: ?
ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS
Mapping "prerender_on_mobile == true" to
"ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS" seems weird - PrerenderManager
shouldn't know that only "CUSTOMTABS" (?) requests prerender on cellular
connections.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:794: GetPredictionStatus() ==
NetworkPredictionStatus::ENABLED);
The reason for disabling seems more relevant here, now that we're selectively
overriding it.  Seems like we should output which of the three states we're in
here.

I'd really like to get rid of GetPredictionStatus(), now that it's not accurate
for all origins, but the other two pre-existing calls are a bit funky.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1296: && origin ==
ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS) {
Do we really want to do this?  How large are AMP pages?  I'm concerned both
about the performance cost and the bandwidth, particularly in emerging markets,
where people are often on 2G connections, and data expenses make up a
significant portion of people's income.

[mmenke, 2016-03-14 20:38:19]

On 2016/03/14 20:38:01, mmenke wrote:
> [+cbentzel]:  Any thoughts on whether we should push back on prerendering on
> cellular?  It doesn't seem like a great idea to me.  I'm not sure we have any
> clear police here.

Erm...police == policy

[cbentzel, 2016-03-14 21:03:26]

Currently decided on policy is to allow it in this CCT case. I can fill you
in with some background.

On Mon, Mar 14, 2016 at 4:38 PM <mmenke@chromium.org> wrote:

> On 2016/03/14 20:38:01, mmenke wrote:
> > [+cbentzel]: Any thoughts on whether we should push back on prerendering
> on
> > cellular? It doesn't seem like a great idea to me. I'm not sure we have
> any
> > clear police here.
>
> Erm...police == policy
>
> https://codereview.chromium.org/1767243002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuf, 2016-03-14 23:08:20]

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:301: bool prerender_on_mobile) {
On 2016/03/14 20:38:01, mmenke wrote:
> This is weird, for a couple reasons:
> 
> * This method is only used on mobile.
> * Presumably we know at build time if we're on mobile.
> * It can be false on mobile.
> 
> I also am not a fan of bool arguments passed in by value - "false" just sin't
> too meaningful.
> 
> I suggest either we make this an enum class within PrerenderManager (And
rename
> it - prerender_on_cellular?) or make a new method
> AddPrerenderOnCellularFromExternalRequest (Or somesuch)

Done. Added AddPrerenderOnCellularFromExternalRequest and renamed all relevant
calls and variables to say cellular instead of mobile. Removed custom tabs
mentions.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:303: ?
ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS
On 2016/03/14 20:38:01, mmenke wrote:
> Mapping "prerender_on_mobile == true" to
> "ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS" seems weird - PrerenderManager
> shouldn't know that only "CUSTOMTABS" (?) requests prerender on cellular
> connections.

Done.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:794: GetPredictionStatus() ==
NetworkPredictionStatus::ENABLED);
On 2016/03/14 20:38:01, mmenke wrote:
> The reason for disabling seems more relevant here, now that we're selectively
> overriding it.  Seems like we should output which of the three states we're in
> here.
> 
> I'd really like to get rid of GetPredictionStatus(), now that it's not
accurate
> for all origins, but the other two pre-existing calls are a bit funky.

By three states do you mean NetworkPredictionStatus values? (disable,
disabled_network, enable)

Sorry, I am not clear on what you mean by the two pre-existing values.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1296: && origin ==
ORIGIN_EXTERNAL_REQUEST_FORCED_CUSTOMTABS) {
On 2016/03/14 20:38:01, mmenke wrote:
> Do we really want to do this?  How large are AMP pages?  I'm concerned both
> about the performance cost and the bandwidth, particularly in emerging
markets,
> where people are often on 2G connections, and data expenses make up a
> significant portion of people's income.

AMP in general is pretty cheap.
On our end, we never call the external request call on low end phones and also
heavily throttle calls that doesn't end up following with a corresponding intent
launch and ban apps from using this codepath that repeatedly get caught doing
this.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_origin.cc (right):

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_origin.cc:29: "External Request Forced Custom
Tabs",
On 2016/03/10 19:19:24, pasko wrote:
> nit: it would be more human readable this way: "Forced External Request by
> Custom Tabs"

Acknowledged.

[mmenke, 2016-03-15 14:34:21]

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:794: GetPredictionStatus() ==
NetworkPredictionStatus::ENABLED);
On 2016/03/14 23:08:20, Yusuf wrote:
> On 2016/03/14 20:38:01, mmenke wrote:
> > The reason for disabling seems more relevant here, now that we're
selectively
> > overriding it.  Seems like we should output which of the three states we're
in
> > here.
> > 
> > I'd really like to get rid of GetPredictionStatus(), now that it's not
> accurate
> > for all origins, but the other two pre-existing calls are a bit funky.
> 
> By three states do you mean NetworkPredictionStatus values? (disable,
> disabled_network, enable)
> 
> Sorry, I am not clear on what you mean by the two pre-existing values.

The two other pre-existing calls to GetPredictionStatus(), besides this one and
the one you replaced with the new method.  It's weird to have separate notions
of general prediction status, an per-origin prediction status...

Regardless, now that we have magic to ignore the disable on cellular state in
certain cases, it seems like we should really be distinguishing the reasons
prerendering was disabled here, since they have different behaviors now.

https://codereview.chromium.org/1767243002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/1767243002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.h:137: PrerenderHandle*
AddPrerenderOnCellularFromExternalRequest(
Needs a comment explaining what it does

[mmenke, 2016-03-15 21:21:59]

Also, we should have a test for this.  See
PrerenderManagerTest.LinkRelNotAllowedWhenDisabled for an example (Though you
want it make sure it is allowed in the case the Cellular method is called, and
should also make sure it's not allowed when the non-Cellular one is called)

[Yusuf, 2016-03-20 06:48:29]

Added there tests. PTAL.

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/1767243002/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:794: GetPredictionStatus() ==
NetworkPredictionStatus::ENABLED);
On 2016/03/15 14:34:21, mmenke wrote:
> On 2016/03/14 23:08:20, Yusuf wrote:
> > On 2016/03/14 20:38:01, mmenke wrote:
> > > The reason for disabling seems more relevant here, now that we're
> selectively
> > > overriding it.  Seems like we should output which of the three states
we're
> in
> > > here.
> > > 
> > > I'd really like to get rid of GetPredictionStatus(), now that it's not
> > accurate
> > > for all origins, but the other two pre-existing calls are a bit funky.
> > 
> > By three states do you mean NetworkPredictionStatus values? (disable,
> > disabled_network, enable)
> > 
> > Sorry, I am not clear on what you mean by the two pre-existing values.
> 
> The two other pre-existing calls to GetPredictionStatus(), besides this one
and
> the one you replaced with the new method.  It's weird to have separate notions
> of general prediction status, an per-origin prediction status...
> 
> Regardless, now that we have magic to ignore the disable on cellular state in
> certain cases, it seems like we should really be distinguishing the reasons
> prerendering was disabled here, since they have different behaviors now.

Done. Added a disabled note. Removed two calls to GetPredictionStatus(). Now the
only ones left are the ones here and the one in AddProfileNetworkBytesIfEnabled
both of which is not aware of origin.

https://codereview.chromium.org/1767243002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/1767243002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.h:137: PrerenderHandle*
AddPrerenderOnCellularFromExternalRequest(
On 2016/03/15 14:34:21, mmenke wrote:
> Needs a comment explaining what it does

Done.

[Yusuf, 2016-03-20 06:48:38]

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

[commit-bot: I haz the power, 2016-03-20 06:48:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1767243002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1767243002/260001

[commit-bot: I haz the power, 2016-03-20 08:04:48]

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

[commit-bot: I haz the power, 2016-03-20 08:04:49]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-03-21 12:00:58]

still lgtm

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:231: 
nit: don't we use only one vertical whitespace in this case

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:398:
chrome_browser_net::NETWORK_PREDICTION_WIFI_ONLY);
since NETWORK_PREDICTION_WIFI_ONLY is deprecated, can NETWORK_PREDICTION_ALWAYS
be used here instead?

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1077:
TEST_F(PrerenderTest,PrerenderNotAllowedOnCellular) {
nit: s/,/, /
same below

[Yusuf, 2016-03-21 16:59:57]

Thanks Egor!

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:231: 
On 2016/03/21 12:00:58, pasko wrote:
> nit: don't we use only one vertical whitespace in this case

Done.

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:398:
chrome_browser_net::NETWORK_PREDICTION_WIFI_ONLY);
On 2016/03/21 12:00:58, pasko wrote:
> since NETWORK_PREDICTION_WIFI_ONLY is deprecated, can
NETWORK_PREDICTION_ALWAYS
> be used here instead?

Done.

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1077:
TEST_F(PrerenderTest,PrerenderNotAllowedOnCellular) {
On 2016/03/21 12:00:58, pasko wrote:
> nit: s/,/, /
> same below

Done.

[mmenke, 2016-03-22 15:29:29]

LGTM, modulo comments.  Sorry about slowness on this review.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1082:
GURL("http://www.example.com")));
nit:  Fits on one line.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1099: url, content::Referrer(),
NULL, kSize));
nullptr

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1100:
CHECK(!prerender_handle.get());
nit:  .get() not needed.  Since we support leaving it out, seems like that
should be the preferred style.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1100:
CHECK(!prerender_handle.get());
Any reason for not making this an ASSERT_FALSE(...)?

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1118: url, content::Referrer(),
NULL, kSize));
nullptr

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1119:
CHECK(prerender_handle.get());
nit:  .get() not needed.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1119:
CHECK(prerender_handle.get());
ASSERT_TRUE?

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1124:
prerender_handle->contents()->origin());
Run "git cl format".  The indentation here is wrong.

[Yusuf, 2016-03-22 19:21:11]

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1082:
GURL("http://www.example.com")));
On 2016/03/22 15:29:29, mmenke wrote:
> nit:  Fits on one line.

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1099: url, content::Referrer(),
NULL, kSize));
On 2016/03/22 15:29:29, mmenke wrote:
> nullptr

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1100:
CHECK(!prerender_handle.get());
On 2016/03/22 15:29:29, mmenke wrote:
> nit:  .get() not needed.  Since we support leaving it out, seems like that
> should be the preferred style.

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1100:
CHECK(!prerender_handle.get());
On 2016/03/22 15:29:29, mmenke wrote:
> Any reason for not making this an ASSERT_FALSE(...)?

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1118: url, content::Referrer(),
NULL, kSize));
On 2016/03/22 15:29:28, mmenke wrote:
> nullptr

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1119:
CHECK(prerender_handle.get());
On 2016/03/22 15:29:28, mmenke wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1119:
CHECK(prerender_handle.get());
On 2016/03/22 15:29:28, mmenke wrote:
> nit:  .get() not needed.

Done.

https://codereview.chromium.org/1767243002/diff/280001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1124:
prerender_handle->contents()->origin());
On 2016/03/22 15:29:29, mmenke wrote:
> Run "git cl format".  The indentation here is wrong.

Done.

[Yusuf, 2016-03-22 19:22:36]

yusufo@chromium.org changed reviewers:
+ holte@chromium.org

[Yusuf, 2016-03-22 19:22:37]

holte@ for histograms change

[Yusuf, 2016-03-22 19:22:44]

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

[commit-bot: I haz the power, 2016-03-22 19:23:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1767243002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1767243002/300001

[commit-bot: I haz the power, 2016-03-22 20:43:59]

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

[commit-bot: I haz the power, 2016-03-22 20:44:00]

Dry run: This issue passed the CQ dry run.

[Yusuf, 2016-03-23 17:41:00]

yusufo@chromium.org changed reviewers:
+ rkaplow@chromium.org
- holte@chromium.org

[Yusuf, 2016-03-23 17:41:02]

rkaplow@ for histograms.xml

[rkaplow, 2016-03-23 17:46:57]

lgtm

[Yusuf, 2016-03-23 17:47:20]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2016-03-23 17:47:20]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org,
dfalcantara@chromium.org, pasko@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1767243002/#ps300001
(title: "mmenke@'s comments")

[commit-bot: I haz the power, 2016-03-23 17:47:42]

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

[commit-bot: I haz the power, 2016-03-23 17:53:54]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-03-23 17:56:18]

Description was changed from

==========
Update prerender policy for custom tabs

- Explicitly early return if the prerender privacy setting is set to off
- If the setting is on then prerendering on mobile is off by default
- Add calls and session params to override the above default if needed

BUG=592635
==========

to

==========
Update prerender policy for custom tabs

- Explicitly early return if the prerender privacy setting is set to off
- If the setting is on then prerendering on mobile is off by default
- Add calls and session params to override the above default if needed

BUG=592635

Committed: https://crrev.com/425b45e3bcf31dff702e3f59fc8bc180977f5730
Cr-Commit-Position: refs/heads/master@{#382875}
==========

[commit-bot: I haz the power, 2016-03-23 17:56:22]

Patchset 16 (id:??) landed as
https://crrev.com/425b45e3bcf31dff702e3f59fc8bc180977f5730
Cr-Commit-Position: refs/heads/master@{#382875}

[pasko, 2016-03-31 15:30:44]

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/1767243002/diff/260001/chrome/browser/prerend...
chrome/browser/prerender/prerender_unittest.cc:1077:
TEST_F(PrerenderTest,PrerenderNotAllowedOnCellular) {
On 2016/03/21 16:59:57, Yusuf wrote:
> On 2016/03/21 12:00:58, pasko wrote:
> > nit: s/,/, /
> > same below
> 
> Done.

Thank you. I just noticed it was not done for tests below. I think I should have
explicitly said something "also below"...