Cancels prerendering on prerendering RenderView crash

Cancels prerendering in the case that the prerendering
RenderView crashes.

BUG=79599
TEST=PrerenderBrowserTest.PrerenderRendererCrash

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82865

[mmenke, 2011-04-22 14:18:19]

I thought about adding a callback parameter for the browser test, but it turns
out that the Callback class only handles object methods.  Tasks work with
functions, too, but automatically delete themselves after the first use.

Suppose we could just subclass TestPrerenderContents and make the
PrerenderContentsFactory into a template<class PrerenderContentsType>, though
that wouldn't work for a FINAL_STATUS_SOURCE_RENDER_VIEW_CLOSED browser test.

I'm open to other ideas.  Suspect we'll be adding more special case code in the
future, and be nice to have some sort of a framework for it.

[cbentzel, 2011-04-22 19:55:10]

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:252: // Cancel the prerender if
the RenderView crashes.
I noticed that TabContents::RenderViewGone makes sure that the render_view_host
argument matches render_view_host_ - should this?

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.h:118: virtual void
RenderViewGone(RenderViewHost* render_view_host,
Add OVERRIDE. 

This could probably be done in a cleanup CL for the entire header.

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

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:252: // Cancel the prerender if
the RenderView crashes.
On 2011/04/22 19:55:10, cbentzel wrote:
> I noticed that TabContents::RenderViewGone makes sure that the
render_view_host
> argument matches render_view_host_ - should this? 

The TabContents can be the delegate for multiple RenderViews - the one currently
in use, and the pending one, for navigations that can't use the active one (Like
going from chrome://blah to http://TheScaryRemoteWilderness).  We don't have to
worry about that.

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6879114/diff/6003/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.h:118: virtual void
RenderViewGone(RenderViewHost* render_view_host,
On 2011/04/22 19:55:10, cbentzel wrote:
> Add OVERRIDE. 
> 
> This could probably be done in a cleanup CL for the entire header.

Done (For this function only).

I'll do the cleanup CL next week.  I'm also happy to (In the same cleanup CL or
in another CL) replace the abbreviated variable names, as Evan requested.

[cbentzel, 2011-04-22 20:16:41]

LGTM

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

http://codereview.chromium.org/6879114/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:81: } else if
(expected_final_status_ == FINAL_STATUS_RENDERER_CRASHED) {
Please add a note indicating why the crash needs to be done this way instead of
directly in the page.

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

http://codereview.chromium.org/6879114/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:252: // Cancel the prerender if
the RenderView crashes.
Add 

DCHECK_EQ(render_view_host_, render_view_host)

[mmenke, 2011-04-22 20:30:30]

Thanks.

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

http://codereview.chromium.org/6879114/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:81: } else if
(expected_final_status_ == FINAL_STATUS_RENDERER_CRASHED) {
On 2011/04/22 20:16:41, cbentzel wrote:
> Please add a note indicating why the crash needs to be done this way instead
of
> directly in the page. 

Done.

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

http://codereview.chromium.org/6879114/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:252: // Cancel the prerender if
the RenderView crashes.
On 2011/04/22 20:16:41, cbentzel wrote:
> Add 
> 
> DCHECK_EQ(render_view_host_, render_view_host)

Done.  Added it to the other RenderViewHostDelegate functions that take it as an
argument, too.

[commit-bot: I haz the power, 2011-04-25 15:03:54]

Change committed as 82865

[Paweł Hajdan Jr., 2011-04-26 12:00:43]

Drive-by with a tiny comment. Could you do a follow-up? (feel free to TBR)

It's mostly about getting rid of bad examples in the code so that people don't
copy them.

http://codereview.chromium.org/6879114/diff/1019/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/6879114/diff/1019/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:86:
render_view_host()->NavigateToURL(GURL("about:crash"));
nit: Please use a constant from content/common/url_constants here.