Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak draft
authorbsilverberg <bsilverberg@mozilla.com>
Wed, 09 Mar 2016 09:30:43 -0500
changeset 339479 375f70af08d191c965624e64ace6fee8b66f21ed
parent 338601 c0764c13b2bf6f070b0bcbbdbd34155141fc597c
child 516000 00bfa8550c94b61ef56e9d501c1ea089336b430a
push id12743
push userbmo:bob.silverberg@gmail.com
push dateFri, 11 Mar 2016 12:19:47 +0000
reviewerskmag, mak
bugs1253652
milestone47.0a1
Bug 1253652 - Fix browser.bookmarks.move() and add tests for it, r?kmag r?mak Update Bookmarks.update to not require a parentGuid when updating just the index MozReview-Commit-ID: JJO2IDyI5oN
browser/components/extensions/ext-bookmarks.js
toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -152,19 +152,18 @@ extensions.registerSchemaAPI("bookmarks"
       move: function(id, destination) {
         let info = {
           guid: id,
         };
 
         if (destination.parentId !== null) {
           info.parentGuid = destination.parentId;
         }
-        if (destination.index !== null) {
-          info.index = destination.index;
-        }
+        info.index = (destination.index === null) ?
+          Bookmarks.DEFAULT_INDEX : destination.index;
 
         try {
           return Bookmarks.update(info).then(convert);
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
 
--- a/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html
@@ -10,17 +10,24 @@
 </head>
 <body>
 
 <script type="text/javascript">
 "use strict";
 
 function backgroundScript() {
   let unsortedId, ourId;
+  let initialBookmarkCount = 0;
+  let createdBookmarks = new Set();
   const nonExistentId = "000000000000";
+  const bookmarkGuids = {
+    menuGuid:    "menu________",
+    toolbarGuid: "toolbar_____",
+    unfiledGuid: "unfiled_____",
+  };
 
   function checkOurBookmark(bookmark) {
     browser.test.assertEq(ourId, bookmark.id, "Bookmark has the expected Id");
     browser.test.assertTrue("parentId" in bookmark, "Bookmark has a parentId");
     browser.test.assertEq(0, bookmark.index, "Bookmark has the expected index"); // We assume there are no other bookmarks.
     browser.test.assertEq("http://example.org/", bookmark.url, "Bookmark has the expected url");
     browser.test.assertEq("test bookmark", bookmark.title, "Bookmark has the expected title");
     browser.test.assertTrue("dateAdded" in bookmark, "Bookmark has a dateAdded");
@@ -49,16 +56,19 @@ function backgroundScript() {
 
     return browser.bookmarks.get([nonExistentId]).then(expectedError, error => {
       browser.test.assertTrue(
         error.message.includes("Bookmark not found"),
         "Expected error thrown when trying to get a bookmark using a non-existent Id"
       );
     });
   }).then(() => {
+    return browser.bookmarks.search({});
+  }).then(results => {
+    initialBookmarkCount = results.length;
     return browser.bookmarks.create({title: "test bookmark", url: "http://example.org"});
   }).then(result => {
     ourId = result.id;
     checkOurBookmark(result);
 
     return browser.bookmarks.get(ourId);
   }).then(results => {
     browser.test.assertEq(results.length, 1);
@@ -145,20 +155,23 @@ function backgroundScript() {
     });
   }).then(() => {
     // test bookmarks.search
     return Promise.all([
       browser.bookmarks.create({title: "MØzillä", url: "http://møzîllä.örg"}),
       browser.bookmarks.create({title: "Example", url: "http://example.org"}),
       browser.bookmarks.create({title: "Mozilla Folder"}),
       browser.bookmarks.create({title: "EFF", url: "http://eff.org"}),
-      browser.bookmarks.create({title: "Menu Item", url: "http://menu.org", parentId: "menu________"}),
-      browser.bookmarks.create({title: "Toolbar Item", url: "http://toolbar.org", parentId: "toolbar_____"}),
+      browser.bookmarks.create({title: "Menu Item", url: "http://menu.org", parentId: bookmarkGuids.menuGuid}),
+      browser.bookmarks.create({title: "Toolbar Item", url: "http://toolbar.org", parentId: bookmarkGuids.toolbarGuid}),
     ]);
   }).then(results => {
+    for (let result of results) {
+      if (result.title !== "Mozilla Folder") createdBookmarks.add(result.id);
+    }
     let createdFolderId = results[2].id;
     return Promise.all([
       browser.bookmarks.create({title: "Mozilla", url: "http://allizom.org", parentId: createdFolderId}),
       browser.bookmarks.create({title: "Mozilla Corporation", url: "http://allizom.com", parentId: createdFolderId}),
       browser.bookmarks.create({title: "Firefox", url: "http://allizom.org/firefox", parentId: createdFolderId}),
     ]).then(results => {
       return browser.bookmarks.create({
         title: "About Mozilla",
@@ -244,29 +257,29 @@ function backgroundScript() {
   }).then(results => {
     browser.test.assertEq(1, results.length, "Expected number of results returned for url search");
     checkBookmark({title: "Example", url: "http://example.org/", index: 2}, results[0]);
 
     // queries the title
     return browser.bookmarks.search("EFF");
   }).then(results => {
     browser.test.assertEq(1, results.length, "Expected number of results returned for title search");
-    checkBookmark({title: "EFF", url: "http://eff.org/", index: 0, parentId: "unfiled_____"}, results[0]);
+    checkBookmark({title: "EFF", url: "http://eff.org/", index: 0, parentId: bookmarkGuids.unfiledGuid}, results[0]);
 
     // finds menu items
     return browser.bookmarks.search("Menu Item");
   }).then(results => {
     browser.test.assertEq(1, results.length, "Expected number of results returned for menu item search");
-    checkBookmark({title: "Menu Item", url: "http://menu.org/", index: 4, parentId: "menu________"}, results[0]);
+    checkBookmark({title: "Menu Item", url: "http://menu.org/", index: 4, parentId: bookmarkGuids.menuGuid}, results[0]);
 
     // finds toolbar items
     return browser.bookmarks.search("Toolbar Item");
   }).then(results => {
     browser.test.assertEq(1, results.length, "Expected number of results returned for toolbar item search");
-    checkBookmark({title: "Toolbar Item", url: "http://toolbar.org/", index: 2, parentId: "toolbar_____"}, results[0]);
+    checkBookmark({title: "Toolbar Item", url: "http://toolbar.org/", index: 2, parentId: bookmarkGuids.toolbarGuid}, results[0]);
 
     // finds folders
     return browser.bookmarks.search("Mozilla Folder");
   }).then(results => {
     browser.test.assertEq(1, results.length, "Expected number of folders returned");
     browser.test.assertEq("Mozilla Folder", results[0].title, "Folder has the expected title");
 
     // is case-insensitive
@@ -347,34 +360,65 @@ function backgroundScript() {
       return browser.bookmarks.getRecent(1.234);
     }).then(expectedError, error => {
       browser.test.assertTrue(
         error.message.includes("Incorrect argument types for bookmarks.getRecent"),
         "Expected error thrown when calling getRecent with a decimal number"
       );
     });
   }).then(() => {
+    return Promise.all([
+      browser.bookmarks.search("corporation"),
+      browser.bookmarks.getChildren(bookmarkGuids.menuGuid),
+    ]);
+  }).then(results => {
+    let corporationBookmark = results[0][0];
+    let childCount = results[1].length;
+
+    browser.test.assertEq(2, corporationBookmark.index, "Bookmark has the expected index");
+
+    return browser.bookmarks.move(corporationBookmark.id, {index: 0}).then(result => {
+      browser.test.assertEq(0, result.index, "Bookmark has the expected index");
+
+      return browser.bookmarks.move(corporationBookmark.id, {parentId: bookmarkGuids.menuGuid});
+    }).then(result => {
+      browser.test.assertEq(bookmarkGuids.menuGuid, result.parentId, "Bookmark has the expected parent");
+      browser.test.assertEq(childCount + 1, result.index, "Bookmark has the expected index");
+
+      return browser.bookmarks.move(corporationBookmark.id, {index: 1});
+    }).then(result => {
+      browser.test.assertEq(bookmarkGuids.menuGuid, result.parentId, "Bookmark has the expected parent");
+      browser.test.assertEq(1, result.index, "Bookmark has the expected index");
+
+      return browser.bookmarks.move(corporationBookmark.id, {parentId: bookmarkGuids.toolbarGuid, index: 1});
+    }).then(result => {
+      browser.test.assertEq(bookmarkGuids.toolbarGuid, result.parentId, "Bookmark has the expected parent");
+      browser.test.assertEq(1, result.index, "Bookmark has the expected index");
+      createdBookmarks.add(corporationBookmark.id);
+    });
+  }).then(() => {
     return browser.bookmarks.getRecent(5);
   }).then(results => {
     browser.test.assertEq(5, results.length, "Expected number of results returned by getRecent");
     browser.test.assertEq("About Mozilla", results[0].title, "Bookmark has the expected title");
     browser.test.assertEq("Firefox", results[1].title, "Bookmark has the expected title");
     browser.test.assertEq("Mozilla Corporation", results[2].title, "Bookmark has the expected title");
     browser.test.assertEq("Mozilla", results[3].title, "Bookmark has the expected title");
     browser.test.assertEq("Toolbar Item", results[4].title, "Bookmark has the expected title");
 
     return browser.bookmarks.search({});
   }).then(results => {
     let startBookmarkCount = results.length;
+
     return browser.bookmarks.search({title: "Mozilla Folder"}).then(result => {
       return browser.bookmarks.removeTree(result[0].id);
     }).then(() => {
       return browser.bookmarks.search({}).then(results => {
         browser.test.assertEq(
-          startBookmarkCount - 5,
+          startBookmarkCount - 4,
           results.length,
           "Expected number of results returned after removeTree");
       });
     });
   }).then(() => {
     return browser.bookmarks.create({title: "Empty Folder"});
   }).then(result => {
     let emptyFolderId = result.id;
@@ -390,16 +434,32 @@ function backgroundScript() {
   }).then(() => {
     return browser.bookmarks.getChildren(nonExistentId).then(expectedError, error => {
       browser.test.assertTrue(
         error.message.includes("root is null"),
         "Expected error thrown when trying to getChildren for a non-existent folder"
       );
     });
   }).then(() => {
+    return Promise.resolve().then(() => {
+      return browser.bookmarks.move(nonExistentId, {});
+    }).then(expectedError, error => {
+      browser.test.assertTrue(
+        error.message.includes("No bookmarks found for the provided GUID"),
+        "Expected error thrown when calling move with a non-existent bookmark"
+      );
+    });
+  }).then(() => {
+    // remove all created bookmarks
+    let promises = Array.from(createdBookmarks, guid => browser.bookmarks.remove(guid));
+    return Promise.all(promises);
+  }).then(() => {
+    return browser.bookmarks.search({});
+  }).then(results => {
+    browser.test.assertEq(initialBookmarkCount, results.length, "All created bookmarks have been removed");
     return browser.test.notifyPass("bookmarks");
   }).catch(error => {
     browser.test.fail(`Error: ${String(error)} :: ${error.stack}`);
     browser.test.notifyFail("bookmarks");
   });
 }
 
 let extensionData = {
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -222,17 +222,16 @@ var Bookmarks = Object.freeze({
   update(info) {
     // The info object is first validated here to ensure it's consistent, then
     // it's compared to the existing item to remove any properties that don't
     // need to be updated.
     let updateInfo = validateBookmarkObject(info,
       { guid: { required: true }
       , index: { requiredIf: b => b.hasOwnProperty("parentGuid")
                , validIf: b => b.index >= 0 || b.index == this.DEFAULT_INDEX }
-      , parentGuid: { requiredIf: b => b.hasOwnProperty("index") }
       });
 
     // There should be at last one more property in addition to guid.
     if (Object.keys(updateInfo).length < 2)
       throw new Error("Not enough properties to update");
 
     return Task.spawn(function* () {
       // Ensure the item exists.
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_update.js
@@ -71,19 +71,16 @@ add_task(function* invalid_input_throws(
                 /Invalid value for property 'title'/);
 
   Assert.throws(() => PlacesUtils.bookmarks.update({ guid: "123456789012" }),
                 /Not enough properties to update/);
 
   Assert.throws(() => PlacesUtils.bookmarks.update({ guid: "123456789012",
                                                      parentGuid: "012345678901" }),
                 /The following properties were expected: index/);
-  Assert.throws(() => PlacesUtils.bookmarks.update({ guid: "123456789012",
-                                                     index: 1 }),
-                /The following properties were expected: parentGuid/);
 });
 
 add_task(function* nonexisting_bookmark_throws() {
   try {
     yield PlacesUtils.bookmarks.update({ guid: "123456789012",
                                          title: "test" });
     Assert.ok(false, "Should have thrown");
   } catch (ex) {
@@ -269,17 +266,16 @@ add_task(function* update_index() {
 
   f2 = yield PlacesUtils.bookmarks.fetch(f2.guid);
   Assert.equal(f2.index, 0);
 
   f3 = yield PlacesUtils.bookmarks.fetch(f3.guid);
   Assert.equal(f3.index, 2);
 
   f3 = yield PlacesUtils.bookmarks.update({ guid: f3.guid,
-                                            parentGuid: f1.parentGuid,
                                             index: 0 });
   f1 = yield PlacesUtils.bookmarks.fetch(f1.guid);
   Assert.equal(f1.index, 2);
 
   f2 = yield PlacesUtils.bookmarks.fetch(f2.guid);
   Assert.equal(f2.index, 1);
 });