Change net's predictor_tab_helper to use the new content Navigation API

Change net's predictor_tab_helper to use the new content Navigation API

This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.

BUG=595730
Committed: https://crrev.com/778dddc6edcfbc8e420e0ed019acf506b7bdd864
Cr-Commit-Position: refs/heads/master@{#383714}

[Charlie Harrison, 2016-03-17 18:16:33]

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

[Charlie Harrison, 2016-03-17 18:16:34]

PTAL. This callback is called slightly after DidStartNavigationToPendingEntry,
and will be called for subframes as well.

[mmenke, 2016-03-17 18:52:15]

LGTM.  I assume you'll be keeping an eye on metrics?  Could even field trial
this pretty trivially, so could do full a/b comparisons.

https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:34: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme))
While we're here, maybe get rid of url local, and use
navigation_handle->GetURL().SchemeIsHTTPOrHTTPS()?

[Charlie Harrison, 2016-03-17 18:58:52]

On 2016/03/17 at 18:52:15, mmenke wrote:
> LGTM.  I assume you'll be keeping an eye on metrics?  Could even field trial
this pretty trivially, so could do full a/b comparisons.
> 
>
https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
> File chrome/browser/net/predictor_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
> chrome/browser/net/predictor_tab_helper.cc:34: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme))
> While we're here, maybe get rid of url local, and use
navigation_handle->GetURL().SchemeIsHTTPOrHTTPS()?

You're right about the field trial. I'll think about anything else interesting
we can do here if we're gonna be doing a field trial anyways...

[mmenke, 2016-03-17 19:06:02]

On 2016/03/17 18:58:52, csharrison wrote:
> On 2016/03/17 at 18:52:15, mmenke wrote:
> > LGTM.  I assume you'll be keeping an eye on metrics?  Could even field trial
> this pretty trivially, so could do full a/b comparisons.
> > 
> >
>
https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
> > File chrome/browser/net/predictor_tab_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
> > chrome/browser/net/predictor_tab_helper.cc:34: if
> (url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme))
> > While we're here, maybe get rid of url local, and use
> navigation_handle->GetURL().SchemeIsHTTPOrHTTPS()?
> 
> You're right about the field trial. I'll think about anything else interesting
> we can do here if we're gonna be doing a field trial anyways...

Great, I'll go ahead an "Not LGTM" this, until you've added the field trial
code, just so make sure that gets reviewed.

[Charlie Harrison, 2016-03-17 21:22:41]

I added the finch code (at least as much as I think I need).

I expect subframe preconnects to be a net win, so I don't think it's worth it to
add another group besides "Enabled".

https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/1813553004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:34: if
(url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kHttpsScheme))
On 2016/03/17 at 18:52:15, mmenke wrote:
> While we're here, maybe get rid of url local, and use
navigation_handle->GetURL().SchemeIsHTTPOrHTTPS()?

ACK

[mmenke, 2016-03-18 15:28:14]

So you dismissed testing 3 things against each other on the linked bug (old
behavior, new behavior for main frames + iframes, new behavior for main frames
only), switching to the new behavior is more like fixing a bug.  I'm not
completely convinced it actually was a bug - we're probably more likely to have
old sockets for renderer-initiated navigations. In that case, preconnecting
doesn't hurt much, but DNS lookups might.  That having been said, it does seem
more likely than likely that was not intended behavior.  It may have regressed
during navigation refactoring, or always been broken, not sure.

Anyhow, if that's the case, I think it makes more sense to compare [new behavior
for main frames only] with [new behavior for iframes and main frames].  WDYT?

[mmenke, 2016-03-18 15:29:33]

On 2016/03/18 15:28:14, mmenke wrote:
> So you dismissed testing 3 things against each other on the linked bug (old
> behavior, new behavior for main frames + iframes, new behavior for main frames
> only), switching to the new behavior is more like fixing a bug.  I'm not
> completely convinced it actually was a bug - we're probably more likely to
have
> old sockets for renderer-initiated navigations. In that case, preconnecting
> doesn't hurt much, but DNS lookups might.  That having been said, it does seem
> more likely than likely that was not intended behavior.  It may have regressed
> during navigation refactoring, or always been broken, not sure.
> 
> Anyhow, if that's the case, I think it makes more sense to compare [new
behavior
> for main frames only] with [new behavior for iframes and main frames].  WDYT?

That means we don't have dueling effects of the bug fix and using the
predirector on iframes in the field trial.

[Charlie Harrison, 2016-03-18 16:20:46]

On 2016/03/18 at 15:29:33, mmenke wrote:
> On 2016/03/18 15:28:14, mmenke wrote:
> > So you dismissed testing 3 things against each other on the linked bug (old
> > behavior, new behavior for main frames + iframes, new behavior for main
frames
> > only), switching to the new behavior is more like fixing a bug.  I'm not
> > completely convinced it actually was a bug - we're probably more likely to
have
> > old sockets for renderer-initiated navigations. In that case, preconnecting
> > doesn't hurt much, but DNS lookups might.  That having been said, it does
seem
> > more likely than likely that was not intended behavior.  It may have
regressed
> > during navigation refactoring, or always been broken, not sure.
> > 
> > Anyhow, if that's the case, I think it makes more sense to compare [new
behavior
> > for main frames only] with [new behavior for iframes and main frames]. 
WDYT?
> 
> That means we don't have dueling effects of the bug fix and using the
predirector on iframes in the field trial.

