Make the renderer-side prerendering code use RenderFrames instead of RenderViews.

Make the renderer-side prerendering code use RenderFrames instead of RenderViews.

This is also part of the work of making ContentRendererClient's OverrideCreatePlugin and CreatePluginReplacement take only a RenderFrame.

BUG=304341
R=creis@chromium.org, mmenke@chromium.org, tburkard@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239860

[jam, 2013-12-09 22:36:43]

Timo: prerendering code
Charlie: the rest

[Charlie Reis, 2013-12-10 00:31:48]

content/ LGTM with the nits below.  I'll let Timo handle the chrome/
prerendering stuff.

https://codereview.chromium.org/107893003/diff/180001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/107893003/diff/180001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:78: int64 parent_frame_tree_node_id,
nit: Wrong indent now.

https://codereview.chromium.org/107893003/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/107893003/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2772:
FOR_EACH_OBSERVER(WebContentsObserver,
Maybe mention that this is only for subframes, and that the main frame
notification goes out in RenderViewCreated.

https://codereview.chromium.org/107893003/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2846: // frame would be
dropped because it's not created yet).
If I understand correctly, maybe say "main RenderFrameHostImpl" instead of "main
render frame."

(I think it's basically saying that we wait to notify observers until the
renderer-side RenderFrame is created, just as we do for RenderViews.)

[jam, 2013-12-10 00:48:42]

https://codereview.chromium.org/107893003/diff/180001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/107893003/diff/180001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:78: int64 parent_frame_tree_node_id,
On 2013/12/10 00:31:48, creis wrote:
> nit: Wrong indent now.

Done.

https://codereview.chromium.org/107893003/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/107893003/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2772:
FOR_EACH_OBSERVER(WebContentsObserver,
On 2013/12/10 00:31:48, creis wrote:
> Maybe mention that this is only for subframes, and that the main frame
> notification goes out in RenderViewCreated.

Done.

https://codereview.chromium.org/107893003/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2846: // frame would be
dropped because it's not created yet).
On 2013/12/10 00:31:48, creis wrote:
> If I understand correctly, maybe say "main RenderFrameHostImpl" instead of
"main
> render frame."
> 
> (I think it's basically saying that we wait to notify observers until the
> renderer-side RenderFrame is created, just as we do for RenderViews.)

Done.

[tburkard, 2013-12-10 16:26:26]

lgtm

but mmenke should take a quick look, too, since he is more familiar with this
code than i am.

[mmenke, 2013-12-10 16:27:40]

On 2013/12/10 16:26:26, tburkard wrote:
> lgtm
> 
> but mmenke should take a quick look, too, since he is more familiar with this
> code than i am.

I'll try to get to this later this afternoon.  If not, tomorrow.

[mmenke, 2013-12-10 20:56:41]

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:558:
render_frame_host->GetRoutingID(), true));
Are there any races here?  I'm not familiar with frame creation/startup, but if
a frame can start loading stuff before this happens, that would be bad.

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.h:167: void PrerenderingDone();
I think this method name is ambiguous, since prerendering is also done when we
decide we aren't using the contents, and destroy it.  Maybe OnBeforeUse /
OnBeforeSwap?  Or OnUse / OnSwap / OnUsePrerender?

Of better yet, do we even need this function?  Seems like it would make sense if
the code were moved to PrepareForUse().

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.h:446: // The RenderFrameHosts for
this webcontents.
Suggest webcontents -> WebContents (or "this webcontents" ->
"prerender_contents_")

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:356: new
ChromeRenderFrameObserver(render_frame);
I'm assuming this is this called for the main frame?  If so, the fact that this
is called for the main frame, but WebContentsObserver::RenderFrameCreated is not
seems like a recipe for bugs.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/plugins...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/plugins...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:60: 
nit:  Blank lines at the start of function bodies are frowned upon by a lot of
reviewers.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/plugins...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:62: // interest in them.
May want to replace "multiple blocked plugins" with "multiple blocked plugins
and other objects".  The PrerenderHelper and PrerenderMediaLoadDeferrer also
listen for this message.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/prerend...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/prerend...
chrome/renderer/prerender/prerender_helper.cc:67: content::RenderFrame* view =
render_frame();
nit:  view -> frame?

