Bug 1258139: Part 1 - [webext] Refactor storage API code. r?rpl draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 25 Mar 2016 01:03:30 +0100
changeset 344619 01b032741b88c3858b4b42b72be3cbf34fe9e2bc
parent 344154 1438f8e8639506fe605ddcd6a4576dddb8112a06
child 344620 7103fe5bfd80c7395a6e4d30dc94755720066a78
push id13883
push usermaglione.k@gmail.com
push dateFri, 25 Mar 2016 00:17:17 +0000
reviewersrpl
bugs1258139
milestone48.0a1
Bug 1258139: Part 1 - [webext] Refactor storage API code. r?rpl MozReview-Commit-ID: L6cO7mVnN5E
toolkit/components/extensions/ExtensionStorage.jsm
toolkit/components/extensions/ext-storage.js
toolkit/components/extensions/schemas/storage.json
toolkit/components/extensions/test/mochitest/test_ext_storage.html
--- a/toolkit/components/extensions/ExtensionStorage.jsm
+++ b/toolkit/components/extensions/ExtensionStorage.jsm
@@ -87,41 +87,33 @@ this.ExtensionStorage = {
 
       return this.write(extensionId);
     });
   },
 
   remove(extensionId, items) {
     return this.read(extensionId).then(extData => {
       let changes = {};
-      if (Array.isArray(items)) {
-        for (let prop of items) {
-          changes[prop] = {oldValue: extData[prop]};
-          delete extData[prop];
-        }
-      } else {
-        let prop = items;
+      for (let prop of [].concat(items)) {
         changes[prop] = {oldValue: extData[prop]};
         delete extData[prop];
       }
 
       this.notifyListeners(extensionId, changes);
 
       return this.write(extensionId);
     });
   },
 
   clear(extensionId) {
     return this.read(extensionId).then(extData => {
       let changes = {};
-      if (extData) {
-        for (let prop of Object.keys(extData)) {
-          changes[prop] = {oldValue: extData[prop]};
-          delete extData[prop];
-        }
+      for (let prop of Object.keys(extData)) {
+        changes[prop] = {oldValue: extData[prop]};
+        delete extData[prop];
       }
 
       this.notifyListeners(extensionId, changes);
 
       return this.write(extensionId);
     });
   },
 
@@ -133,23 +125,18 @@ this.ExtensionStorage = {
       } else if (typeof(keys) == "object" && !Array.isArray(keys)) {
         for (let prop in keys) {
           if (prop in extData) {
             result[prop] = extData[prop];
           } else {
             result[prop] = keys[prop];
           }
         }
-      } else if (typeof(keys) == "string") {
-        let prop = keys;
-        if (prop in extData) {
-          result[prop] = extData[prop];
-        }
       } else {
-        for (let prop of keys) {
+        for (let prop of [].concat(keys)) {
           if (prop in extData) {
             result[prop] = extData[prop];
           }
         }
       }
 
       return result;
     });
--- a/toolkit/components/extensions/ext-storage.js
+++ b/toolkit/components/extensions/ext-storage.js
@@ -9,31 +9,27 @@ Cu.import("resource://gre/modules/Extens
 var {
   EventManager,
 } = ExtensionUtils;
 
 extensions.registerSchemaAPI("storage", "storage", (extension, context) => {
   return {
     storage: {
       local: {
-        get: function(keys, callback) {
-          return context.wrapPromise(
-            ExtensionStorage.get(extension.id, keys), callback);
+        get: function(keys) {
+          return ExtensionStorage.get(extension.id, keys);
         },
-        set: function(items, callback) {
-          return context.wrapPromise(
-            ExtensionStorage.set(extension.id, items), callback);
+        set: function(items) {
+          return ExtensionStorage.set(extension.id, items);
         },
-        remove: function(items, callback) {
-          return context.wrapPromise(
-            ExtensionStorage.remove(extension.id, items), callback);
+        remove: function(items) {
+          return ExtensionStorage.remove(extension.id, items);
         },
-        clear: function(callback) {
-          return context.wrapPromise(
-            ExtensionStorage.clear(extension.id), callback);
+        clear: function() {
+          return ExtensionStorage.clear(extension.id);
         },
       },
 
       onChanged: new EventManager(context, "storage.local.onChanged", fire => {
         let listener = changes => {
           fire(changes, "local");
         };
 
--- a/toolkit/components/extensions/schemas/storage.json
+++ b/toolkit/components/extensions/schemas/storage.json
@@ -26,16 +26,17 @@
       {
         "id": "StorageArea",
         "type": "object",
         "functions": [
           {
             "name": "get",
             "type": "function",
             "description": "Gets one or more items from storage.",
+            "async": "callback",
             "parameters": [
               {
                 "name": "keys",
                 "choices": [
                   { "type": "string" },
                   { "type": "array", "items": { "type": "string" } },
                   {
                     "type": "object",
@@ -61,16 +62,17 @@
               }
             ]
           },
           {
             "name": "getBytesInUse",
             "unsupported": true,
             "type": "function",
             "description": "Gets the amount of space (in bytes) being used by one or more items.",
+            "async": "callback",
             "parameters": [
               {
                 "name": "keys",
                 "choices": [
                   { "type": "string" },
                   { "type": "array", "items": { "type": "string" } }
                 ],
                 "description": "A single key or list of keys to get the total usage for. An empty list will return 0. Pass in <code>null</code> to get the total usage of all of storage.",
@@ -89,16 +91,17 @@
                 ]
               }
             ]
           },
           {
             "name": "set",
             "type": "function",
             "description": "Sets multiple items.",
+            "async": "callback",
             "parameters": [
               {
                 "name": "items",
                 "type": "object",
                 "additionalProperties": { "type": "any" },
                 "description": "<p>An object which gives each key/value pair to update storage with. Any other key/value pairs in storage will not be affected.</p><p>Primitive values such as numbers will serialize as expected. Values with a <code>typeof</code> <code>\"object\"</code> and <code>\"function\"</code> will typically serialize to <code>{}</code>, with the exception of <code>Array</code> (serializes as expected), <code>Date</code>, and <code>Regex</code> (serialize using their <code>String</code> representation).</p>"
               },
               {
@@ -109,16 +112,17 @@
                 "optional": true
               }
             ]
           },
           {
             "name": "remove",
             "type": "function",
             "description": "Removes one or more items from storage.",
+            "async": "callback",
             "parameters": [
               {
                 "name": "keys",
                 "choices": [
                   {"type": "string"},
                   {"type": "array", "items": {"type": "string"}}
                 ],
                 "description": "A single key or a list of keys for items to remove."
@@ -131,16 +135,17 @@
                 "optional": true
               }
             ]
           },
           {
             "name": "clear",
             "type": "function",
             "description": "Removes all items from storage.",
+            "async": "callback",
             "parameters": [
               {
                 "name": "callback",
                 "type": "function",
                 "description": "Callback on success, or on failure (in which case $(ref:runtime.lastError) will be set).",
                 "parameters": [],
                 "optional": true
               }
--- a/toolkit/components/extensions/test/mochitest/test_ext_storage.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_storage.html
@@ -9,59 +9,36 @@
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
 </head>
 <body>
 
 <script type="text/javascript">
 "use strict";
 
 function backgroundScript() {
-  function set(items) {
-    return new Promise(resolve => {
-      browser.storage.local.set(items, resolve);
-    });
-  }
-
-  function get(items) {
-    return new Promise(resolve => {
-      browser.storage.local.get(items, resolve);
-    });
-  }
-
-  function remove(items) {
-    return new Promise(resolve => {
-      browser.storage.local.remove(items, resolve);
-    });
-  }
-
-  function clear(items) {
-    return new Promise(resolve => {
-      browser.storage.local.clear(resolve);
-    });
-  }
-
+  let storage = browser.storage.local;
   function check(prop, value) {
-    return get(null).then(data => {
-      browser.test.assertEq(data[prop], value, "null getter worked for " + prop);
-      return get(prop);
+    return storage.get(null).then(data => {
+      browser.test.assertEq(value, data[prop], "null getter worked for " + prop);
+      return storage.get(prop);
     }).then(data => {
-      browser.test.assertEq(data[prop], value, "string getter worked for " + prop);
-      return get([prop]);
+      browser.test.assertEq(value, data[prop], "string getter worked for " + prop);
+      return storage.get([prop]);
     }).then(data => {
-      browser.test.assertEq(data[prop], value, "array getter worked for " + prop);
-      return get({[prop]: undefined});
+      browser.test.assertEq(value, data[prop], "array getter worked for " + prop);
+      return storage.get({[prop]: undefined});
     }).then(data => {
-      browser.test.assertEq(data[prop], value, "object getter worked for " + prop);
+      browser.test.assertEq(value, data[prop], "object getter worked for " + prop);
     });
   }
 
   let globalChanges = {};
 
   browser.storage.onChanged.addListener((changes, storage) => {
-    browser.test.assertEq(storage, "local", "storage is local");
+    browser.test.assertEq("local", storage, "storage is local");
     Object.assign(globalChanges, changes);
   });
 
   function checkChanges(changes) {
     function checkSub(obj1, obj2) {
       for (let prop in obj1) {
         browser.test.assertEq(obj1[prop].oldValue, obj2[prop].oldValue);
         browser.test.assertEq(obj1[prop].newValue, obj2[prop].newValue);
@@ -71,120 +48,126 @@ function backgroundScript() {
     checkSub(changes, globalChanges);
     checkSub(globalChanges, changes);
     globalChanges = {};
   }
 
   /* eslint-disable dot-notation */
 
   // Set some data and then test getters.
-  set({"test-prop1": "value1", "test-prop2": "value2"}).then(() => {
+  storage.set({"test-prop1": "value1", "test-prop2": "value2"}).then(() => {
     checkChanges({"test-prop1": {newValue: "value1"}, "test-prop2": {newValue: "value2"}});
     return check("test-prop1", "value1");
   }).then(() => {
     return check("test-prop2", "value2");
   }).then(() => {
-    return get({"test-prop1": undefined, "test-prop2": undefined, "other": "default"});
+    return storage.get({"test-prop1": undefined, "test-prop2": undefined, "other": "default"});
   }).then(data => {
-    browser.test.assertEq(data["test-prop1"], "value1", "prop1 correct");
-    browser.test.assertEq(data["test-prop2"], "value2", "prop2 correct");
-    browser.test.assertEq(data["other"], "default", "other correct");
-    return get(["test-prop1", "test-prop2", "other"]);
+    browser.test.assertEq("value1", data["test-prop1"], "prop1 correct");
+    browser.test.assertEq("value2", data["test-prop2"], "prop2 correct");
+    browser.test.assertEq("default", data["other"], "other correct");
+    return storage.get(["test-prop1", "test-prop2", "other"]);
   }).then(data => {
-    browser.test.assertEq(data["test-prop1"], "value1", "prop1 correct");
-    browser.test.assertEq(data["test-prop2"], "value2", "prop2 correct");
+    browser.test.assertEq("value1", data["test-prop1"], "prop1 correct");
+    browser.test.assertEq("value2", data["test-prop2"], "prop2 correct");
     browser.test.assertFalse("other" in data, "other correct");
 
   // Remove data in various ways.
   }).then(() => {
-    return remove("test-prop1");
+    return storage.remove("test-prop1");
   }).then(() => {
     checkChanges({"test-prop1": {oldValue: "value1"}});
-    return get(["test-prop1", "test-prop2"]);
+    return storage.get(["test-prop1", "test-prop2"]);
   }).then(data => {
     browser.test.assertFalse("test-prop1" in data, "prop1 absent");
     browser.test.assertTrue("test-prop2" in data, "prop2 present");
 
-    return set({"test-prop1": "value1"});
+    return storage.set({"test-prop1": "value1"});
   }).then(() => {
     checkChanges({"test-prop1": {newValue: "value1"}});
-    return get(["test-prop1", "test-prop2"]);
+    return storage.get(["test-prop1", "test-prop2"]);
   }).then(data => {
-    browser.test.assertEq(data["test-prop1"], "value1", "prop1 correct");
-    browser.test.assertEq(data["test-prop2"], "value2", "prop2 correct");
+    browser.test.assertEq("value1", data["test-prop1"], "prop1 correct");
+    browser.test.assertEq("value2", data["test-prop2"], "prop2 correct");
   }).then(() => {
-    return remove(["test-prop1", "test-prop2"]);
+    return storage.remove(["test-prop1", "test-prop2"]);
   }).then(() => {
     checkChanges({"test-prop1": {oldValue: "value1"}, "test-prop2": {oldValue: "value2"}});
-    return get(["test-prop1", "test-prop2"]);
+    return storage.get(["test-prop1", "test-prop2"]);
   }).then(data => {
     browser.test.assertFalse("test-prop1" in data, "prop1 absent");
     browser.test.assertFalse("test-prop2" in data, "prop2 absent");
 
   // test storage.clear
   }).then(() => {
-    return set({"test-prop1": "value1", "test-prop2": "value2"});
+    return storage.set({"test-prop1": "value1", "test-prop2": "value2"});
   }).then(() => {
-    return clear();
+    return storage.clear();
   }).then(() => {
     checkChanges({"test-prop1": {oldValue: "value1"}, "test-prop2": {oldValue: "value2"}});
-    return get(["test-prop1", "test-prop2"]);
+    return storage.get(["test-prop1", "test-prop2"]);
   }).then(data => {
     browser.test.assertFalse("test-prop1" in data, "prop1 absent");
     browser.test.assertFalse("test-prop2" in data, "prop2 absent");
 
   // Test cache invalidation.
   }).then(() => {
-    return set({"test-prop1": "value1", "test-prop2": "value2"});
+    return storage.set({"test-prop1": "value1", "test-prop2": "value2"});
   }).then(() => {
     globalChanges = {};
     browser.test.sendMessage("invalidate");
     return new Promise(resolve => browser.test.onMessage.addListener(resolve));
   }).then(() => {
     return check("test-prop1", "value1");
   }).then(() => {
     return check("test-prop2", "value2");
 
   // Make sure we can store complex JSON data.
   }).then(() => {
-    return set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2]}});
+    return storage.set({"test-prop1": {str: "hello", bool: true, undef: undefined, obj: {}, arr: [1, 2]}});
   }).then(() => {
-    browser.test.assertEq(globalChanges["test-prop1"].oldValue, "value1", "oldValue correct");
-    browser.test.assertEq(typeof(globalChanges["test-prop1"].newValue), "object", "newValue is obj");
+    browser.test.assertEq("value1", globalChanges["test-prop1"].oldValue, "oldValue correct");
+    browser.test.assertEq("object", typeof(globalChanges["test-prop1"].newValue), "newValue is obj");
     globalChanges = {};
-    return get({"test-prop1": undefined});
+    return storage.get({"test-prop1": undefined});
   }).then(data => {
     let obj = data["test-prop1"];
 
-    browser.test.assertEq(obj.str, "hello", "string part correct");
-    browser.test.assertEq(obj.bool, true, "bool part correct");
-    browser.test.assertEq(obj.undef, undefined, "undefined part correct");
-    browser.test.assertEq(typeof(obj.obj), "object", "object part correct");
+    browser.test.assertEq("hello", obj.str, "string part correct");
+    browser.test.assertEq(true, obj.bool, "bool part correct");
+    browser.test.assertEq(undefined, obj.undef, "undefined part correct");
+    browser.test.assertEq("object", typeof(obj.obj), "object part correct");
     browser.test.assertTrue(Array.isArray(obj.arr), "array part present");
-    browser.test.assertEq(obj.arr[0], 1, "arr[0] part correct");
-    browser.test.assertEq(obj.arr[1], 2, "arr[1] part correct");
-    browser.test.assertEq(obj.arr.length, 2, "arr.length part correct");
+    browser.test.assertEq(1, obj.arr[0], "arr[0] part correct");
+    browser.test.assertEq(2, obj.arr[1], "arr[1] part correct");
+    browser.test.assertEq(2, obj.arr.length, "arr.length part correct");
   }).then(() => {
     browser.test.notifyPass("storage");
+  }).catch(e => {
+    browser.test.fail(`Error: ${e} :: ${e.stack}`);
+    browser.test.notifyFail("storage");
   });
 }
 
 let extensionData = {
   background: "(" + backgroundScript.toString() + ")()",
   manifest: {
     permissions: ["storage"],
   },
 };
 
-add_task(function* test_contentscript() {
+add_task(function* test_backgroundScript() {
   let extension = ExtensionTestUtils.loadExtension(extensionData);
-  yield Promise.all([extension.startup(), extension.awaitMessage("invalidate")]);
+
+  yield extension.startup();
+
+  yield extension.awaitMessage("invalidate");
   SpecialPowers.invalidateExtensionStorageCache();
   extension.sendMessage("invalidated");
+
   yield extension.awaitFinish("storage");
   yield extension.unload();
-  info("extension unloaded");
 });
 
 </script>
 
 </body>
 </html>