[MD settings] content site list toggles

[MD settings] content site list toggles

This CL makes the boolean settings in the content settings site list
subpage. Also adds the settings-boolean-control-behavior.

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

Committed: https://crrev.com/151575ce0bb6fc26a3381c029fccc3c0ec5bfda0
Cr-Commit-Position: refs/heads/master@{#426666}

[dschuyler, 2016-10-20 19:31:30]

Description was changed from

==========
[MD settings] content site list toggles

This CL makes the boolean settings in the content settings site list
subpage. Also adds the settings-boolean-control-behavior.

BUG=657960
==========

to

==========
[MD settings] content site list toggles

This CL makes the boolean settings in the content settings site list
subpage. Also adds the settings-boolean-control-behavior.

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

[dschuyler, 2016-10-20 19:31:34]

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

[commit-bot: I haz the power, 2016-10-20 19:32:14]

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

[dschuyler, 2016-10-20 20:53:19]

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

[commit-bot: I haz the power, 2016-10-20 20:54:45]

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

[commit-bot: I haz the power, 2016-10-20 20:54:46]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-20 21:47:12]

Design question: Would it make things easier and reduce the amount of code, if

Instead of making a behavior that both a checkbox and a toggle
implement/inherit, took the existing settings_checkbox, rename it to something
more generic like settings-switch, and customize it by adding a new Polymer
property "type", where values could be "checkbox" or "toggle". The HTML would
look like this

<settings-switch pref={{somePref}} type="checkbox"></settings-switch>

You could give it a default value so that we don't even need to update current
clients (besides the renaming).

FWIW, customizing user input via a "type" attribute is fairly common, since it
is the pattern used by the native <input> field with types text/number/date (see
http://www.w3schools.com/tags/att_input_type.asp).

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_toggle_button.html
(right):

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_toggle_button.html:31: <div
class="secondary">[[subLabel]]</div>
Should this be hidden if subLabel does not exist? Per comment in the new
behavior file, this is an optional field.

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

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.html:15:
sdisplay: block;
typo?

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.html:42:
disabled="[[!categoryEnabled]]" id="sessionOnlyCheckbox"
Nit (optional): Maybe put the |id| first? Not a strict rule, but I find it
helpful to visually parse HTML code when the IDs are always first (then
"class=", then anything else).

[dschuyler, 2016-10-20 22:30:40]

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

[commit-bot: I haz the power, 2016-10-20 22:31:14]

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

[dschuyler, 2016-10-20 22:33:38]

We chatted offline about <foo type="bar"></foo> vs.
<bar></bar>. It seems to simply be
a style choice, neither seems provably better so
far, so I'm going with <bar></bar>.

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_toggle_button.html
(right):

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_toggle_button.html:31: <div
class="secondary">[[subLabel]]</div>
On 2016/10/20 21:47:12, dpapad wrote:
> Should this be hidden if subLabel does not exist? Per comment in the new
> behavior file, this is an optional field.

I believe there is no need. I suspect, but haven't proven, that
there would be more overhead trying to hide this div based on a
bound variable than just having an empty div. 

e.g. is there enough runtime difference between these two to add
the second binding (specifically because it's the highly optimized
<div> as a special case - if it were a different tag, I'd have a 
different opinion):

<div class="secondary"></div>

vs.

<div class="secondary" hidden></div>

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

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.html:15:
sdisplay: block;
On 2016/10/20 21:47:12, dpapad wrote:
> typo?

I was testing to see if it impacted the UI if I removed it.
(then forgot to remove it).

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/site_settings/site_settings_category.html:42:
disabled="[[!categoryEnabled]]" id="sessionOnlyCheckbox"
On 2016/10/20 21:47:12, dpapad wrote:
> Nit (optional): Maybe put the |id| first? Not a strict rule, but I find it
> helpful to visually parse HTML code when the IDs are always first (then
> "class=", then anything else).

I'm on board with getting this in a style guide. My 
preference is to go by the style guide, or alphabetical
if the style guide isn't specific. E.g. we make exceptions
for is:, behavior:, and properties: in .js files.
Since this isn't yet in a guide (afaik) it's 'correct'
being alphabetical (imo).

Let's bring this up to others outside of this CL and
get it in a guide either way (and maybe add a presubmit
check) :)

[dpapad, 2016-10-20 22:49:26]

LGTM

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/controls/settings_toggle_button.html
(right):

https://codereview.chromium.org/2437183002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/controls/settings_toggle_button.html:31: <div
class="secondary">[[subLabel]]</div>
On 2016/10/20 at 22:33:38, dschuyler wrote:
> On 2016/10/20 21:47:12, dpapad wrote:
> > Should this be hidden if subLabel does not exist? Per comment in the new
> > behavior file, this is an optional field.
> 
> I believe there is no need. I suspect, but haven't proven, that
> there would be more overhead trying to hide this div based on a
> bound variable than just having an empty div. 
> 
> e.g. is there enough runtime difference between these two to add
> the second binding (specifically because it's the highly optimized
> <div> as a special case - if it were a different tag, I'd have a 
> different opinion):
> 
> <div class="secondary"></div>
> 
> vs.
> 
> <div class="secondary" hidden></div>

SG, as long as it looks visually correct. My concern was that an empty div might
still take up some space (depneding on its styling, margin/border etc), but
probably not the case here.

[commit-bot: I haz the power, 2016-10-21 00:11:50]

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

[commit-bot: I haz the power, 2016-10-21 00:11:51]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2016-10-21 00:41:25]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-10-21 00:42:19]

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-10-21 00:56:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:25:18]

Description was changed from

==========
[MD settings] content site list toggles

This CL makes the boolean settings in the content settings site list
subpage. Also adds the settings-boolean-control-behavior.

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

to

==========
[MD settings] content site list toggles

This CL makes the boolean settings in the content settings site list
subpage. Also adds the settings-boolean-control-behavior.

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

Committed: https://crrev.com/151575ce0bb6fc26a3381c029fccc3c0ec5bfda0
Cr-Commit-Position: refs/heads/master@{#426666}
==========

[commit-bot: I haz the power, 2016-10-21 13:25:19]

Patchset 2 (id:??) landed as
https://crrev.com/151575ce0bb6fc26a3381c029fccc3c0ec5bfda0
Cr-Commit-Position: refs/heads/master@{#426666}