Convert PrerenderContents to use the new navigation callbacks.

Convert PrerenderContents to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2653023002
Cr-Commit-Position: refs/heads/master@{#446424}
Committed: https://chromium.googlesource.com/chromium/src/+/e56263546ae28c14f401a8d48f6059e2605d59d8

[jam, 2017-01-24 18:51:08]

Description was changed from

==========
Convert PrerenderContents to use the new navigation callbacks.

BUG=682002
==========

to

==========
Convert PrerenderContents to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-01-24 18:51:22]

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

[commit-bot: I haz the power, 2017-01-24 18:52:04]

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

[jam, 2017-01-24 18:53:55]

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

[commit-bot: I haz the power, 2017-01-24 18:54:20]

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, 2017-01-24 20:14:38]

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

[commit-bot: I haz the power, 2017-01-24 20:14:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-01-25 05:27:45]

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

[commit-bot: I haz the power, 2017-01-25 05:27:59]

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

[jam, 2017-01-25 05:49:08]

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

[commit-bot: I haz the power, 2017-01-25 05:49:25]

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, 2017-01-25 06:41:03]

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

[commit-bot: I haz the power, 2017-01-25 06:41:03]

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

[jam, 2017-01-25 07:53:00]

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

[commit-bot: I haz the power, 2017-01-25 07:53:09]

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, 2017-01-25 08:52:26]

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

[commit-bot: I haz the power, 2017-01-25 08:52:27]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-25 15:51:07]

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

[pasko, 2017-01-25 17:34:28]

Overall looks good. One question and requesting to wait a little before landing
this. See comments below.

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:582:
navigation_handle->IsErrorPage()) {
I do not know the peculiarities of the new navigation API, so a question:

does it happen in practice that IsErrorPage() is true, and we did not destroy
the prerender contents based on the response code before that?

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (left):

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:179: void DidNavigateMainFrame(
note: there is a concurrent review that changes behavior of
DidNavigateMainFrame() and is aiming to fix a beta blocker:
https://codereview.chromium.org/2656653002/

Can we wait with this one until that lands to reduce conflicts during merge to
beta?

[jam, 2017-01-25 18:13:18]

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.cc:582:
navigation_handle->IsErrorPage()) {
On 2017/01/25 17:34:28, pasko wrote:
> I do not know the peculiarities of the new navigation API, so a question:
> 
> does it happen in practice that IsErrorPage() is true, and we did not destroy
> the prerender contents based on the response code before that?

IsErrorPage() is added to maintain the same behavior of DidFinishNavigation as
DidNavigateMainFrame (since the latter doesn't fire it for errors).

For PrerenderContents, in case of an error, we actually don't get an
IsErrorPage() to true unless in some races. The timing is that there are two
DidStartNavigation/DidFinishNavigation sets of callbacks. The first one is for
the load that doesn't find the page. It's not the error page yet so
IsErrorPage() is false. The second pair is for the loading of the error page, so
IsErrorPage() is true. The issue is that the renderer sends the prefetch
complete message that you added recently between these two pairs, so usually
with the thread hopping/loading times the second pair is not seen.

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_contents.h (left):

https://codereview.chromium.org/2653023002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_contents.h:179: void DidNavigateMainFrame(
On 2017/01/25 17:34:28, pasko wrote:
> note: there is a concurrent review that changes behavior of
> DidNavigateMainFrame() and is aiming to fix a beta blocker:
> https://codereview.chromium.org/2656653002/
> 
> Can we wait with this one until that lands to reduce conflicts during merge to
> beta?

Sure, I'll wait for that cl to land before committing this.

[pasko, 2017-01-25 18:21:52]

lgtm, thank you

[jam, 2017-01-26 16:57:55]

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

[commit-bot: I haz the power, 2017-01-26 16:58:11]

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, 2017-01-26 17:56:11]

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

[commit-bot: I haz the power, 2017-01-26 17:56:12]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-26 18:42:02]

The CQ bit was checked by jam@chromium.org

[jam, 2017-01-26 18:42:03]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2653023002/#ps120001
(title: "merge")

[commit-bot: I haz the power, 2017-01-26 18:42:23]

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, 2017-01-26 20:15:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485456122881010,
"parent_rev": "98e989434b657ea939a41c90c3f413b532f8887b", "commit_rev":
"e56263546ae28c14f401a8d48f6059e2605d59d8"}

[commit-bot: I haz the power, 2017-01-26 20:16:00]

Description was changed from

==========
Convert PrerenderContents to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert PrerenderContents to use the new navigation callbacks.

BUG=682002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2653023002
Cr-Commit-Position: refs/heads/master@{#446424}
Committed:
https://chromium.googlesource.com/chromium/src/+/e56263546ae28c14f401a8d48f60...
==========

[commit-bot: I haz the power, 2017-01-26 20:16:01]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e56263546ae28c14f401a8d48f60...