Settings: All Sites now shows (and sorts by) local storage per origins.

A primary use case for the All sites page should be to view the sites
that are using the most disk space on the user’s device. Currently
grouped sites show the number of cookies. This patch adds support for
showing the amount of local storage used for each origin.

Sorting by data usage in all sites will now sort by local storage, using
the number of cookies as a tie breaker, i.e. sites with any amount of
local storage will be ordered before sites with no local storage and
cookies.

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

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

@@ -97,8 +97,6 @@ Polymer({
         'onLocalStorageListFetched', this.onLocalStorageListFetched.bind(this));
     this.addWebUIListener(
         'contentSettingSitePermissionChanged', this.populateList_.bind(this));
-    this.addEventListener(
-        'site-entry-resized', this.resizeListIfScrollTargetActive_.bind(this));
     this.addEventListener(
         'site-entry-selected',
         (/** @type {!{detail: !{item: !SiteGroup, index: number}}} */ e) => {
@@ -257,14 +255,26 @@ Polymer({
   /**
    * Comparator used to sort SiteGroups by the amount of storage they use. Note
    * this sorts in descending order.
-   * TODO(https://crbug.com/835712): Account for local and website storage in
-   * sorting by storage used. Currently this only takes cookies into account.
+   * TODO(https://crbug.com/835712): Account for website storage in sorting by
+   * storage used.
    * @param {!SiteGroup} siteGroup1
    * @param {!SiteGroup} siteGroup2
    * @private
    */
   storageComparator_: function(siteGroup1, siteGroup2) {
-    return siteGroup2.numCookies - siteGroup1.numCookies;
+    const getOverallUsage = siteGroup => {
+      let usage = 0;
+      siteGroup.origins.forEach(originInfo => {
+        usage += originInfo.usage;
+      });
+      return usage;
+    };
+
+    const siteGroup1Size = getOverallUsage(siteGroup1);
+    const siteGroup2Size = getOverallUsage(siteGroup2);
+    // Use the number of cookies as a tie breaker.
+    return siteGroup2Size - siteGroup1Size ||
+        siteGroup2.numCookies - siteGroup1.numCookies;
   },
 
   /**
@@ -298,16 +308,6 @@ Polymer({
     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.
-   * @private
-   */
-  resizeListIfScrollTargetActive_: function() {
-    if (settings.getCurrentRoute() == this.subpageRoute)
-      this.$.allSitesList.fire('iron-resize');
-  },
-
   /**
    * Forces the all sites list to update its list of items, taking into account
    * the search query and the sort method, then re-renders it.

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

@@ -26,9 +26,23 @@
         display: flex;
       }
 
-      #cookies {
+      .second-line {
         margin-top: 0.1em;
       }
+
+      /* Data units such as "0 KB" should always read left-to-right. */
+      .data-unit {
+        direction: ltr;
+        unicode-bidi: isolate;
+      }
+
+      #collapseChild .data-unit {
+        padding-inline-start: 1ch;
+      }
+
+      .list-frame {
+        padding-inline-end: 0;
+      }
     </style>
     <div id="collapseParent">
       <div class$="settings-box list-item [[getClassForIndex_(listIndex)]]">
@@ -47,9 +61,11 @@
                 [[scheme_(siteGroup, -1)]]
               </span>
             </div>
-            <div id="cookies" class="secondary"
-                hidden$="[[!siteGroup.numCookies]]">
-              [[cookieString_]]
+            <div class="second-line secondary">
+              <span class="data-unit">[[overallUsageString_]]</span>
+              <span id="cookies" hidden$="[[!siteGroup.numCookies]]">
+                &middot; [[cookieString_]]
+              </span>
             </div>
           </div>
           <paper-icon-button-light id="expandIcon" class="icon-expand-more"
@@ -82,7 +98,7 @@
                   style$="[[computeSiteIcon(item.origin)]]">
               </div>
               <div class="site-representation middle text-elide">
-                <span class="url-directionality">
+                <span id="originSiteRepresentation" class="url-directionality">
                   [[siteRepresentation_(siteGroup, index)]]
                 </span>
                 <span class="secondary"
@@ -93,7 +109,13 @@
                     hidden$="[[!scheme_(siteGroup, index)]]">
                   [[scheme_(siteGroup, index)]]
                 </span>
+                <span class="secondary data-unit" hidden$="[[!item.usage]]">
+                  [[originUsagesItem_(originUsages_.*, index)]]
+                </span>
               </div>
+              <paper-icon-button-light class="subpage-arrow">
+                <button aria-labelledby$="originSiteRepresentation"></button>
+              </paper-icon-button-light>
             </div>
           </template>
         </div>

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

@@ -32,10 +32,7 @@ Polymer({
      * The string to display when there is a non-zero number of cookies.
      * @private
      */
-    cookieString_: {
-      type: String,
-      value: '',
-    },
+    cookieString_: String,
 
     /**
      * The position of this site-entry in its parent list.
@@ -44,6 +41,25 @@ Polymer({
       type: Number,
       value: -1,
     },
+
+    /**
+     * The string to display showing the overall usage of this site-entry.
+     * @private
+     */
+    overallUsageString_: String,
+
+    /**
+     * An array containing the strings to display showing the individual disk
+     * usage for each origin in |siteGroup|.
+     * @type {!Array<string>}
+     * @private
+     */
+    originUsages_: {
+      type: Array,
+      value: function() {
+        return [];
+      },
+    },
   },
 
   listeners: {
@@ -109,12 +125,14 @@ Polymer({
       if (this.$.collapseChild.opened)
         this.toggleCollapsible_();
       // Ungrouped site-entries should not show cookies.
-      if (this.cookieString_) {
+      if (this.cookieString_)
         this.cookieString_ = '';
-        this.fire('site-entry-resized');
-      }
     }
-    if (!siteGroup || !this.grouped_(siteGroup))
+    if (!siteGroup)
+      return;
+    this.calculateUsageInfo_(siteGroup);
+
+    if (!this.grouped_(siteGroup))
       return;
 
     const siteList = [this.displayName_];
@@ -133,11 +151,6 @@ Polymer({
               this.localDataBrowserProxy_.getNumCookiesString(numCookies);
         })
         .then(string => {
-          // If there was no cookie string previously and now there is, or vice
-          // versa, the height of this site-entry will have changed.
-          if ((this.cookieString_ == '') != (string == ''))
-            this.fire('site-entry-resized');
-
           this.cookieString_ = string;
         });
   },
@@ -178,6 +191,47 @@ Polymer({
     return this.computeSiteIcon(siteGroup.origins[0].origin);
   },
 
+  /**
+   * Calculates the amount of disk storage used by the given group of origins
+   * and eTLD+1. Also updates the corresponding display strings.
+   * TODO(https://crbug.com/835712): Add website storage as well.
+   * @param {SiteGroup} siteGroup The eTLD+1 group of origins.
+   * @private
+   */
+  calculateUsageInfo_: function(siteGroup) {
+    const getFormattedBytesForSize = (numBytes) => {
+      if (numBytes == 0)
+        return Promise.resolve('0 B');
+      return this.browserProxy.getFormattedBytes(numBytes);
+    };
+
+    let overallUsage = 0;
+    this.originUsages_ = new Array(siteGroup.origins.length);
+    siteGroup.origins.forEach((originInfo, i) => {
+      overallUsage += originInfo.usage;
+      if (this.grouped_(siteGroup)) {
+        getFormattedBytesForSize(originInfo.usage).then((string) => {
+          this.set(`originUsages_.${i}`, string);
+        });
+      }
+    });
+
+    getFormattedBytesForSize(overallUsage).then(string => {
+      this.overallUsageString_ = string;
+    });
+  },
+
+  /**
+   * Array binding for the |originUsages_| array for use in the HTML.
+   * @param {!{base: !Array<string>}} change The change record for the array.
+   * @param {number} index The index of the array item.
+   * @return {string}
+   * @private
+   */
+  originUsagesItem_: function(change, index) {
+    return change.base[index];
+  },
+
   /**
    * Navigates to the corresponding Site Details page for the given origin.
    * @param {string} origin The origin to navigate to the Site Details page for

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

@@ -21,7 +21,8 @@ const ContentSettingProvider = {
 /**
  * Stores origin information.
  * @typedef {{origin: string,
- *            engagement: number}}
+ *            engagement: number,
+ *            usage: number}}
  */
 let OriginInfo;
 
@@ -132,6 +133,14 @@ cr.define('settings', function() {
      */
     getAllSites(contentTypes) {}
 
+    /**
+     * Converts a given number of bytes into a human-readable format, with data
+     * units.
+     * @param {number} numBytes The number of bytes to convert.
+     * @return {!Promise<string>}
+     */
+    getFormattedBytes(numBytes) {}
+
     /**
      * Gets the exceptions (site list) for a particular category.
      * @param {string} contentType The name of the category to query.
@@ -329,6 +338,11 @@ cr.define('settings', function() {
       return cr.sendWithPromise('getAllSites', contentTypes);
     }
 
+    /** @override */
+    getFormattedBytes(numBytes) {
+      return cr.sendWithPromise('getFormattedBytes', numBytes);
+    }
+
     /** @override */
     getExceptionList(contentType) {
       return cr.sendWithPromise('getExceptionList', contentType);

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

@@ -172,6 +172,7 @@ void ConvertSiteGroupMapToListValue(
       origin_object.SetKey(
           "engagement",
           base::Value(engagement_service->GetScore(GURL(origin))));
+      origin_object.SetKey("usage", base::Value(0));
       origin_list.GetList().emplace_back(std::move(origin_object));
     }
     site_group.SetKey(kNumCookies, base::Value(0));
@@ -218,6 +219,10 @@ void SiteSettingsHandler::RegisterMessages() {
       "getAllSites",
       base::BindRepeating(&SiteSettingsHandler::HandleGetAllSites,
                           base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "getFormattedBytes",
+      base::BindRepeating(&SiteSettingsHandler::HandleGetFormattedBytes,
+                          base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "getExceptionList",
       base::BindRepeating(&SiteSettingsHandler::HandleGetExceptionList,
@@ -632,16 +637,45 @@ void SiteSettingsHandler::HandleGetAllSites(const base::ListValue* args) {
 void SiteSettingsHandler::OnLocalStorageFetched(
     const std::list<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
         local_storage_info) {
+  std::map<std::string, int> origin_size_map;
   std::map<std::string, std::set<std::string>> all_sites_map;
   for (const BrowsingDataLocalStorageHelper::LocalStorageInfo& info :
        local_storage_info) {
+    origin_size_map.emplace(info.origin_url.spec(), info.size);
     CreateOrAppendSiteGroupEntry(&all_sites_map, info.origin_url);
   }
   base::Value result(base::Value::Type::LIST);
   ConvertSiteGroupMapToListValue(all_sites_map, &result, profile_);
+
+  // Merge the origin usage number into |result|.
+  for (size_t i = 0; i < result.GetList().size(); ++i) {
+    base::Value* site_group = &result.GetList()[i];
+    base::Value* origin_list = site_group->FindKey(kOriginList);
+
+    for (size_t i = 0; i < origin_list->GetList().size(); ++i) {
+      base::Value* origin_info = &origin_list->GetList()[i];
+      const std::string& origin = origin_info->FindKey("origin")->GetString();
+      const auto& size_info = origin_size_map.find(origin);
+      if (size_info != origin_size_map.end())
+        origin_info->SetKey("usage", base::Value(size_info->second));
+    }
+  }
   FireWebUIListener("onLocalStorageListFetched", result);
 }
 
+void SiteSettingsHandler::HandleGetFormattedBytes(const base::ListValue* args) {
+  AllowJavascript();
+
+  CHECK_EQ(2U, args->GetSize());
+  const base::Value* callback_id;
+  CHECK(args->Get(0, &callback_id));
+  int num_bytes;
+  CHECK(args->GetInteger(1, &num_bytes));
+
+  const base::string16 string = ui::FormatBytes(num_bytes);
+  ResolveJavascriptCallback(*callback_id, base::Value(string));
+}
+
 void SiteSettingsHandler::HandleGetExceptionList(const base::ListValue* args) {
   AllowJavascript();
 

chrome/browser/ui/webui/settings/site_settings_handler.h

@@ -121,6 +121,10 @@ class SiteSettingsHandler : public SettingsPageUIHandler,
       const std::list<BrowsingDataLocalStorageHelper::LocalStorageInfo>&
           local_storage_info);
 
+  // Converts a given number of bytes into a human-readable format, with data
+  // units.
+  void HandleGetFormattedBytes(const base::ListValue* args);
+
   // Returns the list of site exceptions for a given content settings type.
   void HandleGetExceptionList(const base::ListValue* args);
 

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

@@ -604,6 +604,7 @@ TEST_F(SiteSettingsHandlerTest, GetAllSitesLocalStorage) {
   ASSERT_TRUE(origin_list->GetDictionary(0, &origin_info));
   EXPECT_EQ(origin.spec(), origin_info->FindKey("origin")->GetString());
   EXPECT_EQ(0, origin_info->FindKey("engagement")->GetDouble());
+  EXPECT_EQ(1, origin_info->FindKey("usage")->GetDouble());
 }
 
 TEST_F(SiteSettingsHandlerTest, Origins) {

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

@@ -203,7 +203,19 @@ suite('AllSites', function() {
           Polymer.dom.flush();
           let siteEntries =
               testElement.$.listContainer.querySelectorAll('site-entry');
+          // Add additional origins to SiteGroups with cookies to simulate their
+          // being grouped entries, plus add local storage.
+          siteEntries[0].siteGroup.origins[0].usage = 1000;
+          siteEntries[1].siteGroup.origins.push(
+              test_util.createOriginInfo('http://bar.com'));
+          siteEntries[1].siteGroup.origins[0].usage = 500;
+          siteEntries[1].siteGroup.origins[1].usage = 500;
+          siteEntries[2].siteGroup.origins.push(
+              test_util.createOriginInfo('http://google.com'));
 
+          testElement.onSortMethodChanged_();
+          siteEntries =
+              testElement.$.listContainer.querySelectorAll('site-entry');
           // Verify all sites is not sorted by storage.
           assertEquals(3, siteEntries.length);
           assertEquals(
@@ -212,16 +224,6 @@ suite('AllSites', function() {
               'bar.com', siteEntries[1].$.displayName.innerText.trim());
           assertEquals(
               'google.com', siteEntries[2].$.displayName.innerText.trim());
-          // Add additional origins to each SiteGroup to simulate their being
-          // grouped entries. TODO(https://crbug.com/835712): This does not need
-          // to be done for every SiteGroup when sorting by storage does not
-          // only depend on the number of cookies.
-          siteEntries[0].siteGroup.origins.push(
-              test_util.createOriginInfo('http://foo.com'));
-          siteEntries[1].siteGroup.origins.push(
-              test_util.createOriginInfo('http://bar.com'));
-          siteEntries[2].siteGroup.origins.push(
-              test_util.createOriginInfo('http://google.com'));
 
           // Change the sort method, then verify all sites is now sorted by
           // name.
@@ -234,11 +236,20 @@ suite('AllSites', function() {
           let siteEntries =
               testElement.$.listContainer.querySelectorAll('site-entry');
           assertEquals(
-              'bar.com', siteEntries[0].$.displayName.innerText.trim());
+              'bar.com',
+              siteEntries[0]
+                  .root.querySelector('#displayName .url-directionality')
+                  .innerText.trim());
           assertEquals(
-              'google.com', siteEntries[1].$.displayName.innerText.trim());
+              'foo.com',
+              siteEntries[1]
+                  .root.querySelector('#displayName .url-directionality')
+                  .innerText.trim());
           assertEquals(
-              'foo.com', siteEntries[2].$.displayName.innerText.trim());
+              'google.com',
+              siteEntries[2]
+                  .root.querySelector('#displayName .url-directionality')
+                  .innerText.trim());
         });
   });
 

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

@@ -201,7 +201,6 @@ suite('SiteEntry', function() {
     testElement.siteGroup = TEST_MULTIPLE_SITE_GROUP;
     Polymer.dom.flush();
     const cookiesLabel = testElement.$.cookies;
-    assertEquals('', cookiesLabel.textContent.trim());
     assertTrue(cookiesLabel.hidden);
 
     // When the number of cookies is more than zero, the label appears.
@@ -215,7 +214,7 @@ suite('SiteEntry', function() {
         .then((args) => {
           assertEquals(3, args);
           assertFalse(cookiesLabel.hidden);
-          assertEquals('3 cookies', cookiesLabel.textContent.trim());
+          assertEquals('· 3 cookies', cookiesLabel.textContent.trim());
         });
   });
 
@@ -224,12 +223,46 @@ suite('SiteEntry', function() {
     Polymer.dom.flush();
     const cookiesLabel = testElement.$.cookies;
     assertTrue(cookiesLabel.hidden);
-    assertEquals('', cookiesLabel.textContent.trim());
 
     testElement.onSiteGroupChanged_(TEST_SINGLE_SITE_GROUP);
     // Make sure there was never any call to the back end to retrieve cookies.
     assertEquals(0, localDataBrowserProxy.getCallCount('getNumCookiesList'));
-    assertEquals('', cookiesLabel.textContent.trim());
     assertTrue(cookiesLabel.hidden);
   });
+
+  test.only('data usage shown correctly for grouped entries', function() {
+    // Clone this object to avoid propogating changes made in this test.
+    const testSiteGroup = JSON.parse(JSON.stringify(TEST_MULTIPLE_SITE_GROUP));
+    const numBytes1 = 74622;
+    const numBytes2 = 1274;
+    const numBytes3 = 0;
+    testSiteGroup.origins[0].usage = numBytes1;
+    testSiteGroup.origins[1].usage = numBytes2;
+    testSiteGroup.origins[2].usage = numBytes3;
+    testElement.siteGroup = testSiteGroup;
+    Polymer.dom.flush();
+    return browserProxy.whenCalled('getFormattedBytes').then((args) => {
+      const sumBytes = numBytes1 + numBytes2 + numBytes3;
+      assertEquals(
+          `${sumBytes} B`,
+          testElement.root.querySelector('#displayName .data-unit')
+              .textContent.trim());
+    });
+  });
+
+  test('data usage shown correctly for ungrouped entries', function() {
+    // Clone this object to avoid propogating changes made in this test.
+    const testSiteGroup = JSON.parse(JSON.stringify(TEST_SINGLE_SITE_GROUP));
+    const numBytes = 74622;
+    testSiteGroup.origins[0].usage = numBytes;
+    testElement.siteGroup = testSiteGroup;
+    Polymer.dom.flush();
+    return browserProxy.whenCalled('getFormattedBytes').then((args) => {
+      assertEquals(
+          `${numBytes} B`,
+          testElement.root.querySelector('#displayName .data-unit')
+              .textContent.trim());
+
+    });
+  });
 });

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

@@ -26,6 +26,7 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
       'fetchUsbDevices',
       'fetchZoomLevels',
       'getAllSites',
+      'getFormattedBytes',
       'getDefaultValueForContentType',
       'getExceptionList',
       'getOriginPermissions',
@@ -225,6 +226,12 @@ class TestSiteSettingsPrefsBrowserProxy extends TestBrowserProxy {
     return Promise.resolve(result);
   }
 
+  /** @override */
+  getFormattedBytes(numBytes) {
+    this.methodCalled('getFormattedBytes', numBytes);
+    return Promise.resolve(`${numBytes} B`);
+  }
+
   /** @override */
   getDefaultValueForContentType(contentType) {
     this.methodCalled('getDefaultValueForContentType', contentType);

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

@@ -207,6 +207,7 @@ cr.define('test_util', function() {
         {
           origin: origin,
           engagement: 0,
+          usage: 0,
         },
         override);
   }