https://codereview.chromium.org/107893003/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/107893003/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2773: // happens in
RenderViewCreated.
Shouldn't this comment be in the WebContentsObserver header?  Seems like that's
where most people who work on WebContentsObservers will look.

[jam, 2013-12-10 21:14:17]

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:558:
render_frame_host->GetRoutingID(), true));
On 2013/12/10 20:56:41, mmenke wrote:
> Are there any races here?  I'm not familiar with frame creation/startup, but
if
> a frame can start loading stuff before this happens, that would be bad.

no, this is called from the same place that
NOTIFICATION_WEB_CONTENTS_RENDER_VIEW_HOST_CREATED (where we used ot do this) is
fired: WebContentsImpl::RenderViewCreated

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.h:167: void PrerenderingDone();
On 2013/12/10 20:56:41, mmenke wrote:
> I think this method name is ambiguous, since prerendering is also done when we
> decide we aren't using the contents, and destroy it.  Maybe OnBeforeUse /
> OnBeforeSwap?  Or OnUse / OnSwap / OnUsePrerender?
> 
> Of better yet, do we even need this function?  Seems like it would make sense
if
> the code were moved to PrepareForUse().

ah, I missed that method. Switched to it.

https://codereview.chromium.org/107893003/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.h:446: // The RenderFrameHosts for
this webcontents.
On 2013/12/10 20:56:41, mmenke wrote:
> Suggest webcontents -> WebContents (or "this webcontents" ->
> "prerender_contents_")

Done.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:356: new
ChromeRenderFrameObserver(render_frame);
On 2013/12/10 20:56:41, mmenke wrote:
> I'm assuming this is this called for the main frame?

It's called for every RenderFrame that's created.

Right now, we have one RenderFrame per RenderView. That will change soon and
there will be a RenderFrame per blink::WebFrame. The code will work with either
scenario though.

>  If so, the fact that this
> is called for the main frame, but WebContentsObserver::RenderFrameCreated is
not
> seems like a recipe for bugs.

WCO::RenderFrameCreated is created for every RenderFrameHost, and there's a 1:1
mapping between RenderFrameHost and RenderFrame.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/plugins...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/plugins...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:60: 
On 2013/12/10 20:56:41, mmenke wrote:
> nit:  Blank lines at the start of function bodies are frowned upon by a lot of
> reviewers.

Done.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/plugins...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:62: // interest in them.
On 2013/12/10 20:56:41, mmenke wrote:
> May want to replace "multiple blocked plugins" with "multiple blocked plugins
> and other objects".  The PrerenderHelper and PrerenderMediaLoadDeferrer also
> listen for this message.

I just copied this code, but ok.

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/prerend...
File chrome/renderer/prerender/prerender_helper.cc (right):

https://codereview.chromium.org/107893003/diff/200001/chrome/renderer/prerend...
chrome/renderer/prerender/prerender_helper.cc:67: content::RenderFrame* view =
render_frame();
On 2013/12/10 20:56:41, mmenke wrote:
> nit:  view -> frame?

Done.

https://codereview.chromium.org/107893003/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/107893003/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2773: // happens in
RenderViewCreated.
On 2013/12/10 20:56:41, mmenke wrote:
> Shouldn't this comment be in the WebContentsObserver header?

I specifically avoided that because it's an implementation detail of content,
and may change in the future.

Embedders should only care/know that RenderFrameCreated is called for every
RenderFrame.

>  Seems like that's
> where most people who work on WebContentsObservers will look.

[mmenke, 2013-12-10 21:26:39]

LGTM

https://codereview.chromium.org/107893003/diff/200001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/107893003/diff/200001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2773: // happens in
RenderViewCreated.
On 2013/12/10 21:14:17, jam wrote:
> On 2013/12/10 20:56:41, mmenke wrote:
> > Shouldn't this comment be in the WebContentsObserver header?
> 
> I specifically avoided that because it's an implementation detail of content,
> and may change in the future.
> 
> Embedders should only care/know that RenderFrameCreated is called for every
> RenderFrame.

Sorry, I completely misunderstood the comment (I was thinking it meant
RenderFrameCreated was only called for subframes, completely missing the other
call below).