MD Settings: Eliminate use of ES6 for Chrome OS

MD Settings: Eliminate use of ES6 for Chrome OS

The vulcanization currently fails on ES6 syntax, so remove it for now.

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

Review-Url: https://codereview.chromium.org/2651483002
Cr-Commit-Position: refs/heads/master@{#445746}
Committed: https://chromium.googlesource.com/chromium/src/+/1761cd38fa199b8204a4e17a20599b17bd4b3f0a

[stevenjb, 2017-01-21 19:56:57]

Description was changed from

==========
MD Settings: Eliminate use of ES6 for Chrome OS

BUG=673825
==========

to

==========
MD Settings: Eliminate use of ES6 for Chrome OS

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

[stevenjb, 2017-01-21 20:09:57]

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

[commit-bot: I haz the power, 2017-01-21 20:10:00]

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

[stevenjb, 2017-01-21 20:10:16]

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

[commit-bot: I haz the power, 2017-01-21 21:09:06]

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

[commit-bot: I haz the power, 2017-01-21 21:09:07]

Dry run: This issue passed the CQ dry run.

[PhistucK, 2017-01-21 21:47:36]

Why can you not use ECMAScript 2015 on Chrome OS?

[Dan Beam, 2017-01-22 01:35:48]

On 2017/01/21 21:47:36, PhistucK wrote:
> Why can you not use ECMAScript 2015 on Chrome OS?

the CL description is confusing.  what stevenjb@ means is that we can't use ES6
because our minifier (uglifyjs) currently blows up on some new syntaxes.  this
is really only for the settings page on ChromeOS.

[dpapad, 2017-01-23 19:12:54]

Mostly nits, but also some questions.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:231: for (var
d = 0; d <this.deviceList_.length; ++d) {
Space missing before |this.deviceList_.length|. Also it is more common to use
'i' for a counter variable.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:280: var widthStr =
mode.width.toString();
Nit (optional): You can shorten as follows.

var messageId =
    best ? 'displayResolutionTextBest' :
    (mode.isNative ? 'displayResolutionTextNative' : 'displayResolutionText');

return this.i18n(messageId, mode.width.toString(), mode.height.toString());

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:295: for (var d = 0; d
< this.displays.length; ++d) {
Nit (optional): Same nit as before, 'i' is a more common name for a loop
counter, (then 'j', 'k').

'd' would make more sense if the variable was holding a display variable, but it
is holding just an integer, so IMO 'd' is more confusing than worth it.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:300:
this.selectedDisplay = display;
Do we need to keep iterating if we already found the selectedDisplay? Why not
break here?

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:421: displayIds +=
display.id;
Why not hold all the display IDs in an array first, and then just do
displayIds.join(',') at line 427? This would simplify the logic (no need to
manually keep track of when to add the comma or not).

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:152: for (var o
in orphanIds)
Iterating an array with for..in is discouraged (very fragile), see styleguide
https://google.github.io/styleguide/javascriptguide.xml?showone=for-in_loop#f...
as well as
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:162: for (var
oo in orphanIds)
Same here and elsewhere in this file.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:301: var
childId = key;
Nit: Why not mirror the previous logic more closely? (I think it was more
readable).

if (childId == parentId)
  return;
if (value.parentId == parentId) {
  ...
}

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:379: var
otherId = keys.next().value;
(Here and elsewhere): Iterating over a map using both a counter on |size| and an
iterator seems unnecessary. How about the following?

var keys = this.calculatedBoundsMap_.keys();
var next = keys.next();

while (!next.done) {
  ....
  next = keys.next();
}

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_page.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.js:173: for (var e
= 0; e < extensions.length; ++e)
Same (optional) nit as before here. |e| would make more sense if you were doing
the following

extensions.forEach(function(e) {
  this.addVpnProvider_(vpnProviders, e);
}.bind(this));

such that |e| holds an item from the array instead of an index.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_apnlist.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_apnlist.js:265: for (var
i = 0; i < apnList.length; ++i) {
Nit (optional): You can use Array#find instead.

return apnList.find(function(a) {
  return a.AccessPointName == accessPointName;
});

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_nameservers.js:190: var
nameservers = [];
Nit: You can use Array#map instead.

var nameservers = this.MAX_NAMESERVERS.map(function(i) {
  var nameserver = '';
  ...
  return nameserver;
}.bind(this));

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

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:189: jsonHttp
== JSON.stringify(proxy.Manual.SecureHTTPProxy) &&
FWIW: There are no guarantees that JSON.stringify() serializes properties in a
deterministic order, so comparisons like this are a bit fragile.

From the documenatation, "Properties of non-array objects are not guaranteed to
be stringified in any particular order", see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:348:
newDeviceStates[state.Type] = state;
Nit: No need for temporary |state| variable, since it is only used once.
newDeviceStates[state.Type] = opt_deviceStates[s];

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:378: },
this);
Nit (optional): There is no usage of |this| inside the forEach() so, you can
remove the binding to |this|.

[stevenjb, 2017-01-23 19:23:29]

Description was changed from

==========
MD Settings: Eliminate use of ES6 for Chrome OS

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

to

==========
MD Settings: Eliminate use of ES6 for Chrome OS

The vulcanization currently fails on ES6 syntax, so remove it for now.

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

[Dan Beam, 2017-01-23 20:40:45]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org, phistuck@gmail.com

[stevenjb, 2017-01-24 01:28:50]

Patchset #3 (id:40001) has been deleted

[stevenjb, 2017-01-24 01:57:44]

Patchset #3 (id:60001) has been deleted

[stevenjb, 2017-01-24 02:01:50]

Patchset #3 (id:80001) has been deleted

[stevenjb, 2017-01-24 02:02:04]

PTAL

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:231: for (var
d = 0; d <this.deviceList_.length; ++d) {
On 2017/01/23 19:12:53, dpapad wrote:
> Space missing before |this.deviceList_.length|. Also it is more common to use
> 'i' for a counter variable.

done / ack. I tend to prefer more context relevent counter names, but if that's
confusing I'll change it.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:280: var widthStr =
mode.width.toString();
On 2017/01/23 19:12:53, dpapad wrote:
> Nit (optional): You can shorten as follows.
> 
> var messageId =
>     best ? 'displayResolutionTextBest' :
>     (mode.isNative ? 'displayResolutionTextNative' : 'displayResolutionText');
> 
> return this.i18n(messageId, mode.width.toString(), mode.height.toString());

I wanted to keep this CL as simple as possible in case we revert later. That
didn't make sense evereywhere, and we probably won't want to just revert, but
I'd still prefer to keep changes to a minimum.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:295: for (var d = 0; d
< this.displays.length; ++d) {
On 2017/01/23 19:12:53, dpapad wrote:
> Nit (optional): Same nit as before, 'i' is a more common name for a loop
> counter, (then 'j', 'k').
> 
> 'd' would make more sense if the variable was holding a display variable, but
it
> is holding just an integer, so IMO 'd' is more confusing than worth it.

Done.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:300:
this.selectedDisplay = display;
On 2017/01/23 19:12:53, dpapad wrote:
> Do we need to keep iterating if we already found the selectedDisplay? Why not
> break here?

I don't recall for certain, would also want to fix that separately.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display.js:421: displayIds +=
display.id;
On 2017/01/23 19:12:53, dpapad wrote:
> Why not hold all the display IDs in an array first, and then just do
> displayIds.join(',') at line 427? This would simplify the logic (no need to
> manually keep track of when to add the comma or not).

Ditto.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:152: for (var o
in orphanIds)
On 2017/01/23 19:12:54, dpapad wrote:
> Iterating an array with for..in is discouraged (very fragile), see styleguide
>
https://google.github.io/styleguide/javascriptguide.xml?showone=for-in_loop#f...
> as well as
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....

Good catch. This is in fact incorrect, does nothing, and appears to be
unnecessary. Removed.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:162: for (var
oo in orphanIds)
On 2017/01/23 19:12:54, dpapad wrote:
> Same here and elsewhere in this file.

Ditto.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:301: var
childId = key;
On 2017/01/23 19:12:54, dpapad wrote:
> Nit: Why not mirror the previous logic more closely? (I think it was more
> readable).
> 
> if (childId == parentId)
>   return;
> if (value.parentId == parentId) {
>   ...
> }

I find 'return' inside an inlined forEach super confusing. This seemed simplest.
If I were re-writing this I might move the function to a separate helper, but
frankly this seems fine to me.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:379: var
otherId = keys.next().value;
On 2017/01/23 19:12:54, dpapad wrote:
> (Here and elsewhere): Iterating over a map using both a counter on |size| and
an
> iterator seems unnecessary. How about the following?
> 
> var keys = this.calculatedBoundsMap_.keys();
> var next = keys.next();
> 
> while (!next.done) {
>   ....
>   next = keys.next();
> }

I din't know about next.done. that makes sense. Done.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/internet_page.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/internet_page.js:173: for (var e
= 0; e < extensions.length; ++e)
On 2017/01/23 19:12:54, dpapad wrote:
> Same (optional) nit as before here. |e| would make more sense if you were
doing
> the following
> 
> extensions.forEach(function(e) {
>   this.addVpnProvider_(vpnProviders, e);
> }.bind(this));
> 
> such that |e| holds an item from the array instead of an index.

Done.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_apnlist.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_apnlist.js:265: for (var
i = 0; i < apnList.length; ++i) {
On 2017/01/23 19:12:54, dpapad wrote:
> Nit (optional): You can use Array#find instead.
> 
> return apnList.find(function(a) {
>   return a.AccessPointName == accessPointName;
> });

Done.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_nameservers.js:190: var
nameservers = [];
On 2017/01/23 19:12:54, dpapad wrote:
> Nit: You can use Array#map instead.
> 
> var nameservers = this.MAX_NAMESERVERS.map(function(i) {
>   var nameserver = '';
>   ...
>   return nameserver;
> }.bind(this));

MAX_NAMESERVERS is a number, not an Array. But I can pre-allocate the array and
greatly simplify this (very early JS) so I will do that.

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

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_proxy.js:189: jsonHttp
== JSON.stringify(proxy.Manual.SecureHTTPProxy) &&
On 2017/01/23 19:12:54, dpapad wrote:
> FWIW: There are no guarantees that JSON.stringify() serializes properties in a
> deterministic order, so comparisons like this are a bit fragile.
> 
> From the documenatation, "Properties of non-array objects are not guaranteed
to
> be stringified in any particular order", see
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje....

I'm pretty sure that in this context JSON.stringify will be consistent, but it's
not exactly optimal anyway; done.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:348:
newDeviceStates[state.Type] = state;
On 2017/01/23 19:12:54, dpapad wrote:
> Nit: No need for temporary |state| variable, since it is only used once.
> newDeviceStates[state.Type] = opt_deviceStates[s];

Twice. state.Type and state.

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:378: },
this);
On 2017/01/23 19:12:54, dpapad wrote:
> Nit (optional): There is no usage of |this| inside the forEach() so, you can
> remove the binding to |this|.

Acknowledged.

[stevenjb, 2017-01-24 02:02:11]

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

[commit-bot: I haz the power, 2017-01-24 02:04:00]

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

[dpapad, 2017-01-24 02:38:10]

LGTM

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:152: for (var o
in orphanIds)
On 2017/01/24 at 02:02:04, stevenjb wrote:
> On 2017/01/23 19:12:54, dpapad wrote:
> > Iterating an array with for..in is discouraged (very fragile), see
styleguide
> >
https://google.github.io/styleguide/javascriptguide.xml?showone=for-in_loop#f...
> > as well as
> >
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....
> 
> Good catch. This is in fact incorrect, does nothing, and appears to be
unnecessary. Removed.

Ok. I believe you, although reading the comment makes it sound that now
calculatedBoundsMap_ will contain obsolte entries.

https://codereview.chromium.org/2651483002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2651483002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/network_nameservers.js:191: for
(var i = 0; i < this.MAX_NAMESERVERS; ++i) {
Nit: Probably more robust to use nameservers.length now.

[stevenjb, 2017-01-24 02:59:52]

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2651483002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:152: for (var o
in orphanIds)
On 2017/01/24 02:38:10, dpapad wrote:
> On 2017/01/24 at 02:02:04, stevenjb wrote:
> > On 2017/01/23 19:12:54, dpapad wrote:
> > > Iterating an array with for..in is discouraged (very fragile), see
> styleguide
> > >
>
https://google.github.io/styleguide/javascriptguide.xml?showone=for-in_loop#f...
> > > as well as
> > >
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....
> > 
> > Good catch. This is in fact incorrect, does nothing, and appears to be
> unnecessary. Removed.
> 
> Ok. I believe you, although reading the comment makes it sound that now
> calculatedBoundsMap_ will contain obsolte entries.

Yeah, this is not as robust as it should be, but the code is subtle and tricky.
I hope to have someone do a more robust set of tests. We do have quite a few,
but there are a *lot* of (literal) edge cases.

https://codereview.chromium.org/2651483002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/internet_page/network_nameservers.js
(right):

https://codereview.chromium.org/2651483002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/internet_page/network_nameservers.js:191: for
(var i = 0; i < this.MAX_NAMESERVERS; ++i) {
On 2017/01/24 02:38:10, dpapad wrote:
> Nit: Probably more robust to use nameservers.length now.

Acknowledged.

[commit-bot: I haz the power, 2017-01-24 03:17:53]

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

[commit-bot: I haz the power, 2017-01-24 03:17:54]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-01-24 17:06:00]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2017-01-24 17:06:14]

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-01-24 17:11:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485277560291540,
"parent_rev": "233f53ad273ef485ff5b12510b421a5ab4d429ec", "commit_rev":
"1761cd38fa199b8204a4e17a20599b17bd4b3f0a"}

[commit-bot: I haz the power, 2017-01-24 17:11:59]

Description was changed from

==========
MD Settings: Eliminate use of ES6 for Chrome OS

The vulcanization currently fails on ES6 syntax, so remove it for now.

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

to

==========
MD Settings: Eliminate use of ES6 for Chrome OS

The vulcanization currently fails on ES6 syntax, so remove it for now.

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

Review-Url: https://codereview.chromium.org/2651483002
Cr-Commit-Position: refs/heads/master@{#445746}
Committed:
https://chromium.googlesource.com/chromium/src/+/1761cd38fa199b8204a4e17a2059...
==========

[commit-bot: I haz the power, 2017-01-24 17:12:00]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1761cd38fa199b8204a4e17a2059...