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

--HG--
extra : rebase_source : 390d6c80f034f01cc419b1c2c3c9a93722fa1c59

browser/components/extensions/ext-bookmarks.js

@@ -157,9 +157,8 @@ extensions.registerSchemaAPI("bookmarks", "bookmarks", (extension, context) => {
         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);

toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html

@@ -15,7 +15,14 @@
 
 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");
@@ -54,6 +61,9 @@ function backgroundScript() {
       );
     });
   }).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;
@@ -150,10 +160,13 @@ function backgroundScript() {
       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}),
@@ -249,19 +262,19 @@ function backgroundScript() {
     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");
@@ -351,6 +364,36 @@ function backgroundScript() {
         "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 => {
@@ -364,12 +407,13 @@ function backgroundScript() {
     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");
       });
@@ -395,6 +439,22 @@ function backgroundScript() {
       );
     });
   }).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}`);

toolkit/components/places/Bookmarks.jsm

@@ -227,7 +227,6 @@ var Bookmarks = Object.freeze({
       { 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.

toolkit/components/places/tests/bookmarks/test_bookmarks_update.js

@@ -76,9 +76,6 @@ add_task(function* invalid_input_throws() {
   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() {
@@ -274,7 +271,6 @@ add_task(function* update_index() {
   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);