WebUI Settings: Add request queue to CookiesViewHandler

Previously the handler (D)CHECK enforced that only a single operation
could be in progress at any one time. No enforcement of this invariant
occurs in JS code. A delay in retrieving information from storage
providers, or changes to pages which are unaware of this subtlety,
could result in tripping this.

This CL introduces a queue for requests that is opaque to JavaScript
and ensures that only a single tree model batch operation occurs at
a time.

Bug: 1141796
Change-Id: If246d5f5173ed8cb521045d7c5a7216e5fe7179f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390751
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848078}

chrome/browser/resources/settings/lazy_load.js

@@ -88,8 +88,8 @@ export {SafeBrowsingSetting} from './privacy_page/security_page.js';
 export {AndroidInfoBrowserProxyImpl} from './site_settings/android_info_browser_proxy.js';
 // </if>
 export {ChooserType, ContentSetting, ContentSettingsTypes, CookieControlsMode, SITE_EXCEPTION_WILDCARD, SiteSettingSource, SortMethod} from './site_settings/constants.js';
-export {cookieInfo} from './site_settings/cookie_info.js';
-export {CookieList, LocalDataBrowserProxy, LocalDataBrowserProxyImpl, LocalDataItem} from './site_settings/local_data_browser_proxy.js';
+export {CookieDetails, cookieInfo} from './site_settings/cookie_info.js';
+export {LocalDataBrowserProxy, LocalDataBrowserProxyImpl, LocalDataItem} from './site_settings/local_data_browser_proxy.js';
 export {HandlerEntry, ProtocolEntry} from './site_settings/protocol_handlers.js';
 export {kControlledByLookup} from './site_settings/site_settings_behavior.js';
 export {ContentSettingProvider, DefaultContentSetting, RawChooserException, RawSiteException, RecentSitePermissions, SiteException, SiteGroup, SiteSettingsPrefsBrowserProxy, SiteSettingsPrefsBrowserProxyImpl, ZoomLevelEntry} from './site_settings/site_settings_prefs_browser_proxy.js';

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

@@ -12,15 +12,6 @@ import {addSingletonGetter, sendWithPromise} from 'chrome://resources/js/cr.m.js
 import {CookieDetails} from './cookie_info.js';
 // clang-format on
 
-/**
- * @typedef {{
- *   id: string,
- *   start: number,
- *   children: !Array<CookieDetails>,
- * }}
- */
-export let CookieList;
-
 /**
  * @typedef {{
  *   localData: string,
@@ -29,15 +20,6 @@ export let CookieList;
  */
 export let LocalDataItem;
 
-/**
- * TODO(dschuyler): add |filter| and |order|.
- * @typedef {{
- *   items: !Array<!LocalDataItem>,
- *   total: number,
- * }}
- */
-let LocalDataList;
-
 /**
  * Number of cookies attached to a given domain / eTLD+1.
  * @typedef {{
@@ -51,7 +33,7 @@ let EtldPlus1CookieNumber;
 export class LocalDataBrowserProxy {
   /**
    * @param {string} filter Search filter (use "" for none).
-   * @return {!Promise<!LocalDataList>}
+   * @return {!Promise<!Array<!LocalDataItem>>}
    */
   getDisplayList(filter) {}
 
