[MD setting] tool tips on controlledBy site exceptions

[MD setting] tool tips on controlledBy site exceptions

This CL removes the one-off controlledBy indicator code within the
content settings with the cr policy indicator element commonly used in
settings. This provides tooltips on the indicators.

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

Review-Url: https://codereview.chromium.org/2769303004
Cr-Commit-Position: refs/heads/master@{#460189}
Committed: https://chromium.googlesource.com/chromium/src/+/3f291e1026f95c31436d7ae98fd9a8814fbbd440

[dschuyler, 2017-03-25 01:16:56]

Description was changed from

==========
[MD setting] tool tips on controlledBy site exceptions

This CL removes the one-off controlledBy indicator code within the
content settings with the cr policy indicator element commonly used in
settings. This provides tooltips on the indicators.

BUG=704001
==========

to

==========
[MD setting] tool tips on controlledBy site exceptions

This CL removes the one-off controlledBy indicator code within the
content settings with the cr policy indicator element commonly used in
settings. This provides tooltips on the indicators.

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

[dschuyler, 2017-03-25 01:16:59]

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

[commit-bot: I haz the power, 2017-03-25 01:17:19]

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-25 01:20:00]

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

[commit-bot: I haz the power, 2017-03-25 01:43:49]

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

[commit-bot: I haz the power, 2017-03-25 01:43:50]

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_...)

[tommycli, 2017-03-27 16:02:08]

overall lgtm, though I have some concerns about the semantics of the new
'enforcement' field as noted in the below comments.

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:76: <!-- This div
must not contain extra white-space. -->
nitpick: i think 'whitespace' is more common in our code.

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:180: * @param
{string} enforcement Whether the exception is controlled.
nit: you may be able to use the actual chrome.settingsPrivate.Enforcement type
here. 

Also the comment "Whether the..." kind of implies a boolean type variable...
while it's actually more like an enum.

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:168:
var enforcement = '';
Hmm it seems like the enum only has these two possible values:
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/set...

maybe it would not be good to introduce the new "enforcement" concept... since
all the content settings authors are already familiar with the three possible
sources of settings... pref, policy, and extensions.

[dschuyler, 2017-03-28 01:03:31]

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

[commit-bot: I haz the power, 2017-03-28 01:03:55]

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-28 01:05:54]

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.html:76: <!-- This div
must not contain extra white-space. -->
On 2017/03/27 16:02:07, tommycli wrote:
> nitpick: i think 'whitespace' is more common in our code.

Done.

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_list.js:180: * @param
{string} enforcement Whether the exception is controlled.
On 2017/03/27 16:02:07, tommycli wrote:
> nit: you may be able to use the actual chrome.settingsPrivate.Enforcement type
> here. 
> 
> Also the comment "Whether the..." kind of implies a boolean type variable...
> while it's actually more like an enum.

Done.

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:168:
var enforcement = '';
On 2017/03/27 16:02:07, tommycli wrote:
> Hmm it seems like the enum only has these two possible values:
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/set...
> 
> maybe it would not be good to introduce the new "enforcement" concept... since
> all the content settings authors are already familiar with the three possible
> sources of settings... pref, policy, and extensions.

There are three states that enforcement of a setting may be in. The third state
is unenforced. 

Adding enforcement in content settings allows content settings to use existing
chrome policy controls rather than rolling separate one-off code for content
settings.

The separation of enforcement and controlledBy looks like it came in with
a2df3b6 2015-06-11 00:37:10. It's a new concept for content settings, but an
old-ish concept for the rest of settings. I'm interested in getting the
settings/content settings the code using the same concepts, but I'm flexible on
which direction we go in (use separate enforcement or not). For now, content
settings is the odd one out, so I'd like to migrate it to use the common code.

[commit-bot: I haz the power, 2017-03-28 02:18:48]

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

[commit-bot: I haz the power, 2017-03-28 02:18:48]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-03-28 16:20:44]

lgtm

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:168:
var enforcement = '';
On 2017/03/28 01:05:53, dschuyler wrote:
> On 2017/03/27 16:02:07, tommycli wrote:
> > Hmm it seems like the enum only has these two possible values:
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/set...
> > 
> > maybe it would not be good to introduce the new "enforcement" concept...
since
> > all the content settings authors are already familiar with the three
possible
> > sources of settings... pref, policy, and extensions.
> 
> There are three states that enforcement of a setting may be in. The third
state
> is unenforced. 
> 
> Adding enforcement in content settings allows content settings to use existing
> chrome policy controls rather than rolling separate one-off code for content
> settings.
> 
> The separation of enforcement and controlledBy looks like it came in with
> a2df3b6 2015-06-11 00:37:10. It's a new concept for content settings, but an
> old-ish concept for the rest of settings. I'm interested in getting the
> settings/content settings the code using the same concepts, but I'm flexible
on
> which direction we go in (use separate enforcement or not). For now, content
> settings is the odd one out, so I'd like to migrate it to use the common code.

I'm surprised that closure considers '' a valid value of the Enum, but if it
does, then i have no problem with it.

[dschuyler, 2017-03-28 19:27:05]

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

https://codereview.chromium.org/2769303004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:168:
var enforcement = '';
On 2017/03/28 16:20:44, tommycli wrote:
> On 2017/03/28 01:05:53, dschuyler wrote:
> > On 2017/03/27 16:02:07, tommycli wrote:
> > > Hmm it seems like the enum only has these two possible values:
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/closure_compiler/externs/set...
> > > 
> > > maybe it would not be good to introduce the new "enforcement" concept...
> since
> > > all the content settings authors are already familiar with the three
> possible
> > > sources of settings... pref, policy, and extensions.
> > 
> > There are three states that enforcement of a setting may be in. The third
> state
> > is unenforced. 
> > 
> > Adding enforcement in content settings allows content settings to use
existing
> > chrome policy controls rather than rolling separate one-off code for content
> > settings.
> > 
> > The separation of enforcement and controlledBy looks like it came in with
> > a2df3b6 2015-06-11 00:37:10. It's a new concept for content settings, but an
> > old-ish concept for the rest of settings. I'm interested in getting the
> > settings/content settings the code using the same concepts, but I'm flexible
> on
> > which direction we go in (use separate enforcement or not). For now, content
> > settings is the odd one out, so I'd like to migrate it to use the common
code.
> 
> I'm surprised that closure considers '' a valid value of the Enum, but if it
> does, then i have no problem with it.

I see what you mean. It likely should not allow it. I'm going to look at
specifying the enforcement differently (maybe changing the closure type). I'll
do that in a separate CL so we can focus on just that topic.

[dschuyler, 2017-03-28 19:27:16]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-03-28 19:27:41]

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-28 19:34:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490729236853820,
"parent_rev": "afaccab374bddd9bfc6e3609806da73860a5ba7d", "commit_rev":
"3f291e1026f95c31436d7ae98fd9a8814fbbd440"}

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

Description was changed from

==========
[MD setting] tool tips on controlledBy site exceptions

This CL removes the one-off controlledBy indicator code within the
content settings with the cr policy indicator element commonly used in
settings. This provides tooltips on the indicators.

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

to

==========
[MD setting] tool tips on controlledBy site exceptions

This CL removes the one-off controlledBy indicator code within the
content settings with the cr policy indicator element commonly used in
settings. This provides tooltips on the indicators.

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

Review-Url: https://codereview.chromium.org/2769303004
Cr-Commit-Position: refs/heads/master@{#460189}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f291e1026f95c31436d7ae98fd9...
==========

[commit-bot: I haz the power, 2017-03-28 19:36:00]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3f291e1026f95c31436d7ae98fd9...