Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r?evilpie draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 01 Feb 2016 18:03:37 -0800
changeset 328973 4ed79019ea977795987b1ab33d0b4729ec83b5d9
parent 328972 67fc3bdf0f225f0fc4192af6623fa5b0cf205ac1
child 328974 bf4d6101b3cb7a7d3b3c06ce0de05552b508089c
push id10445
push usermaglione.k@gmail.com
push dateThu, 04 Feb 2016 21:38:16 +0000
reviewersevilpie
bugs1234020
milestone47.0a1
Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r?evilpie
toolkit/components/extensions/ext-cookies.js
toolkit/components/extensions/schemas/cookies.json
toolkit/components/extensions/test/mochitest/test_ext_cookies.html
toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
--- a/toolkit/components/extensions/ext-cookies.js
+++ b/toolkit/components/extensions/ext-cookies.js
@@ -2,17 +2,16 @@
 
 const { interfaces: Ci, utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 Cu.import("resource://gre/modules/NetUtil.jsm");
 
 var {
   EventManager,
-  runSafe,
 } = ExtensionUtils;
 
 // Cookies from private tabs currently can't be enumerated.
 var DEFAULT_STORE = "firefox-default";
 
 function convert(cookie) {
   let result = {
     name: cookie.name,
@@ -237,35 +236,34 @@ function* query(detailsIn, props, extens
       yield cookie;
     }
   }
 }
 
 extensions.registerSchemaAPI("cookies", "cookies", (extension, context) => {
   let self = {
     cookies: {
-      get: function(details, callback) {
+      get: function(details) {
         // FIXME: We don't sort by length of path and creation time.
         for (let cookie of query(details, ["url", "name", "storeId"], extension)) {
-          runSafe(context, callback, convert(cookie));
-          return;
+          return Promise.resolve(convert(cookie));
         }
 
         // Found no match.
-        runSafe(context, callback, null);
+        return Promise.resolve(null);
       },
 
-      getAll: function(details, callback) {
+      getAll: function(details) {
         let allowed = ["url", "name", "domain", "path", "secure", "session", "storeId"];
         let result = Array.from(query(details, allowed, extension), convert);
 
-        runSafe(context, callback, result);
+        return Promise.resolve(result);
       },
 
-      set: function(details, callback) {
+      set: function(details) {
         let uri = NetUtil.newURI(details.url).QueryInterface(Ci.nsIURL);
 
         let path;
         if (details.path !== null) {
           path = details.path;
         } else {
           // This interface essentially emulates the behavior of the
           // Set-Cookie header. In the case of an omitted path, the cookie
@@ -278,52 +276,45 @@ extensions.registerSchemaAPI("cookies", 
         let value = details.value !== null ? details.value : "";
         let secure = details.secure !== null ? details.secure : false;
         let httpOnly = details.httpOnly !== null ? details.httpOnly : false;
         let isSession = details.expirationDate === null;
         let expiry = isSession ? 0 : details.expirationDate;
         // Ignore storeID.
 
         let cookieAttrs = { host: details.domain, path: path, isSecure: secure };
-        if (checkSetCookiePermissions(extension, uri, cookieAttrs)) {
-          // TODO: Set |lastError| when false.
-          //
-          // The permission check may have modified the domain, so use
-          // the new value instead.
-          Services.cookies.add(cookieAttrs.host, path, name, value,
-                               secure, httpOnly, isSession, expiry);
+        if (!checkSetCookiePermissions(extension, uri, cookieAttrs)) {
+          return Promise.reject({message: `Permission denied to set cookie ${JSON.stringify(details)}`});
         }
 
-        if (callback) {
-          self.cookies.get(details, callback);
-        }
+        // The permission check may have modified the domain, so use
+        // the new value instead.
+        Services.cookies.add(cookieAttrs.host, path, name, value,
+                             secure, httpOnly, isSession, expiry);
+
+        return self.cookies.get(details);
       },
 
-      remove: function(details, callback) {
+      remove: function(details) {
         for (let cookie of query(details, ["url", "name", "storeId"], extension)) {
           Services.cookies.remove(cookie.host, cookie.name, cookie.path, false);
-          if (callback) {
-            runSafe(context, callback, {
-              url: details.url,
-              name: details.name,
-              storeId: DEFAULT_STORE,
-            });
-          }
           // Todo: could there be multiple per subdomain?
-          return;
+          return Promise.resolve({
+            url: details.url,
+            name: details.name,
+            storeId: DEFAULT_STORE,
+          });
         }
 
-        if (callback) {
-          runSafe(context, callback, null);
-        }
+        return Promise.resolve(null);
       },
 
-      getAllCookieStores: function(callback) {
+      getAllCookieStores: function() {
         // Todo: list all the tabIds for non-private tabs
-        runSafe(context, callback, [{id: DEFAULT_STORE, tabIds: []}]);
+        return Promise.resolve([{id: DEFAULT_STORE, tabIds: []}]);
       },
 
       onChanged: new EventManager(context, "cookies.onChanged", fire => {
         let observer = (subject, topic, data) => {
           let notify = (removed, cookie, cause) => {
             cookie.QueryInterface(Ci.nsICookie2);
 
             if (extension.whiteListedHosts.matchesCookie(cookie)) {
--- a/toolkit/components/extensions/schemas/cookies.json
+++ b/toolkit/components/extensions/schemas/cookies.json
@@ -54,16 +54,17 @@
         "description": "The underlying reason behind the cookie's change. If a cookie was inserted, or removed via an explicit call to $(ref:cookies.remove), \"cause\" will be \"explicit\". If a cookie was automatically removed due to expiry, \"cause\" will be \"expired\". If a cookie was removed due to being overwritten with an already-expired expiration date, \"cause\" will be set to \"expired_overwrite\".  If a cookie was automatically removed due to garbage collection, \"cause\" will be \"evicted\".  If a cookie was automatically removed due to a \"set\" call that overwrote it, \"cause\" will be \"overwrite\". Plan your response accordingly."
       }
     ],
     "functions": [
       {
         "name": "get",
         "type": "function",
         "description": "Retrieves information about a single cookie. If more than one cookie of the same name exists for the given URL, the one with the longest path will be returned. For cookies with the same path length, the cookie with the earliest creation time will be returned.",
+        "async": "callback",
         "parameters": [
           {
             "type": "object",
             "name": "details",
             "description": "Details to identify the cookie being retrieved.",
             "properties": {
               "url": {"type": "string", "description": "The URL with which the cookie to retrieve is associated. This argument may be a full URL, in which case any data following the URL path (e.g. the query string) is simply ignored. If host permissions for this URL are not specified in the manifest file, the API call will fail."},
               "name": {"type": "string", "description": "The name of the cookie to retrieve."},
@@ -80,16 +81,17 @@
             ]
           }
         ]
       },
       {
         "name": "getAll",
         "type": "function",
         "description": "Retrieves all cookies from a single cookie store that match the given information.  The cookies returned will be sorted, with those with the longest path first.  If multiple cookies have the same path length, those with the earliest creation time will be first.",
+        "async": "callback",
         "parameters": [
           {
             "type": "object",
             "name": "details",
             "description": "Information to filter the cookies being retrieved.",
             "properties": {
               "url": {"type": "string", "optional": true, "description": "Restricts the retrieved cookies to those that would match the given URL."},
               "name": {"type": "string", "optional": true, "description": "Filters the cookies by name."},
@@ -110,16 +112,17 @@
             ]
           }
         ]
       },
       {
         "name": "set",
         "type": "function",
         "description": "Sets a cookie with the given cookie data; may overwrite equivalent cookies if they exist.",
+        "async": "callback",
         "parameters": [
           {
             "type": "object",
             "name": "details",
             "description": "Details about the cookie being set.",
             "properties": {
               "url": {"type": "string", "description": "The request-URI to associate with the setting of the cookie. This value can affect the default domain and path values of the created cookie. If host permissions for this URL are not specified in the manifest file, the API call will fail."},
               "name": {"type": "string", "optional": true, "description": "The name of the cookie. Empty by default if omitted."},
@@ -143,16 +146,17 @@
             ]
           }
         ]
       },
       {
         "name": "remove",
         "type": "function",
         "description": "Deletes a cookie by name.",
+        "async": "callback",
         "parameters": [
           {
             "type": "object",
             "name": "details",
             "description": "Information to identify the cookie to remove.",
             "properties": {
               "url": {"type": "string", "description": "The URL associated with the cookie. If host permissions for this URL are not specified in the manifest file, the API call will fail."},
               "name": {"type": "string", "description": "The name of the cookie to remove."},
@@ -178,16 +182,17 @@
             ]
           }
         ]
       },
       {
         "name": "getAllCookieStores",
         "type": "function",
         "description": "Lists all existing cookie stores.",
+        "async": "callback",
         "parameters": [
           {
             "type": "function",
             "name": "callback",
             "parameters": [
               {
                 "name": "cookieStores", "type": "array", "items": {"$ref": "CookieStore"}, "description": "All the existing cookie stores."
               }
--- a/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
@@ -9,46 +9,16 @@
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
 </head>
 <body>
 
 <script type="text/javascript">
 "use strict";
 
 function backgroundScript() {
-  function get(details) {
-    return new Promise(resolve => {
-      browser.cookies.get(details, resolve);
-    });
-  }
-
-  function getAll(details) {
-    return new Promise(resolve => {
-      browser.cookies.getAll(details, resolve);
-    });
-  }
-
-  function set(details) {
-    return new Promise(resolve => {
-      browser.cookies.set(details, resolve);
-    });
-  }
-
-  function remove(details) {
-    return new Promise(resolve => {
-      browser.cookies.remove(details, resolve);
-    });
-  }
-
-  function getAllCookieStores() {
-    return new Promise(resolve => {
-      browser.cookies.getAllCookieStores(resolve);
-    });
-  }
-
   function assertExpected(cookie, expected) {
     for (let key of Object.keys(cookie)) {
       browser.test.assertTrue(key in expected, "found property " + key);
       browser.test.assertEq(cookie[key], expected[key], "property value for " + key + " is wrong");
     }
     browser.test.assertEq(Object.keys(cookie).length, Object.keys(expected).length, "all expected properties found");
   }
 
@@ -63,40 +33,40 @@ function backgroundScript() {
     path: "/",
     secure: false,
     httpOnly: false,
     session: false,
     expirationDate: THE_FUTURE,
     storeId: "firefox-default",
   };
 
-  set({url: TEST_URL, name: "name1", value: "value1", expirationDate: THE_FUTURE}).then(cookie => {
+  browser.cookies.set({url: TEST_URL, name: "name1", value: "value1", expirationDate: THE_FUTURE}).then(cookie => {
     assertExpected(cookie, expected);
-    return get({url: TEST_URL, name: "name1"});
+    return browser.cookies.get({url: TEST_URL, name: "name1"});
   }).then(cookie => {
     assertExpected(cookie, expected);
-    return getAll({domain: "example.org"});
+    return browser.cookies.getAll({domain: "example.org"});
   }).then(cookies => {
     browser.test.assertEq(cookies.length, 1, "only found one cookie for example.org");
     assertExpected(cookies[0], expected);
-    return remove({url: TEST_URL, name: "name1"});
+    return browser.cookies.remove({url: TEST_URL, name: "name1"});
   }).then(details => {
     assertExpected(details, {url: TEST_URL, name: "name1", storeId: "firefox-default"});
-    return get({url: TEST_URL, name: "name1"});
+    return browser.cookies.get({url: TEST_URL, name: "name1"});
   }).then(cookie => {
     browser.test.assertEq(cookie, null);
-    return getAllCookieStores();
+    return browser.cookies.getAllCookieStores();
   }).then(stores => {
     browser.test.assertEq(stores.length, 1);
     browser.test.assertEq(stores[0].id, "firefox-default");
     browser.test.assertEq(stores[0].tabIds.length, 0); // Todo: Implement this.
-    return set({url: TEST_URL, name: "name2", domain: ".example.org", expirationDate: THE_FUTURE});
+    return browser.cookies.set({url: TEST_URL, name: "name2", domain: ".example.org", expirationDate: THE_FUTURE});
   }).then(cookie => {
     browser.test.assertEq(cookie.hostOnly, false, "not a hostOnly cookie");
-    return remove({url: TEST_URL, name: "name2"});
+    return browser.cookies.remove({url: TEST_URL, name: "name2"});
   }).then(details => {
     assertExpected(details, {url: TEST_URL, name: "name2", storeId: "firefox-default"});
   }).then(() => {
     browser.test.notifyPass("cookies");
   });
 }
 
 let extensionData = {
--- a/toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_cookies_permissions.html
@@ -14,40 +14,16 @@
 "use strict";
 
 function* testCookies(options) {
   // Changing the options object is a bit of a hack, but it allows us to easily
   // pass an expiration date to the background script.
   options.expiry = Date.now() / 1000 + 3600;
 
   function background(options) {
-    function get(details) {
-      return new Promise(resolve => {
-        browser.cookies.get(details, resolve);
-      });
-    }
-
-    function getAll(details) {
-      return new Promise(resolve => {
-        browser.cookies.getAll(details, resolve);
-      });
-    }
-
-    function set(details) {
-      return new Promise(resolve => {
-        browser.cookies.set(details, resolve);
-      });
-    }
-
-    function remove(details) {
-      return new Promise(resolve => {
-        browser.cookies.remove(details, resolve);
-      });
-    }
-
     // Ask the parent scope to change some cookies we may or may not have
     // permission for.
     let awaitChanges = new Promise(resolve => {
       browser.test.onMessage.addListener(msg => {
         browser.test.assertEq("cookies-changed", msg, "browser.test.onMessage");
         resolve();
       });
     });
@@ -57,33 +33,38 @@ function* testCookies(options) {
       changed.push(`${event.cookie.name}:${event.cause}`);
     });
     browser.test.sendMessage("change-cookies");
 
 
     // Try to access some cookies in various ways.
     let { url, domain, secure } = options;
 
+    let failures = 0;
+    let tallyFailure = error => {
+      failures++;
+    };
+
     awaitChanges.then(() => {
-      return get({ url, name: "foo" });
+      return browser.cookies.get({ url, name: "foo" });
     }).then(cookie => {
       browser.test.assertEq(options.shouldPass, cookie != null, "should pass == get cookie");
 
-      return getAll({ domain });
+      return browser.cookies.getAll({ domain });
     }).then(cookies => {
       if (options.shouldPass) {
         browser.test.assertEq(2, cookies.length, "expected number of cookies");
       } else {
         browser.test.assertEq(0, cookies.length, "expected number of cookies");
       }
 
       return Promise.all([
-        set({ url, domain, secure, name: "foo", "value": "baz", expirationDate: options.expiry }),
-        set({ url, domain, secure, name: "bar", "value": "quux", expirationDate: options.expiry }),
-        remove({ url, name: "deleted" }),
+        browser.cookies.set({ url, domain, secure, name: "foo", "value": "baz", expirationDate: options.expiry }).catch(tallyFailure),
+        browser.cookies.set({ url, domain, secure, name: "bar", "value": "quux", expirationDate: options.expiry }).catch(tallyFailure),
+        browser.cookies.remove({ url, name: "deleted" }),
       ]);
     }).then(() => {
       if (options.shouldPass) {
         // The order of eviction events isn't guaranteed, so just check that
         // it's there somewhere.
         let evicted = changed.indexOf("evicted:evicted");
         if (evicted < 0) {
           browser.test.fail("got no eviction event");
@@ -94,16 +75,24 @@ function* testCookies(options) {
 
         browser.test.assertEq("x:explicit,x:overwrite,x:explicit,foo:overwrite,bar:explicit,deleted:explicit",
                               changed.join(","), "expected changes");
       } else {
         browser.test.assertEq("", changed.join(","), "expected no changes");
       }
 
       browser.test.notifyPass("cookie-permissions");
+    }).then(() => {
+      if (!(options.shouldPass || options.shouldWrite)) {
+        browser.test.assertEq(2, failures, "Expected failures");
+      } else {
+        browser.test.assertEq(0, failures, "Expected no failures");
+      }
+    }).catch(error => {
+      browser.test.fail(`Error: ${error} :: ${error.stack}`);
     });
   }
 
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       "permissions": options.permissions,
     },