@@ -69,15 +51,16 @@ export class LocalDataBrowserProxy {
   removeShownItems() {}
 
   /**
-   * Remove a specific list item. Completion signaled by on-tree-item-removed.
-   * @param {string} id Which element to delete.
+   * Remove data for a specific site. Completion signaled by
+   * on-tree-item-removed.
+   * @param {string} site Site to delete data for.
    */
-  removeItem(id) {}
+  removeSite(site) {}
 
   /**
    * Gets the cookie details for a particular site.
    * @param {string} site The name of the site.
-   * @return {!Promise<!CookieList>}
+   * @return {!Promise<!Array<!CookieDetails>>}
    */
   getCookieDetails(site) {}
 
@@ -96,11 +79,10 @@ export class LocalDataBrowserProxy {
   reloadCookies() {}
 
   /**
-   * TODO(dschuyler): merge with removeItem().
-   * Removes a given cookie.
-   * @param {string} path The path to the parent cookie.
+   * Removes a given piece of site data.
+   * @param {string} path The path to the item in the tree model.
    */
-  removeCookie(path) {}
+  removeItem(path) {}
 
   /**
    * Removes all SameSite=None cookies, as well as storage available in
@@ -131,8 +113,8 @@ export class LocalDataBrowserProxyImpl {
   }
 
   /** @override */
-  removeItem(id) {
-    chrome.send('localData.removeItem', [id]);
+  removeSite(site) {
+    chrome.send('localData.removeSite', [site]);
   }
 
   /** @override */
@@ -151,8 +133,8 @@ export class LocalDataBrowserProxyImpl {
   }
 
   /** @override */
-  removeCookie(path) {
-    chrome.send('localData.removeCookie', [path]);
+  removeItem(path) {
+    chrome.send('localData.removeItem', [path]);
   }
 
   /** @override */

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

@@ -130,12 +130,10 @@ Polymer({
   currentRouteChanged(currentRoute, previousRoute) {
     GlobalScrollTargetBehaviorImpl.currentRouteChanged.call(this, currentRoute);
     // Reload cookies on navigation to the site data page from a different
-    // route, except when a search query is present, as that will be handled by
-    // onFilterChanged_.
-    // TODO (crbug.com/1141796): Remove this layering violation.
-    const searchQueryParam =
-        Router.getInstance().getQueryParameters().get('searchSubpage');
-    if (currentRoute === routes.SITE_SETTINGS_SITE_DATA && !searchQueryParam) {
+    // page. Avoid reloading on repeated navigations to the same page, as these
+    // are likely search queries.
+    if (currentRoute === routes.SITE_SETTINGS_SITE_DATA &&
+        currentRoute !== previousRoute) {
       this.isLoading_ = true;
       // Needed to fix iron-list rendering issue. The list will not render
       // correctly until a scroll occurs.
@@ -205,12 +203,12 @@ Polymer({
    * @private
    */
   onFilterChanged_(current, previous) {
-    // Ignore the first undefined -> defined transition, unless |current| has a
-    // value. This can occur if the first navigation to the page contains a
-    // search query parameter.
-    // TODO (crbug.com/1141796): Remove requirement to perform this check as
-    // it is subtle and a flow on from a layering violation.
-    if (previous === undefined && !current) {
+    // Ignore filter changes which do not occur on the site data page. The
+    // site settings data details subpage expects the tree model to remain in
+    // the same state.
+    if (previous === undefined ||
+        Router.getInstance().getCurrentRoute() !==
+            routes.SITE_SETTINGS_SITE_DATA) {
       return;
     }
     this.updateSiteList_();
@@ -222,8 +220,8 @@ Polymer({
    */
   updateSiteList_() {
     this.isLoading_ = true;
-    this.browserProxy_.getDisplayList(this.filter).then(listInfo => {
-      this.updateList('sites', item => item.site, listInfo.items);
+    this.browserProxy_.getDisplayList(this.filter).then(localDataItems => {
+      this.updateList('sites', item => item.site, localDataItems);
       this.isLoading_ = false;
       this.fire('site-data-list-complete');
     });

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

@@ -18,7 +18,7 @@ import {routes} from '../route.js';
 import {Route, RouteObserverBehavior, Router} from '../router.m.js';
 
 import {CookieDataForDisplay, CookieDetails, getCookieData} from './cookie_info.js';
-import {CookieList, LocalDataBrowserProxy, LocalDataBrowserProxyImpl} from './local_data_browser_proxy.js';
+import {LocalDataBrowserProxy, LocalDataBrowserProxyImpl} from './local_data_browser_proxy.js';
 
 
 const categoryLabels = {
@@ -60,9 +60,6 @@ Polymer({
 
     /** @private */
     site_: String,
-
-    /** @private */
-    siteId_: String,
   },
 
   /**
@@ -118,12 +115,11 @@ Polymer({
   },
 
   /**
-   * @param {!CookieList} cookies
+   * @param {!Array<!CookieDetails>} cookies
    * @private
    */
   onCookiesLoaded_(cookies) {
-    this.siteId_ = cookies.id;
-    this.entries_ = cookies.children;
+    this.entries_ = cookies;
     // Set up flag for expanding cookie details.
     this.entries_.forEach(function(e) {
       e.expanded_ = false;
@@ -136,12 +132,11 @@ Polymer({
    * @private
    */
   onCookiesLoadFailed_() {
-    this.siteId_ = '';
     this.entries_ = [];
   },
 
   /**
-   * A handler for when the user opts to remove a single cookie.
+   * Retrieves a string description for the provided |item|.
    * @param {!CookieDetails} item
    * @return {string}
    * @private
@@ -167,7 +162,7 @@ Polymer({
   onRemove_(event) {
     MetricsBrowserProxyImpl.getInstance().recordSettingsPageHistogram(
         PrivacyElementInteractions.COOKIE_DETAILS_REMOVE_ITEM);
-    this.browserProxy_.removeCookie(
+    this.browserProxy_.removeItem(
         /** @type {!CookieDetails} */ (event.currentTarget.dataset).idPath);
   },
 
@@ -177,6 +172,6 @@ Polymer({
   removeAll() {
     MetricsBrowserProxyImpl.getInstance().recordSettingsPageHistogram(
         PrivacyElementInteractions.COOKIE_DETAILS_REMOVE_ALL);
-    this.browserProxy_.removeCookie(this.siteId_);
+    this.browserProxy_.removeSite(this.site_);
   },
 });

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

@@ -55,6 +55,6 @@ Polymer({
     e.stopPropagation();
     MetricsBrowserProxyImpl.getInstance().recordSettingsPageHistogram(
         PrivacyElementInteractions.SITE_DATA_REMOVE_SITE);
-    this.browserProxy_.removeItem(this.model.site);
+    this.browserProxy_.removeSite(this.model.site);
   },
 });