MD Settings: Eliminate use of ES6 for Chrome OS

chrome/browser/resources/settings/device_page/layout_behavior.js

Index: chrome/browser/resources/settings/device_page/layout_behavior.js
diff --git a/chrome/browser/resources/settings/device_page/layout_behavior.js b/chrome/browser/resources/settings/device_page/layout_behavior.js
index 82690a726b71269779ea0b7ac106fb89b7ef1937..2ca45d47c3bf3359beaef56aa727f6689dd9f06d 100644
--- a/chrome/browser/resources/settings/device_page/layout_behavior.js
+++ b/chrome/browser/resources/settings/device_page/layout_behavior.js
@@ -58,17 +58,20 @@ var LayoutBehavior = {
     this.mirroring = displays.length > 0 && !!displays[0].mirroringSourceId;
 
     this.displayBoundsMap_.clear();
-    for (let display of displays)
+    for (var d = 0; d < displays.length; ++d) {
+      var display = displays[d];
       this.displayBoundsMap_.set(display.id, display.bounds);
-
+    }
     this.displayLayoutMap_.clear();
-    for (let layout of layouts)
+    for (var l = 0; l < layouts.length; ++l) {
+      var layout = layouts[l];
       this.displayLayoutMap_.set(layout.id, layout);
-
+    }
     this.calculatedBoundsMap_.clear();
-    for (let display of displays) {
+    for (d = 0; d < displays.length; ++d) {
+      display = displays[d];
       if (!this.calculatedBoundsMap_.has(display.id)) {
-        let bounds = display.bounds;
+        var bounds = display.bounds;
         this.calculateBounds_(display.id, bounds.width, bounds.height);
       }
     }
@@ -146,7 +149,7 @@ var LayoutBehavior = {
       // We cannot re-parent the primary display, so instead make all other
       // displays orphans and clear their calculated bounds.
       orphanIds = this.findChildren_(id, true /* recurse */);
-      for (let o in orphanIds)
+      for (var o in orphanIds)

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

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/....

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

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

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

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

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

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.

         this.calculatedBoundsMap_.delete(o);
 
       // Re-parent |dragParentId_|. It will be forced to parent to the dragged
@@ -156,8 +159,8 @@ var LayoutBehavior = {
     } else {
       // All immediate children of |layout| will need to be re-parented.
       orphanIds = this.findChildren_(id, false /* do not recurse */);
-      for (let o in orphanIds)
-        this.calculatedBoundsMap_.delete(o);
+      for (var oo in orphanIds)

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

Same here and elsewhere in this file.

[stevenjb, 2017/01/24 02:02:03]

Ditto.

+        this.calculatedBoundsMap_.delete(oo);
 
       // When re-parenting to a descendant, also parent any immediate child to
       // drag display's current parent.
@@ -220,11 +223,13 @@ var LayoutBehavior = {
    */
   updateOrphans_: function(orphanIds) {
     var orphans = orphanIds.slice();
-    for (let orphan of orphanIds) {
+    for (var i = 0; i < orphanIds.length; ++i) {
+      var orphan = orphanIds[i];
       var newOrphans = this.findChildren_(orphan, true /* recurse */);
       // If the dragged display was re-parented to one of its children,
       // there may be duplicates so merge the lists.
-      for (let o of newOrphans) {
+      for (var ii = 0; ii < newOrphans.length; ++ii) {
+        var o = newOrphans[ii];
         if (!orphans.includes(o))
           orphans.push(o);
       }
@@ -292,10 +297,9 @@ var LayoutBehavior = {
    */
   findChildren_: function(parentId, recurse) {
     var children = [];
-    for (let childId of this.displayLayoutMap_.keys()) {
-      if (childId == parentId)
-        continue;
-      if (this.displayLayoutMap_.get(childId).parentId == parentId) {
+    this.displayLayoutMap_.forEach(function(value, key) {
+      var childId = key;

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

Nit: Why not mirror the previous logic more closely? (I think it was more
readable).

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

[stevenjb, 2017/01/24 02:02:03]

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.

+      if (childId != parentId && value.parentId == parentId) {
         // Insert immediate children at the front of the array.
         children.unshift(childId);
         if (recurse) {
@@ -303,7 +307,7 @@ var LayoutBehavior = {
           children = children.concat(this.findChildren_(childId, true));
         }
       }
-    }
+    }.bind(this));
     return children;
   },
 
@@ -370,7 +374,9 @@ var LayoutBehavior = {
     var y = bounds.top + bounds.height / 2;
     var closestId = '';
     var closestDelta2 = 0;
-    for (let otherId of this.calculatedBoundsMap_.keys()) {
+    var keys = this.calculatedBoundsMap_.keys();
+    for (var o = 0; o < this.calculatedBoundsMap_.size; ++o) {
+      var otherId = keys.next().value;

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

(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();
}

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

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

       if (otherId == displayId)
         continue;
       if (opt_ignoreIds && opt_ignoreIds.includes(otherId))
@@ -551,7 +557,9 @@ var LayoutBehavior = {
     var checkCollisions = true;
     while (checkCollisions) {
       checkCollisions = false;
-      for (let otherId of others) {
+      var othersValues = others.values();
+      for (var o = 0; o < others.size; ++o) {
+        var otherId = othersValues.next().value;
         var otherBounds = this.getCalculatedDisplayBounds(otherId);
         if (this.collideWithBoundsAndModifyDelta_(
                 bounds, otherBounds, deltaPos)) {
@@ -591,24 +599,24 @@ var LayoutBehavior = {
     // the point is already inside.
     if (Math.abs(deltaPos.x) > Math.abs(deltaPos.y)) {
       deltaPos.y = 0;
-      let snapDeltaX;
+      var snapDeltaX;
       if (deltaPos.x > 0) {
-        let x = otherBounds.left - bounds.width;
-        snapDeltaX = Math.max(0, x - bounds.left);
+        snapDeltaX =
+            Math.max(0, (otherBounds.left - bounds.width) - bounds.left);
       } else {
-        let x = otherBounds.left + otherBounds.width;
-        snapDeltaX = Math.min(x - bounds.left, 0);
+        snapDeltaX =
+            Math.min(0, (otherBounds.left + otherBounds.width) - bounds.left);
       }
       deltaPos.x = snapDeltaX;
     } else {
       deltaPos.x = 0;
-      let snapDeltaY;
+      var snapDeltaY;
       if (deltaPos.y > 0) {
-        let y = otherBounds.top - bounds.height;
-        snapDeltaY = Math.min(0, y - bounds.top);
+        snapDeltaY =
+            Math.min(0, (otherBounds.top - bounds.height) - bounds.top);
       } else if (deltaPos.y < 0) {
-        let y = otherBounds.top + otherBounds.height;
-        snapDeltaY = Math.max(y - bounds.top, 0);
+        snapDeltaY =
+            Math.max(0, (otherBounds.top + otherBounds.height) - bounds.top);
       } else {
         snapDeltaY = 0;
       }
@@ -688,7 +696,8 @@ var LayoutBehavior = {
    * @private
    */
   highlightEdge_: function(id, layoutPosition) {
-    for (let layout of this.layouts) {
+    for (var l = 0; l < this.layouts.length; ++l) {
+      var layout = this.layouts[l];
       var highlight = (layout.id == id) ? layoutPosition : undefined;
       var div = this.$$('#_' + layout.id);
       div.classList.toggle(