[MD settings] edit content settings exceptions

[MD settings] edit content settings exceptions

This CL enables the edit dialog for content settings exceptions beyond
the cookies category. Specifically: Automatic Downloads, Background
Sync, Images, JavaScript, Notifications, Plugins, Popups, and
Unsandboxed Plugins. The intention is to match which exceptions are
editable in the old options.

BUG=695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2749023008
Cr-Commit-Position: refs/heads/master@{#458258}
Committed: https://chromium.googlesource.com/chromium/src/+/09ce829191004d5e0599f6dcd0dcc1491214f11d

[dschuyler, 2017-03-16 22:55:42]

Description was changed from

==========
[MD settings] edit content settings exceptions

This CL enables the edit dialog for content settings exceptions beyond
the cookies category. Specifically: Automatic Downloads, Background
Sync, Images, JavaScript, Notifications, Plugins, Popups, and
Unsandboxed Plugins. The intention is to match which exceptions are
editable in the old options.

BUG=695578
==========

to

==========
[MD settings] edit content settings exceptions

This CL enables the edit dialog for content settings exceptions beyond
the cookies category. Specifically: Automatic Downloads, Background
Sync, Images, JavaScript, Notifications, Plugins, Popups, and
Unsandboxed Plugins. The intention is to match which exceptions are
editable in the old options.

BUG=695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-03-16 22:55:44]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 22:56:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 23:56:29]

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

[commit-bot: I haz the power, 2017-03-16 23:56:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dschuyler, 2017-03-18 00:20:24]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 00:21:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-03-18 00:30:14]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 00:30:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-03-18 00:36:11]

dschuyler@chromium.org changed reviewers:
+ dpapad@chromium.org

[dschuyler, 2017-03-18 00:36:12]

This is a CL that got bigger before it got smaller. I initially added the other
content types to the code checking whether 'edit' should be present, then
noticed that it was a compliment to the content types that are already
read-only. So I simplified by removing code. 

In the end, these are *not* editable:

CAMERA
GEOLOCATION,
MIC
MIDI_DEVICES
PROTECTED_CONTENT
PROTOCOL_HANDLERS
USB_DEVICES
ZOOM_LEVELS
 
This *are* editable:

BACKGROUND_SYNC 
COOKIES 
IMAGES 
JAVASCRIPT 
NOTIFICATIONS 
PLUGINS 
POPUPS
UNSANDBOXED_PLUGINS

https://codereview.chromium.org/2749023008/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/site_settings/site_list.html (right):

https://codereview.chromium.org/2749023008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:68:
on-tap="onOriginTap_">
Just wrapping long line. Not required for CL.

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

https://codereview.chromium.org/2749023008/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:406: this.category
== settings.ContentSettingsTypes.UNSANDBOXED_PLUGINS;
Hmm, I could convert this to a dictionary and to a single map lookup, but I'm
concerned for the startup performance cost if I made that outside of this
function -- and it would be a wash if the map were set up right here.

I'd rather make this data driven in the html. I'll look into that.

[commit-bot: I haz the power, 2017-03-18 01:25:20]

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

[commit-bot: I haz the power, 2017-03-18 01:25:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2017-03-18 01:44:39]

I am a bit confused about the "read-only" aspect of some content settings
categories. Specifically see
https://bugs.chromium.org/p/chromium/issues/detail?id=702857 (just filed).

Other than that this CL seems OK to me. Can we update or maybe re-purpose this
test
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/site_lis...,
to ensure that the action menu shows up for all categories that should show it?

[dschuyler, 2017-03-20 19:35:02]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 19:35:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 20:54:10]

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

[commit-bot: I haz the power, 2017-03-20 20:54:11]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-03-20 21:51:58]

On 2017/03/18 01:44:39, dpapad wrote:
> I am a bit confused about the "read-only" aspect of some content settings
> categories. Specifically see
> https://bugs.chromium.org/p/chromium/issues/detail?id=702857 (just filed).

Read-only may need some refinement. I can check with Tommy on that (whom added
read-only). It can be separated from this CL.
 
> Other than that this CL seems OK to me. Can we update or maybe re-purpose this
> test
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/site_lis...,
> to ensure that the action menu shows up for all categories that should show
it?

The unit tests check whether the site list works correctly. The code that
controls which are read-only is done from the Privacy section. I can look into a
follow-up CL expanding the privacy unit tests.

[dpapad, 2017-03-20 22:22:47]

LGTM with question.

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

https://codereview.chromium.org/2749023008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:150: *
@return {SiteException} The expanded (full) SiteException.
SiteException typedef does not have a |category| field,
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s....


Should it be also updated? Having said that, it is a bit alarming that the JS
compiler does not throw any error for this.

[dschuyler, 2017-03-20 23:44:19]

The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 23:45:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dschuyler, 2017-03-20 23:45:31]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-20 23:47:49]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[commit-bot: I haz the power, 2017-03-20 23:53:09]

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

[commit-bot: I haz the power, 2017-03-20 23:53:13]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-03-21 00:12:58]

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

https://codereview.chromium.org/2749023008/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:150: *
@return {SiteException} The expanded (full) SiteException.
On 2017/03/20 22:22:47, dpapad wrote:
> SiteException typedef does not have a |category| field,
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s....
> 
> 
> Should it be also updated? Having said that, it is a bit alarming that the JS
> compiler does not throw any error for this.

I made a CL at https://codereview.chromium.org/2760183002/
to explore this further.

[dschuyler, 2017-03-21 00:13:04]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-03-21 00:14:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 00:41:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490055184896610,
"parent_rev": "c27e679d80dbdcb09c867f32a8a162964460df3a", "commit_rev":
"09ce829191004d5e0599f6dcd0dcc1491214f11d"}

[commit-bot: I haz the power, 2017-03-21 00:42:08]

Description was changed from

==========
[MD settings] edit content settings exceptions

This CL enables the edit dialog for content settings exceptions beyond
the cookies category. Specifically: Automatic Downloads, Background
Sync, Images, JavaScript, Notifications, Plugins, Popups, and
Unsandboxed Plugins. The intention is to match which exceptions are
editable in the old options.

BUG=695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] edit content settings exceptions

This CL enables the edit dialog for content settings exceptions beyond
the cookies category. Specifically: Automatic Downloads, Background
Sync, Images, JavaScript, Notifications, Plugins, Popups, and
Unsandboxed Plugins. The intention is to match which exceptions are
editable in the old options.

BUG=695578
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2749023008
Cr-Commit-Position: refs/heads/master@{#458258}
Committed:
https://chromium.googlesource.com/chromium/src/+/09ce829191004d5e0599f6dcd0dc...
==========

[commit-bot: I haz the power, 2017-03-21 00:42:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/09ce829191004d5e0599f6dcd0dc...