Site Settings Desktop: Polish the Site Data details dialog.

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * 'site-data-details-dialog' provides a dialog to show details of site data
  * stored by a given site.
  */
 Polymer({
@@ skipping 39 matching lines @@
    */
   open: function(site) {
     this.site_ = site;
     this.populateDialog_();
     this.listener_ = cr.addWebUIListener(
         'onTreeItemRemoved', this.onTreeItemRemoved_.bind(this));
     this.$.dialog.showModal();
   },
 
   /**
+   * 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
    */
   populateDialog_: function() {
     this.title_ = loadTimeData.getStringF('siteSettingsCookieDialog',
                                           this.site_.data_.title);
 
     this.entries_ = this.site_.getCookieList();
     if (this.entries_.length < 2) {
       // When there's only one item to show, hide the picker and change the
@@ skipping 51 matching lines @@
       site.addCookieData(root, node);
   },
 
   /**
    * Called when a single item has been removed.
    * @param {!CookieRemovePacket} args The details about what to remove.
    * @private
    */
   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;
     }
 
     if (this.entries_.length <= this.lastSelectedIndex_)
       this.lastSelectedIndex_ = this.entries_.length - 1;
     var selectedId = this.entries_[this.lastSelectedIndex_].id;
     this.$.picker.selected = selectedId;
     this.populateItem_(selectedId, this.site_);
   },
 
@@ skipping 33 matching lines @@
         this.site_, this.site_.data_.id, this.$.picker.selected));
   },
 
   /**
    * A handler for when the user opts to remove all cookies.
    * @private
    */
   onRemoveAll_: function(event) {
     cr.removeWebUIListener(this.listener_);
     this.browserProxy.removeCookie(this.site_.data_.id);
-    this.$.dialog.close();
+    this.close();
   },
 
   /** @private */
   onCancelTap_: function() {
     this.$.dialog.cancel();
   },
 });