Remove calls of PrerenderTracker::TryCancel and TryCancelOnIOThread in ChromeResourceDispatcherHost…

Remove calls of PrerenderTracker::TryCancel and TryCancelOnIOThread in ChromeResourceDispatcherHostDelegate. These short-circuited cancelling the prerendering on the IO thread. Instead, I moved cancelling the prerender on the UI thread since both of these calls end up going to the UI thread right after.

This also removes the last calls of ResourceRequestInfo::GetAssociatedRenderView, which I will remove in a followup.

BUG=304341
R=davidben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243184

[jam, 2014-01-03 23:22:10]

https://codereview.chromium.org/124113003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/124113003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1818: if (!rfh) {
this is unnecessary since WebContents::FromRenderFrameHost handles it

https://codereview.chromium.org/124113003/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/124113003/diff/1/content/browser/loader/resou...
content/browser/loader/resource_loader.cc:257: if (request_->load_flags() &
net::LOAD_PREFETCH) {
I moved this check to the content layer since content, blink, and net are the
ones that handle prefetching, so this check seemed better located here

[jam, 2014-01-05 07:46:55]

btw the PrerenderBrowserTest.PrerenderSSLClientCert* tests are failing because
the bots have no certificates. On my dev box, they pass because I have a
certificate installed. I can see a few options:
-make the tests pass if no certificate is installed
  -this seems the worst option because then a regression could sneak in as we
won't have test coverage on bots
-make the test NSS only and add a certificate at run time, like this test:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
  -while this drops the test coverage to configs which use NSS, this test is for
platform independent code so having coverage on some platforms seems fine
-add a testing flag to force an (empty) certificate dialog to be shown if there
are no certificates

Do you see any other alternatives? What is your preference? I was leaning
towards the second option but I don't feel strongly between it and the last one.

[jam, 2014-01-05 07:47:45]

On 2014/01/05 07:46:55, jam wrote:
> btw the PrerenderBrowserTest.PrerenderSSLClientCert* tests are failing because
> the bots have no certificates. On my dev box, they pass because I have a
> certificate installed. I can see a few options:
> -make the tests pass if no certificate is installed
>   -this seems the worst option because then a regression could sneak in as we
> won't have test coverage on bots
> -make the test NSS only and add a certificate at run time, like this test:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
>   -while this drops the test coverage to configs which use NSS, this test is
for
> platform independent code so having coverage on some platforms seems fine
> -add a testing flag to force an (empty) certificate dialog to be shown if
there
> are no certificates
    -btw the con with this is that the code might actually crash unless we
ensure that the dialog code on each platform handles no certificate lists. so
then we'd complicate production code just for this test.
> 
> Do you see any other alternatives? What is your preference? I was leaning
> towards the second option but I don't feel strongly between it and the last
one.

[jam, 2014-01-06 00:02:34]

ok nvm all the comments above, I found a way to use a test ClientCertStore which
always returns a certificate, so that I can exercise this code path on all bots.

[cbentzel, 2014-01-06 11:29:37]

Sorry for delay - I'm going to have davidben take this over as he's been more
knee-deep in the prerender code lately than I have and will likely do a better
review. I'll check back in.

[davidben, 2014-01-06 22:14:47]

LGTM.

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2538: const
net::CertificateList response_;
Remnant from a previous version?

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:434:
prerender_contents->Destroy(prerender::FINAL_STATUS_AUTH_NEEDED);
Do we want to call handler->CancelAuth() here too? Not sure it matters much
since the WebContents is going away, but that's what the old codepath did. Dunno
if some other component is sensitive to the notification.

[jam, 2014-01-06 22:20:59]

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/prerend...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/prerend...
chrome/browser/prerender/prerender_browsertest.cc:2538: const
net::CertificateList response_;
On 2014/01/06 22:14:47, David Benjamin wrote:
> Remnant from a previous version?

oops, copy and paste. removed

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/ui/logi...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/124113003/diff/1370002/chrome/browser/ui/logi...
chrome/browser/ui/login/login_prompt.cc:434:
prerender_contents->Destroy(prerender::FINAL_STATUS_AUTH_NEEDED);
On 2014/01/06 22:14:47, David Benjamin wrote:
> Do we want to call handler->CancelAuth() here too? Not sure it matters much
> since the WebContents is going away, but that's what the old codepath did.
Dunno
> if some other component is sensitive to the notification.

this will be equivalent to closing the tab; and the code must handle that code
path. the tests all pass, so I'm inclined to leave as is and not add extra code
unless I know for sure that it's needed