MD Settings: compose a <paper-radio-button> instead of wrapping one in <controlled-radio-button>

MD Settings: compose a <paper-radio-button> instead of wrapping one in <controlled-radio-button>

R=dpapad@chromium.org,hcarmona@chromium.org
BUG=709579
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2816883002
Cr-Commit-Position: refs/heads/master@{#464292}
Committed: https://chromium.googlesource.com/chromium/src/+/cf655c64cc8f79abcffe366fea0358decb03e6fd

[Dan Beam, 2017-04-12 23:50:58]

Description was changed from

==========
MD Settings: compose a <paper-radio-button> instead of wrapping one in
<controlled-radio-button>

R=dpapad@chromium.org,hcarmona@chromium.org
BUG=709579
==========

to

==========
MD Settings: compose a <paper-radio-button> instead of wrapping one in
<controlled-radio-button>

R=dpapad@chromium.org,hcarmona@chromium.org
BUG=709579
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-04-13 00:16:30]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2017-04-13 00:16:45]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2017-04-13 00:25:17]

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

[commit-bot: I haz the power, 2017-04-13 00:26:09]

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

[dpapad, 2017-04-13 00:35:09]

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_radio_button.html
(right):

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.html:2: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-radio-button/paper-radio-button.html">
No longer needed?

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:38:
pressed_: Boolean,
@private

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:63:
this.setAttribute('aria-checked', this.checked ? 'true' : 'false');
Nit (optional): The following should be equivalent
 
this.setAttribute('aria-checked', String(this.checked || false));

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:106:
this.pressed_ = ['down', 'focus'].indexOf(e.type) >= 0;
Nit (optional): Maybe use includes() instead of indexOf()?

[Dan Beam, 2017-04-13 00:59:01]

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_radio_button.html
(right):

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.html:2: <link
rel="import"
href="chrome://resources/polymer/v1_0/paper-radio-button/paper-radio-button.html">
On 2017/04/13 00:35:09, dpapad wrote:
> No longer needed?

Done. (with pleasure)

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:38:
pressed_: Boolean,
On 2017/04/13 00:35:09, dpapad wrote:
> @private

Done.

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:63:
this.setAttribute('aria-checked', this.checked ? 'true' : 'false');
On 2017/04/13 00:35:09, dpapad wrote:
> Nit (optional): The following should be equivalent
>  
> this.setAttribute('aria-checked', String(this.checked || false));

so is !!this.checked, i just figured this is less weird

[Dan Beam, 2017-04-13 00:59:07]

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

[commit-bot: I haz the power, 2017-04-13 01:00:10]

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

[dpapad, 2017-04-13 01:08:13]

LGTM. Did some sanity checking, but did not try policy/extension controlled
states myself.

[hcarmona, 2017-04-13 01:18:38]

LGTM

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:23: value:
'',
Nit: add a comment that this is needed to avoid a refactor removing the default
value.

[Dan Beam, 2017-04-13 01:22:58]

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/controls/controlled_radio_button.js
(right):

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:23: value:
'',
On 2017/04/13 01:18:37, hcarmona wrote:
> Nit: add a comment that this is needed to avoid a refactor removing the
default
> value.

Done.

https://codereview.chromium.org/2816883002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/controls/controlled_radio_button.js:106:
this.pressed_ = ['down', 'focus'].indexOf(e.type) >= 0;
On 2017/04/13 00:35:09, dpapad wrote:
> Nit (optional): Maybe use includes() instead of indexOf()?

Done.

[Dan Beam, 2017-04-13 01:24:13]

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

[commit-bot: I haz the power, 2017-04-13 01:25:04]

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-04-13 02:13:13]

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

[commit-bot: I haz the power, 2017-04-13 02:13:14]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Dan Beam, 2017-04-13 02:48:49]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-04-13 02:48:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org,
hcarmona@chromium.org
Link to the patchset: https://codereview.chromium.org/2816883002/#ps150001
(title: "fix tests")

[commit-bot: I haz the power, 2017-04-13 02:49:39]

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-04-13 04:14:44]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1492051729333650,
"parent_rev": "2e00979d16dd569963846451ce467626b1e86a04", "commit_rev":
"cf655c64cc8f79abcffe366fea0358decb03e6fd"}

[commit-bot: I haz the power, 2017-04-13 04:15:43]

Description was changed from

==========
MD Settings: compose a <paper-radio-button> instead of wrapping one in
<controlled-radio-button>

R=dpapad@chromium.org,hcarmona@chromium.org
BUG=709579
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: compose a <paper-radio-button> instead of wrapping one in
<controlled-radio-button>

R=dpapad@chromium.org,hcarmona@chromium.org
BUG=709579
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2816883002
Cr-Commit-Position: refs/heads/master@{#464292}
Committed:
https://chromium.googlesource.com/chromium/src/+/cf655c64cc8f79abcffe366fea03...
==========

[commit-bot: I haz the power, 2017-04-13 04:15:45]

Committed patchset #7 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/cf655c64cc8f79abcffe366fea03...