Site Settings Desktop: Polish the Site Data details dialog.

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

Index: chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
diff --git a/chrome/browser/resources/settings/site_settings/site_data_details_dialog.js b/chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
index 1bf1018d81c7ef49e0b2cf70fa0678a4b431faa4..477dcca7a04968aa331bb1bbb75d6b650618f5c4 100644
--- a/chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
+++ b/chrome/browser/resources/settings/site_settings/site_data_details_dialog.js
@@ -57,6 +57,23 @@ Polymer({
   },
 
   /**
+   * Closes the dialog, if open.
+   */
+  close: function() {
+    var dialog = this.getDialog_();
+    if (dialog.open)
+      dialog.close();
+  },
+
+  /**
+   * @return {!CrDialogElement}
+   * @private
+   */
+  getDialog_: function() {
+    return /** @type {!CrDialogElement} */ (this.$.dialog);
+  },

[Finnur, 2016/09/29 15:54:01]

This was also not mentioned in the bug, but discovered while debugging.

This change is to get rid of an error in the console complaining about
HTMLDialogElement not having an open property and therefore can't be closed. I
took a look at how another dialog does this (bluetooth) and did the same here.

[dschuyler, 2016/09/29 19:40:27]

I think this can be inlined above like:

  var dialog = /** @type {!CrDialogElement} */(this.getDialog_());

If that works, could we skip having this function?

[Finnur, 2016/09/30 09:19:43]

Done, with minor modifications. :)

+
+  /**
    * Populates the dialog with the data about the site.
    * @private
    */
@@ -128,8 +145,8 @@ Polymer({
    */
   onTreeItemRemoved_: function(args) {
     this.entries_ = this.site_.getCookieList();
-    if (args.id == this.site_.data_.id || this.entries_.length == 0) {
-      this.$.dialog.close();
+    if (this.site_.children_.length == 0 || this.entries_.length == 0) {

[Finnur, 2016/09/29 15:54:01]

This is to fix the bug with the dialog closing prematurely (see comment 2 
of the bug).

Before closing the dialog we should be checking to see if site_ has any
remaining children left, because, when a container node is deleted, the
onTreeItemRemoved_ args will say a node with ID 'foo' deleted nodes 
n to n+x.

When a container node is deleted (as opposed to a leaf), the ID provided
is always going to match the top level (site) ID, so the old code thought that
the
site itself had been deleted, when in fact only a child of site had been
deleted.

[dschuyler, 2016/09/29 19:40:27]

Acknowledged.

+      this.close();
       return;
     }
 
@@ -183,7 +200,7 @@ Polymer({
   onRemoveAll_: function(event) {
     cr.removeWebUIListener(this.listener_);
     this.browserProxy.removeCookie(this.site_.data_.id);
-    this.$.dialog.close();
+    this.close();
   },
 
   /** @private */