Bug 1310961 - Stop using nsIDialogParamBlock in deletecert.(js|xul). r=mgoodwin

An nsIDialogParamBlock is unnecessary for how deletecert.(js|xul) is currently
used. Moreover, nsIDialogParamBlock is arguably a poor API, so moving away from
it is also advantageous.

In addition, this patch also fixes this bug:
1. Select a cert to delete in one of the cert manager tabs.
2. Press the delete button to launch the confirmation dialog, but don't accept
or cancel.
3. Switch to another tab in the cert manager.
4. Press the accept button in the confirmation dialog.

ER:
Cert selected in the original tab is deleted.

AR:
Cert at the same index of the new tab is deleted, even though it was never
selected.

MozReview-Commit-ID: 3N8klOhrVzi

--HG--
extra : rebase_source : 92c11209e0fed36ab88f4a9d0fa7e82c88a1ca4a

security/manager/pki/resources/content/certManager.js

@@ -13,8 +13,6 @@ const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
 const nsIX509Cert = Components.interfaces.nsIX509Cert;
 const nsICertTree = Components.interfaces.nsICertTree;
 const nsCertTree = "@mozilla.org/security/nsCertTree;1";
-const nsIDialogParamBlock = Components.interfaces.nsIDialogParamBlock;
-const nsDialogParamBlock = "@mozilla.org/embedcomp/dialogparam;1";
 
 const gCertFileTypes = "*.p7b; *.crt; *.cert; *.cer; *.pem; *.der";
 
@@ -32,10 +30,30 @@ var selected_tree_items = [];
 var selected_index = [];
 var certdb;
 
+/**
+ * Cert tree for the "Authorities" tab.
+ * @type nsICertTree
+ */
 var caTreeView;
+/**
+ * Cert tree for the "Servers" tab.
+ * @type nsICertTree
+ */
 var serverTreeView;
+/**
+ * Cert tree for the "People" tab.
+ * @type nsICertTree
+ */
 var emailTreeView;
+/**
+ * Cert tree for the "Your Certificates" tab.
+ * @type nsICertTree
+ */
 var userTreeView;
+/**
+ * Cert tree for the "Other" tab.
+ * @type nsICertTree
+ */
 var orphanTreeView;
 
 var smartCardObserver = {
@@ -402,58 +420,39 @@ function exportCerts()
   }
 }
 
