Site Settings Desktop: Implement individual cookie removal and RemoveAll.

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

Index: chrome/browser/resources/settings/site_settings/site_data.js
diff --git a/chrome/browser/resources/settings/site_settings/site_data.js b/chrome/browser/resources/settings/site_settings/site_data.js
index 157da97de06eed3e20c55240ffd5c4fef68c38ee..c994793025ff48a84bf6dac8c478e5a4a8ef3191 100644
--- a/chrome/browser/resources/settings/site_settings/site_data.js
+++ b/chrome/browser/resources/settings/site_settings/site_data.js
@@ -38,15 +38,39 @@ Polymer({
         this.onTreeItemRemoved_.bind(this));
     this.treeNodes_ = new settings.CookieTreeNode(null);
     // Start the initial request.
-    this.browserProxy.reloadCookies();
+    this.reloadCookies_();
+  },
+
+  /**
+   * Reloads the whole cookie list.
+   * @private
+   */
+  reloadCookies_: function() {
     this.requests_ = 1;
+    this.browserProxy.reloadCookies();

[dschuyler, 2016/08/18 00:44:03]

Can we change reloadCookies() and removeAllCookies()
to return a promise and remove the this.requests_ 
tracking entirely?

[Finnur, 2016/08/18 16:20:30]

I've converted to promises, but it still doesn't alleviate the need to track
requests. It does, however, make the tracking a bit saner.

The reason I need to track the number of outstanding requests is to figure out
when I'm done fetching data so that I'm not calling getSummaryList() repeatedly
while the data is being populated -- it slows down the page loading.

The CookieTreeModel, when reset (as in reload-all-cookies), will give you the
top-level nodes. It is up to the client to decide what to do with it. We want
all the data, so we keep issuing more requests for more details until we have
everything.

I only want to update the UI when that is complete. So, I have to keep track of
the requests to know when. It feels more natural to do this at the JS layer
because the C++ layer doesn't really care if more requests are coming in and it
doesn't feel right to have it try to decipher what the client will do (or tell
the client when it is done).

Does this make sense?

   },
 
+  /**
+   * Returns whether remove all should be shown.
+   * @param {Array<CookieDataSummaryItem>} sites The sites list to use to
+   * determine whether the button should be visible.
+   * @private
+   */
+  removeAllIsVisible_: function(sites) {
+    return sites.length > 0;
+  },
+
+  /**
+   * Called when the cookie list is ready to be shown.
+   * @private
+   */
   loadChildren_: function(list) {
     var parentId = list[0];
     var data = list[1];
 
     if (parentId == null) {
+      // New root being added, clear the list and add the nodes.
+      this.sites = [];
       this.treeNodes_.addChildNodes(this.treeNodes_, data);
     } else {
       this.treeNodes_.populateChildNodes(parentId, this.treeNodes_, data);
@@ -62,6 +86,11 @@ Polymer({
 
     if (--this.requests_ == 0)
       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);
   },
 
   /**
@@ -73,7 +102,27 @@ Polymer({
   },
 
   /**
-   * @param {!{model: !{item: !{title: string, id: string}}}} event
+   * Deletes all site data for a given site.
+   * @param {!{model: !{item: CookieDataSummaryItem}}} event
+   * @private
+   */
+  onDeleteSite_: function(event) {
+    this.browserProxy.removeCookie(event.model.item.id);
+  },
+
+  /**
+   * Deletes site data for all sites.
+   * @private
+   */
+  onDeleteAllSites_: function() {
+    // removeAllCookies will result in the client list being updated, so update
+    // the list of outstanding requests.
+    this.requests_++;

[dschuyler, 2016/08/18 00:44:03]

nit: please use pre-increment unless post-increment
is needed for the specific case. i.e.
++this.requests_;

[Finnur, 2016/08/18 16:20:30]

Acknowledged.

+    this.browserProxy.removeAllCookies();

[dschuyler, 2016/08/18 00:44:03]

Same comment:

Can we change reloadCookies() and removeAllCookies()
to return a promise and remove the this.requests_ 
tracking entirely?

+  },
+
+  /**
+   * @param {!{model: !{item: CookieDataSummaryItem}}} event
    * @private
    */
   onSiteTap_: function(event) {