Give prerendering RVH's RenderWidgetHostViews.

Adds a RenderWidgetHostView for prerendering RenderViewHosts.
This fixes prerendered RenderViews having 0 size until
they're displayed.

Also cancels prerendering if the source RenderViewHost is
closed before prerendering starts.

BUG=71221
TEST=PrerenderBrowserTest.PrerenderSize
PrerenderManagerTest.SourceRenderViewClosed
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82491

[mmenke, 2011-04-19 16:22:20]

John:  Please review the changes to render_view_host_manager.cc.  Also, please
review the portion of prerender_render_widget_host_view dealing with
plugins/compositing, as it looks like you've reviewed a number of CLs for both.

Scott:  Please review render_view_host_manager.cc and
prerender_render_widget_host_view.  Prerendering creates a hidden RenderViewHost
until it is cancelled/times out, or can be swapped into a TabContents in
response to a navigation.  The new class acts as the RVH's view until the
navigation occurs, or it is destroyed.

Chris:  Please review everything.

[jam, 2011-04-19 16:37:53]

I'm familiar with render_view_host_manager.cc, so Brett, please look at those
changes.

As for prerender_render_widget_host_view.cc, I don't see much logic in there,
they all seem to be stub functions.  What in particular would you like me to
look at?

[mmenke, 2011-04-19 16:40:06]

On 2011/04/19 16:37:53, John Abd-El-Malek wrote:
> I'm familiar with render_view_host_manager.cc, so Brett, please look at those
> changes.
> 
> As for prerender_render_widget_host_view.cc, I don't see much logic in there,
> they all seem to be stub functions.  What in particular would you like me to
> look at?

Sorry, should have been clearer.  Just validating the comments.

[jam, 2011-04-19 16:42:11]

On Tue, Apr 19, 2011 at 9:37 AM, <jam@chromium.org> wrote:

> I'm familiar with render_view_host_manager.cc, so Brett, please look at
> those
> changes.
>

that should be *not* familiar

>
> As for prerender_render_widget_host_view.cc, I don't see much logic in
> there,
> they all seem to be stub functions.  What in particular would you like me
> to
> look at?
>
>
> http://codereview.chromium.org/6685012/
>

[brettw, 2011-04-19 16:56:33]

RVHManager LGTM

[mmenke, 2011-04-19 16:57:24]

On 2011/04/19 16:40:06, Matt Menke wrote:
> Sorry, should have been clearer.  Just validating the comments.

Just to elaborate a little - the comments explain why a function won't be called
or why it should cancel the prerender.  If a function is called, and my comments
say it won't be, there may be a bug.

[sky, 2011-04-19 17:08:37]

