[NoStatePrefetch] Kill renderer after preload scanning

[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361, 649632
Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
Cr-Commit-Position: refs/heads/master@{#427678}

[pasko, 2016-10-11 18:51:32]

Description was changed from

==========
NOT_FOR_COMMIT: [NoStatePrefetch] Kill renderer after preload scanning

Seems to work, a few issues still to be resolved:
* no tests
* checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=none
==========

to

==========
NOT_FOR_COMMIT: [NoStatePrefetch] Kill renderer after preload scanning

Seems to work, a few issues still to be resolved:
* no tests
* checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=none
==========

[pasko, 2016-10-11 18:51:33]

pasko@chromium.org changed reviewers:
+ clamy@chromium.org, droger@chromium.org, mattcary@chromium.org

[pasko, 2016-10-11 19:05:54]

Description was changed from

==========
NOT_FOR_COMMIT: [NoStatePrefetch] Kill renderer after preload scanning

Seems to work, a few issues still to be resolved:
* no tests
* checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=none
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

Seems to work, a few issues still to be resolved:
* no tests
* checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=none
==========

[pasko, 2016-10-11 19:09:27]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

Seems to work, a few issues still to be resolved:
* no tests
* checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=none
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

This is not ready yet, asking for early feedback from reviewers.

A few issues still need to be resolved:
* Need to add browser tests.
* Checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=649632
==========

[pasko, 2016-10-11 19:09:46]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

This is not ready yet, asking for early feedback from reviewers.

A few issues still need to be resolved:
* Need to add browser tests.
* Checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

This is not ready yet, asking for early feedback from reviewers.

A few issues still need to be resolved:
* Need to add browser tests.
* Checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=649632
==========

[pasko, 2016-10-11 19:13:41]

clamy: PTaL at changes in content/ (checkdeps violation and the un-beautiful use
of the RenderProcessHost, I could not find
simple-but-more-architecturally-correct ways to do it)

droger, mattcary: PTaL, adding tests is the only major change in this CL I am
planning (in the best case scenario).

Caveat: With all these layers, I don't know what I am doing.

[droger, 2016-10-12 09:12:58]

This seems like a good approach.

https://codereview.chromium.org/2411863002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2411863002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:927: // Early
determine if running a prefetch. At later stages the |document()| may
Nit: You're still calling document() at the end. Maybe this comment is not
accurate?

[clamy, 2016-10-12 11:19:58]

Thanks! Some comments see below.

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_helper.cc:74: void
PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {
A few things here:

1) It's weird to use a ViewHostMsg for something that is keyed by a frame. This
immediately begets the question: what happens with iframes? Generally speaking
we tend to reserve View*Msg for things that interact with a RenderView* and use
Frame*Msg for things that interact with a RenderFrame*.

2) This is rightly causing a CHECKDEPS violation, and I would not support adding
an exception to the rule since this is a layering violation. You have two
alternatives there: either you add plumbing to RenderFrame so that it will send
a FrameHostMsg itself, or use a PrerenderHostMsg that you send from here. Since
the rest of the content/ code seems to be just forwarding this notification to
an observer in the chrome layer, I would go with the second solution, which
skips any modification to the content code.

3) If more changes to content/ are needed, maybe we should think seriously about
moving the prefetch code from chrome/ to content/. A situation where content/
half knows about it isn't ideal IMO.

[pasko, 2016-10-12 13:59:40]

TL;DR: clamy: looking at fixing the layering violation, droger: Done.

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_helper.cc:74: void
PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {
On 2016/10/12 11:19:57, clamy wrote:
> A few things here:

Thanks!

> 1) It's weird to use a ViewHostMsg for something that is keyed by a frame.
This
> immediately begets the question: what happens with iframes? Generally speaking
> we tend to reserve View*Msg for things that interact with a RenderView* and
use
> Frame*Msg for things that interact with a RenderFrame*.

Feels weird to me as well. I will attempt to change that and will let you know
how it goes.

