MD Site Settings: Add five new top level categories

Add five new top level categories and corresponding entries on the Site Details page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because the enum value of the ContentSettingsTypes is variable depending on platform, so we don't want to be hardcoding those in JS. This is similar to the Allow/Block values (the PermissionValues enum), which used to be ints, but are now converted int <-> string on the JS/C++ boundary.

TBR=sky
BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/155a202f6076fad0cd5d86bfefb5c317d8a3708e
Cr-Commit-Position: refs/heads/master@{#397819}

[Finnur, 2016-05-31 21:45:31]

Description was changed from

==========
TBD

BUG=TBD
==========

to

==========
TBD

BUG=TBD
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-05-31 21:59:44]

Description was changed from

==========
TBD

BUG=TBD
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-05-31 21:59:44]

finnur@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Finnur, 2016-05-31 21:59:55]

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/add_site_dialog.js (left):

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:52: },
Moved to the behavior.

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/add_site_dialog.js (right):

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/add_site_dialog.js:47: pattern,
'', this.category, settings.PermissionValues.ALLOW);
I found that the old settings UI just added entries based on primary patterns,
so I changed this to match.

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:110:
// TODO(finnur): Get missing image assets from designer.
In progress, separate thread and separate CL.

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (left):

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:105: };
This and next three deleted sections were moved to the helper.

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/ui/webui/sit...
File chrome/browser/ui/webui/site_settings_helper.cc (right):

https://codereview.chromium.org/2021343003/diff/1/chrome/browser/ui/webui/sit...
chrome/browser/ui/webui/site_settings_helper.cc:23: struct
ContentSettingsTypeNameEntry {
This code was moved from content_settings_handler (and not changed).

[Finnur, 2016-06-01 20:40:35]

Friendly ping. :)

