[MD settings] show icon when slider controlled by something

[MD settings] show icon when slider controlled by something

This CL adds a controlled-by indicator when a slider is controlled by
policy or an extention.

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

Review-Url: https://codereview.chromium.org/2737083002
Cr-Commit-Position: refs/heads/master@{#456247}
Committed: https://chromium.googlesource.com/chromium/src/+/141e9ca007617a802a8a22e5e8a635d2d4e7dcff

[dschuyler, 2017-03-07 22:01:06]

Description was changed from

==========
[MD settings] show icon when slider controlled by something

This CL adds a controlled-by indicator when a slider is controlled by
policy or an extention.

BUG=693302
==========

to

==========
[MD settings] show icon when slider controlled by something

This CL adds a controlled-by indicator when a slider is controlled by
policy or an extention.

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

[dschuyler, 2017-03-07 22:01:08]

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

[commit-bot: I haz the power, 2017-03-07 22:03:24]

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-07 22:19:23]

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

[commit-bot: I haz the power, 2017-03-07 22:19:24]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dschuyler, 2017-03-07 22:53:10]

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

[dschuyler, 2017-03-07 22:53:28]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-07 22:54:03]

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-07 22:57:10]

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

[dschuyler, 2017-03-07 22:57:28]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-07 22:58:40]

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-07 23:01:10]

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

[dschuyler, 2017-03-07 23:01:11]

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right):

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:14:
-webkit-margin-start: var(--checkbox-spacing);
Imo, checkbox-spacing needs to be renamed. I'm planning to do that separate from
this CL.

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:67: <div id="labels"
disabled$="[[shouldDisableSlider_(pref.*)]]">
I'm a bit reluctant to call shouldDisableSlider_ in two places (line 67 and line
63), but the other options I considered seemed worse. If there's a clearly
better way to handle this please let me know.

[stevenjb, 2017-03-07 23:11:48]

I feel like we need a test for this... :) (We do already have
cr_slider_tests.js, so shouldn't be hard to add one)

https://codereview.chromium.org/2737083002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2737083002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:26: pref: {
Given that there are many things being set here, can we call this
selectedModePref_ ? Also, we should type it properly. Is there any reason for it
not to be private?

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right):

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:14:
-webkit-margin-start: var(--checkbox-spacing);
On 2017/03/07 23:01:11, dschuyler wrote:
> Imo, checkbox-spacing needs to be renamed. I'm planning to do that separate
from
> this CL.

I'm fine either way, but sgtm.

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:67: <div id="labels"
disabled$="[[shouldDisableSlider_(pref.*)]]">
On 2017/03/07 23:01:11, dschuyler wrote:
> I'm a bit reluctant to call shouldDisableSlider_ in two places (line 67 and
line
> 63), but the other options I considered seemed worse. If there's a clearly
> better way to handle this please let me know.

You can use a computed property, e.g. 'disableSlider_: { type: Boolean,
computed: shouldDisableSlider_(pref.*) }
Maybe rename the method computeDisableSlider_.

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:55: return this.disabled
|| this.isPrefEnforced();
Did you test this? I thought we needed to to use getAttribute for attributes?

[commit-bot: I haz the power, 2017-03-07 23:51:59]

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

[commit-bot: I haz the power, 2017-03-07 23:52:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-03-08 21:37:36]

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

[commit-bot: I haz the power, 2017-03-08 21:38:16]

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-08 21:44:08]

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

[commit-bot: I haz the power, 2017-03-08 21:44:24]

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-08 21:46:49]

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

[commit-bot: I haz the power, 2017-03-08 21:47:29]

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-08 21:47:41]

Patchset #2 (id:60001) has been deleted

[dschuyler, 2017-03-08 21:47:49]

Patchset #2 (id:80001) has been deleted

[dschuyler, 2017-03-08 21:50:12]

https://codereview.chromium.org/2737083002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2737083002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:26: pref: {
On 2017/03/07 23:11:48, stevenjb wrote:
> Given that there are many things being set here, can we call this
> selectedModePref_ ? Also, we should type it properly. Is there any reason for
it
> not to be private?

Done.

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.html (right):

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.html:67: <div id="labels"
disabled$="[[shouldDisableSlider_(pref.*)]]">
On 2017/03/07 23:11:48, stevenjb wrote:
> On 2017/03/07 23:01:11, dschuyler wrote:
> > I'm a bit reluctant to call shouldDisableSlider_ in two places (line 67 and
> line
> > 63), but the other options I considered seemed worse. If there's a clearly
> > better way to handle this please let me know.
> 
> You can use a computed property, e.g. 'disableSlider_: { type: Boolean,
> computed: shouldDisableSlider_(pref.*) }
> Maybe rename the method computeDisableSlider_.

Done.

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/40001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:55: return this.disabled
|| this.isPrefEnforced();
On 2017/03/07 23:11:48, stevenjb wrote:
> Did you test this? I thought we needed to to use getAttribute for attributes?

or hasAttribute for boolean attributes.

https://codereview.chromium.org/2737083002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js
(left):

https://codereview.chromium.org/2737083002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:42: };
These values have been moved into settings_private_test_constants.js
so that they may be shared among tests.