PrerenderManager keys its slaves by RenderProcessHost*, so I thought about
sending a FrameHostMsg, but (a) was not sure whether it shares the channel with
PrerenderHelper and (b) found that it would need to be plumbed back from the
Frame to Process on the browser side, and seemed to be partially in content/,
which I was not happy about, did not look very closely though.

> 2) This is rightly causing a CHECKDEPS violation, and I would not support
adding
> an exception to the rule since this is a layering violation.

Ah, just realized what this violation actually is about: I saw includes of
content/public and thought that content/common was also OK, but apparently
content/common is internal to content? That makes sense.

> You have two
> alternatives there: either you add plumbing to RenderFrame so that it will
send
> a FrameHostMsg itself,

See my thoughts about it above.

> or use a PrerenderHostMsg that you send from here. Since
> the rest of the content/ code seems to be just forwarding this notification to
> an observer in the chrome layer, I would go with the second solution, which
> skips any modification to the content code.

I considered it too, one problem was that currently PrerenderHostMsgs are only
handled for link rel=prerender messages, and are routed with help of
PrerenderLinkManager, which was not trivial to change for some reason. OK, this
would be my first thing to try now.

> 3) If more changes to content/ are needed, maybe we should think seriously
about
> moving the prefetch code from chrome/ to content/. A situation where content/
> half knows about it isn't ideal IMO.

Sounds good. Currently we just hooked into Prerender to be able to do A/B
testing more comfortably and be able to move clients to the new thing one by
one. Once Prerender code is removed, we will know more what Prefetch requires to
touch, and it will then be easier to move some parts to content/ in case there
are benefits of doing so. We have not figured multi-prefetch in a process yet,
for example.

https://codereview.chromium.org/2411863002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2411863002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:927: // Early
determine if running a prefetch. At later stages the |document()| may
On 2016/10/12 09:12:58, droger wrote:
> Nit: You're still calling document() at the end. Maybe this comment is not
> accurate?

The comment is accurate, but confusing a lot, I agree. It seems that in the
prefetch cases the |document()| document does not vanish so early. I am actually
not sure why I wanted the notification to be sent so late in the
HTMLDocumentParser::end(), after all, all the requests were already made. Moved
it to be the first thing to do, and it eliminates the need in this confusing
comment.

[mattcary, 2016-10-12 14:09:17]

https://codereview.chromium.org/2411863002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.h (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.h:70:
FINAL_STATUS_PREFETCH_FINISHED = 55,
nit: it seems likely in the future we'll have different kinds of prefetch, so
adding NOSTATE (as is done in the full description) seems reasonable.

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_helper.cc:74: void
PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {
On 2016/10/12 13:59:40, pasko wrote:
> On 2016/10/12 11:19:57, clamy wrote:
> > A few things here:
> 
> Thanks!
> 
> > 1) It's weird to use a ViewHostMsg for something that is keyed by a frame.
> This
> > immediately begets the question: what happens with iframes? Generally
speaking
> > we tend to reserve View*Msg for things that interact with a RenderView* and
> use
> > Frame*Msg for things that interact with a RenderFrame*.
> 
> Feels weird to me as well. I will attempt to change that and will let you know
> how it goes.
> 
> PrerenderManager keys its slaves by RenderProcessHost*, so I thought about
> sending a FrameHostMsg, but (a) was not sure whether it shares the channel
with
> PrerenderHelper and (b) found that it would need to be plumbed back from the
> Frame to Process on the browser side, and seemed to be partially in content/,
> which I was not happy about, did not look very closely though.
> 
> > 2) This is rightly causing a CHECKDEPS violation, and I would not support
> adding
> > an exception to the rule since this is a layering violation.
> 
> Ah, just realized what this violation actually is about: I saw includes of
> content/public and thought that content/common was also OK, but apparently
> content/common is internal to content? That makes sense.
> 
> > You have two
> > alternatives there: either you add plumbing to RenderFrame so that it will
> send
> > a FrameHostMsg itself,
> 
> See my thoughts about it above.
> 
> > or use a PrerenderHostMsg that you send from here. Since
> > the rest of the content/ code seems to be just forwarding this notification
to
> > an observer in the chrome layer, I would go with the second solution, which
> > skips any modification to the content code.
> 
> I considered it too, one problem was that currently PrerenderHostMsgs are only
> handled for link rel=prerender messages, and are routed with help of
> PrerenderLinkManager, which was not trivial to change for some reason. OK,
this
> would be my first thing to try now.
> 
> > 3) If more changes to content/ are needed, maybe we should think seriously
> about
> > moving the prefetch code from chrome/ to content/. A situation where
content/
> > half knows about it isn't ideal IMO.
> 
> Sounds good. Currently we just hooked into Prerender to be able to do A/B
> testing more comfortably and be able to move clients to the new thing one by
> one. Once Prerender code is removed, we will know more what Prefetch requires
to
> touch, and it will then be easier to move some parts to content/ in case there
> are benefits of doing so. We have not figured multi-prefetch in a process yet,
> for example.

