customtabs: Add a flag to mayLaunchUrl() to force prefetch.

customtabs: Add a flag to mayLaunchUrl() to force prefetch.

This adds a flag to mayLaunchUrl() to prevent it from doing a prerender,
forcing the high confidence version to only issue a prefetch
request (using the resource_prefetch_predictor here). This is needed to
test and evaluate it, and is intended for testing purposes only, as the
"no prerendering" flag is as well.

BUG=650250, 631966
Committed: https://crrev.com/588e88df5847e11703ffdf063f621d38a66a5fa4
Cr-Commit-Position: refs/heads/master@{#424742}

[Benoit L, 2016-10-10 15:30:40]

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

[pasko, 2016-10-11 11:08:26]

slightly confused with the amount of options (once amount of blows and whistles
reaches more than three, I usually get confused, sorry, being human..)

see below for details

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Top-level: we have different strings that we check for in the bundle and decide
to do one of the things:
* prerender
* prefetch
* preconnect
* do nothing

.. and low confidence thing is on the side .. and we will have more

Without looking at the code it is not possible to tell which string is supposed
to override which. To make it 'better' for the debugging flavors we could
probably make it something like an enum or a reserved key with only one of
possible values at a time? The idea is that this debugging enum/value can only
ask for one of the speculation strategies and, if present, overrides all things
provided by the officially supported ABI. If that's not easier to use and
maintain, I'd like to know why :)

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:77:
"android.support.customtabs.maylaunchurl.NO_PRERENDERING";
This is for testing, right? A reader might think that this is for low confidence
mayLaunchUrl() and be confused. A comment would be enough I think.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:79:
static final String PREFETCH_ONLY_KEY =
"android.support.customtabs.maylaunchurl.PREFETCH_ONLY";
Probably worth briefly noting somewhere in a comment that this is for
resource_prefetch_predictor.* and not other flavors of prefetch.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:83:
/** Holds the parameters for current speculation. */
nit: s/current/the current/

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:275:
boolean prefetchOnly = extras != null && extras.getBoolean(PREFETCH_ONLY_KEY);
if we name s/prefetchOnly/downgradeToPrefetch/ the logic would become more
obvious in the part where NO_PRERENDERING_KEY && PREFETCH_ONLY_KEY does not do
prefetch

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:290:
warmupManager.maybePreconnectUrlAndSubResources(profile, url);
it was preconnecting before prerendering, now it is not, wondering if we should
preserve this property

[Benoit L, 2016-10-11 16:46:13]

Thanks!

All done.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/10/11 11:08:26, pasko wrote:
> Top-level: we have different strings that we check for in the bundle and
decide
> to do one of the things:
> * prerender
> * prefetch
> * preconnect
> * do nothing
> 
> .. and low confidence thing is on the side .. and we will have more
> 
> Without looking at the code it is not possible to tell which string is
supposed
> to override which. To make it 'better' for the debugging flavors we could
> probably make it something like an enum or a reserved key with only one of
> possible values at a time? The idea is that this debugging enum/value can only
> ask for one of the speculation strategies and, if present, overrides all
things
> provided by the officially supported ABI. If that's not easier to use and
> maintain, I'd like to know why :)

Thanks for the suggestion!
Done.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:77:
"android.support.customtabs.maylaunchurl.NO_PRERENDERING";
On 2016/10/11 11:08:26, pasko wrote:
> This is for testing, right? A reader might think that this is for low
confidence
> mayLaunchUrl() and be confused. A comment would be enough I think.

Done.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:79:
static final String PREFETCH_ONLY_KEY =
"android.support.customtabs.maylaunchurl.PREFETCH_ONLY";
On 2016/10/11 11:08:26, pasko wrote:
> Probably worth briefly noting somewhere in a comment that this is for
> resource_prefetch_predictor.* and not other flavors of prefetch.

Done.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:83:
/** Holds the parameters for current speculation. */
On 2016/10/11 11:08:25, pasko wrote:
> nit: s/current/the current/

Done.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:275:
boolean prefetchOnly = extras != null && extras.getBoolean(PREFETCH_ONLY_KEY);
On 2016/10/11 11:08:25, pasko wrote:
> if we name s/prefetchOnly/downgradeToPrefetch/ the logic would become more
> obvious in the part where NO_PRERENDERING_KEY && PREFETCH_ONLY_KEY does not do
> prefetch

Done.

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:290:
warmupManager.maybePreconnectUrlAndSubResources(profile, url);
On 2016/10/11 11:08:25, pasko wrote:
> it was preconnecting before prerendering, now it is not, wondering if we
should
> preserve this property

Done.

[Benoit L, 2016-10-11 16:46:35]

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

[commit-bot: I haz the power, 2016-10-11 16:47:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 17:19:18]

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

[commit-bot: I haz the power, 2016-10-11 17:19:19]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-10-12 14:18:15]

