Move the mixed scripting infobar to a page action icon / content setting.

Move the mixed scripting infobar to a page action icon / content setting.

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

[Chris Evans, 2012-06-20 00:00:41]

Tom / Bernhard -- please review. I can go shopping for OWNERS later.

This change operates within the following constraints:
- As small as possible so we can merge it to M20 if so asked to do (this is
likely).
- No resource (image, string) changes permitted.
- No user visible or settable exception support (can be revisited later if
desired)

Other things I propose we keep out-of-scope for this CL:
- The "mixed display" (as opposed to "mixed script") infobar was not moved over.
It's not a default thing and is approximately unused.
- I don't think it's fully wired in to the preference system yet. We agreed it
does seem desirable for admins to be able to whitelist (e.g. internal) hosts
longer term.


Generally, the change seems mostly mechanical, which is pleasing. If you want I
can upload screenshots to the bug if it helps achieve LGTMs :)

[Tom Sepez, 2012-06-20 04:16:07]

Just a few small points:

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:1946: // We don't (yet?) implment user-settable
exceptions for mixed script
nit: implement

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:1947: // loading, so bounce to an explanatory page
for now.
nit: exceptions are for blocking not loading, eh?  since default is block?

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:1949: GURL(kInsecureContentHelpUrl), Referrer(),
Are we sure we want to send a referrer here?   SSL page to other domain (Gs) ssl
page?

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/content_sett...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/content_sett...
chrome/browser/ui/content_settings/content_setting_image_model.cc:62:
IDR_OMNIBOX_HTTPS_INVALID,
Can we stick  some COMPILE_ASSERTS here to catch the lack of sufficient elements
in the future?

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:674: if (type ==
CONTENT_SETTINGS_TYPE_MIXEDSCRIPT)
Am I being dense?  Shouldn't these two just be cases in the switch below rather
than separate if-statements?

http://codereview.chromium.org/10577028/diff/1/chrome/renderer/content_settin...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/10577028/diff/1/chrome/renderer/content_settin...
chrome/renderer/content_settings_observer.h:66: void DidNotAllowMixedScript();
Seems strange that this one doesn't need the frame as an argument.  The others
seem to take it (and then ignore it in the method itself.  Do we want to do the
same for the sake of foolish consistency?

[Bernhard Bauer, 2012-06-20 17:01:43]

A screenshot would indeed be nice; I'm not completely sure what the bubble
actually looks like.

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/content...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/content...
chrome/browser/content_settings/content_settings_default_provider.cc:46:
CONTENT_SETTING_BLOCK,    // CONTENT_SETTINGS_TYPE_MIXEDSCRIPT
Do we have something in place that DCHECKs that we're not trying to set or read
content settings for this type? For example, there is a method
HostContentSettingsMap::IsValidSettingForType() that could always return false
for this type, and you could set the default value to _DEFAULT (which means "no
value", and is automatically DCHECKed when reading content settings).

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/content...
chrome/browser/content_settings/content_settings_default_provider.cc:162:
"ContentSettings.DefaultMixedScriptSetting",
Is this value user-changeable? I thought the setting above is only to make the
compiler assertion work, in which case we don't need UMA stats here.

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/brow...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:242:
"https://support.google.com/chrome/bin/answer.py?hl=en-US&answer=1342714";
Please don't hardcode this to an english Help Center article. There is a method
google_util::AppendGoogleLocaleParam() that you can use.

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/brow...
chrome/browser/ui/browser.cc:1949: GURL(kInsecureContentHelpUrl), Referrer(),
On 2012/06/20 04:16:07, Tom Sepez wrote:
> Are we sure we want to send a referrer here?   SSL page to other domain (Gs)
ssl
> page?

I think this sends an empty referrer.

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_image_model.cc:62:
IDR_OMNIBOX_HTTPS_INVALID,
On 2012/06/20 04:16:07, Tom Sepez wrote:
> Can we stick  some COMPILE_ASSERTS here to catch the lack of sufficient
elements
> in the future?

What we should do here is have a ContentSettingsTypeIdEntry array like in
content_setting_bubble_model, to avoid having to add zeroes for every new
content type.

[Chris Evans, 2012-06-20 22:19:56]

On 2012/06/20 04:16:07, Tom Sepez wrote:
> Just a few small points:
> 
> http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc#n...
> chrome/browser/ui/browser.cc:1946: // We don't (yet?) implment user-settable
> exceptions for mixed script
> nit: implement

Done.

> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc#n...
> chrome/browser/ui/browser.cc:1947: // loading, so bounce to an explanatory
page
> for now.
> nit: exceptions are for blocking not loading, eh?  since default is block?

Done.

> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/browser.cc#n...
> chrome/browser/ui/browser.cc:1949: GURL(kInsecureContentHelpUrl), Referrer(),
> Are we sure we want to send a referrer here?   SSL page to other domain (Gs)
ssl
> page?

I agree with Bernhard that this shouldn't send a referer (it's the empty
constructor).

> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/content_sett...
> File chrome/browser/ui/content_settings/content_setting_image_model.cc
(right):
> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/content_sett...
> chrome/browser/ui/content_settings/content_setting_image_model.cc:62:
> IDR_OMNIBOX_HTTPS_INVALID,
> Can we stick  some COMPILE_ASSERTS here to catch the lack of sufficient
elements
> in the future?

Implemented a key/value pair lookup as requested by Bernhard.

> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/webui/option...
> File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):
> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/browser/ui/webui/option...
> chrome/browser/ui/webui/options2/content_settings_handler2.cc:674: if (type ==
> CONTENT_SETTINGS_TYPE_MIXEDSCRIPT)
> Am I being dense?  Shouldn't these two just be cases in the switch below
rather
> than separate if-statements?

