Make --allow-cross-domain-auth-prompt equivalent to a preference.

Make --allow-cross-domain-auth-prompt equivalent to a preference.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86903

[Tom Sepez, 2011-05-25 20:59:49]

Glenn, please review.  This is the preference work we've been discussing.

[jam, 2011-05-25 21:08:45]

http://codereview.chromium.org/6992081/diff/26/content/browser/renderer_host/...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6992081/diff/26/content/browser/renderer_host/...
content/browser/renderer_host/resource_dispatcher_host.cc:21: #include
"chrome/browser/browser_process.h"
this is adding a dependency from content to chrome, which will have to be
removed as part of the content refactoring.  better not to add it in the first
place and do this in a way that won't have to change.  i.e. perhaps the browser
code can set an option on RDH.

[Tom Sepez, 2011-05-25 21:12:02]

Thanks John.  I went part way down that very path before deciding this would be
simpler.  I'll convert it.

[Glenn Wilson, 2011-05-25 21:39:18]

I'm likely not the best reviewer for this stuff anymore, Mattias can probably
direct you to the right person though.

[Tom Sepez, 2011-05-25 22:41:57]

Ok, new patch set for review by reviewers.  Thanks.

[jam, 2011-05-25 23:25:22]

http://codereview.chromium.org/6992081/diff/5007/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6992081/diff/5007/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:683: 
nit

http://codereview.chromium.org/6992081/diff/5007/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:951: // This is needed under content/, so
it can't register itself.
nit: this comment is a little off.  content doesn't know or care about prefs. 
it's chrome that wants to tell content to change behavior.

http://codereview.chromium.org/6992081/diff/5007/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6992081/diff/5007/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:265: // Controle
whether third-party sub-content can pop-up HTTP basic auth
nit: spelling

http://codereview.chromium.org/6992081/diff/5007/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:267: static bool
allow_cross_origin_auth_prompt();
does this really have to be static?  you can just call  inside the
CreateResourceDispatcherHost function

[Tom Sepez, 2011-05-26 00:21:48]

On 2011/05/25 23:25:22, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6992081/diff/5007/chrome/browser/browser_proce...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
http://codereview.chromium.org/6992081/diff/5007/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:683: 
> nit
> 
>
http://codereview.chromium.org/6992081/diff/5007/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:951: // This is needed under content/,
so
> it can't register itself.
> nit: this comment is a little off.  content doesn't know or care about prefs. 
> it's chrome that wants to tell content to change behavior.
> 
> 
Done.

http://codereview.chromium.org/6992081/diff/5007/content/browser/renderer_hos...
> File content/browser/renderer_host/resource_dispatcher_host.h (right):
> 
>
http://codereview.chromium.org/6992081/diff/5007/content/browser/renderer_hos...
> content/browser/renderer_host/resource_dispatcher_host.h:265: // Controle
> whether third-party sub-content can pop-up HTTP basic auth
> nit: spelling
> 
> 
Done.

http://codereview.chromium.org/6992081/diff/5007/content/browser/renderer_hos...
> content/browser/renderer_host/resource_dispatcher_host.h:267: static bool
> allow_cross_origin_auth_prompt();
> does this really have to be static?  you can just call  inside the
> CreateResourceDispatcherHost function
Done.

[Mattias Nissler (ping if slow), 2011-05-26 09:55:51]

Some nits and a small issue.

http://codereview.chromium.org/6992081/diff/7006/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6992081/diff/7006/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:683: 
excess whitespace

http://codereview.chromium.org/6992081/diff/7006/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:955:
local_state_->RegisterBooleanPref(prefs::kAllowCrossOriginAuthPrompt, false);
Doing ad hoc pref registration is nowadays discouraged (the code above should
also be fixed at some point). Central trigger for registering prefs is
browser::RegisterLocalState(), which tells the subsystems to register their
respective prefs. The rationale is that we want to have all supported prefs
readable right after PrefService initialization.

http://codereview.chromium.org/6992081/diff/7006/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6992081/diff/7006/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1264: // Boolean that specifies whether to allow
basic auth promptin on cross-
promptin -> prompt in

[Tom Sepez, 2011-05-26 17:11:19]

Thanks.  Looking at similar preferences, (kAuthSchemes etc.), these get
registered in io_thread.cc's IOThread::RegisterPrefs(). 
kAllowCrossOriginAuthPrompt could move there, but I was unclear as to which
object should then become the observer for changes in this pref?

[Tom Sepez, 2011-05-26 18:48:15]

Please review. Thanks !

[Mattias Nissler (ping if slow), 2011-05-26 19:34:26]

LGTM

[jam, 2011-05-26 21:47:27]

lgtm