Browser test for prerendering in general

Browser test for prerendering in general
(PrerenderBrowserTest.PrerenderPage)

Also switch PrerenderManager from inheriting from
NonThreadSafe to using explicit DCHECKs, so doesn't
cause a debug assertion when destroyed on another
thread.

BUG=70398
TEST=PrerenderBrowserTest.PrerenderPage


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

[cbentzel, 2011-01-21 18:37:02]

LGTM

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

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:19: /* Prerender tests work as
follows:
I haven't really seen /* style commenting in Chrome - although style guide
allows it, // is far more common.

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:48: bool did_finish_loading()
{ return did_finish_loading_; }
Nit: const

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:59: const std::vector<GURL>&
alias_urls) {
Nit: line up with PrerenderManager.

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:112:
ui_test_utils::RunMessageLoop();
So, if we break prerendering this will fail with a timeout since the UI message
loop quit never fires, right?

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

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:146: if
(prerender_contents_factory_.get()) {
Should you just have a default factory which is created in the PrerenderManager
constructor? Removes the get here. Not worried about optimizations, just fewer
NULL checks required.

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:154: DeleteOldEntries();
I'm not sure if you need to do pruning here.

http://codereview.chromium.org/6255005/diff/41001/chrome/test/data/prerender/...
File chrome/test/data/prerender/plugin_delay_load.html (right):

http://codereview.chromium.org/6255005/diff/41001/chrome/test/data/prerender/...
chrome/test/data/prerender/plugin_delay_load.html:21: return plugin.getURL !=
undefined;
Is there any race condition where the plugin isn't swapped in after
NavigateToURL completes [and this is eval'ed]?

[cbentzel, 2011-01-21 19:16:11]

Oh, and as we discussed it's possible that the PrerenderManager may need to be
destroyed on the UI thread due to the clean up of remnant PrerenderContents, but
I'm not convinced about that yet.

[mmenke, 2011-01-21 20:12:20]

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

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:19: /* Prerender tests work as
follows:
On 2011/01/21 18:37:02, cbentzel wrote:
> I haven't really seen /* style commenting in Chrome - although style guide
> allows it, // is far more common. 

You're right.  Hadn't really thought about it.  Changed.

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:48: bool did_finish_loading()
{ return did_finish_loading_; }
On 2011/01/21 18:37:02, cbentzel wrote:
> Nit: const

Done.

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:59: const std::vector<GURL>&
alias_urls) {
On 2011/01/21 18:37:02, cbentzel wrote:
> Nit: line up with PrerenderManager.

Done

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:112:
ui_test_utils::RunMessageLoop();
On 2011/01/21 18:37:02, cbentzel wrote:
> So, if we break prerendering this will fail with a timeout since the UI
message
> loop quit never fires, right? 

Yes.  I've thought about trying to get something to assert on instead in that
case, by using ui_test_utils::NavigateToURL(), which waits until the first
navigation is complete.  Then I could assert on the existence of the prerendered
page, and then run the message loop, but I'm concerned about race conditions
between the two pages.

While it could currently work fine, even if the prerendered page finishes first,
as NavigateToURL does the same thing we do, it still seems like a bad idea.


Another option would be to use the source page's onload event to create the
prefetch link, but I'm still worried about race conditions.  Depending on the
timing of things with such complicated relationships just doesn't seem like a
good idea.

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

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:146: if
(prerender_contents_factory_.get()) {
On 2011/01/21 18:37:02, cbentzel wrote:
> Should you just have a default factory which is created in the
PrerenderManager
> constructor? Removes the get here. Not worried about optimizations, just fewer
> NULL checks required. 

Done.  I've seen both done in the code, but think always using a factory is a
bit more consistent (Though I wouldn't be surprised if the null check were
actually faster, on a pointless side note).

http://codereview.chromium.org/6255005/diff/41001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:154: DeleteOldEntries();
On 2011/01/21 18:37:02, cbentzel wrote:
> I'm not sure if you need to do pruning here.

Removed.  I was thinking it would be easier to debug issues caused
DeleteOldEntries() if it occurs here, rather than if it occurs in the middle of
swapping out a page.

That having been said, we do have unit tests for that, anyways.

http://codereview.chromium.org/6255005/diff/41001/chrome/test/data/prerender/...
File chrome/test/data/prerender/plugin_delay_load.html (right):

http://codereview.chromium.org/6255005/diff/41001/chrome/test/data/prerender/...
chrome/test/data/prerender/plugin_delay_load.html:21: return plugin.getURL !=
undefined;
On 2011/01/21 18:37:02, cbentzel wrote:
> Is there any race condition where the plugin isn't swapped in after
> NavigateToURL completes [and this is eval'ed]?

I've spent a little time looking into this.  Plugin creation seems to happen
synchronously.  Since webkit isn't reentrant in most cases, I don't believe we
can do anything involving Javascript while creating a plugin.

That having been said, it's a legitimate concern.  I'll spend some more time
looking through just what happens on plugin creation before landing this CL.

[mmenke, 2011-01-21 20:19:59]

On 2011/01/21 19:16:11, cbentzel wrote:
> Oh, and as we discussed it's possible that the PrerenderManager may need to be
> destroyed on the UI thread due to the clean up of remnant PrerenderContents,
but
> I'm not convinced about that yet.

Oh, and I should add that I completely agree with this.  Would probably be
pretty easy to just use weak pointers, since it's only called on one thread.