Bug 1253652 - Fix browser.bookmarks.move() and add tests for it. r=kmag, r=mak
authorbsilverberg <bsilverberg@mozilla.com>
Wed, 09 Mar 2016 09:30:43 -0500
changeset 288377 282a763e8fe7d6a3def65e07bd483da9d9c0ddba
parent 288376 93f0785edcc78554577ed7e5417ecb38bc100bd0
child 288378 b8e6b7d7ff083ea314eef52657fd6b9222ac5333
push id30079
push userryanvm@gmail.com
push dateSat, 12 Mar 2016 20:24:19 +0000
treeherdermozilla-central@d1d47ba19ce9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag, mak
bugs1253652
milestone48.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 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);
 });