MD Settings: tweak tap and actionable-ness of <settings-toggle-button>

MD Settings: tweak tap and actionable-ness of <settings-toggle-button>

R=dschuyler@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/60a1f4fca3a7c30c17667fecd69ed034b360575f
Cr-Commit-Position: refs/heads/master@{#436103}

[Dan Beam, 2016-12-01 04:46:06]

Description was changed from

==========
MD Settings: tweak tap and actionable-ness of <settings-toggle-button>

R=dschuyler@chromium.org
BUG=614265
==========

to

==========
MD Settings: tweak tap and actionable-ness of <settings-toggle-button>

R=dschuyler@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-12-01 04:52:55]

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

[commit-bot: I haz the power, 2016-12-01 04:52:59]

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, 2016-12-01 05:02:56]

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

[commit-bot: I haz the power, 2016-12-01 05:02:56]

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

[Dan Beam, 2016-12-01 06:02:07]

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

[commit-bot: I haz the power, 2016-12-01 06:02:25]

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

[Dan Beam, 2016-12-01 06:02:27]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-12-01 06:50:27]

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

[commit-bot: I haz the power, 2016-12-01 06:50:28]

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

[Dan Beam, 2016-12-01 06:59:20]

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

[Dan Beam, 2016-12-01 06:59:28]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-01 06:59:31]

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, 2016-12-01 08:54:59]

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

[commit-bot: I haz the power, 2016-12-01 08:55:00]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-12-02 21:20:51]

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
(right):

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:12:
var SettingsBooleanControlBehaviorImpl = {
Optional:
This my be my (prior) bad, but if we use
isPrefPolicyControlled(), should we add a 
behaviors: here for CrPolicyPrefBehavior?

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:114:
return this.disabled || this.isPrefPolicyControlled(assert(this.pref));
Is the error within isPrefPolicyControlled unclear when this.pref is not set?
I'm curious about how much the assert is adding/helping.

[Dan Beam, 2016-12-02 23:09:10]

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
(right):

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:114:
return this.disabled || this.isPrefPolicyControlled(assert(this.pref));
On 2016/12/02 21:20:51, dschuyler wrote:
> Is the error within isPrefPolicyControlled unclear when this.pref is not set?
> I'm curious about how much the assert is adding/helping.

it's required to closure compile.  this.pref should always be set.

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:120:
CrPolicyPrefBehavior,
^ do you mean add this behavior?

[dschuyler, 2016-12-02 23:17:08]

lgtm

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
(right):

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:12:
var SettingsBooleanControlBehaviorImpl = {
On 2016/12/02 21:20:51, dschuyler wrote:
> Optional:
> This my be my (prior) bad, but if we use
> isPrefPolicyControlled(), should we add a 
> behaviors: here for CrPolicyPrefBehavior?

Whoops, yeah, it's already being added below
on line 120.

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:114:
return this.disabled || this.isPrefPolicyControlled(assert(this.pref));
On 2016/12/02 23:09:10, Dan Beam wrote:
> On 2016/12/02 21:20:51, dschuyler wrote:
> > Is the error within isPrefPolicyControlled unclear when this.pref is not
set?
> > I'm curious about how much the assert is adding/helping.
> 
> it's required to closure compile.  this.pref should always be set.

Okay. I wonder if an if() or something could check the var and make closure
happy. It looks like we may be importing a file and making a runtime function
call for the closure compiler.

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:120:
CrPolicyPrefBehavior,
On 2016/12/02 23:09:10, Dan Beam wrote:
> ^ do you mean add this behavior?

I totally do.

[Dan Beam, 2016-12-02 23:21:28]

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js
(right):

https://codereview.chromium.org/2538373002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/controls/settings_boolean_control_behavior.js:114:
return this.disabled || this.isPrefPolicyControlled(assert(this.pref));
On 2016/12/02 23:17:08, dschuyler wrote:
> On 2016/12/02 23:09:10, Dan Beam wrote:
> > On 2016/12/02 21:20:51, dschuyler wrote:
> > > Is the error within isPrefPolicyControlled unclear when this.pref is not
> set?
> > > I'm curious about how much the assert is adding/helping.
> > 
> > it's required to closure compile.  this.pref should always be set.
> 
> Okay. I wonder if an if() or something could check the var and make closure
> happy. It looks like we may be importing a file and making a runtime function
> call for the closure compiler.

we are doing both of those things, BUT:

a) assert.html will be de-duped, it's used many many many times throughout our
page but every subsequent <link rel=import> is ignored

b) there are many places that run assert() and it's a good thing that we're
checking that this.pref is set.  because if not, there's problems.

[Dan Beam, 2016-12-02 23:21:40]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-12-02 23:22:30]

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, 2016-12-03 00:43:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480720900225130,
"parent_rev": "63e0b6f7a61a31c31ab916ab065761ab1cab7b08", "commit_rev":
"4bd6806a2d35d00c236c530f30f711838d34b618"}

[commit-bot: I haz the power, 2016-12-03 00:43:31]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2016-12-03 00:45:59]

Description was changed from

==========
MD Settings: tweak tap and actionable-ness of <settings-toggle-button>

R=dschuyler@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: tweak tap and actionable-ness of <settings-toggle-button>

R=dschuyler@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/60a1f4fca3a7c30c17667fecd69ed034b360575f
Cr-Commit-Position: refs/heads/master@{#436103}
==========

[commit-bot: I haz the power, 2016-12-03 00:46:00]

Patchset 1 (id:??) landed as
https://crrev.com/60a1f4fca3a7c30c17667fecd69ed034b360575f
Cr-Commit-Position: refs/heads/master@{#436103}