You've convinced me to separate main frame / all frames. I think I'll just do a
3-way comparison. I'm curious to know how we affect performance over the
existing code.

[mmenke, 2016-03-18 16:21:39]

On 2016/03/18 16:20:46, csharrison wrote:
> On 2016/03/18 at 15:29:33, mmenke wrote:
> > On 2016/03/18 15:28:14, mmenke wrote:
> > > So you dismissed testing 3 things against each other on the linked bug
(old
> > > behavior, new behavior for main frames + iframes, new behavior for main
> frames
> > > only), switching to the new behavior is more like fixing a bug.  I'm not
> > > completely convinced it actually was a bug - we're probably more likely to
> have
> > > old sockets for renderer-initiated navigations. In that case,
preconnecting
> > > doesn't hurt much, but DNS lookups might.  That having been said, it does
> seem
> > > more likely than likely that was not intended behavior.  It may have
> regressed
> > > during navigation refactoring, or always been broken, not sure.
> > > 
> > > Anyhow, if that's the case, I think it makes more sense to compare [new
> behavior
> > > for main frames only] with [new behavior for iframes and main frames]. 
> WDYT?
> > 
> > That means we don't have dueling effects of the bug fix and using the
> predirector on iframes in the field trial.
> 
> You've convinced me to separate main frame / all frames. I think I'll just do
a
> 3-way comparison. I'm curious to know how we affect performance over the
> existing code.

SGTM

[mmenke, 2016-03-18 19:26:03]

Sorry, hadn't realized you updated the CL.

https://codereview.chromium.org/1813553004/diff/60001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1813553004/diff/60001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_android.json:237: "group_name":
"Enabled"
Shouldn't we have multiple groups here?

[Charlie Harrison, 2016-03-18 21:38:17]

Updated the json. Thanks.

https://codereview.chromium.org/1813553004/diff/60001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_android.json (right):

https://codereview.chromium.org/1813553004/diff/60001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_android.json:237: "group_name":
"Enabled"
On 2016/03/18 at 19:26:03, mmenke wrote:
> Shouldn't we have multiple groups here?

...yes. Done.

[mmenke, 2016-03-18 21:40:04]

LGTM

[Charlie Harrison, 2016-03-18 21:50:18]

csharrison@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Charlie Harrison, 2016-03-18 21:50:19]

+asvitkine@, is this an okay use of the new FeatureList API? It feels odd to
have two features for what is essentially one feature and two groups.

[Alexei Svitkine (slow), 2016-03-21 19:42:12]

https://codereview.chromium.org/1813553004/diff/80001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/1813553004/diff/80001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:27: "PreconnectMoreMainFrameOnly",
base::FEATURE_DISABLED_BY_DEFAULT};
I think it would be better to use a top-level feature and use the variations
params api (go/variations-params) to control a more specific specialization in
behavior.

https://codereview.chromium.org/1813553004/diff/80001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_linux.json (right):

https://codereview.chromium.org/1813553004/diff/80001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_linux.json:101: "group_name":
"PreconnectMoreAllFrames"
This doesn't actually enable the features unless you specify "enable_features":

[Charlie Harrison, 2016-03-22 16:26:36]

Thanks for the review. I changed the configs to include "params" and
"enable_features" keys.

https://codereview.chromium.org/1813553004/diff/80001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/1813553004/diff/80001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:27: "PreconnectMoreMainFrameOnly",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/03/21 at 19:42:12, asvitkine (no revs until Thu) wrote:
> I think it would be better to use a top-level feature and use the variations
params api (go/variations-params) to control a more specific specialization in
behavior.

Thanks for the suggestion. I was unsure of relationship between the variation
params api and the feature list api and assumed the latter subsumed the former.

https://codereview.chromium.org/1813553004/diff/80001/testing/variations/fiel...
File testing/variations/fieldtrial_testing_config_linux.json (right):

https://codereview.chromium.org/1813553004/diff/80001/testing/variations/fiel...
testing/variations/fieldtrial_testing_config_linux.json:101: "group_name":
"PreconnectMoreAllFrames"
On 2016/03/21 at 19:42:12, asvitkine (no revs until Thu) wrote:
> This doesn't actually enable the features unless you specify
"enable_features":

Ah ok. Updated.

[Alexei Svitkine (slow), 2016-03-24 15:18:58]

lgtm

[Charlie Harrison, 2016-03-29 13:08:12]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-03-29 13:08:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1813553004/#ps100001
(title: "asvitkine@ review - added variations to feature")

[commit-bot: I haz the power, 2016-03-29 13:08:21]

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

[commit-bot: I haz the power, 2016-03-29 13:51:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-29 13:52:51]

Description was changed from

==========
Change net's predictor_tab_helper to use the new content Navigation API

This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.

BUG=595730
==========

to

==========
Change net's predictor_tab_helper to use the new content Navigation API

This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.

BUG=595730

Committed: https://crrev.com/778dddc6edcfbc8e420e0ed019acf506b7bdd864
Cr-Commit-Position: refs/heads/master@{#383714}
==========

[commit-bot: I haz the power, 2016-03-29 13:52:52]

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

[Charlie Harrison, 2016-04-01 15:28:26]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1851093003/ by csharrison@chromium.org.

The reason for reverting is: This caused regressions in Android page_cycler perf
bots of ~150-200ms. See crbug.com/599502..