Bug 1096837 - Allow PlacesUtils.bookmarks.update({ parentGuid, index: DEFAULT_INDEX }). r=mak

toolkit/components/places/Bookmarks.jsm

@@ -94,7 +94,7 @@ let Bookmarks = Object.freeze({
   TYPE_SEPARATOR: 3,
 
   /**
-   * Default index used to append a bookmark-item at the end of a folder. 
+   * Default index used to append a bookmark-item at the end of a folder.
    * This should stay consistent with nsINavBookmarksService.idl
    */
   DEFAULT_INDEX: -1,
@@ -238,7 +238,7 @@ let Bookmarks = Object.freeze({
     let updateInfo = validateBookmarkObject(info,
       { guid: { required: true }
       , index: { requiredIf: b => b.hasOwnProperty("parentGuid")
-               , validIf: b => b.index >= 0 }
+               , validIf: b => b.index >= 0 || b.index == this.DEFAULT_INDEX }
       , parentGuid: { requiredIf: b => b.hasOwnProperty("index") }
       });
 
@@ -307,9 +307,15 @@ let Bookmarks = Object.freeze({
         // the same container.  Thus we know it exists.
         if (!parent)
           parent = yield fetchBookmark({ guid: item.parentGuid });
-        // Set index in the appending case.
-        if (updateInfo.index > parent._childCount)
-          updateInfo.index = parent._childCount;
+
+        if (updateInfo.index >= parent._childCount ||
+            updateInfo.index == this.DEFAULT_INDEX) {
+           updateInfo.index = parent._childCount;
+
+          // Fix the index when moving within the same container.
+          if (parent.guid == item.parentGuid)
+             updateInfo.index--;
+        }
       }
 
       let updatedItem = yield updateBookmark(updateInfo, item, parent);
@@ -1158,7 +1164,7 @@ function rowsToItemsArray(rows) {
     }
 
     return item;
-  });  
+  });
 }
 
 /**
@@ -1267,7 +1273,7 @@ function validateBookmarkObject(input, behavior={}) {
     }
   }
   if (required.size > 0)
-    throw new Error(`The following properties were expected: ${[...required].join(", ")}`); 
+    throw new Error(`The following properties were expected: ${[...required].join(", ")}`);
   return normalizedInput;
 }
 

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

@@ -402,6 +402,71 @@ add_task(function* update_move() {
   Assert.equal(descendant.index, 1);
 });
 
+add_task(function* update_move_append() {
+  let folder_a =
+    yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                         type: PlacesUtils.bookmarks.TYPE_FOLDER });
+  checkBookmarkObject(folder_a);
+  let folder_b =
+    yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                         type: PlacesUtils.bookmarks.TYPE_FOLDER });
+  checkBookmarkObject(folder_b);
+
+  /* folder_a: [sep_1, sep_2, sep_3], folder_b: [] */
+  let sep_1 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid,
+                                                   type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
+  checkBookmarkObject(sep_1);
+  let sep_2 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid,
+                                                   type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
+  checkBookmarkObject(sep_2);
+  let sep_3 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder_a.guid,
+                                                   type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
+  checkBookmarkObject(sep_3);
+
+  function ensurePosition(info, parentGuid, index) {
+    checkBookmarkObject(info);
+    Assert.equal(info.parentGuid, parentGuid);
+    Assert.equal(info.index, index);
+  }
+
+  // folder_a: [sep_2, sep_3, sep_1], folder_b: []
+  sep_1.index = PlacesUtils.bookmarks.DEFAULT_INDEX;
+  // Note sep_1 includes parentGuid even though we're not moving the item to
+  // another folder
+  sep_1 = yield PlacesUtils.bookmarks.update(sep_1);
+  ensurePosition(sep_1, folder_a.guid, 2);
+  sep_2 = yield PlacesUtils.bookmarks.fetch(sep_2.guid);
+  ensurePosition(sep_2, folder_a.guid, 0);
+  sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid);
+  ensurePosition(sep_3, folder_a.guid, 1);
+  sep_1 = yield PlacesUtils.bookmarks.fetch(sep_1.guid);
+  ensurePosition(sep_1, folder_a.guid, 2);
+
+  // folder_a: [sep_2, sep_1], folder_b: [sep_3]
+  sep_3.index = PlacesUtils.bookmarks.DEFAULT_INDEX;
+  sep_3.parentGuid = folder_b.guid;
+  sep_3 = yield PlacesUtils.bookmarks.update(sep_3);
+  ensurePosition(sep_3, folder_b.guid, 0);
+  sep_2 = yield PlacesUtils.bookmarks.fetch(sep_2.guid);
+  ensurePosition(sep_2, folder_a.guid, 0);
+  sep_1 = yield PlacesUtils.bookmarks.fetch(sep_1.guid);
+  ensurePosition(sep_1, folder_a.guid, 1);
+  sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid);
+  ensurePosition(sep_3, folder_b.guid, 0);
+
+  // folder_a: [sep_1], folder_b: [sep_3, sep_2]
+  sep_2.index = Number.MAX_SAFE_INTEGER;
+  sep_2.parentGuid = folder_b.guid;
+  sep_2 = yield PlacesUtils.bookmarks.update(sep_2);
+  ensurePosition(sep_2, folder_b.guid, 1);
+  sep_1 = yield PlacesUtils.bookmarks.fetch(sep_1.guid);
+  ensurePosition(sep_1, folder_a.guid, 0);
+  sep_3 = yield PlacesUtils.bookmarks.fetch(sep_3.guid);
+  ensurePosition(sep_3, folder_b.guid, 0);
+  sep_2 = yield PlacesUtils.bookmarks.fetch(sep_2.guid);
+  ensurePosition(sep_2, folder_b.guid, 1);
+});
+
 function run_test() {
   run_next_test();
 }