Silly question: is the render_process_id_ that the link manager has the same id
as from prerender_contents->GetRenderViewHost()->GetProcess()->GetId()?

[clamy, 2016-10-12 14:16:26]

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_helper.cc:74: void
PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {
On 2016/10/12 13:59:40, pasko wrote:
> On 2016/10/12 11:19:57, clamy wrote:
> > A few things here:
> 
> Thanks!
> 
> > 1) It's weird to use a ViewHostMsg for something that is keyed by a frame.
> This
> > immediately begets the question: what happens with iframes? Generally
speaking
> > we tend to reserve View*Msg for things that interact with a RenderView* and
> use
> > Frame*Msg for things that interact with a RenderFrame*.
> 
> Feels weird to me as well. I will attempt to change that and will let you know
> how it goes.
> 
> PrerenderManager keys its slaves by RenderProcessHost*, so I thought about
> sending a FrameHostMsg, but (a) was not sure whether it shares the channel
with
> PrerenderHelper and (b) found that it would need to be plumbed back from the
> Frame to Process on the browser side, and seemed to be partially in content/,
> which I was not happy about, did not look very closely though.

Regarding the RenderProcessHost* issue, the RenderProcessHost* you would get at
from RenderFrameHost is the same as the one from the RenderViewHost unless the
frame is in a different process from the top level frame (OOPIF, Top Document
isolation etc...). In that case, the RenderProcessHost* reflects which process
the frame happens to be rendered in. You could still get the top RPH by looking
at the RPH for the root of the FrameTree.

> 
> > 2) This is rightly causing a CHECKDEPS violation, and I would not support
> adding
> > an exception to the rule since this is a layering violation.
> 
> Ah, just realized what this violation actually is about: I saw includes of
> content/public and thought that content/common was also OK, but apparently
> content/common is internal to content? That makes sense.

