Fix PredictorTabHelper to work with PlzNavigate.

Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only for the non-PlzNavigate case, since with PlzNavigate all navigations happen in the browser.

This fixes
PredictorBrowserTest.RendererInitiatedNavigationPreconnect
with PlzNavigate.

BUG=504347
Committed: https://crrev.com/394b470df3c428ce3f16ee6d399cea0bc55a24e0
Cr-Commit-Position: refs/heads/master@{#437581}

[jam, 2016-12-09 02:12:04]

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

[commit-bot: I haz the power, 2016-12-09 02:13:03]

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-12-09 03:34:39]

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

[commit-bot: I haz the power, 2016-12-09 03:34:39]

Dry run: This issue passed the CQ dry run.

[jam, 2016-12-09 06:32:06]

Description was changed from

==========
foo

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

to

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PlatformAppUrlRedirectorBrowserTest.PrerenderedClickInTabIntercepted
with PlzNavigate.

BUG=504347
==========

[jam, 2016-12-09 06:32:06]

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

[Charlie Harrison, 2016-12-09 13:05:51]

We have an experiment going which initiates preconnects for renderer-initiated
navigations. The experiment did not show much PLT improvement, but added *lot*
of extra DNS requests and connections, and slightly regressed performance on 2G
networks.

If possible, could we keep the experiment design here for PlzNavigate, and only
issue preconnects in DidStartNavigation for  requests which initiated without an
IPC from an existing renderer?

As an aside, a big reason this class is helpful is that we initiate preconnects
for browser initiated navigations such we avoid some of the cost of render
process init (similar goal of PlzNavigate). When PlzNavigate launches, we may
need to re-evaluate the value of this class (or alter it in some ways).

[jam, 2016-12-09 16:03:58]

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

[commit-bot: I haz the power, 2016-12-09 16:04:32]

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

[jam, 2016-12-09 16:05:57]

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

[commit-bot: I haz the power, 2016-12-09 16:06:25]

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

[jam, 2016-12-09 16:11:21]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-09 16:43:47]

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

[commit-bot: I haz the power, 2016-12-09 16:43:48]

Dry run: This issue passed the CQ dry run.

[jam, 2016-12-09 17:06:53]

Description was changed from

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PlatformAppUrlRedirectorBrowserTest.PrerenderedClickInTabIntercepted
with PlzNavigate.

BUG=504347
==========

to

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PredictorBrowserTest.RendererInitiatedNavigationPreconnect
with PlzNavigate.

BUG=504347
==========

[jam, 2016-12-09 17:08:04]

On 2016/12/09 13:05:51, Charlie Harrison wrote:
> We have an experiment going which initiates preconnects for renderer-initiated
> navigations. The experiment did not show much PLT improvement, but added *lot*
> of extra DNS requests and connections, and slightly regressed performance on
2G
> networks.
> 
> If possible, could we keep the experiment design here for PlzNavigate, and
only
> issue preconnects in DidStartNavigation for  requests which initiated without
an
> IPC from an existing renderer?

Ah, thanks for the explanation, I got thrown off by the experiment name. I've
updated the cl and the comment about the experiment, ptal.

> 
> As an aside, a big reason this class is helpful is that we initiate
preconnects
> for browser initiated navigations such we avoid some of the cost of render
> process init (similar goal of PlzNavigate). When PlzNavigate launches, we may
> need to re-evaluate the value of this class (or alter it in some ways).

[Charlie Harrison, 2016-12-09 17:23:10]

LGTM with a few notes

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

https://codereview.chromium.org/2559203003/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:40:
navigation_handle->IsRendererInitiated()))
Note: IsRendererInitiated is a flaky api (crbug.com/637345). Presumably with
PlzNavigate enabled it won't be, though.

https://codereview.chromium.org/2559203003/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:57: if
(content::IsBrowserSideNavigationEnabled())
Is the method still called in PlzNavigate mode? If not we could make this a
DCHECK.

[jam, 2016-12-09 17:28:48]

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

https://codereview.chromium.org/2559203003/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:57: if
(content::IsBrowserSideNavigationEnabled())
On 2016/12/09 17:23:10, Charlie Harrison wrote:
> Is the method still called in PlzNavigate mode? If not we could make this a
> DCHECK.

Yes it is, but it's planned to be removed.

[jam, 2016-12-09 17:28:52]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-12-09 17:29:25]

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-12-09 17:51:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481304532104380,
"parent_rev": "b9053afb7e3844b8c39a9fe73280bdc8a7e9d451", "commit_rev":
"bbe4eb6b16caa18b9d683e423e4ddbdc57b433a6"}

[commit-bot: I haz the power, 2016-12-09 17:51:49]

Description was changed from

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PredictorBrowserTest.RendererInitiatedNavigationPreconnect
with PlzNavigate.

BUG=504347
==========

to

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PredictorBrowserTest.RendererInitiatedNavigationPreconnect
with PlzNavigate.

BUG=504347

Review-Url: https://codereview.chromium.org/2559203003
==========

[commit-bot: I haz the power, 2016-12-09 17:51:50]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-12-12 14:36:03]

Description was changed from

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PredictorBrowserTest.RendererInitiatedNavigationPreconnect
with PlzNavigate.

BUG=504347

Review-Url: https://codereview.chromium.org/2559203003
==========

to

==========
Fix PredictorTabHelper to work with PlzNavigate.

The workaround in that class to watch DidStartNavigationToPendingEntry is only
for the non-PlzNavigate case, since with PlzNavigate all navigations happen in
the browser.

This fixes
PredictorBrowserTest.RendererInitiatedNavigationPreconnect
with PlzNavigate.

BUG=504347

Committed: https://crrev.com/394b470df3c428ce3f16ee6d399cea0bc55a24e0
Cr-Commit-Position: refs/heads/master@{#437581}
==========

[commit-bot: I haz the power, 2016-12-12 14:36:03]

Patchset 2 (id:??) landed as
https://crrev.com/394b470df3c428ce3f16ee6d399cea0bc55a24e0
Cr-Commit-Position: refs/heads/master@{#437581}