MD Settings: Eliminate use of ES6 for Chrome OS

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview Behavior for handling display layout, specifically
  *     edge snapping and collisions.
  */
 
 /** @polymerBehavior */
@@ skipping 40 matching lines @@
    * @param {!Array<!chrome.system.display.DisplayUnitInfo>} displays
    * @param {!Array<!chrome.system.display.DisplayLayout>} layouts
    */
   initializeDisplayLayout: function(displays, layouts) {
     this.dragLayoutId = '';
     this.dragParentId_ = '';
 
     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);
       }
     }
   },
 
   /**
    * Called when a drag event occurs. Checks collisions and updates the layout.
    * @param {string} id
    * @param {!chrome.system.display.Bounds} newBounds The new calculated
    *     bounds for the display.
@@ skipping 57 matching lines @@
     var layout = this.displayLayoutMap_.get(id);
 
     var orphanIds;
     if (!layout || layout.parentId == '') {
       // Primary display. Set the calculated position to |dragBounds_|.
       this.setCalculatedDisplayBounds_(id, this.dragBounds_);
 
       // 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
       // display since it is the only non-orphan.
       this.reparentOrphan_(this.dragParentId_, orphanIds);
       orphanIds.splice(orphanIds.indexOf(this.dragParentId_), 1);
     } 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.
       var topLayout = this.displayLayoutMap_.get(this.dragParentId_);
       while (topLayout && topLayout.parentId != '') {
         if (topLayout.parentId == id) {
           topLayout.parentId = layout.parentId;
           break;
         }
         topLayout = this.displayLayoutMap_.get(topLayout.parentId);
@@ skipping 42 matching lines @@
             Object.assign({}, bounds)));
   },
 
   /**
    * Re-parents all entries in |orphanIds| and any children.
    * @param {!Array<string>} orphanIds The list of ids affected by the move.
    * @private
    */
   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);
       }
     }
 
     // Remove each orphan from the list as it is re-parented so that
     // subsequent orphans can be parented to it.
     while (orphans.length) {
       var orphanId = orphans.shift();
       this.reparentOrphan_(orphanId, orphans);
@@ skipping 47 matching lines @@
   },
 
   /**
    * @param {string} parentId
    * @param {boolean} recurse Include descendants of children.
    * @return {!Array<string>}
    * @private
    */
   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) {
           // Descendants get added to the end of the list.
           children = children.concat(this.findChildren_(childId, true));
         }
       }
-    }
+    }.bind(this));
     return children;
   },
 
   /**
    * Recursively calculates the absolute bounds of a display.
    * Caches the display bounds so that parent bounds are only calculated once.
    * @param {string} id
    * @param {number} width
    * @param {number} height
    * @private
@@ skipping 46 matching lines @@
    * @param {!chrome.system.display.Bounds} bounds
    * @param {Array<string>=} opt_ignoreIds Ids to ignore.
    * @return {string}
    * @private
    */
   findClosest_: function(displayId, bounds, opt_ignoreIds) {
     var x = bounds.left + bounds.width / 2;
     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))
         continue;
       var otherBounds = this.getCalculatedDisplayBounds(otherId);
       var left = otherBounds.left;
       var top = otherBounds.top;
       var width = otherBounds.width;
       var height = otherBounds.height;
       if (x >= left && x < left + width && y >= top && y < top + height)
@@ skipping 160 matching lines @@
    * @param {!chrome.system.display.Bounds} bounds
    * @param {!{x: number, y: number}} deltaPos
    */
   collideAndModifyDelta_: function(id, bounds, deltaPos) {
     var keys = this.calculatedBoundsMap_.keys();
     var others = new Set(keys);
     others.delete(id);
     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)) {
           if (deltaPos.x == 0 && deltaPos.y == 0)
             return;
           others.delete(otherId);
           checkCollisions = true;
           break;
         }
       }
@@ skipping 19 matching lines @@
         (newY >= otherBounds.top + otherBounds.height)) {
       return false;
     }
 
     // |deltaPos| should already be restricted to X or Y. This shortens the
     // delta to stay outside the bounds, however it does not change the sign of
     // the delta, i.e. it does not "push" the point outside the bounds if
     // 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;
       }
       deltaPos.y = snapDeltaY;
     }
 
     return true;
   },
 
   /**
@@ skipping 59 matching lines @@
 
   /**
    * Highlights the edge of the div associated with |id| based on
    * |layoutPosition| and removes any other highlights. If |layoutPosition| is
    * undefined, removes all highlights.
    * @param {string} id
    * @param {chrome.system.display.LayoutPosition|undefined} layoutPosition
    * @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(
           'highlight-right',
           highlight == chrome.system.display.LayoutPosition.RIGHT);
       div.classList.toggle(
           'highlight-left',
           highlight == chrome.system.display.LayoutPosition.LEFT);
       div.classList.toggle(
           'highlight-top',
           highlight == chrome.system.display.LayoutPosition.TOP);
       div.classList.toggle(
           'highlight-bottom',
           highlight == chrome.system.display.LayoutPosition.BOTTOM);
     }
   },
 };