WebUI: swap paper-dropdown-menu out for md-select.

WebUI: swap paper-dropdown-menu out for md-select.

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

Review-Url: https://codereview.chromium.org/2938933002
Cr-Commit-Position: refs/heads/master@{#480904}
Committed: https://chromium.googlesource.com/chromium/src/+/452a15c46bcd0f6235788fd62b210c90336636ef

[scottchen, 2017-06-14 21:05:15]

Description was changed from

==========
WebUI: swap paper-dropdown-menu out for md-select.

BUG=720927
==========

to

==========
WebUI: swap paper-dropdown-menu out for md-select.

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

[scottchen, 2017-06-15 00:28:36]

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

[scottchen, 2017-06-15 00:28:36]

One thing I am a bit concern about is that the "placeholder" label for md-select
isn't as nice as paper-dropdown.

See: http://imgur.com/a/sovw1

One potential solution I have in mind is, we could allow a "md-select-label" in
the "md-select-wrapper" and style it like a placeholder label, much like the
"md-select-underline". Problem (probably solvable) with that is how to hide the
first option in the dropdown list such that it's not there until chosen.

Thoughts on the situation overall?

[dpapad, 2017-06-15 01:35:37]

Posting some initial comments. Will investigate more the problem your are
describing tomorrow.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/emulator/battery_settings.html (right):

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/emulator/battery_settings.html:10: <link
rel="import" href="chrome://resources/polymer/v1_0/paper-item/paper-item.html">
paper-item, paper-listbox, paper-dropdown-menu imports can be removed now.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/emulator/battery_settings.html:82: <template
is="dom-repeat" items="[[powerOptions]]"
Is it necessary to use a dom-repeat for static content (high, low)? Would it
make things easier or harder to not use it?

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/keyboard_shortcuts.html (right):

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.html:6: <link
rel="import" href="chrome://resources/polymer/v1_0/paper-item/paper-item.html">
Is this still used?

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.html:93: <option
value="[[scope.value]]" selected="[[
FYI, I recall now that binding the "selected" attribute (using "$=") is
problematic, where as binding to the "selected" JS property (using simply "=" is
safe).

Previous issue where I came to this conclusion
https://bugs.chromium.org/p/chromium/issues/detail?id=698189#c5, and resulting
fix at https://codereview.chromium.org/2736803002.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/keyboard_shortcuts.js (right):

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.js:19: /** @private
{Array<Object>} */
Can this be !Array<!Object> ?

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.js:76: * @param
{!string} commandScope the current command's scope value
"!" not necessary with strings.

[scottchen, 2017-06-15 23:50:51]

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/emulator/battery_settings.html (right):

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/emulator/battery_settings.html:10: <link
rel="import" href="chrome://resources/polymer/v1_0/paper-item/paper-item.html">
On 2017/06/15 01:35:36, dpapad wrote:
> paper-item, paper-listbox, paper-dropdown-menu imports can be removed now.

Done.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/emulator/battery_settings.html:82: <template
is="dom-repeat" items="[[powerOptions]]"
On 2017/06/15 01:35:36, dpapad wrote:
> Is it necessary to use a dom-repeat for static content (high, low)? Would it
> make things easier or harder to not use it?

Discussed offline with dpapad, hard-coding this in the template is easier since
the enum's short anyway.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/keyboard_shortcuts.html (right):

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.html:6: <link
rel="import" href="chrome://resources/polymer/v1_0/paper-item/paper-item.html">
On 2017/06/15 01:35:36, dpapad wrote:
> Is this still used?

Nope, removing.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.html:93: <option
value="[[scope.value]]" selected="[[
On 2017/06/15 01:35:37, dpapad wrote:
> FYI, I recall now that binding the "selected" attribute (using "$=") is
> problematic, where as binding to the "selected" JS property (using simply "="
is
> safe).
> 
> Previous issue where I came to this conclusion
> https://bugs.chromium.org/p/chromium/issues/detail?id=698189#c5, and resulting
> fix at https://codereview.chromium.org/2736803002.

Discussed with dpapad offline, see new patch for cleaner solution.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/keyboard_shortcuts.js (right):

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.js:19: /** @private
{Array<Object>} */
On 2017/06/15 01:35:37, dpapad wrote:
> Can this be !Array<!Object> ?

This property is removed in the newest patch.

https://codereview.chromium.org/2938933002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_extensions/keyboard_shortcuts.js:76: * @param
{!string} commandScope the current command's scope value
On 2017/06/15 01:35:37, dpapad wrote:
> "!" not necessary with strings.

Acknowledged, though this function is removed in the newest patch.

[dpapad, 2017-06-16 00:55:19]

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/emulator/battery_settings.html (right):

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/emulator/battery_settings.html:79: <option
value="high">
Nit: Can we fit those into one line?

<option value="high">High</option>

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/keyboard_shortcuts.html (right):

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/keyboard_shortcuts.html:88: when
CommandScope_ becomes available. -->
Maybe s/when/only after

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.html:110:
-webkit-margin-start: calc(-1 * var(--md-select-side-padding));
I've found two other places where we do similar thing, see
https://cs.chromium.org/search/?q=%22Mitigate+for+md-select%22&type=cs. Should
we update those to also use the variable?

[scottchen, 2017-06-16 01:53:20]

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/emulator/battery_settings.html (right):

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/chromeos/emulator/battery_settings.html:79: <option
value="high">
On 2017/06/16 00:55:19, dpapad wrote:
> Nit: Can we fit those into one line?
> 
> <option value="high">High</option>

Done.

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_extensions/keyboard_shortcuts.html (right):

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_extensions/keyboard_shortcuts.html:88: when
CommandScope_ becomes available. -->
On 2017/06/16 00:55:19, dpapad wrote:
> Maybe s/when/only after

Done.

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/2938933002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.html:110:
-webkit-margin-start: calc(-1 * var(--md-select-side-padding));
On 2017/06/16 00:55:19, dpapad wrote:
> I've found two other places where we do similar thing, see
> https://cs.chromium.org/search/?q=%22Mitigate+for+md-select%22&type=cs. Should
> we update those to also use the variable?

Yes!

[dpapad, 2017-06-16 02:45:31]

LGTM

[scottchen, 2017-06-16 21:14:27]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-16 21:14:51]

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-06-16 22:03:05]

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

