Stop login prompt from showing for prefetches

Stop login prompt from showing for prefetches

BUG=70231
TEST=LoginPromptBrowserTest.PrefetchAuthCancels

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

[gavinp, 2011-01-20 17:04:15]

Ziga found GWS exhibiting this bug a little while ago; this should fix it in
chrome.

[cbentzel, 2011-01-20 17:59:45]

Mostly looks good.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6321013/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1044:
request->CancelAuth();
Nit: this looks like it was indented one character too many.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_browsertest.cc:183:
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, PrefetchAuthCancels) {
Should this test also add a LoginPromptBrowserTestObserver and ensure that
observer.handlers_.empty() after load_stop_waiter.Wait() completes? 

It is kind of implicit that there are no authentication prompts due to the load
completing, but it's probably worth explicitly adding that test.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_browsertest.cc:188: const bool
was_prefetch_enabled =
Would it make sense to add a simple RAII class to do this and ensure that
prefetch enabled is returned to original state at the end of the test?

Push back if it's overkill.

[asanka (google), 2011-01-20 18:16:29]

LGTM.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_browsertest.cc:183:
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, PrefetchAuthCancels) {
An alternative would be to wait for both AUTH_NEEDED and LOAD_STOP, but is
probably overkill.

http://codereview.chromium.org/6321013/diff/1/chrome/test/data/login/prefetch...
File chrome/test/data/login/prefetch.html (right):

http://codereview.chromium.org/6321013/diff/1/chrome/test/data/login/prefetch...
chrome/test/data/login/prefetch.html:2: <link rel="prefetch"
href="/auth-basic/resource.png">
FTR: we have special handling for .gif files in testserver, but not for .png. 
Shouldn't be an issue here since the request would be cancelled after a 401
response.

[cbentzel, 2011-01-20 18:19:19]

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_browsertest.cc:183:
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, PrefetchAuthCancels) {
On 2011/01/20 18:16:29, asanka (google) wrote:
> An alternative would be to wait for both AUTH_NEEDED and LOAD_STOP, but is
> probably overkill.

In this case we do not expect an AUTH_NEEDED, so that won't work.

[gavinp, 2011-01-20 18:36:15]

Thanks for the reviews, this upload addresses many comments.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6321013/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1044:
request->CancelAuth();
On 2011/01/20 17:59:45, cbentzel wrote:
> Nit: this looks like it was indented one character too many.

Done.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_browsertest.cc:183:
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, PrefetchAuthCancels) {
Added the assertion for the obvserver handlers being empty; this is a good belts
& suspenders move.

http://codereview.chromium.org/6321013/diff/1/chrome/browser/ui/login/login_p...
chrome/browser/ui/login/login_prompt_browsertest.cc:188: const bool
was_prefetch_enabled =
On 2011/01/20 17:59:45, cbentzel wrote:
> Would it make sense to add a simple RAII class to do this and ensure that
> prefetch enabled is returned to original state at the end of the test?
> 
> Push back if it's overkill. 

Done.

http://codereview.chromium.org/6321013/diff/1/chrome/test/data/login/prefetch...
File chrome/test/data/login/prefetch.html (right):

http://codereview.chromium.org/6321013/diff/1/chrome/test/data/login/prefetch...
chrome/test/data/login/prefetch.html:2: <link rel="prefetch"
href="/auth-basic/resource.png">
On 2011/01/20 18:16:29, asanka (google) wrote:
> FTR: we have special handling for .gif files in testserver, but not for .png. 
> Shouldn't be an issue here since the request would be cancelled after a 401
> response.

Done.

[asanka (google), 2011-01-20 18:56:50]

Looks good.

http://codereview.chromium.org/6321013/diff/10004/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/10004/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:192:
ResourceDispatcherHost::set_is_prefetch_enabled(true);
Did you mean ResourceDispatcherHost::set_is_prefetch_enabled(prefetch) ?

Or perhaps not use a parameter for the constructor.

[gavinp, 2011-01-20 19:02:59]

http://codereview.chromium.org/6321013/diff/10004/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/10004/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:192:
ResourceDispatcherHost::set_is_prefetch_enabled(true);
On 2011/01/20 18:56:50, asanka (google) wrote:
> Did you mean ResourceDispatcherHost::set_is_prefetch_enabled(prefetch) ?
> 
> Or perhaps not use a parameter for the constructor.

Done.

[cbentzel, 2011-01-20 19:26:30]

LGTM

http://codereview.chromium.org/6321013/diff/15002/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/15002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:188: class
SetPrefetchForTest {
Do you also want to enforce that prerender is not enabled? That's unfortunately
done on the current profile rather than the ResourceDispatcherHost.

http://codereview.chromium.org/6321013/diff/15002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:200: }
set_prefetch_for_test(true);
I didn't realize you could do this in C++.

[ziga-google, 2011-01-21 09:25:29]

LGTM

[commit-bot: I haz the power, 2011-01-21 14:58:37]

Patch failed to apply against 72131.

[gavinp, 2011-01-21 15:14:36]

it's ok that commit bot failed; i landed it myself earlier as 72129.

http://codereview.chromium.org/6321013/diff/15002/chrome/browser/ui/login/log...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

http://codereview.chromium.org/6321013/diff/15002/chrome/browser/ui/login/log...
chrome/browser/ui/login/login_prompt_browsertest.cc:188: class
SetPrefetchForTest {
We should be good no matter the prerender setting.

On 2011/01/20 19:26:30, cbentzel wrote:
> Do you also want to enforce that prerender is not enabled? That's
unfortunately
> done on the current profile rather than the ResourceDispatcherHost.