Add a dropdown to change the default content setting to the Website Settings page.

Add a dropdown to change the default content setting to the Website Settings page.

BUG=372607
Committed: https://crrev.com/05c16951ed9d187003763fa7ffa0e6c78986fa4f
Cr-Commit-Position: refs/heads/master@{#294464}

[Daniel Nishi, 2014-09-08 22:00:58]

Patchset #1 (id:1) has been deleted

[Daniel Nishi, 2014-09-08 22:01:03]

Patchset #1 (id:20001) has been deleted

[Daniel Nishi, 2014-09-08 22:01:17]

Patchset #1 (id:40001) has been deleted

[Daniel Nishi, 2014-09-08 22:04:26]

dhnishi@chromium.org changed reviewers:
+ bauerb@chromium.org

[Daniel Nishi, 2014-09-08 22:04:27]

bauerb: PTAL when you get a chance.

[Bernhard Bauer, 2014-09-09 08:56:22]

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.css (right):

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:121:
#website-settings-page #global-setting {
If #global-setting is an id, you don't need the other selector.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:21: 
Nit: remove one blank line.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:24: this.permissionsLookup
= {
This can probably be made a constant in a var scope.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:231: permissions =
['Allow', 'Ask', 'Block'];
I would rather fix this up in ContentSettingsHandler::GetLocalizedValues() to
make the keys in loadTimeData consistent, than try to work around it here.

Right now, ContentSettingsHandler::GetLocalizedValues() is this horrible mess of
different styles for the keys. I would suggest using lowerCaseCamelCase where
possible. I would be okay with using an underscore to separate different parts
of the key (contentSettingType_setting), but *only* for that, not as a general
word separator.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:242: if (loadString != '')
Do you actually need this check?

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:243: options.add(new
Option(loadTimeData.getString(valueId),
You can use loadString here.

Also, looking at this part of the code three minutes later, |loadString| is a
strange name (it sounds like an action). Maybe |label|?

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:250: else {
Nit: Move this to the same line as the closing brace

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:257:
WebsiteSettingsManager.populateOrigins = function(allowedDict, blockedDict) {
It might start making sense to create these forwarding methods in bulk (like in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...).

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:446: std::string
provider_id;
Move this to right before it's used?

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:671: default_setting
=
Initialize the variable when it is declared.

[Daniel Nishi, 2014-09-09 16:11:57]

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.css (right):

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.css:121:
#website-settings-page #global-setting {
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> If #global-setting is an id, you don't need the other selector.

Done.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/website_settings.js (right):

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:21: 
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> Nit: remove one blank line.

Done.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:24: this.permissionsLookup
= {
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> This can probably be made a constant in a var scope.

Done.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:231: permissions =
['Allow', 'Ask', 'Block'];
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> I would rather fix this up in ContentSettingsHandler::GetLocalizedValues() to
> make the keys in loadTimeData consistent, than try to work around it here.
> 
> Right now, ContentSettingsHandler::GetLocalizedValues() is this horrible mess
of
> different styles for the keys. I would suggest using lowerCaseCamelCase where
> possible. I would be okay with using an underscore to separate different parts
> of the key (contentSettingType_setting), but *only* for that, not as a general
> word separator.

I converted over the Content Setting permission strings to use lowerCamelCase
and a TODO to standardize the rest.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:242: if (loadString != '')
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> Do you actually need this check?

Nope.

Removed.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:243: options.add(new
Option(loadTimeData.getString(valueId),
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> You can use loadString here.
> 
> Also, looking at this part of the code three minutes later, |loadString| is a
> strange name (it sounds like an action). Maybe |label|?

Eliminated the use of an intermediate variable all together.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:250: else {
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> Nit: Move this to the same line as the closing brace

Done.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/website_settings.js:257:
WebsiteSettingsManager.populateOrigins = function(allowedDict, blockedDict) {
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> It might start making sense to create these forwarding methods in bulk (like
in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...).

Looking at that made me realize one of these functions was out of place. I've
moved it. I think that temporarily solves this problem.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/website_settings_handler.cc (right):

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:446: std::string
provider_id;
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> Move this to right before it's used?

Done.

https://codereview.chromium.org/543983005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/website_settings_handler.cc:671: default_setting
=
On 2014/09/09 08:56:21, Bernhard Bauer wrote:
> Initialize the variable when it is declared.

Done.

[Bernhard Bauer, 2014-09-09 16:58:00]

LGTM, thanks!

[Daniel Nishi, 2014-09-10 23:25:29]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:49:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543983005/80001

[commit-bot: I haz the power, 2014-09-11 00:18:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 00:18:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniel Nishi, 2014-09-11 16:17:44]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-11 16:22:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543983005/100001

[commit-bot: I haz the power, 2014-09-11 17:00:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:00:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniel Nishi, 2014-09-11 17:48:36]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:09:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543983005/120001

[commit-bot: I haz the power, 2014-09-11 21:03:41]

Committed patchset #4 (id:120001) as da9b42ccf4ca4974411cc5c05147ea530c94a025

[commit-bot: I haz the power, 2014-09-11 21:22:50]

Patchset 4 (id:??) landed as
https://crrev.com/05c16951ed9d187003763fa7ffa0e6c78986fa4f
Cr-Commit-Position: refs/heads/master@{#294464}