[MD settings] use promises rather than a counter var to fetch cookie data

chrome/browser/resources/settings/site_settings/site_data.js

Index: chrome/browser/resources/settings/site_settings/site_data.js
diff --git a/chrome/browser/resources/settings/site_settings/site_data.js b/chrome/browser/resources/settings/site_settings/site_data.js
index e17c67bf4926b9f00c2b0cb57090bbaf1a5c7bef..180d462b519229bb9e7b32ecf6e77a94835757f7 100644
--- a/chrome/browser/resources/settings/site_settings/site_data.js
+++ b/chrome/browser/resources/settings/site_settings/site_data.js
@@ -28,12 +28,6 @@ Polymer({
     treeNodes_: Object,
 
     /**
-     * Keeps track of how many outstanding requests for more data there are.
-     * @private
-     */
-    requests_: Number,
-
-    /**
      * The current filter applied to the cookie data list.
      * @private
      */
@@ -108,37 +102,46 @@ Polymer({
    * @private
    */
   loadChildren_: function(list) {
-    var parentId = list.id;
+    assert(list.id == null);
+    // New root being added, clear the list and add the nodes.
+    this.sites = [];
     var data = list.children;
-
-    if (parentId == null) {
-      // New root being added, clear the list and add the nodes.
-      this.sites = [];
-      this.requests_ = 0;
-      this.treeNodes_.addChildNodes(this.treeNodes_, data);
-    } else {
-      this.treeNodes_.populateChildNodes(parentId, this.treeNodes_, data);
+    this.treeNodes_.addChildNodes(this.treeNodes_, data);
+    var promiseList = [];

[dpapad, 2016/10/21 22:03:08]

Nit: s/promiseList/promises/. It's shorter, and conveys the same info.

[dschuyler, 2016/10/22 00:50:45]

Done.

+    for (var i = 0; i < data.length; ++i) {

[dpapad, 2016/10/21 22:03:07]

Nit(optional): A more functional way to do this (and more compact) is

var promises = list.children.
    filter(function(c) { return c.hasChildren; }).
    map(function(c) {
      return this.browserProxy.loadCookieChildren(data[i].id).then(
          this.loadChildrenRecurse_.bind(this));
    }.bind(this));

Just mentioning it as a potential style. No need to change it unless you like
the more functional aspect of it.

[dschuyler, 2016/10/22 00:50:45]

javascript map is often suggested as alternative to a
for loop, but each time I've tested it in the past
the for loop is faster at run-time (or the same if the
jit converts the map to a for loop). I find the for 
loop easier to read as well, but that's just preference.
If map were faster I'd switch to using it more often.

[dpapad, 2016/10/22 01:05:57]

forEach() and for() are usually compared in terms of performance, and you are
right, in all benchmarks I've seen there is a difference.

Trivia: map() is a bit different though, because strictly speaking you gain some
performance by pre-allocating the array instead of growing it little by little
with push() (which ends up copying the array and doubling its size as many times
as needed).

Basically for the for vs map comparison to be entirely equivalent one has to do
the following
 
var array = new Array(count); // Pre-allocate space.
for (var i = 0; i < count; i++) {
  array[i] = ...;
}

Perhaps the new for..of is the best of both worlds between for and forEach,
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/....
Easier syntax, no new function calls, no need to bind, so I am guessing same
performance with for().

[dschuyler, 2016/10/22 01:17:11]

The poor performance of map, afaik, comes from the function call overhead.

I switched to for...of too.

+      if (data[i].hasChildren) {
+        promiseList.push(this.browserProxy.loadCookieChildren(
+            data[i].id).then(function(list) {

[dpapad, 2016/10/21 22:03:07]

Nit (optional): I think is more readable if you skip the anonymous function.

 promiseList.push(this.browserProxy.loadCookieChildren(data[i].id).then(
    this.loadChildrenRecurse_.bind(this));

[dschuyler, 2016/10/22 00:50:45]

Thanks

Done.

+          return this.loadChildrenRecurse_(list);
+        }.bind(this)));
+      }
     }
+    Promise.all(promiseList).then(function() {
+      this.sites = this.treeNodes_.getSummaryList();
+    }.bind(this));
+  },
 
+  /**
+   * Called when the cookie list is ready to be shown.
+   * @param {!CookieList} list The cookie list to show.
+   * @private
+   */
+  loadChildrenRecurse_: function(list) {

[dpapad, 2016/10/21 22:03:07]

loadChildrenRecurse_ and loadChildren_ seem almost identical. Is there a way we
can reduce the repeated logic? Something like (pseudocode, I've ommitted calls
to bind() for simplicity).

loadChildren_: function(rootList) {
 ...// initial stuff here.
 function loadChildrenRecurse_(list) {...};

 // Start recursion from rootList.
 return loadChildrenRecurse_(rootList).then(function() {
   this.sites = this.treeNodes_.getSummaryList();
 });
};

Basically currently, it seems as if we are starting the recursion from the
children of the root list, instead of the root list itself, unnecessarily.

[dschuyler, 2016/10/22 00:50:45]

Done.

+    assert(list.id != null);

[dpapad, 2016/10/21 22:03:07]

Do we need to assert, on top of relying on the compiler type checking, which
indicates that list is never null? (same for assertion at 105).

[dschuyler, 2016/10/22 00:50:45]

The assserts are testing whether id is null (rather
than list), I'd put them in
to be sure I wasn't messing up when converting the 
code, now that I think it's correct they can be removed.

+    var parentId = list.id;
+    var data = list.children;
+    this.treeNodes_.populateChildNodes(parentId, this.treeNodes_, data);
+    var promiseList = [];
+    var prefix = parentId + ', ';
     for (var i = 0; i < data.length; ++i) {
-      var prefix = parentId == null ? '' : parentId + ', ';
       if (data[i].hasChildren) {
-        ++this.requests_;
-        this.browserProxy.loadCookieChildren(
+        promiseList.push(this.browserProxy.loadCookieChildren(
             prefix + data[i].id).then(function(list) {
-          --this.requests_;
-          this.loadChildren_(list);
-        }.bind(this));
+          return this.loadChildrenRecurse_(list);
+        }.bind(this)));
       }
     }
-
-    if (this.requests_ == 0)
-      this.sites = this.treeNodes_.getSummaryList();
-
-    // If this reaches below zero then we're forgetting to increase the
-    // outstanding request count and the summary list won't be updated at the
-    // end.
-    assert(this.requests_ >= 0);
+    return Promise.all(promiseList);
   },
 
   /**