PrerenderContents uses RESOURCE_RECEIVED_REDIRECT notification.

PrerenderContents uses RESOURCE_RECEIVED_REDIRECT notification.

This removes the dependency on OnDidRedirectProvisionalLoad.

I also added a number of redirect-oriented browser tests.

BUG=78512
TEST=browser_tests --gtest_filter=PrerenderBrowserTest.*

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

[cbentzel, 2011-04-11 20:16:48]

tburkard: Review the whole thing
creis: Review changes to prerender_contents
gavinp+mmenke: Review browser test changes. Gavin, I thought you might be
interested in the HTTP redirect cases I added for your own tests.

[cbentzel, 2011-04-11 20:19:25]

I closed the other CL in favor of this one.

[Charlie Reis, 2011-04-11 21:15:52]

Thanks-- comments inline.

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:142: // Register for redirects.
Please add "from this RenderViewHostDelegate," since that's useful for
understanding why this is correct.

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:335: // is redirected, nothing
changes.
How are you distinguishing whether it's top-level or a sub-resource?  It's
possible for the same URL to be used for both.  Should you be checking
details->resource_type()?

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:339:
CHECK(resource_redirect_details != NULL);
I think Chrome style is to omit the "!= NULL".

[cbentzel, 2011-04-11 21:18:35]

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:335: // is redirected, nothing
changes.
On 2011/04/11 21:15:52, creis wrote:
> How are you distinguishing whether it's top-level or a sub-resource?  It's
> possible for the same URL to be used for both.  Should you be checking
> details->resource_type()?

I'll change to resource_type(). This was using "MatchesURL" to see if the
previous URL is one of the previously seen URL in the top level page.

[mmenke, 2011-04-12 15:56:36]

The browser test changes LGTM.

[cbentzel, 2011-04-12 18:07:59]

Thanks for reviews.

I changed to look at resource_type() over doing the MatchesURL test, which is
clearer and simpler.

I added a number of browser tests since the last review, mainly around redirects
inside of iframes in a prerendered page. These fail if you remove the
resource_type logic, which was why I wanted them.

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:142: // Register for redirects.
On 2011/04/11 21:15:52, creis wrote:
> Please add "from this RenderViewHostDelegate," since that's useful for
> understanding why this is correct.

Done.

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:335: // is redirected, nothing
changes.
On 2011/04/11 21:18:35, cbentzel wrote:
> On 2011/04/11 21:15:52, creis wrote:
> > How are you distinguishing whether it's top-level or a sub-resource?  It's
> > possible for the same URL to be used for both.  Should you be checking
> > details->resource_type()?
> 
> I'll change to resource_type(). This was using "MatchesURL" to see if the
> previous URL is one of the previously seen URL in the top level page.
> 

resource_type() is much clearer about what's going on than MatchesURL [and
potentially a little faster].

http://codereview.chromium.org/6824054/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:339:
CHECK(resource_redirect_details != NULL);
On 2011/04/11 21:15:52, creis wrote:
> I think Chrome style is to omit the "!= NULL".

Done.

[Charlie Reis, 2011-04-12 18:42:59]

prerender_contents LGTM.  Thanks!

[tburkard, 2011-04-12 18:51:15]

LGTM