Settings: All Sites now can sort sites by domain (eTLD+1).

Add functionality to sort the All Sites list, with an initial option to sort by
a site group's eTLD+1 name. Note that sites shown individually (ungrouped) in
All Sites will also be sorted by their eTLD+1.

See a screenshot of this feature at
https://bugs.chromium.org/p/chromium/issues/detail?id=835712#c19

Bug: 835712
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ic58e1bf294189b4fefc2cf0f8532725a71f2b36c
Reviewed-on: https://chromium-review.googlesource.com/1128782
Commit-Queue: Patti <patricialor@chromium.org>
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575165}

chrome/app/settings_strings.grdp

@@ -2657,6 +2657,12 @@
   <message name="IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SEARCH" desc="Placeholder text for the search text box that allows the user to filter the All Sites list with their search query.">
     Search
   </message>
+  <message name="IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT" desc="Label on a selection menu to choose the sorting method to display the All Sites list by.">
+    Sort by
+  </message>
+  <message name="IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT_METHOD_NAME" desc="A selection menu option to sort the All Sites list by the name of the site, which is derived from its URL.">
+    Name
+  </message>
   <message name="IDS_SETTINGS_SITE_SETTINGS_SITE_REPRESENTATION_SEPARATOR" desc="The separator used to split up the display of a URL into host and scheme/protocol, when the scheme is not HTTPS. For example, it will be used in a string that looks like 'google.co.uk — http'.">
       —
   </message>

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

 <link rel="import" href="chrome://resources/html/polymer.html">
 
+<link rel="import" href="chrome://resources/html/md_select_css.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/iron-list/iron-list.html">
 <link rel="import" href="../global_scroll_target_behavior.html">
 <link rel="import" href="../route.html">
 <link rel="import" href="../settings_page/settings_subpage_search.html">
 <link rel="import" href="../settings_shared_css.html">
+<link rel="import" href="../settings_vars_css.html">
 <link rel="import" href="site_entry.html">
 
 <dom-module id="all-sites">
   <template>
-    <style include="settings-shared">
+    <style include="settings-shared md-select">
       /* Align the search box search icon with the site-entry icons. */
       #searchAndSort {
-        -webkit-margin-start: var(--cr-icon-button-margin-start);
-        -webkit-padding-start: var(--settings-box-row-padding);
+        display: flex;
+        justify-content: space-between;
+        margin: 0 var(--cr-icon-button-margin-start);
         margin-bottom: 50px;
+        padding: 0 var(--settings-box-row-padding);
+      }
+
+      #sortMethod {
+        -webkit-margin-start: 1em;
       }
 
       /* There is only one top-level heading for All Sites, so remove the
@@ -27,6 +35,18 @@
       <settings-subpage-search label="$i18n{siteSettingsAllSitesSearch}"
           on-search-changed="onSearchChanged_">
       </settings-subpage-search>
+
+      <div>
+        <label id="sortLabel">$i18n{siteSettingsAllSitesSort}</label>
+        <select id="sortMethod" class="md-select" aria-labelledby="sortLabel"
+          on-change="onSortMethodChanged_">
+          <!-- TODO(https://crbug.com/835712): Implement remaining two sort
+              methods. -->
+          <option value="[[sortMethods_.name]]">
+            $i18n{siteSettingsAllSitesSortMethodName}
+          </option>
+        </select>
+      </div>
     </div>
     <div class="list-frame" hidden$="[[siteGroupList.length]]">
       <div class="list-item secondary">$i18n{noSitesAdded}</div>

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

@@ -47,7 +47,24 @@ Polymer({
     searchQuery_: {
       type: String,
       value: '',
-    }
+    },
+
+    /**
+     * All possible sort methods.
+     * @type {Object}
+     * @private
+     */
+    sortMethods_: {
+      type: Object,
+      value: function() {
+        return {
+          name: 'name',
+          mostVisited: 'most-visited',
+          storage: 'data-stored',
+        };
+      },
+      readOnly: true,
+    },
   },
 
   /** @override */
@@ -83,7 +100,7 @@ Polymer({
       contentTypes.push(settings.ContentSettingsTypes.COOKIES);
 
     this.browserProxy_.getAllSites(contentTypes).then((response) => {
-      this.siteGroupList = response;
+      this.siteGroupList = this.sortSiteGroupList_(response);
     });
   },
 
@@ -105,17 +122,43 @@ Polymer({
     });
   },
 