Done.

> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/renderer/content_settin...
> File chrome/renderer/content_settings_observer.h (right):
> 
>
http://codereview.chromium.org/10577028/diff/1/chrome/renderer/content_settin...
> chrome/renderer/content_settings_observer.h:66: void DidNotAllowMixedScript();
> Seems strange that this one doesn't need the frame as an argument.  The others
> seem to take it (and then ignore it in the method itself.  Do we want to do
the
> same for the sake of foolish consistency?

I agree the lack of consistency is bothering. Since these methods are not
virtual, we're free to change them to the simplest variant: they now all take no
params.

[Chris Evans, 2012-06-20 22:21:07]

On 2012/06/20 17:01:43, Bernhard Bauer wrote:
> A screenshot would indeed be nice; I'm not completely sure what the bubble
> actually looks like.

Will upload a couple shortly.

> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/content...
> File chrome/browser/content_settings/content_settings_default_provider.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/content...
> chrome/browser/content_settings/content_settings_default_provider.cc:46:
> CONTENT_SETTING_BLOCK,    // CONTENT_SETTINGS_TYPE_MIXEDSCRIPT
> Do we have something in place that DCHECKs that we're not trying to set or
read
> content settings for this type? For example, there is a method
> HostContentSettingsMap::IsValidSettingForType() that could always return false
> for this type, and you could set the default value to _DEFAULT (which means
"no
> value", and is automatically DCHECKed when reading content settings).

Done.

> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/content...
> chrome/browser/content_settings/content_settings_default_provider.cc:162:
> "ContentSettings.DefaultMixedScriptSetting",
> Is this value user-changeable? I thought the setting above is only to make the
> compiler assertion work, in which case we don't need UMA stats here.

Removed / done.

> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/brow...
> File chrome/browser/ui/browser.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/brow...
> chrome/browser/ui/browser.cc:242:
> "https://support.google.com/chrome/bin/answer.py?hl=en-US&answer=1342714";
> Please don't hardcode this to an english Help Center article. There is a
method
> google_util::AppendGoogleLocaleParam() that you can use.

Oops, good catch. Done.

> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/brow...
> chrome/browser/ui/browser.cc:1949: GURL(kInsecureContentHelpUrl), Referrer(),
> On 2012/06/20 04:16:07, Tom Sepez wrote:
> > Are we sure we want to send a referrer here?   SSL page to other domain (Gs)
> ssl
> > page?
> 
> I think this sends an empty referrer.
> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/cont...
> File chrome/browser/ui/content_settings/content_setting_image_model.cc
(right):
> 
>
https://chromiumcodereview.appspot.com/10577028/diff/1/chrome/browser/ui/cont...
> chrome/browser/ui/content_settings/content_setting_image_model.cc:62:
> IDR_OMNIBOX_HTTPS_INVALID,
> On 2012/06/20 04:16:07, Tom Sepez wrote:
> > Can we stick  some COMPILE_ASSERTS here to catch the lack of sufficient
> elements
> > in the future?
> 
> What we should do here is have a ContentSettingsTypeIdEntry array like in
> content_setting_bubble_model, to avoid having to add zeroes for every new
> content type.

Done. May even result in less lines of code :)

[Evan Stade, 2012-06-20 22:44:11]

content settings handler lgtm

[Tom Sepez, 2012-06-20 22:46:45]

LGTM

[Peter Kasting, 2012-06-21 00:57:21]

chrome/browser/ui/browser.cc OWNERS LGTM

[Bernhard Bauer, 2012-06-21 11:02:01]

LGTM w/ a tiny nit :)

http://codereview.chromium.org/10577028/diff/1014/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

http://codereview.chromium.org/10577028/diff/1014/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_image_model.cc:42: namespace
{
Nit: newline