Bug 1234020: Part 2h.1 - [webext] Convert async API errors to rejected promises. r?billm draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 25 Jan 2016 15:35:22 -0800
changeset 328980 085c449680592a756fec6a812a65ce7d7bb40084
parent 328979 bce3ef2ff299cc79e068eb81500086cbc8cd22fd
child 328981 1c959d09485300c54717cb1e846c62a53d4af4fb
push id10445
push usermaglione.k@gmail.com
push dateThu, 04 Feb 2016 21:38:16 +0000
reviewersbillm
bugs1234020
milestone47.0a1
Bug 1234020: Part 2h.1 - [webext] Convert async API errors to rejected promises. r?billm
browser/components/extensions/ext-bookmarks.js
browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
browser/components/extensions/test/browser/browser_ext_contextMenus.js
toolkit/components/extensions/Extension.jsm
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -131,69 +131,53 @@ extensions.registerSchemaAPI("bookmarks"
         }
 
         if (bookmark.parentId !== null) {
           info.parentGuid = bookmark.parentId;
         } else {
           info.parentGuid = Bookmarks.unfiledGuid;
         }
 
-        try {
-          return Bookmarks.insert(info).then(convert);
-        } catch (e) {
-          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
-        }
+        return Bookmarks.insert(info).then(convert);
       },
 
       move: function(id, destination) {
         let info = {
           guid: id,
         };
 
         if (destination.parentId !== null) {
           info.parentGuid = destination.parentId;
         }
         if (destination.index !== null) {
           info.index = destination.index;
         }
 
-        try {
-          return Bookmarks.update(info).then(convert);
-        } catch (e) {
-          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
-        }
+        return Bookmarks.update(info).then(convert);
       },
 
       update: function(id, changes) {
         let info = {
           guid: id,
         };
 
         if (changes.title !== null) {
           info.title = changes.title;
         }
         if (changes.url !== null) {
           info.url = changes.url;
         }
 
-        try {
-          return Bookmarks.update(info).then(convert);
-        } catch (e) {
-          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
-        }
+        return Bookmarks.update(info).then(convert);
       },
 
       remove: function(id) {
         let info = {
           guid: id,
         };
 
         // The API doesn't give you the old bookmark at the moment
-        try {
-          return Bookmarks.remove(info).then(result => {});
-        } catch (e) {
-          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
-        }
+        return Bookmarks.remove(info).then(result => {});
       },
     },
   };
 });
 
--- a/browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
+++ b/browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
@@ -228,26 +228,31 @@ add_task(function* testInvalidIconSizes(
       "browser_action": {},
       "page_action": {},
     },
 
     background: function() {
       browser.tabs.query({ active: true, currentWindow: true }, tabs => {
         let tabId = tabs[0].id;
 
+        let promises = [];
         for (let api of ["pageAction", "browserAction"]) {
           // helper function to run setIcon and check if it fails
           let assertSetIconThrows = function(detail, error, message) {
+            detail.tabId = tabId;
             try {
-              detail.tabId = tabId;
-              browser[api].setIcon(detail);
-
-              browser.test.fail("Expected an error on invalid icon size.");
-              browser.test.notifyFail("setIcon with invalid icon size");
-              return;
+              promises.push(
+                browser[api].setIcon(detail).then(
+                  () => {
+                    browser.test.fail("Expected an error on invalid icon size.");
+                    browser.test.notifyFail("setIcon with invalid icon size");
+                  },
+                  error => {
+                    browser.test.succeed("setIcon with invalid icon size");
+                  }));
             } catch (e) {
               browser.test.succeed("setIcon with invalid icon size");
             }
           };
 
           // test invalid icon size inputs
           for (let type of ["path", "imageData"]) {
             assertSetIconThrows({ [type]: { "abcdef": "test.png" } });
@@ -260,17 +265,19 @@ add_task(function* testInvalidIconSizes(
               "5": "test.png"
             }});
           }
 
           assertSetIconThrows({ imageData: { "abcdef": "test.png" }, path: {"5": "test.png"} });
           assertSetIconThrows({ path: { "abcdef": "test.png" }, imageData: {"5": "test.png"} });
         }
 
-        browser.test.notifyPass("setIcon with invalid icon size");
+        Promise.all(promises).then(() => {
+          browser.test.notifyPass("setIcon with invalid icon size");
+        });
       });
     }
   });
 
   yield Promise.all([extension.startup(), extension.awaitFinish("setIcon with invalid icon size")]);
 
   yield extension.unload();
 });
