Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site data management. r=Gijs draft
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 05 Feb 2018 17:42:54 +0100
changeset 755538 bc202a92d62ff8d4ddfd40ff4096c9ad1fdb0735
parent 755537 90e6666e565aaa74bd2545e13534bdebe8f090ee
child 755539 04fdb5c952a676cc99d73f2c7dfa54300682b977
push id99178
push userjhofmann@mozilla.com
push dateThu, 15 Feb 2018 11:57:05 +0000
reviewersGijs
bugs1421737
milestone60.0a1
Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site data management. r=Gijs We are no longer implicitly deleting cookies when removing site data because cookies are now listed as part of the site data manager. We're also no longer deleting cookies based on the base domain, which makes most of the UI in the removal dialog unnecessary. Instead of a tree box with sub domains we're now showing a simple listbox domains to be deleted. MozReview-Commit-ID: GWv5QVxEiiy
browser/components/preferences/siteDataRemoveSelected.js
browser/components/preferences/siteDataRemoveSelected.xul
browser/components/preferences/siteDataSettings.js
browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd
browser/themes/shared/incontentprefs/siteDataSettings.css
--- a/browser/components/preferences/siteDataRemoveSelected.js
+++ b/browser/components/preferences/siteDataRemoveSelected.js
@@ -1,199 +1,44 @@
 /* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
-ChromeUtils.import("resource://gre/modules/Services.jsm");
 
 "use strict";
 
 let gSiteDataRemoveSelected = {
 
-  _tree: null,
-
   init() {
     let bundlePreferences = document.getElementById("bundlePreferences");
     let acceptBtn = document.getElementById("SiteDataRemoveSelectedDialog")
                             .getButton("accept");
     acceptBtn.label = bundlePreferences.getString("acceptRemove");
 
-    // Organize items for the tree from the argument
-    let hostsTable = window.arguments[0].hostsTable;
-    let visibleItems = [];
-    let itemsTable = new Map();
-    for (let [ baseDomain, hosts ] of hostsTable) {
-      // In the beginning, only display base domains in the topmost level.
-      visibleItems.push({
-        level: 0,
-        opened: false,
-        host: baseDomain
-      });
-      // Other hosts are in the second level.
-      let items = hosts.map(host => {
-        return { host, level: 1 };
-      });
-      items.sort(sortByHost);
-      itemsTable.set(baseDomain, items);
-    }
-    visibleItems.sort(sortByHost);
-    this._view.itemsTable = itemsTable;
-    this._view.visibleItems = visibleItems;
-    this._tree = document.getElementById("sitesTree");
-    this._tree.view = this._view;
-
-    function sortByHost(a, b) {
-      let aHost = a.host.toLowerCase();
-      let bHost = b.host.toLowerCase();
-      return aHost.localeCompare(bHost);
-    }
+    let hosts = window.arguments[0].hosts;
+    hosts.sort();
+    let tree = document.getElementById("sitesTree");
+    this._view._hosts = hosts;
+    tree.view = this._view;
   },
 
   ondialogaccept() {
     window.arguments[0].allowed = true;
   },
 
   ondialogcancel() {
     window.arguments[0].allowed = false;
   },
 
   _view: {
-    _selection: null,
-
-    itemsTable: null,
-
-    visibleItems: null,
+    _hosts: null,
 
     get rowCount() {
-      return this.visibleItems.length;
-    },
-
-    getCellText(index, column) {
-      let item = this.visibleItems[index];
-      return item ? item.host : "";
-    },
-
-    isContainer(index) {
-      let item = this.visibleItems[index];
-      if (item && item.level === 0) {
-        return true;
-      }
-      return false;
-    },
-
-    isContainerEmpty() {
-      return false;
-    },
-
-    isContainerOpen(index) {
-      let item = this.visibleItems[index];
-      if (item && item.level === 0) {
-        return item.opened;
-      }
-      return false;
+      return this._hosts.length;
     },
-
-    getLevel(index) {
-      let item = this.visibleItems[index];
-      return item ? item.level : 0;
-    },
-
-    hasNextSibling(index, afterIndex) {
-      let item = this.visibleItems[index];
-      if (item) {
-        let thisLV = this.getLevel(index);
-        for (let i = afterIndex + 1; i < this.rowCount; ++i) {
-          let nextLV = this.getLevel(i);
-          if (nextLV == thisLV) {
-            return true;
-          }
-          if (nextLV < thisLV) {
-            break;
-          }
-        }
-      }
-      return false;
-    },
-
-    getParentIndex(index) {
-      if (!this.isContainer(index)) {
-        for (let i = index - 1; i >= 0; --i) {
-          if (this.isContainer(i)) {
-            return i;
-          }
-        }
-      }
-      return -1;
+    getCellText(index, column) {
+      return this._hosts[index];
     },
-
-    toggleOpenState(index) {
-      let item = this.visibleItems[index];
-      if (!this.isContainer(index)) {
-        return;
-      }
-
-      if (item.opened) {
-        item.opened = false;
-
-        let deleteCount = 0;
-        for (let i = index + 1; i < this.visibleItems.length; ++i) {
-          if (!this.isContainer(i)) {
-            ++deleteCount;
-          } else {
-            break;
-          }
-        }
-
-        if (deleteCount) {
-          this.visibleItems.splice(index + 1, deleteCount);
-          this.treeBox.rowCountChanged(index + 1, -deleteCount);
-        }
-      } else {
-        item.opened = true;
-
-        let childItems = this.itemsTable.get(item.host);
-        for (let i = 0; i < childItems.length; ++i) {
-          this.visibleItems.splice(index + i + 1, 0, childItems[i]);
-        }
-        this.treeBox.rowCountChanged(index + 1, childItems.length);
-      }
-      this.treeBox.invalidateRow(index);
+    getLevel(index) {
+      return 0;
     },
-
-    get selection() {
-      return this._selection;
-    },
-    set selection(v) {
-      this._selection = v;
-      return v;
-    },
-    setTree(treeBox) {
-      this.treeBox = treeBox;
-    },
-    isSeparator(index) {
-      return false;
-    },
-    isSorted(index) {
-      return false;
-    },
-    canDrop() {
-      return false;
-    },
-    drop() {},
-    getRowProperties() {},
-    getCellProperties() {},
-    getColumnProperties() {},
-    hasPreviousSibling(index) {},
-    getImageSrc() {},
-    getCellValue() {},
-    cycleHeader() {},
-    selectionChanged() {},
-    cycleCell() {},
-    isEditable() {},
-    isSelectable() {},
-    setCellValue() {},
-    setCellText() {},
-    performAction() {},
-    performActionOnRow() {},
-    performActionOnCell() {}
-  }
+  },
 };
--- a/browser/components/preferences/siteDataRemoveSelected.xul
+++ b/browser/components/preferences/siteDataRemoveSelected.xul
@@ -8,17 +8,17 @@
 <?xml-stylesheet href="chrome://browser/content/preferences/siteDataSettings.css" type="text/css"?>
 <?xml-stylesheet href="chrome://browser/skin/preferences/in-content/siteDataSettings.css" type="text/css"?>
 
 <!DOCTYPE dialog SYSTEM "chrome://browser/locale/preferences/siteDataSettings.dtd" >
 
 <dialog id="SiteDataRemoveSelectedDialog"
         windowtype="Browser:SiteDataRemoveSelected"
         width="500"
-        title="&removingDialog.title;"
+        title="&removingDialog1.title;"
         onload="gSiteDataRemoveSelected.init();"
         ondialogaccept="gSiteDataRemoveSelected.ondialogaccept(); return true;"
         ondialogcancel="gSiteDataRemoveSelected.ondialogcancel(); return true;"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
 
   <script src="chrome://browser/content/preferences/siteDataRemoveSelected.js"/>
 
   <stringbundle id="bundlePreferences"
@@ -30,26 +30,26 @@
         <image class="question-icon"/>
       </vbox>
       <vbox flex="1">
         <!-- Only show this label on OS X because of no dialog title -->
         <label id="removing-label"
 #ifndef XP_MACOSX
                hidden="true"
 #endif
-        >&removingDialog.title;</label>
+        >&removingDialog1.title;</label>
         <separator class="thin"/>
-        <description id="removing-description">&removingSelected.description;</description>
+        <description id="removing-description">&removingSelected1.description;</description>
       </vbox>
     </hbox>
 
     <separator />
 
     <vbox flex="1">
-      <label>&siteTree2.label;</label>
+      <label>&siteTree3.label;</label>
       <separator class="thin"/>
       <tree id="sitesTree" flex="1" seltype="single" hidecolumnpicker="true">
         <treecols>
           <treecol primary="true" flex="1" hideheader="true"/>
         </treecols>
         <treechildren />
       </tree>
     </vbox>
--- a/browser/components/preferences/siteDataSettings.js
+++ b/browser/components/preferences/siteDataSettings.js
@@ -213,80 +213,33 @@ let gSiteDataSettings = {
       if (siteForHost) {
         siteForHost.userAction = "remove";
       }
       item.remove();
     }
     this._updateButtonsState();
   },
 
-  _getBaseDomainFromHost(host) {
-    let result = host;
-    try {
-      result = Services.eTLD.getBaseDomainFromHost(host);
-    } catch (e) {
-      if (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS ||
-          e.result == Cr.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {
-        // For this 2 expected errors, just take the host as the result.
-        // - NS_ERROR_HOST_IS_IP_ADDRESS: the host is in ipv4/ipv6.
-        // - NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS: not enough domain part to extract.
-        result = host;
-      } else {
-        throw e;
-      }
-    }
-    return result;
-  },
+  saveChanges() {
+    // Tracks whether the user confirmed their decision.
+    let allowed = false;
 
-  saveChanges() {
-    let allowed = true;
+    let removals = this._sites
+      .filter(site => site.userAction == "remove")
+      .map(site => site.host);
 
-    // Confirm user really wants to remove site data starts
-    let removals = new Set();
-    this._sites = this._sites.filter(site => {
-      if (site.userAction === "remove") {
-        removals.add(site.host);
-        return false;
-      }
-      return true;
-    });
-
-    if (removals.size > 0) {
-      if (this._sites.length == 0) {
-        if (SiteDataManager.promptSiteDataRemoval(window)) {
+    if (removals.length > 0) {
+      if (this._sites.length == removals.length) {
+        allowed = SiteDataManager.promptSiteDataRemoval(window);
+        if (allowed) {
           SiteDataManager.removeAll();
         }
       } else {
-        // User only removes partial sites.
-        // We will remove cookies based on base domain, say, user selects "news.foo.com" to remove.
-        // The cookies under "music.foo.com" will be removed together.
-        // We have to prompt user about this action.
-        let hostsTable = new Map();
-        // Group removed sites by base domain
-        for (let host of removals) {
-          let baseDomain = this._getBaseDomainFromHost(host);
-          let hosts = hostsTable.get(baseDomain);
-          if (!hosts) {
-            hosts = [];
-            hostsTable.set(baseDomain, hosts);
-          }
-          hosts.push(host);
-        }
-
-        // Pick out sites with the same base domain as removed sites
-        for (let site of this._sites) {
-          let baseDomain = this._getBaseDomainFromHost(site.host);
-          let hosts = hostsTable.get(baseDomain);
-          if (hosts) {
-            hosts.push(site.host);
-          }
-        }
-
         let args = {
-          hostsTable,
+          hosts: removals,
           allowed: false
         };
         let features = "centerscreen,chrome,modal,resizable=no";
         window.openDialog("chrome://browser/content/preferences/siteDataRemoveSelected.xul", "", features, args);
         allowed = args.allowed;
         if (allowed) {
           try {
             SiteDataManager.remove(removals);
@@ -294,19 +247,22 @@ let gSiteDataSettings = {
             // Hit error, maybe remove unknown site.
             // Let's print out the error, then proceed to close this settings dialog.
             // When we next open again we will once more get sites from the SiteDataManager and refresh the list.
             Cu.reportError(e);
           }
         }
       }
     }
-    // Confirm user really wants to remove site data ends
 
-    this.close();
+    // If the user cancelled the confirm dialog keep the site data window open,
+    // they can still press cancel again to exit.
+    if (allowed) {
+      this.close();
+    }
   },
 
   close() {
     window.close();
   },
 
   onClickTreeCol(e) {
     this._sortSites(this._sites, e.target);
--- a/browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd
+++ b/browser/locales/en-US/chrome/browser/preferences/siteDataSettings.dtd
@@ -10,11 +10,11 @@
 <!ENTITY     searchTextboxPlaceHolder             "Search websites">
 <!ENTITY     searchTextboxPlaceHolder.accesskey   "S">
 <!ENTITY     removeSelected.label          "Remove Selected">
 <!ENTITY     removeSelected.accesskey      "r">
 <!ENTITY     save.label                    "Save Changes">
 <!ENTITY     save.accesskey                "a">
 <!ENTITY     cancel.label                  "Cancel">
 <!ENTITY     cancel.accesskey              "C">
-<!ENTITY     removingDialog.title          "Removing Site Data">
-<!ENTITY     removingSelected.description  "Removing site data will also remove related cookies and offline web content. This may log you out of websites. Are you sure you want to make the changes?">
-<!ENTITY     siteTree2.label               "The following website cookies will be removed">
+<!ENTITY     removingDialog1.title         "Removing Cookies and Site Data">
+<!ENTITY     removingSelected1.description "Removing cookies and site data may log you out of websites. Are you sure you want to make the changes?">
+<!ENTITY     siteTree3.label               "Cookies and site data for the following websites will be removed">
--- a/browser/themes/shared/incontentprefs/siteDataSettings.css
+++ b/browser/themes/shared/incontentprefs/siteDataSettings.css
@@ -20,16 +20,17 @@
   width: 50px;
 }
 
 /**
  * Confirmation dialog of removing sites selected
  */
 #SiteDataRemoveSelectedDialog {
   padding: 16px;
+  min-height: 36em;
 }
 
 #contentContainer {
   font-size: 1.2em;
   margin-bottom: 10px;
 }
 
 .question-icon {