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

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

The original CL had issues due to slow process spawn on
Android, where the DidStartNavigation call could be delayed,
stalling preconnects. This patch keeps the old code while adding
a path for renderer initiated navigations to issue preconnects.

Previous CL: https://codereview.chromium.org/1813553004/
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/f6e401951f8feb8bfc19fbfc9cafbc9fab60a1f6
Cr-Commit-Position: refs/heads/master@{#393754}

[Charlie Harrison, 2016-04-04 17:06:59]

Description was changed from

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

BUG=
==========

to

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

Previous CL: https://codereview.chromium.org/1813553004/
This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.


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

[Charlie Harrison, 2016-04-04 17:06:59]

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

[Charlie Harrison, 2016-04-04 17:09:17]

Description was changed from

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

Previous CL: https://codereview.chromium.org/1813553004/
This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.


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

to

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

The original CL had issues due to slow process spawn on
Android, where the DidStartNavigation call could be delayed,
stalling preconnects. This patch keeps the old code while adding
a path for renderer initiated navigations to issue preconnects.

Previous CL: https://codereview.chromium.org/1813553004/
This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.


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

[Charlie Harrison, 2016-04-04 17:11:20]

clamy@, this patch uses a simple model to catch navigations that do not go
through DidStartNavigationToPendingEntry.

Is there a better way to do this?

[Charlie Harrison, 2016-04-07 17:54:44]

Friendly ping. I'd just like a quick sanity check for the tab_helper.

[clamy, 2016-04-08 11:01:06]

tab_helper lgtm. Sorry about the delay :).

[Charlie Harrison, 2016-04-08 12:16:49]

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

[Charlie Harrison, 2016-04-08 12:16:50]

Thanks, clamy@!

mmenke@, asvitkine@ PTAL. This is pretty similar to the previous CL, though I
simplified that experiment to only consider main frames (subframes are actually
handled elsewhere already).

https://codereview.chromium.org/1857573002/diff/1/chrome/browser/net/predicto...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/1857573002/diff/1/chrome/browser/net/predicto...
chrome/browser/net/predictor_tab_helper.cc:36: if
(predicted_from_pending_entry_) {
Staring at this, I think this if statement should go below the check for
subframe navigations. This approach means

1. Type facebook.com (DidStartNavigationToPendingEntry)
2. A racy subframe navigation in the tab occurs
3. facebook.com didStartProvisionalLoad received from (1)

The racy subframe nav will uncheck the |predicted_from_pending_entry_bit_|. I'll
change this in my next patch set.

[Charlie Harrison, 2016-04-08 14:54:38]

https://codereview.chromium.org/1857573002/diff/1/testing/variations/fieldtri...
File testing/variations/fieldtrial_testing_config_mac.json (right):

https://codereview.chromium.org/1857573002/diff/1/testing/variations/fieldtri...
testing/variations/fieldtrial_testing_config_mac.json:122: {
I'll remove "Default" from these as isherman@ mentioned FYI bots will run
default configs.

[mmenke, 2016-04-08 17:42:07]

I think we should have a browser test for this.  Learn something from a
renderer-initiated navigation, and then make sure we use that learned knowledge
in another renderer-initiated navigation.

[Charlie Harrison, 2016-04-14 22:09:19]

On 2016/04/08 at 17:42:07, mmenke wrote:
> I think we should have a browser test for this.  Learn something from a
renderer-initiated navigation, and then make sure we use that learned knowledge
in another renderer-initiated navigation.

Good idea. I'll wait for the test suite to land first so I can reuse that
infrastructure.

[Alexei Svitkine (slow), 2016-04-15 16:25:10]

lgtm

[Charlie Harrison, 2016-05-13 14:54:19]

I added a renderer initiated navigation browser test and confirmed that it fails
without the experiment on.

As an added bonus, the test was very simple to write :)

[mmenke, 2016-05-13 21:05:50]

LGTM

[Charlie Harrison, 2016-05-14 13:53:09]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-05-14 13:53:10]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1857573002/#ps60001
(title: "Update finch test config on chromeos")

[commit-bot: I haz the power, 2016-05-14 13:53:33]

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

[commit-bot: I haz the power, 2016-05-14 20:54:32]

Description was changed from

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

The original CL had issues due to slow process spawn on
Android, where the DidStartNavigation call could be delayed,
stalling preconnects. This patch keeps the old code while adding
a path for renderer initiated navigations to issue preconnects.

Previous CL: https://codereview.chromium.org/1813553004/
This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.


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

to

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

The original CL had issues due to slow process spawn on
Android, where the DidStartNavigation call could be delayed,
stalling preconnects. This patch keeps the old code while adding
a path for renderer initiated navigations to issue preconnects.

Previous CL: https://codereview.chromium.org/1813553004/
This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.


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

[commit-bot: I haz the power, 2016-05-14 20:54:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-14 20:56:19]

Description was changed from

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

The original CL had issues due to slow process spawn on
Android, where the DidStartNavigation call could be delayed,
stalling preconnects. This patch keeps the old code while adding
a path for renderer initiated navigations to issue preconnects.

Previous CL: https://codereview.chromium.org/1813553004/
This change moves the trigger for preconnects/preresolves from
DidStartNavigationToPendingEntry to DidStartNavigation, broadening the
navigations that we can predict from.


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

to

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

The original CL had issues due to slow process spawn on
Android, where the DidStartNavigation call could be delayed,
stalling preconnects. This patch keeps the old code while adding
a path for renderer initiated navigations to issue preconnects.

Previous CL: https://codereview.chromium.org/1813553004/
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/f6e401951f8feb8bfc19fbfc9cafbc9fab60a1f6
Cr-Commit-Position: refs/heads/master@{#393754}
==========

[commit-bot: I haz the power, 2016-05-14 20:56:20]

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

[Ken Rockot(use gerrit already), 2016-05-15 06:53:13]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-05-15 06:53:14]

This might be responsible for new memory fyi failures. e.g.:

https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%2...

Unfortunately there doesn't appear to be much useful information in the logs,
but it looks like
PredictorBrowserTest.CrossSiteSimplePredictionAfterTwoNavigations2 is timing
out.

Could you please take a look?

[Charlie Harrison, 2016-05-15 07:20:59]

On 2016/05/15 at 06:53:14, rockot wrote:
> This might be responsible for new memory fyi failures. e.g.:
> 
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%2...
> 
> Unfortunately there doesn't appear to be much useful information in the logs,
but it looks like
PredictorBrowserTest.CrossSiteSimplePredictionAfterTwoNavigations2 is timing
out.
> 
> Could you please take a look?

Hm I think more likely is that test (that was recently added) is flaky or wrong
in some way. That was https://codereview.chromium.org/1881463003/. 

I'll look into it tomorrow.