Use md-select instead of paper-dropdown-menu-light in internet page

Use md-select instead of paper-dropdown-menu-light in internet page

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

Committed: https://crrev.com/b0e7fb52816a7b5efc7c1ca92a99cb3e5a6d1e15
Cr-Commit-Position: refs/heads/master@{#425853}

[stevenjb, 2016-10-17 22:30:14]

Description was changed from

==========
Use md-select instead of paper-dropdown-menu-light in internet page

BUG=653689
==========

to

==========
Use md-select instead of paper-dropdown-menu-light in internet page

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

[stevenjb, 2016-10-17 22:35:59]

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

[dpapad, 2016-10-17 22:57:05]

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_apnlist.js (left):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_apnlist.js:144: // We
need to flush the DOM here, otherwise the paper-dropdown-menu-light
Native <select> has the same problem described by this comment. Setting its
|value| is ignored if the <option> that has the corresponding |value| attribute
has not been rendered yet.

Are you sure that this works? I had to do a call to async() to bypass this, see
for example
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo....

Basically

tihs.apnSelectList_ = result;
this.async(function() {
  this.selectedApn_ = ...;
  this.$.selectApn.value = this.selectedApn_;
}.bind(this));

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_apnlist.js (right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_apnlist.js:178: * @param
{!{target: !{value: string}}} event
I think the proper way to type annotate this is as follows

@param {!Event} e
...

var accessPointName = /** @type {!HTMLSelectElement} */ (event.target).value;

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_nameservers.js:108:
this.$.nameserverType.value = this.nameserversType_;
I don't think this observer needed? You can use a 1-way binding to change the
displayed selected value.

<select .... value="[[nameServersType_]]">

This will work just fine as long as you have ensured that the dom-repeat
template has been rendered before setting a value to the <select>'s |value|. See
example at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_nameservers.js:151: *
@param {!{target: !{value: string}}} e
Same here.

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_proxy.js (right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_proxy.js:151:
this.$.proxyType.value = this.proxy.Type;
Same question here. Can we use a 1-way binding to |value| instead of having this
observer?

[stevenjb, 2016-10-17 23:10:19]

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_apnlist.js (left):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_apnlist.js:144: // We
need to flush the DOM here, otherwise the paper-dropdown-menu-light
On 2016/10/17 22:57:05, dpapad wrote:
> Native <select> has the same problem described by this comment. Setting its
> |value| is ignored if the <option> that has the corresponding |value|
attribute
> has not been rendered yet.
> 
> Are you sure that this works? I had to do a call to async() to bypass this,
see
> for example
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/passwo....
> 
> Basically
> 
> tihs.apnSelectList_ = result;
> this.async(function() {
>   this.selectedApn_ = ...;
>   this.$.selectApn.value = this.selectedApn_;
> }.bind(this));

Hrm, yeah, you're right, I need to us async() here.

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_apnlist.js (right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_apnlist.js:178: * @param
{!{target: !{value: string}}} event
On 2016/10/17 22:57:05, dpapad wrote:
> I think the proper way to type annotate this is as follows
> 
> @param {!Event} e
> ...
> 
> var accessPointName = /** @type {!HTMLSelectElement} */ (event.target).value;

Sure. Done.

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_nameservers.js:108:
this.$.nameserverType.value = this.nameserversType_;
On 2016/10/17 22:57:05, dpapad wrote:
> I don't think this observer needed? You can use a 1-way binding to change the
> displayed selected value.
> 
> <select .... value="[[nameServersType_]]">
> 
> This will work just fine as long as you have ensured that the dom-repeat
> template has been rendered before setting a value to the <select>'s |value|.
See
> example at
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....

Hmm, I'm not sure I understand how using data binding ensures "the dom-repeat
template has been rendered before setting a value to the <select>'s |value|".
I'll ping you to discuss this.

[stevenjb, 2016-10-17 23:58:01]

PTAL

Note: I used async() in all 3 cases so that we don't have to think too hard
about whether or not the dom-repeat contents are dynamic, and whether or not the
static contents are guaranteed to be stamped before networkPropertiesChanged_
gets called (I'm not sure that they are).

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_nameservers.js:108:
this.$.nameserverType.value = this.nameserversType_;
On 2016/10/17 23:10:19, stevenjb wrote:
> On 2016/10/17 22:57:05, dpapad wrote:
> > I don't think this observer needed? You can use a 1-way binding to change
the
> > displayed selected value.
> > 
> > <select .... value="[[nameServersType_]]">
> > 
> > This will work just fine as long as you have ensured that the dom-repeat
> > template has been rendered before setting a value to the <select>'s |value|.
> See
> > example at
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....
> 
> Hmm, I'm not sure I understand how using data binding ensures "the dom-repeat
> template has been rendered before setting a value to the <select>'s |value|".
> I'll ping you to discuss this.

After looking over the settings-dropdown-menu implementation I understand what
you meant now.

This is pretty awkward, a helper would be nice here, but at this point I'm just
going to push this forward.

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_nameservers.js:151: *
@param {!{target: !{value: string}}} e
On 2016/10/17 22:57:05, dpapad wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/internet_page/network_proxy.js (right):

https://codereview.chromium.org/2422423002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/internet_page/network_proxy.js:151:
this.$.proxyType.value = this.proxy.Type;
On 2016/10/17 22:57:05, dpapad wrote:
> Same question here. Can we use a 1-way binding to |value| instead of having
this
> observer?

Acknowledged.

[stevenjb, 2016-10-17 23:59:06]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-10-18 00:09:39]

On 2016/10/17 at 23:58:01, stevenjb wrote:
> PTAL
> 
> Note: I used async() in all 3 cases so that we don't have to think too hard
about whether or not the dom-repeat contents are dynamic, and whether or not the
static contents are guaranteed to be stamped before networkPropertiesChanged_
gets called (I'm not sure that they are).

SG. Ideally tests should cover all those cases, for example
 - Initialize element to be tested, check that dropdowns are prepopulated with
expected value.
 - Dynamically change <dom-repeat> contents, check that dropdown gets
prepopulated correctly.

Ideally.... But since the objective here is to migrate (not to increase test
coverage), I am not blocking the CL on this.

[dpapad, 2016-10-18 00:09:51]

LGTM.

https://codereview.chromium.org/2422423002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_proxy.js (right):

https://codereview.chromium.org/2422423002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:152:
useSameProxyChanged_: function() {
Nit: This seems to have been moved, no modification. Consider reverting to make
diff smaller (unless you are moving intentionally to a more intuitive method
ordering).

[stevenjb, 2016-10-18 00:31:02]

On 2016/10/18 00:09:39, dpapad wrote:
> On 2016/10/17 at 23:58:01, stevenjb wrote:
> > PTAL
> > 
> > Note: I used async() in all 3 cases so that we don't have to think too hard
> about whether or not the dom-repeat contents are dynamic, and whether or not
the
> static contents are guaranteed to be stamped before networkPropertiesChanged_
> gets called (I'm not sure that they are).
> 
> SG. Ideally tests should cover all those cases, for example
>  - Initialize element to be tested, check that dropdowns are prepopulated with
> expected value.
>  - Dynamically change <dom-repeat> contents, check that dropdown gets
> prepopulated correctly.
> 
> Ideally.... But since the objective here is to migrate (not to increase test
> coverage), I am not blocking the CL on this.

Ideally. Complete test coverage for the internet settings UI will be...
daunting.

[stevenjb, 2016-10-18 00:32:58]

https://codereview.chromium.org/2422423002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_proxy.js (right):

https://codereview.chromium.org/2422423002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:152:
useSameProxyChanged_: function() {
On 2016/10/18 00:09:51, dpapad wrote:
> Nit: This seems to have been moved, no modification. Consider reverting to
make
> diff smaller (unless you are moving intentionally to a more intuitive method
> ordering).

I've been fixing ordering and comments to be consistent with other JS files.
I've been doing this as I go in places that got missed by a (semi) recent
consistency pass.

[stevenjb, 2016-10-18 00:33:02]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-10-18 00:33:33]

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-18 02:00:09]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-10-18 02:02:30]

Description was changed from

==========
Use md-select instead of paper-dropdown-menu-light in internet page

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

to

==========
Use md-select instead of paper-dropdown-menu-light in internet page

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

Committed: https://crrev.com/b0e7fb52816a7b5efc7c1ca92a99cb3e5a6d1e15
Cr-Commit-Position: refs/heads/master@{#425853}
==========

[commit-bot: I haz the power, 2016-10-18 02:02:31]

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