[commit-bot: I haz the power, 2017-06-16 22:03:06]

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

[scottchen, 2017-06-16 22:36:42]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-06-16 22:36:42]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2938933002/#ps200001
(title: "fix test")

[commit-bot: I haz the power, 2017-06-16 22:37:02]

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

[commit-bot: I haz the power, 2017-06-16 22:37:03]

This CL has an open dependency (Issue 2939243003 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[scottchen, 2017-06-16 22:55:38]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-16 22:55:59]

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-06-17 00:03:57]

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

[commit-bot: I haz the power, 2017-06-17 00:03:58]

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

[scottchen, 2017-06-19 23:45:17]

Patchset #12 (id:220001) has been deleted

[scottchen, 2017-06-19 23:54:59]

The CQ bit was checked by scottchen@chromium.org

[scottchen, 2017-06-19 23:55:00]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2938933002/#ps240001
(title: "fix test")

[commit-bot: I haz the power, 2017-06-19 23:55:29]

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-06-20 01:08:24]

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

[commit-bot: I haz the power, 2017-06-20 01:08:25]

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

[scottchen, 2017-06-20 18:06:34]

The CQ bit was checked by scottchen@chromium.org

[commit-bot: I haz the power, 2017-06-20 18:06:54]

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-06-20 19:33:27]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497981994033860,
"parent_rev": "99c2df715bdf733d259528f9f5f15a35c89a81de", "commit_rev":
"452a15c46bcd0f6235788fd62b210c90336636ef"}

[commit-bot: I haz the power, 2017-06-20 19:33:40]

Description was changed from

==========
WebUI: swap paper-dropdown-menu out for md-select.

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

to

==========
WebUI: swap paper-dropdown-menu out for md-select.

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

Review-Url: https://codereview.chromium.org/2938933002
Cr-Commit-Position: refs/heads/master@{#480904}
Committed:
https://chromium.googlesource.com/chromium/src/+/452a15c46bcd0f6235788fd62b21...
==========

[commit-bot: I haz the power, 2017-06-20 19:33:42]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/452a15c46bcd0f6235788fd62b21...