Bug 1367453 - Do not throw an exception from ExtensionSettingsStore when trying to remove a setting that was not previously set, r?mixedpuppy draft
authorBob Silverberg <bsilverberg@mozilla.com>
Wed, 24 May 2017 12:29:16 -0400
changeset 583769 df4df74c953ff6e16318b0443a104d19160245b3
parent 583701 d93182f36b3c134a3b1c6718f09eb87c2913e364
child 630196 41e5223f41948b6df6e1e1ef80f4ab12a26e5367
push id60539
push userbmo:bob.silverberg@gmail.com
push dateWed, 24 May 2017 16:30:37 +0000
reviewersmixedpuppy
bugs1367453
milestone55.0a1
Bug 1367453 - Do not throw an exception from ExtensionSettingsStore when trying to remove a setting that was not previously set, r?mixedpuppy This changes the behaviour of ExtensionSettingsStore so that attempting to remove a setting that was not previously set does not throw an exception. This allows things like privacy.network.webRTCIPHandlingPolicy.clear() to be called without having previously called privacy.network.webRTCIPHandlingPolicy.set(). MozReview-Commit-ID: FFCOFHk5lhb
toolkit/components/extensions/ExtensionSettingsStore.jsm
toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js
--- a/toolkit/components/extensions/ExtensionSettingsStore.jsm
+++ b/toolkit/components/extensions/ExtensionSettingsStore.jsm
@@ -127,24 +127,30 @@ function precedenceComparator(a, b) {
  *          the current top precedent setting has not changed.
  */
 async function alterSetting(extension, type, key, action) {
   let returnItem;
   let store = getStore(type);
 
   let keyInfo = store.data[type][key];
   if (!keyInfo) {
+    if (action === "remove") {
+      return null;
+    }
     throw new Error(
       `Cannot alter the setting for ${type}:${key} as it does not exist.`);
   }
 
   let id = extension.id;
   let foundIndex = keyInfo.precedenceList.findIndex(item => item.id == id);
 
   if (foundIndex === -1) {
+    if (action === "remove") {
+      return null;
+    }
     throw new Error(
       `Cannot alter the setting for ${type}:${key} as it does not exist.`);
   }
 
   switch (action) {
     case "remove":
       keyInfo.precedenceList.splice(foundIndex, 1);
       break;
--- a/toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js
@@ -148,22 +148,19 @@ add_task(async function test_settings_st
       "getLevelOfControl returns correct levelOfControl when this extension is in control.");
   }
 
   for (let extension of extensions) {
     let items = await ExtensionSettingsStore.getAllForExtension(extension, TEST_TYPE);
     deepEqual(items, KEY_LIST, "getAllForExtension returns expected keys.");
   }
 
-  // Attempting to remove a setting that has not been set should throw an exception.
-  await Assert.rejects(
-    ExtensionSettingsStore.removeSetting(
-      extensions[0], "myType", "unset_key"),
-    /Cannot alter the setting for myType:unset_key as it does not exist/,
-    "removeSetting rejects with an unset key.");
+  // Attempting to remove a setting that has not been set should *not* throw an exception.
+  let removeResult = await ExtensionSettingsStore.removeSetting(extensions[0], "myType", "unset_key");
+  equal(removeResult, null, "Removing a setting that was not previously set returns null.");
 
   // Attempting to disable a setting that has not been set should throw an exception.
   await Assert.rejects(
     ExtensionSettingsStore.disable(extensions[0], "myType", "unset_key"),
     /Cannot alter the setting for myType:unset_key as it does not exist/,
     "disable rejects with an unset key.");
 
   // Attempting to enable a setting that has not been set should throw an exception.
@@ -361,23 +358,19 @@ add_task(async function test_settings_st
       null,
       "getSetting returns null after all are removed.");
     levelOfControl = await ExtensionSettingsStore.getLevelOfControl(extensions[1], TEST_TYPE, key);
     equal(
       levelOfControl,
       "controllable_by_this_extension",
       "getLevelOfControl returns correct levelOfControl after all are removed.");
 
-    // Attempting to remove a setting that has had all extensions removed should throw an exception.
-    let expectedMessage = new RegExp(`Cannot alter the setting for ${TEST_TYPE}:${key} as it does not exist`);
-    await Assert.rejects(
-      ExtensionSettingsStore.removeSetting(
-        extensions[1], TEST_TYPE, key),
-      expectedMessage,
-      "removeSetting rejects with an key that has all records removed.");
+    // Attempting to remove a setting that has had all extensions removed should *not* throw an exception.
+    removeResult = await ExtensionSettingsStore.removeSetting(extensions[1], TEST_TYPE, key);
+    equal(removeResult, null, "Removing a setting that has had all extensions removed returns null.");
   }
 
   // Test adding a setting with a value in callbackArgument.
   let extensionIndex = 0;
   let testKey = "callbackArgumentKey";
   let callbackArgumentValue = Date.now();
   // Add the setting.
   let item = await ExtensionSettingsStore.addSetting(