LGTM

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_render_widget_host_view.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:210: void
PrerenderRenderWidgetHostView::StartPluginIme() {
Add newlines between all methods (you were initially but stopped here for some
reason).

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:694: // Remove old
RenderWidgetHostView before creating a new one.
Please document why you need to remove the old.

[mmenke, 2011-04-19 17:29:38]

Thanks

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_render_widget_host_view.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:210: void
PrerenderRenderWidgetHostView::StartPluginIme() {
On 2011/04/19 17:08:37, sky wrote:
> Add newlines between all methods (you were initially but stopped here for some
> reason).

Done.

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:694: // Remove old
RenderWidgetHostView before creating a new one.
On 2011/04/19 17:08:37, sky wrote:
> Please document why you need to remove the old.

Done.

[cbentzel, 2011-04-19 17:42:41]

I wish there was a way to do this without PrerenderRenderWidgetHostView due to
projected maintenance/complexity burden, but I haven't come up with a good
alternative.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:677: // TODO(mmenke): 
Investigate this.
Add a bug for this. Feels pretty bad - even if you get to a situation where
trybots pass, it may break on other people's machines. Is there a way to
determine whether hardware support is enabled for the current platform at
runtime, and early exit if it's not supported?

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:686: // Checks that scripts
can retrieve the correct window size while preredering.
Nit: s/preredering/prerendering

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:206: pending_final_status <
FINAL_STATUS_MAX);
Do you want to DCHECK_EQ(FINAL_STATUS_MAX, pending_final_status_)? Or can it be
done multiple times?

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.h (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.h:33: FINAL_STATUS_COMPOSITING,
Is this compositing specific, or GPU oriented? Will stuff like WebGL trigger
this case?

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:189: // no view.  The latter
should only happen when the RenderView has closed.
Is it possible to add a browser test [or at least a unit test] where this case
happens? It sounds like it will be tricky to do due to tight timing, but might
be a good idea. Skipping the prerender in this case makes sense, regardless of
getting the view extents.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:435: }
I'd really like to see a unit test for the PrerenderManager pruning entries
which have a pending final status.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:103: child_id_(-1),
Where was this constructor being called? I'm surprised we didn't get any
warnings for uninitialized values. Thanks for fixing.

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:706: //
TODO(tburkard,cbentzel): Figure out if this hack can be removed.
You inherited this, but please add http://crbug.com/79891 link here.

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_render_widget_host_view.cc (right):

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:27:
cocoa_view_bounds_ = view->GetViewBounds();
Should this be done inside of SetBounds?

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:32: //
PrerenderRenderWidgetHostViews are always hidden.  This has reduces the
nit: This reduces the

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:43: void
PrerenderRenderWidgetHostView::InitAsFullscreen() {
What happens if a prerender is triggered while Chrome is in full screen mode?

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:224: NOTREACHED();
Why is this NOTREACHED() with stuff below it? Where is this called?

[mmenke, 2011-04-19 20:00:01]

Thanks.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:677: // TODO(mmenke): 
Investigate this.
On 2011/04/19 17:42:41, cbentzel wrote:
> Add a bug for this. Feels pretty bad - even if you get to a situation where
> trybots pass, it may break on other people's machines. Is there a way to
> determine whether hardware support is enabled for the current platform at
> runtime, and early exit if it's not supported?

Bug filed.  I couldn't find a way to do so, but I'm sure it's possible.  The
renderer process apparently knows not to send us compositing requests, somehow,
afterall.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:686: // Checks that scripts
can retrieve the correct window size while preredering.
On 2011/04/19 17:42:41, cbentzel wrote:
> Nit: s/preredering/prerendering
Done.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:206: pending_final_status <
FINAL_STATUS_MAX);
On 2011/04/19 17:42:41, cbentzel wrote:
> Do you want to DCHECK_EQ(FINAL_STATUS_MAX, pending_final_status_)? Or can it
be
> done multiple times?

It can be done multiple times.  I just wait for the next call to
DeleteOldEntries() before actual deletion, and anything can happen in the
meantime.  There's no way to prevent this, though I could keep the first "final"
status we get, instead of the final one.  I don't think it really matters,
either way.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.h (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.h:33: FINAL_STATUS_COMPOSITING,
On 2011/04/19 17:42:41, cbentzel wrote:
> Is this compositing specific, or GPU oriented? Will stuff like WebGL trigger
> this case?

WebGL uses compositing, I believe, so it will.  Basically anything that causes
the renderer to ask us for a window handle that it can draw to will result in
this failure, since we can't provide one.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:189: // no view.  The latter
should only happen when the RenderView has closed.
On 2011/04/19 17:42:41, cbentzel wrote:
> Is it possible to add a browser test [or at least a unit test] where this case
> happens? It sounds like it will be tricky to do due to tight timing, but might
> be a good idea. Skipping the prerender in this case makes sense, regardless of
> getting the view extents. 

I'm not sure about a browser test using our current framework.  Simplest way may
be to create a TestPrerenderManager and have it destroy the original
RenderViewHost in AddPreload.

I suppose we could create a prerender request for a non-existent RenderView in a
browser test, but at that point, I'm not sure how much we gain from not just
doing a unit test instead.

I've added the aforementioned unit test, and I've filed a bug for a browser
test.

Also, I added a FinalStatus for cancelling in this case, which I had forgotten
to do.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:435: }
On 2011/04/19 17:42:41, cbentzel wrote:
> I'd really like to see a unit test for the PrerenderManager pruning entries
> which have a pending final status.

Added two tests for this.

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6685012/diff/61005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:103: child_id_(-1),
On 2011/04/19 17:42:41, cbentzel wrote:
> Where was this constructor being called? I'm surprised we didn't get any
> warnings for uninitialized values. Thanks for fixing.

This is only being used by prerender_resource_handler_unittest.

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6685012/diff/61005/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:706: //
TODO(tburkard,cbentzel): Figure out if this hack can be removed.
On 2011/04/19 17:42:41, cbentzel wrote:
> You inherited this, but please add http://crbug.com/79891 link here. 

Done.

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_render_widget_host_view.cc (right):

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:27:
cocoa_view_bounds_ = view->GetViewBounds();
On 2011/04/19 17:42:41, cbentzel wrote:
> Should this be done inside of SetBounds?

We need the original view to do get those bounds, and I don't think it's worth
the effort to keep track of it.

I don't believe SetBounds() will be called at any other time, but I'm not
completely positive of that.  I suppose we could just ignore SetBounds() and do
everything here, if we really wanted.

I'm not completely sure of just when these bounds are used, either.  The code
mentions popups, but not sure if there more general uses.  One of many things I
want to look into (Including the Windows/Linux equivalents of these, which I
believe are calculated from the native view, so they won't work).

I'll go ahead and file a bug for looking into these issues.

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:32: //
PrerenderRenderWidgetHostViews are always hidden.  This has reduces the
On 2011/04/19 17:42:41, cbentzel wrote:
> nit: This reduces the

Fixed.

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:43: void
PrerenderRenderWidgetHostView::InitAsFullscreen() {
On 2011/04/19 17:42:41, cbentzel wrote:
> What happens if a prerender is triggered while Chrome is in full screen mode?

It seems to work fine.  There's no is_fullscreen() function to cause issues -
just need the right bounds, which we have.

http://codereview.chromium.org/6685012/diff/56022/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_render_widget_host_view.cc:224: NOTREACHED();
On 2011/04/19 17:42:41, cbentzel wrote:
> Why is this NOTREACHED() with stuff below it? Where is this called?

On at least some platforms, compositing calls in to this.  This won't be called
in that particular case, as the compositing functions are also part of RWHV;
however, I could see a future where something else, other than plugins, does so
as well (And does so directly).  I'd rather not break if that happens for the
first time in a release build.

I could just get rid of the NOTREACHED, but I think it would be best if we
noticed if anything new doesn't work with prerendering.

[mmenke, 2011-04-19 20:04:33]

On 2011/04/19 17:42:41, cbentzel wrote:
> I wish there was a way to do this without PrerenderRenderWidgetHostView due to
> projected maintenance/complexity burden, but I haven't come up with a good
> alternative.

Yea, I'd love to not have to mock out such a large class that's used all over
the place.

[Paweł Hajdan Jr., 2011-04-20 06:49:37]

Drive-by with a test comment. No need to wait for me.

http://codereview.chromium.org/6685012/diff/50038/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6685012/diff/50038/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:486: delete pc1;
Could you switch the pointers to scoped_ptrs instead? You can still .reset()
such a pointer, but handling resources manually in tests is a known source of
memory leaks.

[mmenke, 2011-04-20 15:15:19]

http://codereview.chromium.org/6685012/diff/50038/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/6685012/diff/50038/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager_unittest.cc:486: delete pc1;
On 2011/04/20 06:49:37, Paweł Hajdan Jr. wrote:
> Could you switch the pointers to scoped_ptrs instead? You can still .reset()
> such a pointer, but handling resources manually in tests is a known source of
> memory leaks.

Done.  Reworked these tests slightly, to avoid simply removing the deletes and
adding resets() for all non-used entries, which wouldn't really fix anything,
since an extra reset() and missing delete would be pretty much equivalent.

[cbentzel, 2011-04-20 15:18:39]

On Wed, Apr 20, 2011 at 11:15 AM, <mmenke@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6685012/diff/50038/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager_unittest.cc (right):
>
>
>
http://codereview.chromium.org/6685012/diff/50038/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager_unittest.cc:486: delete pc1;
> On 2011/04/20 06:49:37, Paweł Hajdan Jr. wrote:
>
>> Could you switch the pointers to scoped_ptrs instead? You can still
>>
> .reset()
>
>> such a pointer, but handling resources manually in tests is a known
>>
> source of
>
>> memory leaks.
>>
>
> Done.  Reworked these tests slightly, to avoid simply removing the
> deletes and adding resets() for all non-used entries, which wouldn't
> really fix anything, since an extra reset() and missing delete would be
> pretty much equivalent.


I'll take a look. The reason we did raw deletes in these tests were you
ended up needing to do a bunch of releases/resets with scoped_ptr's, and it
wasn't any simpler. Leaks in the test weren't a concern since they would get
caught by valgrind and fixed.

[mmenke, 2011-04-20 15:33:05]

On 2011/04/20 15:18:39, cbentzel wrote:
> I'll take a look. The reason we did raw deletes in these tests were you
> ended up needing to do a bunch of releases/resets with scoped_ptr's, and it
> wasn't any simpler. Leaks in the test weren't a concern since they would get
> caught by valgrind and fixed.

I'm happy either way, though I do think using reset() instead of
delete isn't a great idea, as we have about half as many with
FINAL_STATUS_USED as with other statuses, so we'd need twice as many
resets, which seems potentially more leak-prone to me than having
fewer deletes.

[Paweł Hajdan Jr., 2011-04-20 15:41:38]

On Wed, Apr 20, 2011 at 17:18, Chris Bentzel <cbentzel@chromium.org> wrote:

> I'll take a look. The reason we did raw deletes in these tests were you
> ended up needing to do a bunch of releases/resets with scoped_ptr's, and it
> wasn't any simpler. Leaks in the test weren't a concern since they would get
> caught by valgrind and fixed.
>

When they get caught by Valgrind there is noticeable overhead: Valgrind
waterfall goes red, a bug gets filed, another patch has to be made, and so
on. I think it's generally more effective to avoid known problems rather
than relying on some detection technique.

[mmenke, 2011-04-20 20:03:00]

Compositing has been modified in the last day or so to always allocate a handle
when the RenderView is created.  As this makes it impossible to safely handle
prerender compositing solely through a RenderWidgetHostView, I've removed all
compositing-related code from this CL.

I've also removed the pending_final_status code, though I suspect I'll add it
back in another CL.

[cbentzel, 2011-04-20 20:13:56]

LGTM

http://codereview.chromium.org/6685012/diff/56040/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6685012/diff/56040/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:167: // Destroy entries that have a
pending final status or are expired.
This comment is now out of date.

[mmenke, 2011-04-20 20:15:58]

http://codereview.chromium.org/6685012/diff/56040/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6685012/diff/56040/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:167: // Destroy entries that have a
pending final status or are expired.
On 2011/04/20 20:13:56, cbentzel wrote:
> This comment is now out of date.

Deleted.

[cbentzel, 2011-04-20 20:21:25]

LGTM

On Wed, Apr 20, 2011 at 4:15 PM, <mmenke@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6685012/diff/56040/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager.h (right):
>
>
>
http://codereview.chromium.org/6685012/diff/56040/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:167: // Destroy entries
> that have a pending final status or are expired.
> On 2011/04/20 20:13:56, cbentzel wrote:
>
>> This comment is now out of date.
>>
>
> Deleted.
>
>
> http://codereview.chromium.org/6685012/
>