[michaelpg, 2016-06-02 12:58:31]

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1012: <message
name="IDS_SETTINGS_SITE_SETTINGS_KEYGEN_BLOCK" desc="The block label for
background sync in site settings.">
keygen

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1015: <message
name="IDS_SETTINGS_SITE_SETTINGS_KEYGEN_BLOCK_RECOMMENDED" desc="The block label
for background sync in site settings (with the 'recommended' suffix).">
keygen

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1021: <message
name="IDS_SETTINGS_SITE_SETTINGS_AUTOMATIC_DOWNLOAD_ASK_RECOMMENDED" desc="The
allow label for automatic download in site settings.">
nit: recommended, as above

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1030: <message
name="IDS_SETTINGS_SITE_SETTINGS_UNSANDBOXED_PLUGINS_ASK_RECOMMENDED" desc="The
allow label for unsandboxed plugins in site settings.">
nit: recommended, as above

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.html (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:78:
<site-details-permission site="[[site]]" id="javascript"
plugins

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.js
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:55:
if (origin == this.removePatternWildcard_(site.origin)) {
nit: you could make a comparison function instead of running the function twice
with a temporary variable:

    if (this.sameOrigin_(exceptionList[i].origin, site.origin)) {

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:68:
computeCategoryTextId: function(category) {
Can you adjust the IDs to match so you don't need this (string => string)
function at all?

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:111:
ContentSettingsTypes.BACKGROUND_SYNC)]]
+2 indent

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:122:
ContentSettingsTypes.KEYGEN)]]
+2 indent

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:133:
ContentSettingsTypes.AUTOMATIC_DOWNLOADS)]]
+2 indent

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:144:
ContentSettingsTypes.UNSANDBOXED_PLUGINS)]]
+2 indent

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.h (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.h:45: // Gets a string identifier
for the group name, for use in HTML.
", for use in HTML" => ""

https://codereview.chromium.org/2021343003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:31: media_stream_camera: [],
nit x3: move media_stream_camera to after keygen

[Finnur, 2016-06-02 17:31:36]

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1012: <message
name="IDS_SETTINGS_SITE_SETTINGS_KEYGEN_BLOCK" desc="The block label for
background sync in site settings.">
On 2016/06/02 12:58:30, michaelpg wrote:
> keygen

Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1015: <message
name="IDS_SETTINGS_SITE_SETTINGS_KEYGEN_BLOCK_RECOMMENDED" desc="The block label
for background sync in site settings (with the 'recommended' suffix).">
On 2016/06/02 12:58:30, michaelpg wrote:
> keygen

Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1021: <message
name="IDS_SETTINGS_SITE_SETTINGS_AUTOMATIC_DOWNLOAD_ASK_RECOMMENDED" desc="The
allow label for automatic download in site settings.">
On 2016/06/02 12:58:30, michaelpg wrote:
> nit: recommended, as above

Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:1030: <message
name="IDS_SETTINGS_SITE_SETTINGS_UNSANDBOXED_PLUGINS_ASK_RECOMMENDED" desc="The
allow label for unsandboxed plugins in site settings.">
On 2016/06/02 12:58:30, michaelpg wrote:
> nit: recommended, as above

Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details.html (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details.html:78:
<site-details-permission site="[[site]]" id="javascript"
Good catch. Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.js
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:55:
if (origin == this.removePatternWildcard_(site.origin)) {
On 2016/06/02 12:58:30, michaelpg wrote:
> nit: you could make a comparison function instead of running the function
twice
> with a temporary variable:
> 
>     if (this.sameOrigin_(exceptionList[i].origin, site.origin)) {

Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:68:
computeCategoryTextId: function(category) {
This was my thinking at first too, but the problem is that removing it exposes
the user (through the URL) to the details of how we store the pref setting on
disk. For example, 'unsandboxed-plugins' would appear as 'ppapi-broker', which
wouldn't make much sense (to the user), camera would be 'media-stream-camera'
(which looks a bit unwieldy) and once we introduce protected content there would
be a mix of hyphenation conventions ('protectedContent' vs 'background-sync').

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:111:
ContentSettingsTypes.BACKGROUND_SYNC)]]
I'm confused... For HTML we use a 2 space indent and one applies 2x when a line
wraps, right?
That means line 111 should be four spaces indented... which it is, no? :s

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/site_settings_helper.h (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/site_settings_helper.h:45: // Gets a string identifier
for the group name, for use in HTML.
On 2016/06/02 12:58:31, michaelpg wrote:
> ", for use in HTML" => ""

Done.

https://codereview.chromium.org/2021343003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:31: media_stream_camera: [],
Actually, the media_stream_ prefix is a remnant of the time we used prefs, but
it is not required, so I removed it.

[michaelpg, 2016-06-03 13:37:13]

michaelpg@chromium.org changed reviewers:
+ stevenjb@chromium.org

[michaelpg, 2016-06-03 13:37:13]

michaelpg@chromium.org changed required reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-06-03 13:37:15]

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_details_permission.js
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_details_permission.js:55:
if (origin == this.removePatternWildcard_(site.origin)) {
On 2016/06/02 17:31:36, Finnur wrote:
> On 2016/06/02 12:58:30, michaelpg wrote:
> > nit: you could make a comparison function instead of running the function
> twice
> > with a temporary variable:
> > 
> >     if (this.sameOrigin_(exceptionList[i].origin, site.origin)) {
> 
> Done.

Thanks, definitely more readable!

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:68:
computeCategoryTextId: function(category) {
On 2016/06/02 17:31:36, Finnur wrote:
> This was my thinking at first too, but the problem is that removing it exposes
> the user (through the URL) to the details of how we store the pref setting on
> disk. For example, 'unsandboxed-plugins' would appear as 'ppapi-broker', which
> wouldn't make much sense (to the user), camera would be 'media-stream-camera'
> (which looks a bit unwieldy) and once we introduce protected content there
would
> be a mix of hyphenation conventions ('protectedContent' vs 'background-sync').

Cool, makes sense then.

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/site_settings_page/site_settings_page.html
(right):

https://codereview.chromium.org/2021343003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings_page/site_settings_page.html:111:
ContentSettingsTypes.BACKGROUND_SYNC)]]
On 2016/06/02 17:31:36, Finnur wrote:
> I'm confused... For HTML we use a 2 space indent and one applies 2x when a
line
> wraps, right?
> That means line 111 should be four spaces indented... which it is, no? :s

We have a new style guideline of wrapping computed functions 4 spaces. It's a
bit tricky and, in this case, apparently not definitive. +stevenjb

See the examples under "Wrap computed bindings to keep within the 80-character
column limit, indenting the next line an additional 4 spaces." in the draft
Polymer WebUI Style Guide:
https://www.chromium.org/developers/web-development-style-guide

I added this as a 3rd example for us to decide there and wound up tentatively
agreeing with you. Either way please feel free to land this as-is.

https://codereview.chromium.org/2021343003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_details_permission.js
(right):

https://codereview.chromium.org/2021343003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_details_permission.js:45: *
http://[*.]google.com.
nit: closing paren (or if parens and wildcards don't mix well, change the
opening paren to a comma

https://codereview.chromium.org/2021343003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_details_permission.js:46: *
@param {!string} left The first origin to compare.
nit here & below: by default strings are non-nullable, so we only mark Objects
as being nullable or non-nullable: {string}

[michaelpg, 2016-06-03 13:37:30]

lgtm

[Finnur, 2016-06-03 19:58:06]

Thanks for the review. Checking in.

https://codereview.chromium.org/2021343003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_details_permission.js
(right):

https://codereview.chromium.org/2021343003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_details_permission.js:45: *
http://[*.]google.com.
On 2016/06/03 13:37:14, michaelpg wrote:
> nit: closing paren (or if parens and wildcards don't mix well, change the
> opening paren to a comma

Done.

https://codereview.chromium.org/2021343003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_details_permission.js:46: *
@param {!string} left The first origin to compare.
Dang. Keep forgetting. :)

[Finnur, 2016-06-03 19:58:12]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2016-06-03 19:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2021343003/#ps120001
(title: "Address feedback and add icons")

[commit-bot: I haz the power, 2016-06-03 19:58:38]

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

[commit-bot: I haz the power, 2016-06-03 20:07:12]

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

[commit-bot: I haz the power, 2016-06-03 20:07:13]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Finnur, 2016-06-03 20:09:14]

Description was changed from

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

TBR=sky
BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Finnur, 2016-06-03 20:09:14]

finnur@chromium.org changed reviewers:
+ sky@chromium.org

[Finnur, 2016-06-03 20:09:31]

TBR Scott for a minor change to chrome_pages.cc.

[Finnur, 2016-06-03 20:09:38]

The CQ bit was checked by finnur@chromium.org

[commit-bot: I haz the power, 2016-06-03 20:09:57]

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

[sky, 2016-06-03 20:30:10]

LGTM

[commit-bot: I haz the power, 2016-06-03 22:07:39]

Description was changed from

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

TBR=sky
BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

TBR=sky
BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-03 22:07:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-03 22:09:35]

Description was changed from

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

TBR=sky
BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add five new top level categories and corresponding entries on the Site Details
page.

Automatic downloads
Background sync
Key generation
Plugins
Unsandboxed plugin access

Also changed it so that, in JS land, the Category variable is a string, because
the enum value of the ContentSettingsTypes is variable depending on platform, so
we don't want to be hardcoding those in JS. This is similar to the Allow/Block
values (the PermissionValues enum), which used to be ints, but are now converted
int <-> string on the JS/C++ boundary.

TBR=sky
BUG=614277, 543635
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/155a202f6076fad0cd5d86bfefb5c317d8a3708e
Cr-Commit-Position: refs/heads/master@{#397819}
==========

[commit-bot: I haz the power, 2016-06-03 22:09:37]

Patchset 7 (id:??) landed as
https://crrev.com/155a202f6076fad0cd5d86bfefb5c317d8a3708e
Cr-Commit-Position: refs/heads/master@{#397819}