@@ -342,35 +349,34 @@ add_task(function* testSecureURLsDenied(
 
     background: function() {
       browser.tabs.query({ active: true, currentWindow: true }, tabs => {
         let tabId = tabs[0].id;
 
         let urls = ["chrome://browser/content/browser.xul",
                     "javascript:true"];
 
+        let promises = [];
         for (let url of urls) {
           for (let api of ["pageAction", "browserAction"]) {
-            try {
-              browser[api].setIcon({tabId, path: url});
-
-              browser.test.fail(`Load of '${url}' succeeded. Expected failure.`);
-              browser.test.notifyFail("setIcon security tests");
-              return;
-            } catch (e) {
-              // We can't actually inspect the error here, since the
-              // error object belongs to the privileged scope of the API,
-              // rather than to the extension scope that calls into it.
-              // Just assume it's the expected security error, for now.
-              browser.test.succeed(`Load of '${url}' failed. Expected failure.`);
-            }
+            promises.push(
+              browser[api].setIcon({tabId, path: url}).then(
+                () => {
+                  browser.test.fail(`Load of '${url}' succeeded. Expected failure.`);
+                  browser.test.notifyFail("setIcon security tests");
+                },
+                error => {
+                  browser.test.succeed(`Load of '${url}' failed. Expected failure. ${error}`);
+                }));
           }
         }
 
-        browser.test.notifyPass("setIcon security tests");
+        Promise.all(promises).then(() => {
+          browser.test.notifyPass("setIcon security tests");
+        });
       });
     },
   });
 
   yield extension.startup();
 
   yield extension.awaitFinish("setIcon security tests");
   yield extension.unload();
--- a/browser/components/extensions/test/browser/browser_ext_contextMenus.js
+++ b/browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ -49,22 +49,23 @@ add_task(function* () {
 
       let parentToDel = browser.contextMenus.create({ title: "parentToDel" });
       browser.contextMenus.create(
         { title: "child1", parentId: parentToDel, onclick: genericOnClick });
       browser.contextMenus.create(
         { title: "child2", parentId: parentToDel, onclick: genericOnClick });
       browser.contextMenus.remove(parentToDel);
 
-      try {
-        browser.contextMenus.update(parent, { parentId: child2 });
-        browser.test.notifyFail();
-      } catch (e) {
-        browser.test.notifyPass();
-      }
+      browser.contextMenus.update(parent, { parentId: child2 }).then(
+        () => {
+          browser.test.notifyFail();
+        },
+        () => {
+          browser.test.notifyPass();
+        });
     },
   });
 
   let expectedClickInfo;
   function checkClickInfo(info) {
     info = JSON.parse(info);
     for (let i in expectedClickInfo) {
       is(info[i], expectedClickInfo[i],
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -377,19 +377,16 @@ GlobalManager = {
 
             let promise;
             try {
               // TODO: Stop passing the callback once all APIs return
               // promises.
               promise = schemaApi[ns][name](...args, callback);
             } catch (e) {
               promise = Promise.reject(e);
-              // TODO: Certain tests are still expecting API methods to
-              // throw errors.
-              throw e;
             }
 
             // TODO: This check should no longer be necessary
             // once all async methods return promises.
             if (promise) {
               return context.wrapPromise(promise, callback);
             }
             return undefined;