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..50005cb23bd8404e43fc4e488b03fef82fa93c6d 100644
--- a/chrome/browser/resources/settings/site_settings/site_data.js
+++ b/chrome/browser/resources/settings/site_settings/site_data.js
@@ -33,20 +33,45 @@ Polymer({
   },
 
   ready: function() {
-    this.addWebUIListener('loadChildren', this.loadChildren_.bind(this));
     this.addWebUIListener('onTreeItemRemoved',
         this.onTreeItemRemoved_.bind(this));
     this.treeNodes_ = new settings.CookieTreeNode(null);
     // Start the initial request.
-    this.browserProxy.reloadCookies();
-    this.requests_ = 1;
+    this.reloadCookies_();
   },
 
+  /**
+   * Reloads the whole cookie list.
+   * @private
+   */
+  reloadCookies_: function() {
+    this.browserProxy.reloadCookies().then(function(list) {
+      this.loadChildren_(list);
+    }.bind(this));
+  },
+
+  /**
+   * Returns whether remove all should be shown.
+   * @param {Array<CookieDataSummaryItem>} sites The sites list to use to
+   * determine whether the button should be visible.

[dschuyler, 2016/08/18 22:51:10]

nit: indent 'determine...'

[Finnur, 2016/08/19 12:18:32]

Done.

+   * @private
+   */
+  removeAllIsVisible_: function(sites) {
+    return sites.length > 0;
+  },
+
+  /**
+   * Called when the cookie list is ready to be shown.
+   * @private
+   */
   loadChildren_: function(list) {

[dschuyler, 2016/08/18 22:51:10]

@param for 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.requests_ = 0;
       this.treeNodes_.addChildNodes(this.treeNodes_, data);
     } else {
       this.treeNodes_.populateChildNodes(parentId, this.treeNodes_, data);
@@ -55,17 +80,26 @@ Polymer({
     for (var i = 0; i < data.length; ++i) {
       var prefix = parentId == null ? '' : parentId + ', ';
       if (data[i].hasChildren) {
-        this.requests_ += 1;
-        this.browserProxy.loadCookieChildren(prefix + data[i].id);
+        ++this.requests_;
+        this.browserProxy.loadCookieChildren(
+            prefix + data[i].id).then(function(list) {
+          --this.requests_;
+          this.loadChildren_(list);
+        }.bind(this));
       }
     }
 
-    if (--this.requests_ == 0)
+    if (this.requests_ == 0)
       this.sites = this.treeNodes_.getSummaryList();

[dschuyler, 2016/08/18 22:51:10]

It looks like this may be called more often
than once. i.e. it may trigger once per level
in the tree (*I think*). If the goal is just
to reduce the calls to getSummaryList, this 
may be fine. If getSummeryList should not
be called more than once (in a strict sense)
then there /may/ be an issue here.

[Finnur, 2016/08/18 23:27:38]

I don't think this is an issue, but will check tomorrow when I get in to work
again.

[Finnur, 2016/08/19 12:18:32]

I verified this again and getSummaryList is called only once (at the end, when
all levels have finished updating). 

+
+    // 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);
   },
 
   /**
-   * Called when an item is removed.
+   * Called when a single item has been removed (not during delete all).
    */
   onTreeItemRemoved_: function(args) {

[dschuyler, 2016/08/18 22:51:10]

Can we @param the args?

[Finnur, 2016/08/18 23:27:38]

Umm, yeah -- |we| probably can. :) I, however, already made an attempt at it
yesterday and gave up. I'm just not sure how to represent the data types
involved to Closure. Your input here would be valuable.

Essentially, I wanted to represent that |args| always looks like this:

Array<String, Number, Number>

In other words, it is a list value with always the same three data types.

But Closure doesn't like that kind of definition. I also tried something like
Array<String|Number>, but then Closure complains about the call to
removeByParentId below (because it wants String but gets String|Number.

What's the right way to do this?

[Finnur, 2016/08/19 12:18:32]

Actually, scratch that...

Coming in this morning with a fresh mind I realize that sending the data the way
the handler is doing it is a bit dumb. So I changed it to a dictionary and added
the @params.

     this.treeNodes_.removeByParentId(args[0], args[1], args[2]);
@@ -73,7 +107,26 @@ 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() {
+    this.browserProxy.removeAllCookies().then(function(list) {
+      this.loadChildren_(list);
+    }.bind(this));
+  },
+
+  /**
+   * @param {!{model: !{item: CookieDataSummaryItem}}} event
    * @private
    */
   onSiteTap_: function(event) {