https://codereview.chromium.org/2737083002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/cr_elements/settings_private_test_constants.js
(right):

https://codereview.chromium.org/2737083002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/cr_elements/settings_private_test_constants.js:6: //
NOTE: These need to be kept in sync with settings_private.idl.
copied from cr_policy_pref_indicator_tests.js

[stevenjb, 2017-03-08 22:35:44]

lgtm w/ nits

https://codereview.chromium.org/2737083002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2737083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display.js:179: /** @type {number}
*/(this.selectedModePref_.value);
nit: clang should be putting a ' ' after */ ? (here and below)

https://codereview.chromium.org/2737083002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:21: disabled_: {
nit: having a 'disabled' attribute and a 'disbaled_' property is a little iffy
to me, I would prefer something like disableSlider_, but just a nit. Also,
private members usually follow public ones.

[commit-bot: I haz the power, 2017-03-08 22:37:55]

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

[commit-bot: I haz the power, 2017-03-08 22:37:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-03-08 23:52:41]

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

[commit-bot: I haz the power, 2017-03-08 23:53:36]

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-08 23:53:39]

https://codereview.chromium.org/2737083002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2737083002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display.js:179: /** @type {number}
*/(this.selectedModePref_.value);
On 2017/03/08 22:35:44, stevenjb wrote:
> nit: clang should be putting a ' ' after */ ? (here and below)

Done.

https://codereview.chromium.org/2737083002/diff/100001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/100001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:21: disabled_: {
On 2017/03/08 22:35:44, stevenjb wrote:
> nit: having a 'disabled' attribute and a 'disbaled_' property is a little iffy
> to me, I would prefer something like disableSlider_, but just a nit. Also,
> private members usually follow public ones.

Done.

[commit-bot: I haz the power, 2017-03-09 01:19:12]

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

[commit-bot: I haz the power, 2017-03-09 01:19:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-03-09 20:05:10]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-03-09 20:05:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2737083002/#ps60002
(title: "nits")

[commit-bot: I haz the power, 2017-03-09 20:05:34]

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-09 21:43:21]

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

[commit-bot: I haz the power, 2017-03-09 21:43:23]

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

[dschuyler, 2017-03-10 21:51:00]

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

[commit-bot: I haz the power, 2017-03-10 21:51:25]

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-10 23:23:35]

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

[commit-bot: I haz the power, 2017-03-10 23:23:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dschuyler, 2017-03-10 23:29:40]

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

[commit-bot: I haz the power, 2017-03-10 23:30:05]

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-11 00:51:31]

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

[commit-bot: I haz the power, 2017-03-11 00:51:32]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-03-11 01:06:16]

unit tests working, thanks.

[Dan Beam, 2017-03-11 01:08:27]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2017-03-11 01:08:28]

https://codereview.chromium.org/2737083002/diff/150001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/150001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:20: /** @type
{!chrome.settingsPrivate.PrefObject} */
why are you making this less generic / using settings-specific types in
ui/webui?

[Dan Beam, 2017-03-11 01:30:51]

https://codereview.chromium.org/2737083002/diff/150001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/150001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:20: /** @type
{!chrome.settingsPrivate.PrefObject} */
On 2017/03/11 01:08:27, Dan Beam wrote:
> why are you making this less generic / using settings-specific types in
> ui/webui?

talked with dschuyler@ offline

let's just move <cr-slider> to <settings-slider> in a followup.  it's only used
from settings.

[dschuyler, 2017-03-11 01:30:55]

https://codereview.chromium.org/2737083002/diff/150001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/cr_slider/cr_slider.js (right):

https://codereview.chromium.org/2737083002/diff/150001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/cr_slider/cr_slider.js:20: /** @type
{!chrome.settingsPrivate.PrefObject} */
On 2017/03/11 01:08:27, Dan Beam wrote:
> why are you making this less generic / using settings-specific types in
> ui/webui?

This will move into the settings/controls directory as settings-slider in a
follow-up CL.

[dschuyler, 2017-03-11 01:31:21]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2017-03-11 01:31:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2737083002/#ps150001
(title: "fix mouse pointer unit test")

[commit-bot: I haz the power, 2017-03-11 01:31: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-11 01:38:48]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1489195881190440,
"parent_rev": "11e28fd6b9380b77273db51ef0b6ccc7ea341944", "commit_rev":
"141e9ca007617a802a8a22e5e8a635d2d4e7dcff"}

[commit-bot: I haz the power, 2017-03-11 01:40:16]

Description was changed from

==========
[MD settings] show icon when slider controlled by something

This CL adds a controlled-by indicator when a slider is controlled by
policy or an extention.

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

to

==========
[MD settings] show icon when slider controlled by something

This CL adds a controlled-by indicator when a slider is controlled by
policy or an extention.

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

Review-Url: https://codereview.chromium.org/2737083002
Cr-Commit-Position: refs/heads/master@{#456247}
Committed:
https://chromium.googlesource.com/chromium/src/+/141e9ca007617a802a8a22e5e8a6...
==========

[commit-bot: I haz the power, 2017-03-11 01:40:17]

Committed patchset #5 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/141e9ca007617a802a8a22e5e8a6...