Cancel prerenders that spawn post requests.

Cancel prerenders that spawn post requests.

BUG=77177
TEST=PrerenderBrowserTest.PrerenderXhr*


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

[cbentzel, 2011-04-29 20:00:30]

+jam for the ResourceDispatcherHost change.

Can you do tests for all common HTTP methods?

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

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:793: <<<<<<< HEAD
bad merge

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_final_status.h (right):

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_final_status.h:36: FINAL_STATUS_NOT_GET,
I'd rename this to 

FINAL_STATUS_INVALID_HTTP_METHOD

because things like HEAD should be allowed.

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

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:156: if (!prerender_manager ||
!prerender_manager->is_enabled())
In this case, you should call even if the prerender_manager is disabled.

For example, a prerender could have started, the user disables prerender in the
preferences, and then an XHR POST happens before the prerender times out.

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:325: for
(std::list<PrerenderContentsData>::iterator it = prerender_list_.begin();
There should only be on PrerenderContents per child_route_id_pair. 

I think you could move this into a separate function of
FindPrerenderContentsForChildRouteIdPair() and share with AddPendingPreload().

http://codereview.chromium.org/6901128/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6901128/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:427: if
(is_prerendering && request_data.method != "GET") {
HEAD should be allowed. 

OPTIONS could probably be allowed.

http://codereview.chromium.org/6901128/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:428:
CHECK(IsPrerenderingChildRoutePair(child_id, route_id));
This CHECK is not necessary - is_prerendering is set using the same code.

[dominich, 2011-04-29 21:13:52]

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

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:793: <<<<<<< HEAD
On 2011/04/29 20:00:30, cbentzel wrote:
> bad merge

Done.

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_final_status.h (right):

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_final_status.h:36: FINAL_STATUS_NOT_GET,
On 2011/04/29 20:00:30, cbentzel wrote:
> I'd rename this to 
> 
> FINAL_STATUS_INVALID_HTTP_METHOD
> 
> because things like HEAD should be allowed. 

Done.

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

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:156: if (!prerender_manager ||
!prerender_manager->is_enabled())
On 2011/04/29 20:00:30, cbentzel wrote:
> In this case, you should call even if the prerender_manager is disabled.
> 
> For example, a prerender could have started, the user disables prerender in
the
> preferences, and then an XHR POST happens before the prerender times out. 

Done.

http://codereview.chromium.org/6901128/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:325: for
(std::list<PrerenderContentsData>::iterator it = prerender_list_.begin();
On 2011/04/29 20:00:30, cbentzel wrote:
> There should only be on PrerenderContents per child_route_id_pair. 
> 
> I think you could move this into a separate function of
> FindPrerenderContentsForChildRouteIdPair() and share with AddPendingPreload().
> 

Done.

http://codereview.chromium.org/6901128/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6901128/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:427: if
(is_prerendering && request_data.method != "GET") {
On 2011/04/29 20:00:30, cbentzel wrote:
> HEAD should be allowed. 
> 
> OPTIONS could probably be allowed. 

Done.

http://codereview.chromium.org/6901128/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:428:
CHECK(IsPrerenderingChildRoutePair(child_id, route_id));
On 2011/04/29 20:00:30, cbentzel wrote:
> This CHECK is not necessary - is_prerendering is set using the same code. 

Done.

[cbentzel, 2011-04-30 01:03:33]

Generally looks good

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

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:793: // Checks that ftp scheme
is not supported for prerender.
You'll need to merge with trunk - I reverted the PrerenderBadScheme* CL when it
failed on main build tree.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:948:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderXhrGet) {
Please add browser tests for applicable methods that are supported with xhrs

http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:52: const char*
kValidHttpMethods[] = {
I've usually seen as 

const char* const

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:53: "OPTIONS", "GET", "HEAD",
"TRACE", "CONNECT",
I'd do one of these per-line, and not include the others.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:121: if (method ==
kValidHttpMethods[i])
I'm not sure if these are canonicalized to upper case by the time this is called
- please check.

http://codereview.chromium.org/6901128/diff/6002/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_xhr_get.html (right):

http://codereview.chromium.org/6901128/diff/6002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_xhr_get.html:20: try { xhr = new
ActiveXObject('Msxml2.XMLHTTP'); }
You can just do XMLHttpRequest since this is running on Chrome only - don't need
all the funky old IE variations.

[jam, 2011-05-02 16:21:33]

lgtm

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:47: void
DestroyPreloadForChildRouteIdPairOnUIThread(
nit: this name seems a bit long.  we usually use "Child" when it's something
that applies for different child  process types, i.e. renderer/worker/plugin
etc.

in this case, prerendering is only for renderers, so it can be

void DestroyPreloadForRenderView and you can pass in "int render_process_id, int
render_view_id".  also, is the OnUIThread necessary in the name?  it can have a
DCHECK that it's being called on the right thread.  there should be a comment on
this function anyways, and it can specify which thread it's safe to call on.

[dominich, 2011-05-02 16:45:55]

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

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:793: // Checks that ftp scheme
is not supported for prerender.
On 2011/04/30 01:03:34, cbentzel wrote:
> You'll need to merge with trunk - I reverted the PrerenderBadScheme* CL when
it
> failed on main build tree.

Done.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:948:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderXhrGet) {
On 2011/04/30 01:03:34, cbentzel wrote:
> Please add browser tests for applicable methods that are supported with xhrs
> 
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html

Done.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:52: const char*
kValidHttpMethods[] = {
On 2011/04/30 01:03:34, cbentzel wrote:
> I've usually seen as 
> 
> const char* const 

Done.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:53: "OPTIONS", "GET", "HEAD",
"TRACE", "CONNECT",
On 2011/04/30 01:03:34, cbentzel wrote:
> I'd do one of these per-line, and not include the others. 

Done.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:121: if (method ==
kValidHttpMethods[i])
On 2011/04/30 01:03:34, cbentzel wrote:
> I'm not sure if these are canonicalized to upper case by the time this is
called
> - please check. 

Tested and they are. Added comment to confirm.

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6901128/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:47: void
DestroyPreloadForChildRouteIdPairOnUIThread(
On 2011/05/02 16:21:33, John Abd-El-Malek wrote:
> nit: this name seems a bit long.  we usually use "Child" when it's something
> that applies for different child  process types, i.e. renderer/worker/plugin
> etc.
> 
> in this case, prerendering is only for renderers, so it can be
> 
> void DestroyPreloadForRenderView and you can pass in "int render_process_id,
int
> render_view_id".  

Done and I will fix the HandlePrefetchTag above to have the same.

> also, is the OnUIThread necessary in the name?  it can have a
> DCHECK that it's being called on the right thread.  there should be a comment
on
> this function anyways, and it can specify which thread it's safe to call on.

It has a DCHECK and will have a comment. I will also change the
HandlePrefetchTagOnUIThread above to not have the OnUIThread suffix and have a
comment instead.

http://codereview.chromium.org/6901128/diff/6002/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_xhr_get.html (right):

http://codereview.chromium.org/6901128/diff/6002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_xhr_get.html:20: try { xhr = new
ActiveXObject('Msxml2.XMLHTTP'); }
On 2011/04/30 01:03:34, cbentzel wrote:
> You can just do XMLHttpRequest since this is running on Chrome only - don't
need
> all the funky old IE variations.

Done.

[cbentzel, 2011-05-02 20:27:38]

LGTM

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

http://codereview.chromium.org/6901128/diff/5012/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:810: // Checks that
prerendering a JPG works correctly.
Thanks!

http://codereview.chromium.org/6901128/diff/5012/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6901128/diff/5012/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:124: for (size_t i = 0; i <
arraysize(kValidHttpMethods); ++i) {
DCHECK_EQ(method, StringToUpperASCII(method)) 

at the top of the method to make sure.

http://codereview.chromium.org/6901128/diff/5012/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_xhr_get.html (right):

http://codereview.chromium.org/6901128/diff/5012/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_xhr_get.html:23:
document.getElementById("dynamic").innerHTML =
Is it guaranteed that the GET will have been issued [and completed] before
DidPrerenderPass and DidDisplayPass are called?

[dominich, 2011-05-02 21:04:55]

http://codereview.chromium.org/6901128/diff/5012/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6901128/diff/5012/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:124: for (size_t i = 0; i <
arraysize(kValidHttpMethods); ++i) {
On 2011/05/02 20:27:38, cbentzel wrote:
> DCHECK_EQ(method, StringToUpperASCII(method)) 
> 
> at the top of the method to make sure.

Done.

http://codereview.chromium.org/6901128/diff/5012/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_xhr_get.html (right):

http://codereview.chromium.org/6901128/diff/5012/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_xhr_get.html:23:
document.getElementById("dynamic").innerHTML =
On 2011/05/02 20:27:38, cbentzel wrote:
> Is it guaranteed that the GET will have been issued [and completed] before
> DidPrerenderPass and DidDisplayPass are called? 

No. I could change the XHRequests to be synchronous instead but we don't need
them to be complete. I don't care about the response as the issuing of the
request is enough to catch it and kill it.

I could remove the onreadystatechange callback altogether, but it's useful for
local testing of the html.

[jam, 2011-05-03 06:37:56]

still lgtm

[cbentzel, 2011-05-03 14:24:46]

LGTM

http://codereview.chromium.org/6901128/diff/5012/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_xhr_get.html (right):

http://codereview.chromium.org/6901128/diff/5012/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_xhr_get.html:23:
document.getElementById("dynamic").innerHTML =
On 2011/05/02 21:04:55, dominic wrote:
> On 2011/05/02 20:27:38, cbentzel wrote:
> > Is it guaranteed that the GET will have been issued [and completed] before
> > DidPrerenderPass and DidDisplayPass are called? 
> 
> No. I could change the XHRequests to be synchronous instead but we don't need
> them to be complete. I don't care about the response as the issuing of the
> request is enough to catch it and kill it.
> 
> I could remove the onreadystatechange callback altogether, but it's useful for
> local testing of the html.
> 

OK.

[commit-bot: I haz the power, 2011-05-03 14:27:52]

Can't apply patch for file chrome/browser/prerender/prerender_browsertest.cc.
patching file chrome/browser/prerender/prerender_browsertest.cc
Hunk #1 FAILED at 807.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/prerender/prerender_browsertest.cc.rej

[commit-bot: I haz the power, 2011-05-03 15:35:54]

Change committed as 83889