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

chrome/browser/resources/settings/site_settings/site_data.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
  * 'site-data' handles showing the local storage summary list for all sites.
  */
 
 Polymer({
   is: 'site-data',
 
   behaviors: [SiteSettingsBehavior, WebUIListenerBehavior],
 
   properties: {
     /**
      * A summary list of all sites and how many entities each contain.
      * @type {Array<CookieDataSummaryItem>}
      */
     sites: Array,
 
     /**
      * The cookie tree with the details needed to display individual sites and
      * their contained data.
      * @type {!settings.CookieTreeNode}
      * @private

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

Nit: @private {!settings.CookieTreeNode}
for consistency within this file.

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

Discussed this offline, leaving this out.

      */
     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
      */
     filterString_: {
       type: String,
       value: '',
     },
 
     /** @private */
     confirmationDeleteMsg_: String,
@@ skipping 43 matching lines @@
   isRemoveButtonVisible_: function(sites, renderedItemCount) {
     return renderedItemCount != 0;
   },
 
   /**
    * Returns the string to use for the Remove label.
    * @return {string} filterString The current filter string.
    * @private
    */
   computeRemoveLabel_: function(filterString) {
     if (filterString.length == 0)

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

Nit(optional): I know you are not touching this part. This can be compressed as
follows

return loadTimeData.getString(filterString.length == 0 ?
    'siteSettingsCookieRemoveAll' :
    'siteSettingsCookieRemoveAllShown';

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

I'd rather put these in the html with $i18n
and not call loadTimeData; let's leave that
for another CL.

       return loadTimeData.getString('siteSettingsCookieRemoveAll');
     return loadTimeData.getString('siteSettingsCookieRemoveAllShown');
   },
 
   /**
    * Called when the cookie list is ready to be shown.
    * @param {!CookieList} list The cookie list to show.
    * @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.

-      var prefix = parentId == null ? '' : parentId + ', ';
       if (data[i].hasChildren) {
-        ++this.requests_;
-        this.browserProxy.loadCookieChildren(
-            prefix + data[i].id).then(function(list) {
-          --this.requests_;
-          this.loadChildren_(list);
-        }.bind(this));
+        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)));
       }
     }
-
-    if (this.requests_ == 0)
+    Promise.all(promiseList).then(function() {
       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);
+    }.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) {
+      if (data[i].hasChildren) {
+        promiseList.push(this.browserProxy.loadCookieChildren(
+            prefix + data[i].id).then(function(list) {
+          return this.loadChildrenRecurse_(list);
+        }.bind(this)));
+      }
+    }
+    return Promise.all(promiseList);
+  },
+
+  /**
    * Called when a single item has been removed (not during delete all).
    * @param {!CookieRemovePacket} args The details about what to remove.
    * @private
    */
   onTreeItemRemoved_: function(args) {
     this.treeNodes_.removeByParentId(args.id, args.start, args.count);
     this.sites = this.treeNodes_.getSummaryList();
   },
 
   /** @private */
@@ skipping 60 matching lines @@
     this.shadowRoot.appendChild(dialog);
 
     var node = this.treeNodes_.fetchNodeById(event.model.item.id, false);
     dialog.open(node);
 
     dialog.addEventListener('close', function(event) {
       dialog.remove();
     });
   },
 });