[MD settings] separate all-sites from site-list

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

Index: chrome/browser/resources/settings/site_settings/all_sites.js
diff --git a/chrome/browser/resources/settings/site_settings/all_sites.js b/chrome/browser/resources/settings/site_settings/all_sites.js
index 98e88c72bc95d7a1555692f5871ea17ca3b8ba85..c1dc9261b98632b952e236d2d294ea181e46c253 100644
--- a/chrome/browser/resources/settings/site_settings/all_sites.js
+++ b/chrome/browser/resources/settings/site_settings/all_sites.js
@@ -10,5 +10,138 @@
 Polymer({
   is: 'all-sites',
 
-  behaviors: [SiteSettingsBehavior],
+  behaviors: [SiteSettingsBehavior, WebUIListenerBehavior],
+
+  properties: {
+    /**
+     * Array of sites to display in the widget.
+     * @type {!Array<SiteException>}
+     */
+    sites: {
+      type: Array,
+      value: function() {
+        return [];
+      },
+    },
+  },
+
+  ready: function() {

[dpapad, 2017/04/28 23:54:05]

@override missing.

[dschuyler, 2017/05/18 23:16:11]

Done.

+    this.browserProxy_ =
+        settings.SiteSettingsPrefsBrowserProxyImpl.getInstance();
+    this.addWebUIListener(
+        'contentSettingSitePermissionChanged', this.populateList_.bind(this));
+    this.populateList_();
+  },
+
+  /**
+   * Retrieves a list of all known sites with site details.
+   * @return {!Promise}

[dpapad, 2017/04/28 23:54:05]

What is the type of |promiseList|? Can we make the annotation here more
descriptive (like Promise<SomeTypeGoesHere>).

[dschuyler, 2017/05/18 23:16:11]

Done.

+   * @private
+   */
+  getAllSitesList_: function() {
+    var promiseList = [];
+    for (var type in settings.ContentSettingsTypes) {

[dpapad, 2017/04/28 23:54:05]

Do we need to iterate on the keys of ContentSettingsTypes when we only care
about the values? How about the following shorter version:


var types = Object.getValues(settings.ContentSettingsTypes);
for (var i = 0; i < values.length; i++) {
  var type = types[i];
  if (type == settings.ContentSettingsTypes.PROTOCOL_HANDLERS ||
      type == settings.ContentSettingsTypes.USB_DEVICES ||
      type == settings.ContentSettingsTypes.ZOOM_LEVELS) {
    // Some categories store their data in a custom way.
    continue;
  }

  promiseList.push(this.browserProxy_.getExceptionList(type));
}

[dschuyler, 2017/05/18 23:16:11]

done.

+      if (settings.ContentSettingsTypes[type] ==
+              settings.ContentSettingsTypes.PROTOCOL_HANDLERS ||
+          settings.ContentSettingsTypes[type] ==
+              settings.ContentSettingsTypes.USB_DEVICES ||
+          settings.ContentSettingsTypes[type] ==
+              settings.ContentSettingsTypes.ZOOM_LEVELS) {
+        // Some categories store their data in a custom way.
+        continue;
+      }
+
+      promiseList.push(this.browserProxy_.getExceptionList(
+          settings.ContentSettingsTypes[type]));
+    }
+
+    return Promise.all(promiseList);
+  },
+
+  /**
+   * A handler for selecting a site (by clicking on the origin).
+   * @param {!{model: !{item: !SiteException}}} event
+   * @private
+   */
+  onOriginTap_: function(event) {
+    settings.navigateTo(
+        settings.Route.SITE_SETTINGS_SITE_DETAILS,
+        new URLSearchParams('site=' + event.model.item.origin));
+  },
+
+  /** @private */
+  populateList_: function() {
+    this.getAllSitesList_().then(function(lists) {

[dpapad, 2017/04/28 23:54:05]

this.getAllSitesList_().then(this.processExceptions_.bind(this));

[dschuyler, 2017/05/18 23:16:11]

Done.

+      this.processExceptions_(lists);
+    }.bind(this));
+  },
+
+  /**
+   * Process the exception list returned from the native layer.
+   * @param {!Array<!Array<RawSiteException>>} data List of sites (exceptions)
+   *     to process.
+   * @private
+   */
+  processExceptions_: function(data) {
+    var sites = /** @type {!Array<RawSiteException>} */ ([]);
+    for (var i = 0; i < data.length; ++i) {
+      var exceptionList = data[i];
+      for (var k = 0; k < exceptionList.length; ++k) {
+        sites.push(exceptionList[k]);
+      }
+    }
+    this.sites = this.toSiteArray_(sites);
+  },
+
+  /**
+   * TODO(dschuyler): Move this processing to C++ handler.
+   * Converts a list of exceptions received from the C++ handler to
+   * full SiteException objects. The list is sorted by site name, then protocol
+   * and port and de-duped (by origin).
+   * @param {!Array<RawSiteException>} sites A list of sites to convert.

[dpapad, 2017/04/28 23:54:05]

Should this be !Array<!RawSiteException>? Similar question elsewhere in this
file.

[dschuyler, 2017/05/18 23:16:11]

Done.

+   * @return {!Array<SiteException>} A list of full SiteExceptions. Sorted and
+   *    deduped.
+   * @private
+   */
+  toSiteArray_: function(sites) {
+    var self = this;
+    sites.sort(function(a, b) {
+      var url1 = self.toUrl(a.origin);
+      var url2 = self.toUrl(b.origin);
+      var comparison = url1.host.localeCompare(url2.host);
+      if (comparison == 0) {
+        comparison = url1.protocol.localeCompare(url2.protocol);
+        if (comparison == 0) {
+          comparison = url1.port.localeCompare(url2.port);
+          if (comparison == 0) {

[dpapad, 2017/04/28 23:54:05]

Do those three if (comparison == 0) checks need to be nested? How about the
following?

var comparison = url1.host.localeCompare(url2.host) ||
    url1.protocol.localeCompare(url2.protocol) ||
    url1.port.localeCompare(url2.port);

if (comparison == 0) {
  ...
  return host1.localeCompare(host2);
}

return comparison;

[dschuyler, 2017/05/18 23:16:11]

I agree that would work. Is it more readable, or faster, or just different?
Using the logical-or short-circuit is fewer lines, which is often good; will it
be clear to other's what is going on; i.e. is the trick obvious to others (I'm
not sure, I'm really asking)?

[dpapad, 2017/05/18 23:37:08]

Treat this suggestion as optional. FWIW, there is already a TODO at line 97 to
move this logic elsewhere.
I suggested this because IMO eliminating unnecessary nesting is more readable
and more understandable. Can't speak for everyone else (readability is
subjective). Feel free to add a 2nd reviewer to get a bigger sample of opinions.
It is a pretty standard technique in JS, for example it is used to give default
values to optional params.

var boolean = opt_boolean || false;
var number = opt_number || 0;
var string = opt_string || '';

[dschuyler, 2017/05/18 23:44:22]

I'll think on it more and/or ask for another opinion and follow-up in a separate
CL if desired. I'm not sure that I have a strong feelings either way, but I do
favor leaving it alone if there is equal benefit with either choice.

+            // Compare hosts for the embedding origins.
+            var host1 = self.toUrl(a.embeddingOrigin);
+            var host2 = self.toUrl(b.embeddingOrigin);
+            host1 = (host1 == null) ? '' : host1.host;
+            host2 = (host2 == null) ? '' : host2.host;
+            return host1.localeCompare(host2);
+          }
+        }
+      }
+      return comparison;
+    });
+    var results = /** @type {!Array<SiteException>} */ ([]);
+    var lastOrigin = '';
+    var lastEmbeddingOrigin = '';
+    for (var i = 0; i < sites.length; ++i) {
+      /** @type {!SiteException} */
+      var siteException = this.expandSiteException(sites[i]);
+
+      // Remove duplicates.
+      if (siteException.origin == lastOrigin &&

[dpapad, 2017/04/28 23:54:05]

|origin| and |embeddingOrigin| already exist on |sites[i]|. Can we delay calling
|expandSiteException| until after line we throw out the duplicates? This would
save unnecessary work.

[dschuyler, 2017/05/18 23:16:11]

Sure, but I don't want to change this code too much, the intent was to just move
it.

[dpapad, 2017/05/18 23:37:08]

Can you add a TODO so that we don't completely forget about this?

[dschuyler, 2017/05/18 23:44:22]

Sorry, my response above wasn't clear - I see that now - I did make the change.
I should have included "done."

[dpapad, 2017/05/19 19:25:31]

Ah ok. Yes it was not clear that it was addressed. Still LGTM.

+          siteException.embeddingOrigin == lastEmbeddingOrigin) {
+        continue;
+      }
+
+      results.push(siteException);
+      lastOrigin = siteException.origin;
+      lastEmbeddingOrigin = siteException.embeddingOrigin;
+    }
+    return results;
+  },
 });