-function deleteCerts()
-{
+/**
+ * Deletes the selected certs in the active tab.
+ */
+function deleteCerts() {
   getSelectedTreeItems();
-  var numcerts = selected_tree_items.length;
+  let numcerts = selected_tree_items.length;
   if (numcerts == 0) {
     return;
   }
 
-  var params = Components.classes[nsDialogParamBlock].createInstance(nsIDialogParamBlock);
+  const treeViewMap = {
+    "mine_tab": userTreeView,
+    "websites_tab": serverTreeView,
+    "ca_tab": caTreeView,
+    "others_tab": emailTreeView,
+    "orphan_tab": orphanTreeView,
+  };
+  let selTab = document.getElementById("certMgrTabbox").selectedItem;
+  let selTabID = selTab.getAttribute("id");
 
-  var selTab = document.getElementById('certMgrTabbox').selectedItem;
-  var selTabID = selTab.getAttribute('id');
-
-  switch (selTabID) {
-    case "mine_tab":
-    case "websites_tab":
-    case "ca_tab":
-    case "others_tab":
-    case "orphan_tab":
-      params.SetString(0, selTabID);
-      break;
-    default:
-      return;
+  if (!(selTabID in treeViewMap)) {
+    return;
   }
 
-  let array = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
-  for (let treeItem of selected_tree_items) {
-    array.appendElement(treeItem, false);
-  }
-  params.objects = array;
+  let retVals = {
+    deleteConfirmed: false,
+  };
+  window.openDialog("chrome://pippki/content/deletecert.xul", "",
+                    "chrome,centerscreen,modal", selTabID, selected_tree_items,
+                    retVals);
 
-  window.openDialog('chrome://pippki/content/deletecert.xul', "",
-                    'chrome,centerscreen,modal', params);
-
-  if (params.GetInt(1) == 1) {
-    // user closed dialog with OK
-    var treeView = null;
-    var loadParam = null;
-
-    selTab = document.getElementById('certMgrTabbox').selectedItem;
-    selTabID = selTab.getAttribute('id');
-    if (selTabID == 'mine_tab') {
-      treeView = userTreeView;
-    } else if (selTabID == "others_tab") {
-      treeView = emailTreeView;
-    } else if (selTabID == "websites_tab") {
-      treeView = serverTreeView;
-    } else if (selTabID == "ca_tab") {
-      treeView = caTreeView;
-    } else if (selTabID == "orphan_tab") {
-      treeView = orphanTreeView;
-    }
+  if (retVals.deleteConfirmed) {
+    let treeView = treeViewMap[selTabID];
 
     for (let t = numcerts - 1; t >= 0; t--) {
       treeView.deleteEntryObject(selected_index[t]);
@@ -462,7 +461,7 @@ function deleteCerts()
     selected_tree_items = [];
     selected_index = [];
     treeView.selection.clearSelection();
-    if (selTabID == 'mine_tab') {
+    if (selTabID == "mine_tab") {
       enableBackupAllButton();
     }
   }

security/manager/pki/resources/content/deletecert.js

@@ -4,13 +4,26 @@
 /* import-globals-from pippki.js */
 "use strict";
 
-const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+/**
+ * @file Implements the functionality of deletecert.xul: a dialog that allows a
+ *       user to confirm whether to delete certain certificates.
+ * @argument {String} window.arguments[0]
+ *           One of the tab IDs listed in certManager.xul.
+ * @argument {nsICertTreeItem[]} window.arguments[1]
+ *           An array of cert tree items representing the certs to delete.
+ * @argument {DeleteCertReturnValues} window.arguments[2]
+ *           Object holding the return values of calling the dialog.
+ */
 
 /**
- * Param block to get passed in args and to set return values to.
- * @type nsIDialogParamBlock
+ * @typedef DeleteCertReturnValues
+ * @type Object
+ * @property {Boolean} deleteConfirmed
+ *           Set to true if the user confirmed deletion of the given certs,
+ *           false otherwise.
  */
-var gParams;
+
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
 /**
  * Returns the most appropriate string to represent the given nsICertTreeItem.
@@ -41,11 +54,11 @@ function certTreeItemToString(certTreeItem) {
   return bundle.getFormattedString("certWithSerial", [cert.serialNumber]);
 }
 
-function setWindowName()
-{
-  gParams = window.arguments[0].QueryInterface(Ci.nsIDialogParamBlock);
-
-  let typeFlag = gParams.GetString(0);
+/**
+ * onload() handler.
+ */
+function onLoad() {
+  let typeFlag = window.arguments[0];
   let bundle = document.getElementById("pippki_bundle");
   let title;
   let confirm;
@@ -86,10 +99,10 @@ function setWindowName()
   setText("confirm", confirm);
 
   let box = document.getElementById("certlist");
-  for (let x = 0; x < gParams.objects.length; x++) {
+  let certTreeItems = window.arguments[1];
+  for (let certTreeItem of certTreeItems) {
     let listItem = document.createElement("richlistitem");
     let label = document.createElement("label");
-    let certTreeItem = gParams.objects.queryElementAt(x, Ci.nsICertTreeItem);
     label.setAttribute("value", certTreeItemToString(certTreeItem));
     listItem.appendChild(label);
     box.appendChild(listItem);
@@ -98,14 +111,24 @@ function setWindowName()
   setText("impact", impact);
 }
 
-function doOK()
-{
-  gParams.SetInt(1, 1); // means OK
+/**
+ * ondialogaccept() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogAccept() {
+  let retVals = window.arguments[2];
+  retVals.deleteConfirmed = true;
   return true;
 }
 
-function doCancel()
-{
-  gParams.SetInt(1, 0); // means CANCEL
+/**
+ * ondialogcancel() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogCancel() {
+  let retVals = window.arguments[2];
+  retVals.deleteConfirmed = false;
   return true;
 }

security/manager/pki/resources/content/deletecert.xul

@@ -10,10 +10,10 @@
 <dialog id="deleteCertificate"
   title="&certmgr.deletecert.title;"
   xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
-  onload="setWindowName();"
+  onload="onLoad();"
   buttons="accept,cancel"
-  ondialogaccept="return doOK();"
-  ondialogcancel="return doCancel();">
+  ondialogaccept="return onDialogAccept();"
+  ondialogcancel="return onDialogCancel();">
 
   <stringbundle id="pippki_bundle" src="chrome://pippki/locale/pippki.properties"/>
 

security/manager/ssl/tests/mochitest/browser/browser_deleteCert_ui.js

@@ -11,9 +11,9 @@
 
 /**
  * An array of tree items corresponding to TEST_CASES.
- * @type nsIMutableArray<nsICertTreeItem>
+ * @type nsICertTreeItem[]
  */
-var gCertArray = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
+var gCertArray = [];
 
 const FAKE_HOST_PORT = "Fake host and port";
 
@@ -56,20 +56,18 @@ const TEST_CASES = [
  *          A promise that resolves when the dialog has finished loading, with
  *          an array consisting of:
  *            1. The window of the opened dialog.
- *            2. The nsIDialogParamBlock passed to the dialog.
+ *            2. The return value object passed to the dialog.
  */
 function openDeleteCertConfirmDialog(tabID) {
-  let params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
-                 .createInstance(Ci.nsIDialogParamBlock);
-  params.SetString(0, tabID);
-  params.objects = gCertArray;
-
+  let retVals = {
+    deleteConfirmed: false,
+  };
   let win = window.openDialog("chrome://pippki/content/deletecert.xul", "", "",
-                              params);
+                              tabID, gCertArray, retVals);
   return new Promise((resolve, reject) => {
     win.addEventListener("load", function onLoad() {
       win.removeEventListener("load", onLoad);
-      resolve([win, params]);
+      resolve([win, retVals]);
     });
   });
 }
@@ -91,7 +89,7 @@ add_task(function* setup() {
         throw new Error(Cr.NS_ERROR_NO_INTERFACE);
       }
     };
-    gCertArray.appendElement(certTreeItem, false);
+    gCertArray.push(certTreeItem);
   }
 });
 
@@ -108,7 +106,7 @@ add_task(function* setup() {
  *        Impact the dialog is expected to show.
  */
 function* testHelper(tabID, expectedTitle, expectedConfirmMsg, expectedImpact) {
-  let [win, params] = yield openDeleteCertConfirmDialog(tabID);
+  let [win, retVals] = yield openDeleteCertConfirmDialog(tabID);
   let certList = win.document.getElementById("certlist");
 
   Assert.equal(win.document.title, expectedTitle,
@@ -196,22 +194,22 @@ add_task(function* testDeleteOtherCerts() {
 
 // Test that the right values are returned when the dialog is accepted.
 add_task(function* testAcceptDialogReturnValues() {
-  let [win, params] = yield openDeleteCertConfirmDialog("ca_tab" /*arbitrary*/);
+  let [win, retVals] = yield openDeleteCertConfirmDialog("ca_tab" /*arbitrary*/);
   info("Accepting dialog");
   win.document.getElementById("deleteCertificate").acceptDialog();
   yield BrowserTestUtils.windowClosed(win);
 
-  Assert.equal(params.GetInt(1), 1,
-               "1 should be returned to signal user accepted");
+  Assert.ok(retVals.deleteConfirmed,
+            "Return value should signal user accepted");
 });
 
 // Test that the right values are returned when the dialog is canceled.
 add_task(function* testCancelDialogReturnValues() {
-  let [win, params] = yield openDeleteCertConfirmDialog("ca_tab" /*arbitrary*/);
+  let [win, retVals] = yield openDeleteCertConfirmDialog("ca_tab" /*arbitrary*/);
   info("Canceling dialog");
   win.document.getElementById("deleteCertificate").cancelDialog();
   yield BrowserTestUtils.windowClosed(win);
 
-  Assert.equal(params.GetInt(1), 0,
-               "0 should be returned to signal user canceled");
+  Assert.ok(!retVals.deleteConfirmed,
+            "Return value should signal user did not accept");
 });