Bug 1234020: Part 2f - [webext] Return promises from the bookmarks API. r=evilpie draft
authorKris Maglione <maglione.k@gmail.com>
Mon, 01 Feb 2016 18:12:45 -0800
changeset 328977 17c48c04f53eee3efea78d6f294530d77c3611e8
parent 328976 aa95c195026cfe0145ed87b3a796c861d40f45f8
child 328978 83846197d4562d443e2fda56b4f0ab3132e6dd2b
push id10445
push usermaglione.k@gmail.com
push dateThu, 04 Feb 2016 21:38:16 +0000
reviewersevilpie
bugs1234020
milestone47.0a1
Bug 1234020: Part 2f - [webext] Return promises from the bookmarks API. r=evilpie
browser/components/extensions/ext-bookmarks.js
browser/components/extensions/schemas/bookmarks.json
toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -3,19 +3,16 @@
 "use strict";
 
 const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 var Bookmarks = PlacesUtils.bookmarks;
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
-var {
-  runSafe,
-} = ExtensionUtils;
 
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                   "resource://gre/modules/Task.jsm");
 
 function getTree(rootGuid, onlyChildren) {
   function convert(node, parent) {
     let treenode = {
       id: node.guid,
@@ -80,68 +77,48 @@ function convert(result) {
   }
 
   return node;
 }
 
 extensions.registerSchemaAPI("bookmarks", "bookmarks", (extension, context) => {
   return {
     bookmarks: {
-      get: function(idOrIdList, callback) {
+      get: function(idOrIdList) {
         let list = Array.isArray(idOrIdList) ? idOrIdList : [idOrIdList];
 
-        Task.spawn(function* () {
+        return Task.spawn(function* () {
           let bookmarks = [];
           for (let id of list) {
-            let bookmark;
-            try {
-              bookmark = yield Bookmarks.fetch({guid: id});
-              if (!bookmark) {
-                // TODO: set lastError, not found
-                return [];
-              }
-            } catch (e) {
-              // TODO: set lastError, probably an invalid guid
-              return [];
+            let bookmark = yield Bookmarks.fetch({guid: id});
+            if (!bookmark) {
+              throw new Error("Bookmark not found");
             }
             bookmarks.push(convert(bookmark));
           }
           return bookmarks;
-        }).then(results => runSafe(context, callback, results));
-      },
-
-      getChildren: function(id, callback) {
-        // TODO: We should optimize this.
-        getTree(id, true).then(result => {
-          runSafe(context, callback, result);
-        }, reason => {
-          // TODO: Set lastError
-          runSafe(context, callback, []);
         });
       },
 
-      getTree: function(callback) {
-        getTree(Bookmarks.rootGuid, false).then(result => {
-          runSafe(context, callback, result);
-        }, reason => {
-          runSafe(context, callback, []);
-        });
+      getChildren: function(id) {
+        // TODO: We should optimize this.
+        return getTree(id, true);
       },
 
-      getSubTree: function(id, callback) {
-        getTree(id, false).then(result => {
-          runSafe(context, callback, result);
-        }, reason => {
-          runSafe(context, callback, []);
-        });
+      getTree: function() {
+        return getTree(Bookmarks.rootGuid, false);
+      },
+
+      getSubTree: function(id) {
+        return getTree(id, false);
       },
 
       // search
 
-      create: function(bookmark, callback) {
+      create: function(bookmark) {
         let info = {
           title: bookmark.title || "",
         };
 
         // If url is NULL or missing, it will be a folder.
         if (bookmark.url !== null) {
           info.type = Bookmarks.TYPE_BOOKMARK;
           info.url = bookmark.url || "";
@@ -154,111 +131,69 @@ extensions.registerSchemaAPI("bookmarks"
         }
 
         if (bookmark.parentId !== null) {
           info.parentGuid = bookmark.parentId;
         } else {
           info.parentGuid = Bookmarks.unfiledGuid;
         }
 
-        let failure = reason => {
-          // TODO: set lastError.
-          if (callback) {
-            runSafe(context, callback, null);
-          }
-        };
-
         try {
-          Bookmarks.insert(info).then(result => {
-            if (callback) {
-              runSafe(context, callback, convert(result));
-            }
-          }, failure);
+          return Bookmarks.insert(info).then(convert);
         } catch (e) {
-          failure(e);
+          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
         }
       },
 
-      move: function(id, destination, callback) {
+      move: function(id, destination) {
         let info = {
           guid: id,
         };
 
         if (destination.parentId !== null) {
           info.parentGuid = destination.parentId;
         }
         if (destination.index !== null) {
           info.index = destination.index;
         }
 
-        let failure = reason => {
-          if (callback) {
-            runSafe(context, callback, null);
-          }
-        };
-
         try {
-          Bookmarks.update(info).then(result => {
-            if (callback) {
-              runSafe(context, callback, convert(result));
-            }
-          }, failure);
+          return Bookmarks.update(info).then(convert);
         } catch (e) {
-          failure(e);
+          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
         }
       },
 
-      update: function(id, changes, callback) {
+      update: function(id, changes) {
         let info = {
           guid: id,
         };
 
         if (changes.title !== null) {
           info.title = changes.title;
         }
         if (changes.url !== null) {
           info.url = changes.url;
         }
 
-        let failure = reason => {
-          if (callback) {
-            runSafe(context, callback, null);
-          }
-        };
-
         try {
-          Bookmarks.update(info).then(result => {
-            if (callback) {
-              runSafe(context, callback, convert(result));
-            }
-          }, failure);
+          return Bookmarks.update(info).then(convert);
         } catch (e) {
-          failure(e);
+          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
         }
       },
 
-      remove: function(id, callback) {
+      remove: function(id) {
         let info = {
           guid: id,
         };
 
-        let failure = reason => {
-          if (callback) {
-            runSafe(context, callback, null);
-          }
-        };
-
+        // The API doesn't give you the old bookmark at the moment
         try {
-          Bookmarks.remove(info).then(result => {
-            if (callback) {
-              // The API doesn't give you the old bookmark at the moment
-              runSafe(context, callback);
-            }
-          }, failure);
+          return Bookmarks.remove(info).then(result => {});
         } catch (e) {
-          failure(e);
+          return Promise.reject({ message: `Invalid bookmark: ${JSON.stringify(info)}` });
         }
       },
     },
   };
 });
 
-
--- a/browser/components/extensions/schemas/bookmarks.json
+++ b/browser/components/extensions/schemas/bookmarks.json
@@ -104,16 +104,17 @@
         }
       }
     ],
     "functions": [
       {
         "name": "get",
         "type": "function",
         "description": "Retrieves the specified BookmarkTreeNode(s).",
+        "async": "callback",
         "parameters": [
           {
             "name": "idOrIdList",
             "description": "A single string-valued id, or an array of string-valued ids",
             "choices": [
               {
                 "type": "string"
               },
@@ -138,16 +139,17 @@
             ]
           }
         ]
       },
       {
         "name": "getChildren",
         "type": "function",
         "description": "Retrieves the children of the specified BookmarkTreeNode id.",
+        "async": "callback",
         "parameters": [
           {
             "type": "string",
             "name": "id"
           },
           {
             "type": "function",
             "name": "callback",
@@ -161,16 +163,17 @@
           }
         ]
       },
       {
         "name": "getRecent",
         "unsupported": true,
         "type": "function",
         "description": "Retrieves the recently added bookmarks.",
+        "async": "callback",
         "parameters": [
           {
             "type": "integer",
             "minimum": 1,
             "name": "numberOfItems",
             "description": "The maximum number of items to return."
           },
           {
@@ -185,16 +188,17 @@
             ]
           }
         ]
       },
       {
         "name": "getTree",
         "type": "function",
         "description": "Retrieves the entire Bookmarks hierarchy.",
+        "async": "callback",
         "parameters": [
           {
             "type": "function",
             "name": "callback",
             "parameters": [
               {
                 "name": "results",
                 "type": "array",
@@ -203,16 +207,17 @@
             ]
           }
         ]
       },
       {
         "name": "getSubTree",
         "type": "function",
         "description": "Retrieves part of the Bookmarks hierarchy, starting at the specified node.",
+        "async": "callback",
         "parameters": [
           {
             "type": "string",
             "name": "id",
             "description": "The ID of the root of the subtree to retrieve."
           },
           {
             "type": "function",
@@ -227,16 +232,17 @@
           }
         ]
       },
       {
         "name": "search",
         "unsupported": true,
         "type": "function",
         "description": "Searches for BookmarkTreeNodes matching the given query. Queries specified with an object produce BookmarkTreeNodes matching all specified properties.",
+        "async": "callback",
         "parameters": [
           {
             "name": "query",
             "description": "Either a string of words and quoted phrases that are matched against bookmark URLs and titles, or an object. If an object, the properties <code>query</code>, <code>url</code>, and <code>title</code> may be specified and bookmarks matching all specified properties will be produced.",
             "choices": [
               {
                 "type": "string",
                 "description": "A string of words and quoted phrases that are matched against bookmark URLs and titles."
@@ -276,16 +282,17 @@
             ]
           }
         ]
       },
       {
         "name": "create",
         "type": "function",
         "description": "Creates a bookmark or folder under the specified parentId.  If url is NULL or missing, it will be a folder.",
+        "async": "callback",
         "parameters": [
           {
             "$ref": "CreateDetails",
             "name": "bookmark"
           },
           {
             "type": "function",
             "name": "callback",
@@ -298,16 +305,17 @@
             ]
           }
         ]
       },
       {
         "name": "move",
         "type": "function",
         "description": "Moves the specified BookmarkTreeNode to the provided location.",
+        "async": "callback",
         "parameters": [
           {
             "type": "string",
             "name": "id"
           },
           {
             "type": "object",
             "name": "destination",
@@ -335,16 +343,17 @@
             ]
           }
         ]
       },
       {
         "name": "update",
         "type": "function",
         "description": "Updates the properties of a bookmark or folder. Specify only the properties that you want to change; unspecified properties will be left unchanged.  <b>Note:</b> Currently, only 'title' and 'url' are supported.",
+        "async": "callback",
         "parameters": [
           {
             "type": "string",
             "name": "id"
           },
           {
             "type": "object",
             "name": "changes",
@@ -371,16 +380,17 @@
             ]
           }
         ]
       },
       {
         "name": "remove",
         "type": "function",
         "description": "Removes a bookmark or an empty bookmark folder.",
+        "async": "callback",
         "parameters": [
           {
             "type": "string",
             "name": "id"
           },
           {
             "type": "function",
             "name": "callback",
@@ -389,16 +399,17 @@
           }
         ]
       },
       {
         "name": "removeTree",
         "unsupported": true,
         "type": "function",
         "description": "Recursively removes a bookmark folder.",
+        "async": "callback",
         "parameters": [
           {
             "type": "string",
             "name": "id"
           },
           {
             "type": "function",
             "name": "callback",
@@ -407,30 +418,32 @@
           }
         ]
       },
       {
         "name": "import",
         "unsupported": true,
         "type": "function",
         "description": "Imports bookmarks from an html bookmark file",
+        "async": "callback",
         "parameters": [
           {
             "type": "function",
             "name": "callback",
             "optional": true,
             "parameters": []
           }
         ]
       },
       {
         "name": "export",
         "unsupported": true,
         "type": "function",
         "description": "Exports bookmarks to an html bookmark file",
+        "async": "callback",
         "parameters": [
           {
             "type": "function",
             "name": "callback",
             "optional": true,
             "parameters": []
           }
         ]
--- a/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
@@ -9,130 +9,92 @@
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
 </head>
 <body>
 
 <script type="text/javascript">
 "use strict";
 
 function backgroundScript() {
-  function get(idOrIdList) {
-    return new Promise(resolve => {
-      browser.bookmarks.get(idOrIdList, resolve);
-    });
-  }
-
-  function create(bookmark) {
-    return new Promise(resolve => {
-      browser.bookmarks.create(bookmark, resolve);
-    });
-  }
-
-  function getChildren(id) {
-    return new Promise(resolve => {
-      browser.bookmarks.getChildren(id, resolve);
-    });
-  }
-
-  function update(id, changes) {
-    return new Promise(resolve => {
-      browser.bookmarks.update(id, changes, resolve);
-    });
-  }
-
-  function getTree(id) {
-    return new Promise(resolve => {
-      browser.bookmarks.getTree(resolve);
-    });
-  }
-
-  function remove(idOrIdList) {
-    return new Promise(resolve => {
-      browser.bookmarks.remove(idOrIdList, resolve);
-    });
-  }
-
   let unsortedId, ourId;
 
   function checkOurBookmark(bookmark) {
-    browser.test.assertEq(bookmark.id, ourId);
+    browser.test.assertEq(ourId, bookmark.id);
     browser.test.assertTrue("parentId" in bookmark);
-    browser.test.assertEq(bookmark.index, 0); // We assume there are no other bookmarks.
-    browser.test.assertEq(bookmark.url, "http://example.org/");
-    browser.test.assertEq(bookmark.title, "test bookmark");
+    browser.test.assertEq(0, bookmark.index); // We assume there are no other bookmarks.
+    browser.test.assertEq("http://example.org/", bookmark.url);
+    browser.test.assertEq("test bookmark", bookmark.title);
     browser.test.assertTrue("dateAdded" in bookmark);
     browser.test.assertFalse("dateGroupModified" in bookmark);
     browser.test.assertFalse("unmodifiable" in bookmark);
   }
 
-  get(["not-a-bookmark-guid"]).then(result => {
-    // TODO: check lastError
-    browser.test.assertEq(result.length, 0, "invalid bookmark guid returned nothing");
-    return get(["000000000000"]);
+  let failures = 0;
+  let tallyFailure = error => {
+    browser.test.succeed(`Got expected error: ${error}`);
+    failures++;
+  };
+
+  browser.bookmarks.get(["not-a-bookmark-guid"]).catch(tallyFailure).then(result => {
+    return browser.bookmarks.get(["000000000000"]).catch(tallyFailure);
   }).then(results => {
-    // TODO: check lastError
-    browser.test.assertEq(results.length, 0, "correctly did not find bookmark");
-    return create({title: "test bookmark", url: "http://example.org"});
+    return browser.bookmarks.create({title: "test bookmark", url: "http://example.org"});
   }).then(result => {
     ourId = result.id;
     checkOurBookmark(result);
 
-    return get(ourId);
+    return browser.bookmarks.get(ourId);
   }).then(results => {
     browser.test.assertEq(results.length, 1);
     checkOurBookmark(results[0]);
 
     unsortedId = results[0].parentId;
-    return get(unsortedId);
+    return browser.bookmarks.get(unsortedId);
   }).then(results => {
     let folder = results[0];
     browser.test.assertEq(results.length, 1);
 
-    browser.test.assertEq(folder.id, unsortedId);
+    browser.test.assertEq(unsortedId, folder.id);
     browser.test.assertTrue("parentId" in folder);
     browser.test.assertTrue("index" in folder);
     browser.test.assertFalse("url" in folder);
-    browser.test.assertEq(folder.title, "Unsorted Bookmarks");
+    browser.test.assertEq("Unsorted Bookmarks", folder.title);
     browser.test.assertTrue("dateAdded" in folder);
     browser.test.assertTrue("dateGroupModified" in folder);
     browser.test.assertFalse("unmodifiable" in folder); // TODO: Do we want to enable this?
 
-    return getChildren(unsortedId);
+    return browser.bookmarks.getChildren(unsortedId);
   }).then(results => {
-    browser.test.assertEq(results.length, 1);
+    browser.test.assertEq(1, results.length);
     checkOurBookmark(results[0]);
 
-    return update(ourId, {title: "new test title"});
-  }).then(result => {
-    browser.test.assertEq(result.title, "new test title");
-    browser.test.assertEq(result.id, ourId);
-
-    return getTree();
-  }).then(results => {
-    browser.test.assertEq(results.length, 1);
-    let bookmark = results[0].children.find(bookmark => bookmark.id == unsortedId);
-    browser.test.assertEq(bookmark.title, "Unsorted Bookmarks");
-
-    return create({parentId: "invalid"});
+    return browser.bookmarks.update(ourId, {title: "new test title"});
   }).then(result => {
-    // TODO: Check lastError
-    browser.test.assertEq(result, null);
+    browser.test.assertEq("new test title", result.title);
+    browser.test.assertEq(ourId, result.id);
 
-    return remove(ourId);
-  }).then(() => {
-    return get(ourId);
+    return browser.bookmarks.getTree();
   }).then(results => {
-    // TODO: Check lastError
-    browser.test.assertEq(results.length, 0);
+    browser.test.assertEq(1, results.length);
+    let bookmark = results[0].children.find(bookmark => bookmark.id == unsortedId);
+    browser.test.assertEq("Unsorted Bookmarks", bookmark.title);
 
-    return remove("000000000000");
+    return browser.bookmarks.create({parentId: "invalid"}).catch(tallyFailure);
+  }).then(result => {
+    return browser.bookmarks.remove(ourId);
   }).then(() => {
-    // TODO: Check lastError
+    return browser.bookmarks.get(ourId).catch(tallyFailure);
+  }).then(results => {
+    return browser.bookmarks.remove("000000000000").catch(tallyFailure);
   }).then(() => {
+    browser.test.assertEq(5, failures, "Expected failures");
+
     browser.test.notifyPass("bookmarks");
+  }).catch(error => {
+    browser.test.fail(`Error: ${String(error)} :: ${error.stack}`);
   });
 }
 
 let extensionData = {
   background: "(" + backgroundScript.toString() + ")()",
   manifest: {
     permissions: ["bookmarks"],
   },