Bug 1379798 - Ensure `insertTree` notifies observers with the correct parent ID. r=standard8
authorKit Cambridge <kit@yakshaving.ninja>
Mon, 10 Jul 2017 14:38:21 -0700
changeset 368503 07faa05d6e824fd0d6cd9643358a8e8db3c26812
parent 368502 54e4545b0fe03e103b464011738f60caa5f621a6
child 368504 1e12285e2131c8748d6332a5415a63d8d93a88a8
push id46340
push userkcambridge@mozilla.com
push dateWed, 12 Jul 2017 16:56:23 +0000
treeherderautoland@07faa05d6e82 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1379798
milestone56.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 1379798 - Ensure `insertTree` notifies observers with the correct parent ID. r=standard8 MozReview-Commit-ID: LBm8VddumPJ
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_insertTree.js
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -420,57 +420,56 @@ var Bookmarks = Object.freeze({
     }
 
     // We want to validate synchronously, but we can't know the index at which
     // we're inserting into the parent. We just use NULL instead,
     // and the SQL query with which we insert will update it as necessary.
     let lastAddedForParent = appendInsertionInfoForInfoArray(tree.children, null, tree.guid);
 
     return (async function() {
-      let parent = await fetchBookmark({ guid: tree.guid });
-      if (!parent) {
+      let treeParent = await fetchBookmark({ guid: tree.guid });
+      if (!treeParent) {
         throw new Error("The parent you specified doesn't exist.");
       }
 
-      if (parent._parentId == PlacesUtils.tagsFolderId) {
+      if (treeParent._parentId == PlacesUtils.tagsFolderId) {
         throw new Error("Can't use insertTree to insert tags.");
       }
 
-      await insertBookmarkTree(insertInfos, source, parent,
+      await insertBookmarkTree(insertInfos, source, treeParent,
                                urlsThatMightNeedPlaces, lastAddedForParent);
 
       await insertLivemarkData(insertLivemarkInfos);
 
       // Now update the indices of root items in the objects we return.
       // These may be wrong if someone else modified the table between
       // when we fetched the parent and inserted our items, but the actual
       // inserts will have been correct, and we don't want to query the DB
       // again if we don't have to. bug 1347230 covers improving this.
-      let rootIndex = parent._childCount;
+      let rootIndex = treeParent._childCount;
       for (let insertInfo of insertInfos) {
         if (insertInfo.parentGuid == tree.guid) {
           insertInfo.index += rootIndex++;
         }
       }
       // We need the itemIds to notify, though once the switch to guids is
       // complete we may stop using them.
       let itemIdMap = await PlacesUtils.promiseManyItemIds(insertInfos.map(info => info.guid));
       // Notify onItemAdded to listeners.
       let observers = PlacesUtils.bookmarks.getObservers();
       for (let i = 0; i < insertInfos.length; i++) {
         let item = insertInfos[i];
         let itemId = itemIdMap.get(item.guid);
         let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null;
         // For sub-folders, we need to make sure their children have the correct parent ids.
         let parentId;
-        if (item.guid === parent.guid ||
-            Bookmarks.userContentRoots.includes(item.parentGuid)) {
-          // We're the item being inserted at the top-level, or we're a top-level
-          // folder, so the parent id won't have changed.
-          parentId = parent._id;
+        if (item.parentGuid === treeParent.guid) {
+          // This is a direct child of the tree parent, so we can use the
+          // existing parent's id.
+          parentId = treeParent._id;
         } else {
           // This is a parent folder that's been updated, so we need to
           // use the new item id.
           parentId = itemIdMap.get(item.parentGuid);
         }
 
         notify(observers, "onItemAdded", [ itemId, parentId, item.index,
                                            item.type, uri, item.title,
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_insertTree.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_insertTree.js
@@ -322,8 +322,77 @@ add_task(async function insert_many_non_
     if (bm.type == PlacesUtils.bookmarks.TYPE_BOOKMARK) {
       Assert.greater(frecencyForUrl(bm.url), 0, "Check frecency has been updated for bookmark " + bm.url);
     }
     Assert.equal(bm.parentGuid, PlacesUtils.bookmarks.unfiledGuid);
   }
   Assert.equal(obsInvoked, bms.length);
   Assert.equal(obsInvoked, 6);
 });
+
+add_task(async function create_in_folder() {
+  let mozFolder = await PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.menuGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    title: "Mozilla",
+  });
+
+  let notifications = [];
+  let obs = {
+    onItemAdded(itemId, parentId, index, type, uri, title, dateAdded, guid, parentGuid) {
+      notifications.push({ itemId, parentId, index, title, guid, parentGuid });
+    },
+  };
+  PlacesUtils.bookmarks.addObserver(obs);
+
+  let bms = await PlacesUtils.bookmarks.insertTree({children: [{
+    url: "http://getfirefox.com",
+    title: "Get Firefox!",
+  }, {
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    title: "Community",
+    children: [
+      {
+        url: "http://getthunderbird.com",
+        title: "Get Thunderbird!",
+      },
+      {
+        url: "https://www.seamonkey-project.org",
+        title: "SeaMonkey",
+      },
+    ],
+  }], guid: mozFolder.guid});
+  await PlacesTestUtils.promiseAsyncUpdates();
+
+  PlacesUtils.bookmarks.removeObserver(obs);
+
+  let mozFolderId = await PlacesUtils.promiseItemId(mozFolder.guid);
+  let commFolderId = await PlacesUtils.promiseItemId(bms[1].guid);
+  deepEqual(notifications, [{
+    itemId: await PlacesUtils.promiseItemId(bms[0].guid),
+    parentId: mozFolderId,
+    index: 0,
+    title: "Get Firefox!",
+    guid: bms[0].guid,
+    parentGuid: mozFolder.guid,
+  }, {
+    itemId: commFolderId,
+    parentId: mozFolderId,
+    index: 1,
+    title: "Community",
+    guid: bms[1].guid,
+    parentGuid: mozFolder.guid,
+  }, {
+    itemId: await PlacesUtils.promiseItemId(bms[2].guid),
+    parentId: commFolderId,
+    index: 0,
+    title: "Get Thunderbird!",
+    guid: bms[2].guid,
+    parentGuid: bms[1].guid,
+  }, {
+    itemId: await PlacesUtils.promiseItemId(bms[3].guid),
+    parentId: commFolderId,
+    index: 1,
+    title: "SeaMonkey",
+    guid: bms[3].guid,
+    parentGuid: bms[1].guid,
+  }]);
+});