Bug 1234020: Part 2b - [webext] Return promises from the cookies APIs. r=evilpie
authorKris Maglione <maglione.k@gmail.com>
Mon, 01 Feb 2016 18:03:37 -0800
changeset 283313 56a0bd3479b00a95c485a382f0c711d9c0a45308
parent 283312 3c8e2be83df67250aca88bb55981c752eb67db73
child 283314 d10ae45a04b90d8d2e82f791471b976969e26fb8
push id29978
push userphilringnalda@gmail.com
push dateSun, 07 Feb 2016 03:05:08 +0000
treeherderautoland@281de9bf9ff6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersevilpie
bugs1234020
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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,
     },