lgtm with nits, thank you

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:290:
warmupManager.maybePreconnectUrlAndSubResources(profile, url);
On 2016/10/11 16:46:13, Benoit L wrote:
> On 2016/10/11 11:08:25, pasko wrote:
> > it was preconnecting before prerendering, now it is not, wondering if we
> should
> > preserve this property
> 
> Done.

really Done? in the new patchset we are still changing to eliminate
preconnect+prerender. I actually prefer to have it this way, but curiosity
remains.

https://codereview.chromium.org/2403023002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/2403023002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:279:
int debugOverrideValue = extras != null ? extras.getInt(DEBUG_OVERRIDE_KEY, -1)
: -1;
nit: -1 seems there for consistency with parsing extras in general, but it is
slightly weird to have something like:

-1: no override
0: unused
1: disable prerendering
2: override prerendering with prefetch

https://codereview.chromium.org/2403023002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:284:
if (debugOverrideValue == PREFETCH_ONLY) {
nit: a better name would probably be
s/PREFETCH_ONLY/OVERRIDE_PRERENDER_WITH_PREFETCH/ (or something of that sort),
since "only" does not mean it is the only speculation, we'd still preconnect for
low confidence stuff.

really a nit, feel free to leave it as it is, if feels overly verbose

[Benoit L, 2016-10-12 15:25:12]

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/2403023002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:290:
warmupManager.maybePreconnectUrlAndSubResources(profile, url);
On 2016/10/12 14:18:14, pasko wrote:
> On 2016/10/11 16:46:13, Benoit L wrote:
> > On 2016/10/11 11:08:25, pasko wrote:
> > > it was preconnecting before prerendering, now it is not, wondering if we
> > should
> > > preserve this property
> > 
> > Done.
> 
> really Done? in the new patchset we are still changing to eliminate
> preconnect+prerender. I actually prefer to have it this way, but curiosity
> remains.

We do preconnect when prerendering now,
We do preconnect when prerendering, since this is done for normal navigations.
(at least, that's what I think I remember).

https://codereview.chromium.org/2403023002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java
(right):

https://codereview.chromium.org/2403023002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:279:
int debugOverrideValue = extras != null ? extras.getInt(DEBUG_OVERRIDE_KEY, -1)
: -1;
On 2016/10/12 14:18:15, pasko wrote:
> nit: -1 seems there for consistency with parsing extras in general, but it is
> slightly weird to have something like:
> 
> -1: no override
> 0: unused
> 1: disable prerendering
> 2: override prerendering with prefetch

Per offline discussion, let's leave it as is.

https://codereview.chromium.org/2403023002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:284:
if (debugOverrideValue == PREFETCH_ONLY) {
On 2016/10/12 14:18:15, pasko wrote:
> nit: a better name would probably be
> s/PREFETCH_ONLY/OVERRIDE_PRERENDER_WITH_PREFETCH/ (or something of that sort),
> since "only" does not mean it is the only speculation, we'd still preconnect
for
> low confidence stuff.
> 
> really a nit, feel free to leave it as it is, if feels overly verbose

Acknowledged.

[Benoit L, 2016-10-12 15:25:19]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-10-12 15:25:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 15:30:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-12 15:32:58]

Description was changed from

==========
customtabs: Add a flag to mayLaunchUrl() to force prefetch.

This adds a flag to mayLaunchUrl() to prevent it from doing a prerender,
forcing the high confidence version to only issue a prefetch
request (using the resource_prefetch_predictor here). This is needed to
test and evaluate it, and is intended for testing purposes only, as the
"no prerendering" flag is as well.

BUG=650250,631966
==========

to

==========
customtabs: Add a flag to mayLaunchUrl() to force prefetch.

This adds a flag to mayLaunchUrl() to prevent it from doing a prerender,
forcing the high confidence version to only issue a prefetch
request (using the resource_prefetch_predictor here). This is needed to
test and evaluate it, and is intended for testing purposes only, as the
"no prerendering" flag is as well.

BUG=650250,631966

Committed: https://crrev.com/588e88df5847e11703ffdf063f621d38a66a5fa4
Cr-Commit-Position: refs/heads/master@{#424742}
==========

[commit-bot: I haz the power, 2016-10-12 15:32:59]

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