+  /**
+   * Sorts the given SiteGroup list with the currently selected sort method.
+   * @param {!Array<!SiteGroup>} siteGroupList The list of sites to sort.
+   * @return {!Array<!SiteGroup>}
+   * @private
+   */
+  sortSiteGroupList_: function(siteGroupList) {
+    const sortMethod = this.$.sortMethod.value;
+    if (sortMethod == this.sortMethods_.name)
+      siteGroupList.sort(this.nameComparator_);
+    return siteGroupList;
+  },
+
+  nameComparator_: function(siteGroup1, siteGroup2) {
+    return siteGroup1.etldPlus1.localeCompare(siteGroup2.etldPlus1);
+  },
+
   /**
    * Called when the input text in the search textbox is updated.
-   * @param {Event} e
    * @private
    */
-  onSearchChanged_: function(e) {
+  onSearchChanged_: function() {
     const searchElement = /** @type {SettingsSubpageSearchElement} */ (
         this.$$('settings-subpage-search'));
     this.searchQuery_ = searchElement.getSearchInput().value.toLowerCase();
   },
 
+  /**
+   * Called when the user chooses a different sort method to the default.
+   * @private
+   */
+  onSortMethodChanged_: function() {
+    this.siteGroupList = this.sortSiteGroupList_(this.siteGroupList);
+    // Force the iron-list to rerender its items, as the order has changed.
+    this.$.allSitesList.fire('iron-resize');
+  },
+
   /**
    * Called when a list item changes its size, and thus the positions and sizes
    * of the items in the entire list also need updating.

chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc

@@ -2194,6 +2194,9 @@ void AddSiteSettingsStrings(content::WebUIDataSource* html_source,
     {"siteSettingsAllSitesDescription",
      IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_DESCRIPTION},
     {"siteSettingsAllSitesSearch", IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SEARCH},
+    {"siteSettingsAllSitesSort", IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT},
+    {"siteSettingsAllSitesSortMethodName",
+     IDS_SETTINGS_SITE_SETTINGS_ALL_SITES_SORT_METHOD_NAME},
     {"siteSettingsSiteRepresentationSeparator",
      IDS_SETTINGS_SITE_SETTINGS_SITE_REPRESENTATION_SEPARATOR},
     {"siteSettingsAutomaticDownloads",

chrome/test/data/webui/settings/all_sites_tests.js

@@ -135,4 +135,24 @@ suite('AllSites', function() {
           }
         });
   });
+
+  test('can be sorted by name', function() {
+    setUpCategory(prefsVarious);
+    testElement.populateList_();
+    return browserProxy.whenCalled('getAllSites').then(() => {
+      Polymer.dom.flush();
+      const siteEntries =
+          testElement.$.listContainer.querySelectorAll('site-entry');
+
+      // TODO(https://crbug.com/835712): When there is more than one sort
+      // method, check that the all sites list can be sorted by name from an
+      // existing all sites list that is out of order. Currently this is not
+      // possible as the all sites list will always initially be sorted by the
+      // default sort method, and the default sort method is by name.
+      assertEquals(3, siteEntries.length);
+      assertEquals('bar.com', siteEntries[0].$.displayName.innerText.trim());
+      assertEquals('foo.com', siteEntries[1].$.displayName.innerText.trim());
+      assertEquals('google.com', siteEntries[2].$.displayName.innerText.trim());
+    });
+  });
 });

chrome/test/data/webui/settings/test_site_settings_prefs_browser_proxy.js

@@ -203,10 +203,13 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
     const origins_array = [...origins_set];
     let result = [];
     origins_array.forEach((origin, index) => {
-      // Functionality to extract the eTLD+1 from an origin exists only on the
-      // C++ side, so just use a placeholder string in testing client-side code.
       let entry = {};
-      entry['etldPlus1'] = 'eTLD Name ' + index;
+      // Functionality to get the eTLD+1 from an origin exists only on the
+      // C++ side, so just do an (incorrect) approximate extraction here.
+      const host = new URL(origin).host;
+      let urlParts = host.split('.');
+      urlParts = urlParts.slice(Math.max(urlParts.length - 2, 0));
+      entry['etldPlus1'] = urlParts.join('.');
       entry['origins'] = [origin];
       result.push(entry);
     });