Precisely - you can include anything that is in content/public/* outside of
content/ but not the rest. In that case, content/public/common/ is fine, but not
content/common.

> 
> > You have two
> > alternatives there: either you add plumbing to RenderFrame so that it will
> send
> > a FrameHostMsg itself,
> 
> See my thoughts about it above.
> 
> > or use a PrerenderHostMsg that you send from here. Since
> > the rest of the content/ code seems to be just forwarding this notification
to
> > an observer in the chrome layer, I would go with the second solution, which
> > skips any modification to the content code.
> 
> I considered it too, one problem was that currently PrerenderHostMsgs are only
> handled for link rel=prerender messages, and are routed with help of
> PrerenderLinkManager, which was not trivial to change for some reason. OK,
this
> would be my first thing to try now.
> 
> > 3) If more changes to content/ are needed, maybe we should think seriously
> about
> > moving the prefetch code from chrome/ to content/. A situation where
content/
> > half knows about it isn't ideal IMO.
> 
> Sounds good. Currently we just hooked into Prerender to be able to do A/B
> testing more comfortably and be able to move clients to the new thing one by
> one. Once Prerender code is removed, we will know more what Prefetch requires
to
> touch, and it will then be easier to move some parts to content/ in case there
> are benefits of doing so. We have not figured multi-prefetch in a process yet,
> for example.

[pasko, 2016-10-12 14:28:09]

https://codereview.chromium.org/2411863002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.h (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.h:70:
FINAL_STATUS_PREFETCH_FINISHED = 55,
On 2016/10/12 14:09:17, mattcary wrote:
> nit: it seems likely in the future we'll have different kinds of prefetch, so
> adding NOSTATE (as is done in the full description) seems reasonable.

Done.

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_helper.cc:74: void
PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {
On 2016/10/12 14:16:26, clamy wrote:
> On 2016/10/12 13:59:40, pasko wrote:
> > On 2016/10/12 11:19:57, clamy wrote:
> > > A few things here:
> > 
> > Thanks!
> > 
> > > 1) It's weird to use a ViewHostMsg for something that is keyed by a frame.
> > This
> > > immediately begets the question: what happens with iframes? Generally
> speaking
> > > we tend to reserve View*Msg for things that interact with a RenderView*
and
> > use
> > > Frame*Msg for things that interact with a RenderFrame*.
> > 
> > Feels weird to me as well. I will attempt to change that and will let you
know
> > how it goes.
> > 
> > PrerenderManager keys its slaves by RenderProcessHost*, so I thought about
> > sending a FrameHostMsg, but (a) was not sure whether it shares the channel
> with
> > PrerenderHelper and (b) found that it would need to be plumbed back from the
> > Frame to Process on the browser side, and seemed to be partially in
content/,
> > which I was not happy about, did not look very closely though.
> 
> Regarding the RenderProcessHost* issue, the RenderProcessHost* you would get
at
> from RenderFrameHost is the same as the one from the RenderViewHost unless the
> frame is in a different process from the top level frame (OOPIF, Top Document
> isolation etc...). In that case, the RenderProcessHost* reflects which process
> the frame happens to be rendered in. You could still get the top RPH by
looking
> at the RPH for the root of the FrameTree.

Thanks. We are prefetching a top-level frame only for now, so this is not an
issue.

Also, even though ViewHostMsg_ is sent here, it is terminated at the
RenderProcessHost as a control message (i.e. without routing), which is another
uncomfortable weirdness. Going to change that now.

https://codereview.chromium.org/2411863002/diff/1/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_helper.cc:74: void
PrerenderHelper::SendPrefetchFinished(content::RenderFrame* render_frame) {
On 2016/10/12 14:09:17, mattcary wrote:
> On 2016/10/12 13:59:40, pasko wrote:
> > On 2016/10/12 11:19:57, clamy wrote:
> > > A few things here:
> > 
> > Thanks!
> > 
> > > 1) It's weird to use a ViewHostMsg for something that is keyed by a frame.
> > This
> > > immediately begets the question: what happens with iframes? Generally
> speaking
> > > we tend to reserve View*Msg for things that interact with a RenderView*
and
> > use
> > > Frame*Msg for things that interact with a RenderFrame*.
> > 
> > Feels weird to me as well. I will attempt to change that and will let you
know
> > how it goes.
> > 
> > PrerenderManager keys its slaves by RenderProcessHost*, so I thought about
> > sending a FrameHostMsg, but (a) was not sure whether it shares the channel
> with
> > PrerenderHelper and (b) found that it would need to be plumbed back from the
> > Frame to Process on the browser side, and seemed to be partially in
content/,
> > which I was not happy about, did not look very closely though.
> > 
> > > 2) This is rightly causing a CHECKDEPS violation, and I would not support
> > adding
> > > an exception to the rule since this is a layering violation.
> > 
> > Ah, just realized what this violation actually is about: I saw includes of
> > content/public and thought that content/common was also OK, but apparently
> > content/common is internal to content? That makes sense.
> > 
> > > You have two
> > > alternatives there: either you add plumbing to RenderFrame so that it will
> > send
> > > a FrameHostMsg itself,
> > 
> > See my thoughts about it above.
> > 
> > > or use a PrerenderHostMsg that you send from here. Since
> > > the rest of the content/ code seems to be just forwarding this
notification
> to
> > > an observer in the chrome layer, I would go with the second solution,
which
> > > skips any modification to the content code.
> > 
> > I considered it too, one problem was that currently PrerenderHostMsgs are
only
> > handled for link rel=prerender messages, and are routed with help of
> > PrerenderLinkManager, which was not trivial to change for some reason. OK,
> this
> > would be my first thing to try now.
> > 
> > > 3) If more changes to content/ are needed, maybe we should think seriously
> > about
> > > moving the prefetch code from chrome/ to content/. A situation where
> content/
> > > half knows about it isn't ideal IMO.
> > 
> > Sounds good. Currently we just hooked into Prerender to be able to do A/B
> > testing more comfortably and be able to move clients to the new thing one by
> > one. Once Prerender code is removed, we will know more what Prefetch
requires
> to
> > touch, and it will then be easier to move some parts to content/ in case
there
> > are benefits of doing so. We have not figured multi-prefetch in a process
yet,
> > for example.
> 
> Silly question: is the render_process_id_ that the link manager has the same
id
> as from prerender_contents->GetRenderViewHost()->GetProcess()->GetId()?

That's a good suggestion, thanks!

[pasko, 2016-10-14 14:19:42]

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

[commit-bot: I haz the power, 2016-10-14 14:19:57]

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

[pasko, 2016-10-14 14:40:37]

clamy: PTAL

the new version just uploaded sends a PrerenderHostMsg_. In the past those were
used only to notify about link elements inserted/deleted, now it looks slightly
weird to notify about prefetches in WebPrerenderingSupport, but I think it is OK
for now, and anyway .. this is in blink.

[pasko, 2016-10-14 14:42:01]

oh, need to update some webkit_unit_tests to make them compile .. looking into
it

[commit-bot: I haz the power, 2016-10-14 14:48:18]

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

[commit-bot: I haz the power, 2016-10-14 14:48:19]

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

[pasko, 2016-10-14 15:32:35]

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

[commit-bot: I haz the power, 2016-10-14 15:32:57]

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

[pasko, 2016-10-14 15:35:05]

On 2016/10/14 14:42:01, pasko wrote:
> oh, need to update some webkit_unit_tests to make them compile .. looking into
> it

this should be resolved now, waiting for more tweaks suggested by bots, PTAL

[clamy, 2016-10-14 15:42:20]

The general architecture looks reasonable to me, but I'm not an owner nor very
proficient with prerender. In any case, from a layering perspective this is
better :).

[mattcary, 2016-10-14 15:53:21]

lgtm

[commit-bot: I haz the power, 2016-10-14 19:05:01]

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

[commit-bot: I haz the power, 2016-10-14 19:05:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pasko, 2016-10-17 10:14:02]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

This is not ready yet, asking for early feedback from reviewers.

A few issues still need to be resolved:
* Need to add browser tests.
* Checkdeps fails with:
  chrome/renderer/prerender/prerender_helper.cc
        Illegal include: "content/common/view_messages.h"
  (not sure whether adding a checkdeps rule is appropriate here or there is a
   better layering/routing)

BUG=649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

The design doc is linked in the first bug. Main goal: remove Prerender.

This does not add browsertests yet, which are planned really soon, after the
infrastructure for it lands and sticks:
https://codereview.chromium.org/2304953002/

BUG=632361,649632
==========

[pasko, 2016-10-17 10:15:30]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

The design doc is linked in the first bug. Main goal: remove Prerender.

This does not add browsertests yet, which are planned really soon, after the
infrastructure for it lands and sticks:
https://codereview.chromium.org/2304953002/

BUG=632361,649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

The design doc is linked in the first bug. Main goal: remove Prerender.

This does not add browsertests yet, which are planned really soon, after the
infrastructure for it lands and sticks:
https://codereview.chromium.org/2304953002/

BUG=632361,649632
==========

[pasko, 2016-10-17 10:15:31]

pasko@chromium.org changed reviewers:
+ mkwst@chromium.org
- clamy@chromium.org

[pasko, 2016-10-17 10:19:35]

mkwst: please take a look at the new IPC and the Blink side

droger: this is now somewhat finalized, PTAL

[pasko, 2016-10-17 11:06:27]

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

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

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

[Mike West, 2016-10-17 11:20:09]

IPC LGTM.

[pasko, 2016-10-17 11:35:28]

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

[pasko, 2016-10-17 11:37:24]

csharrison: please review a small change in core/html/parser

[commit-bot: I haz the power, 2016-10-17 12:07:22]

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

[commit-bot: I haz the power, 2016-10-17 12:07:23]

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_...)

[Charlie Harrison, 2016-10-17 13:25:50]

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:926: if
(document()->isPrefetchOnly())
This might make a little more sense in Document::finishedParsing rather than
here. That's generally where document-y things hook parse finish.

[pasko, 2016-10-17 14:25:11]

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:926: if
(document()->isPrefetchOnly())
On 2016/10/17 13:25:50, Charlie Harrison wrote:
> This might make a little more sense in Document::finishedParsing rather than
> here. That's generally where document-y things hook parse finish.

OK!

Any recommendation on what part of Document::finishedParsing() is more suitable?
I see parser wrap up closer to the end, can put these new checks at the very
end, would it be the most readable place? Would the Document.this still be alive
at that point?

I'll try that after I rebase (which is a non-trivial rebase, mattcary@ added
tests that are relying on old behavior).

[Charlie Harrison, 2016-10-17 14:49:17]

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:926: if
(document()->isPrefetchOnly())
On 2016/10/17 14:25:11, pasko wrote:
> On 2016/10/17 13:25:50, Charlie Harrison wrote:
> > This might make a little more sense in Document::finishedParsing rather than
> > here. That's generally where document-y things hook parse finish.
> 
> OK!
> 
> Any recommendation on what part of Document::finishedParsing() is more
suitable?
> I see parser wrap up closer to the end, can put these new checks at the very
> end, would it be the most readable place? Would the Document.this still be
alive
> at that point?
> 
> I'll try that after I rebase (which is a non-trivial rebase, mattcary@ added
> tests that are relying on old behavior).

Adding the call to the very end SGTM. Yeah the Document should still be alive by
the end of that method.

[droger, 2016-10-24 11:32:41]

LGTM sorry for the delay.

[pasko, 2016-10-25 12:43:43]

Patchset #8 (id:140001) has been deleted

[pasko, 2016-10-25 12:43:54]

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

[commit-bot: I haz the power, 2016-10-25 12:44:07]

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-25 13:45:18]

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

[commit-bot: I haz the power, 2016-10-25 13:45:19]

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_...)

[pasko, 2016-10-25 14:21:37]

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

[commit-bot: I haz the power, 2016-10-25 14:21:50]

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

[pasko, 2016-10-25 14:43:40]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

The design doc is linked in the first bug. Main goal: remove Prerender.

This does not add browsertests yet, which are planned really soon, after the
infrastructure for it lands and sticks:
https://codereview.chromium.org/2304953002/

BUG=632361,649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632
==========

[pasko, 2016-10-25 14:49:40]

Merged this with the browsertests from mattcary@. (finaly)

Tests should pass now! Had to disable two tests, and re-enabled one more test,
which seems non-racy any more.

csharrison: PTAL

droger: there are non-trivial changes in
prerender_nostate_prefetch_browsertest.cc and prerender_test_utils.* to
accommodate for PrerenderContents disappearing before page loads happen, I've
put more details about these changes in the CL description. PTAL.

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2411863002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:926: if
(document()->isPrefetchOnly())
On 2016/10/17 14:49:16, Charlie Harrison wrote:
> On 2016/10/17 14:25:11, pasko wrote:
> > On 2016/10/17 13:25:50, Charlie Harrison wrote:
> > > This might make a little more sense in Document::finishedParsing rather
than
> > > here. That's generally where document-y things hook parse finish.
> > 
> > OK!
> > 
> > Any recommendation on what part of Document::finishedParsing() is more
> suitable?
> > I see parser wrap up closer to the end, can put these new checks at the very
> > end, would it be the most readable place? Would the Document.this still be
> alive
> > at that point?
> > 
> > I'll try that after I rebase (which is a non-trivial rebase, mattcary@ added
> > tests that are relying on old behavior).
> 
> Adding the call to the very end SGTM. Yeah the Document should still be alive
by
> the end of that method.

Done.

[Charlie Harrison, 2016-10-25 15:05:27]

core/dom non-owner lgtm w/ nit.

https://codereview.chromium.org/2411863002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2411863002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:386: void onPrefetchFinished()
const;
Can this be private?

[pasko, 2016-10-25 15:17:30]

https://codereview.chromium.org/2411863002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2411863002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:386: void onPrefetchFinished()
const;
On 2016/10/25 15:05:27, Charlie Harrison wrote:
> Can this be private?

Good catch. Actually now the whole method is not needed, inlined its 1-line
contents into the Document::finishedParsing().

[Charlie Harrison, 2016-10-25 15:18:19]

https://codereview.chromium.org/2411863002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2411863002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:386: void onPrefetchFinished()
const;
On 2016/10/25 15:17:30, pasko wrote:
> On 2016/10/25 15:05:27, Charlie Harrison wrote:
> > Can this be private?
> 
> Good catch. Actually now the whole method is not needed, inlined its 1-line
> contents into the Document::finishedParsing().

That works too :)

[pasko, 2016-10-25 15:31:37]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632
==========

[pasko, 2016-10-25 15:31:37]

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

[pasko, 2016-10-25 15:32:14]

jochen: please review changes in third_party/WebKit/Source/core/dom

[pasko, 2016-10-25 15:33:08]

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

[commit-bot: I haz the power, 2016-10-25 15:33:31]

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-25 16:58:35]

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

[commit-bot: I haz the power, 2016-10-25 16:58:36]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-26 08:17:48]

lgtm

[droger, 2016-10-26 08:52:16]

still lgtm

https://codereview.chromium.org/2411863002/diff/200001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2411863002/diff/200001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:424: // URLs
are ignored in renderers, and teh test server has no support for them.
s/teh/the/

[pasko, 2016-10-26 09:54:52]

thanks all for review! time to land

https://codereview.chromium.org/2411863002/diff/200001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2411863002/diff/200001/chrome/browser/prerend...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:424: // URLs
are ignored in renderers, and teh test server has no support for them.
On 2016/10/26 08:52:16, droger wrote:
> s/teh/the/

Done.

[pasko, 2016-10-26 10:03:31]

The CQ bit was checked by pasko@chromium.org

[pasko, 2016-10-26 10:03:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org, mkwst@chromium.org, csharrison@chromium.org,
droger@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2411863002/#ps240001
(title: "fixed a comment, removed unnecessary change from disabled test")

[commit-bot: I haz the power, 2016-10-26 10:03:56]

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-26 11:06:54]

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

[commit-bot: I haz the power, 2016-10-26 11:06:55]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[pasko, 2016-10-26 11:30:21]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2016-10-26 11:30:41]

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-26 13:48:12]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632
==========

[commit-bot: I haz the power, 2016-10-26 13:48:14]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-10-26 13:49:59]

Description was changed from

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632
==========

to

==========
[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to
allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361,649632

Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
Cr-Commit-Position: refs/heads/master@{#427678}
==========

[commit-bot: I haz the power, 2016-10-26 13:50:02]

Patchset 12 (id:??) landed as
https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
Cr-Commit-Position: refs/heads/master@{#427678}

[foolip, 2016-10-27 09:07:11]

A revert of this CL (patchset #12 id:240001) has been created in
https://codereview.chromium.org/2452313002/ by foolip@chromium.org.

The reason for reverting is:
"NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
shortly after this landed.

BUG=659697
.