Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers. r?mak draft
authorKit Cambridge <kcambridge@mozilla.com>
Fri, 16 Sep 2016 13:22:47 -0700
changeset 414594 26a2d0d69c9c442486173eb3014fdcf277b53436
parent 413613 8a494adbc5cced90a4edf0c98cffde906bf7f3ae
child 414624 63bc057dd5ec2bc055129a320133d9ad05ccba9b
child 414719 ec28951f1695fb21fdc47826343e9546ece9073f
child 414721 b2bcc2ca95fd4e7a5572da23ef2d2dc294a51781
child 416370 74e3d4f368e83ec83868f5605a9b3312ea7e1bf5
push id29720
push userbmo:kcambridge@mozilla.com
push dateFri, 16 Sep 2016 20:49:38 +0000
reviewersmak
bugs1303405
milestone51.0a1
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers. r?mak MozReview-Commit-ID: 5HpDKEmsjRW
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/tests/bookmarks/test_1303405.js
toolkit/components/places/tests/bookmarks/xpcshell.ini
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -1135,23 +1135,24 @@ add_task(function* test_onItemDeleted_re
     _("Execute the remove folder transaction");
     txn.doTransaction();
     yield verifyTrackedItems(["menu", folder_guid, fx_guid, tb_guid]);
     do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 6);
     yield resetTracker();
 
     _("Undo the remove folder transaction");
     txn.undoTransaction();
-    yield verifyTrackedItems(["menu"]);
-    do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE);
-    yield resetTracker();
 
     // At this point, the restored folder has the same ID, but a different GUID.
     let new_folder_guid = yield PlacesUtils.promiseItemGuid(folder_id);
 
+    yield verifyTrackedItems(["menu", new_folder_guid]);
+    do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 2);
+    yield resetTracker();
+
     _("Redo the transaction");
     txn.redoTransaction();
     yield verifyTrackedItems(["menu", new_folder_guid]);
     do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE * 2);
   } finally {
     _("Clean up.");
     yield cleanup();
   }
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -341,17 +341,17 @@ nsNavBookmarks::InsertBookmarkInDB(int64
   MOZ_ASSERT(aPlaceId && (aPlaceId == -1 || aPlaceId > 0));
 
   nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
     "INSERT INTO moz_bookmarks "
       "(id, fk, type, parent, position, title, "
        "dateAdded, lastModified, guid) "
     "VALUES (:item_id, :page_id, :item_type, :parent, :item_index, "
             ":item_title, :date_added, :last_modified, "
-            "IFNULL(:item_guid, GENERATE_GUID()))"
+            ":item_guid)"
   );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
 
   nsresult rv;
   if (*_itemId != -1)
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), *_itemId);
   else
@@ -390,21 +390,26 @@ nsNavBookmarks::InsertBookmarkInDB(int64
   }
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Could use IsEmpty because our callers check for GUID validity,
   // but it doesn't hurt.
   if (_guid.Length() == 12) {
     MOZ_ASSERT(IsValidGUID(_guid));
     rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("item_guid"), _guid);
+    NS_ENSURE_SUCCESS(rv, rv);
   }
   else {
-    rv = stmt->BindNullByName(NS_LITERAL_CSTRING("item_guid"));
+    nsAutoCString guid;
+    rv = GenerateGUID(guid);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("item_guid"), guid);
+    NS_ENSURE_SUCCESS(rv, rv);
+    _guid.Assign(guid);
   }
-  NS_ENSURE_SUCCESS(rv, rv);
 
   rv = stmt->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (*_itemId == -1) {
     // Get the newly inserted item id and GUID.
     nsCOMPtr<mozIStorageStatement> lastInsertIdStmt = mDB->GetStatement(
       "SELECT id, guid "
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/bookmarks/test_1303405.js
@@ -0,0 +1,68 @@
+/**
+ * This test ensures that reinserting a folder within a transaction gives it
+ * a different GUID, and passes the GUID to the observers.
+ */
+
+add_task(function* test_removeFolderTransaction_reinsert() {
+  let folderId = PlacesUtils.bookmarks.createFolder(
+    PlacesUtils.bookmarksMenuFolderId,
+    "Test folder",
+    PlacesUtils.bookmarks.DEFAULT_INDEX
+  );
+  let folderGuid = yield PlacesUtils.promiseItemGuid(folderId);
+  let fxId = PlacesUtils.bookmarks.insertBookmark(
+    folderId,
+    uri("http://getfirefox.com"),
+    PlacesUtils.bookmarks.DEFAULT_INDEX,
+    "Get Firefox!"
+  );
+  let fxGuid = yield PlacesUtils.promiseItemGuid(fxId);
+  let tbId = PlacesUtils.bookmarks.insertBookmark(
+    folderId,
+    uri("http://getthunderbird.com"),
+    PlacesUtils.bookmarks.DEFAULT_INDEX,
+    "Get Thunderbird!"
+  );
+  let tbGuid = yield PlacesUtils.promiseItemGuid(tbId);
+
+  let notifications = [];
+  function checkNotifications(expected, message) {
+    deepEqual(notifications, expected, message);
+    notifications.length = 0;
+  }
+
+  PlacesUtils.bookmarks.addObserver({
+    onItemAdded(itemId, parentId, index, type, uri, title, dateAdded, guid,
+                parentGuid) {
+      notifications.push(["onItemAdded", itemId, parentId, guid, parentGuid]);
+    },
+    onItemRemoved(itemId, parentId, index, type, uri, guid, parentGuid) {
+      notifications.push(["onItemRemoved", itemId, parentId, guid, parentGuid]);
+    },
+  }, false);
+
+  let transaction = PlacesUtils.bookmarks.getRemoveFolderTransaction(folderId);
+  deepEqual(notifications, [], "We haven't executed the transaction yet");
+
+  transaction.doTransaction();
+  checkNotifications([
+    ["onItemRemoved", tbId, folderId, tbGuid, folderGuid],
+    ["onItemRemoved", fxId, folderId, fxGuid, folderGuid],
+    ["onItemRemoved", folderId, PlacesUtils.bookmarksMenuFolderId, folderGuid,
+      PlacesUtils.bookmarks.menuGuid],
+  ], "Executing transaction should remove folder and its descendants");
+
+  transaction.undoTransaction();
+  // At this point, the restored folder has the same ID, but a different GUID.
+  let newFolderGuid = yield PlacesUtils.promiseItemGuid(folderId);
+  checkNotifications([
+    ["onItemAdded", folderId, PlacesUtils.bookmarksMenuFolderId, newFolderGuid,
+      PlacesUtils.bookmarks.menuGuid],
+  ], "Undo should reinsert folder with same ID and different GUID");
+
+  transaction.redoTransaction();
+  checkNotifications([
+    ["onItemRemoved", folderId, PlacesUtils.bookmarksMenuFolderId,
+      newFolderGuid, PlacesUtils.bookmarks.menuGuid],
+  ], "Redo should forward new GUID to observer");
+});
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -22,16 +22,17 @@ skip-if = toolkit == 'android' || toolki
 [test_675416.js]
 [test_711914.js]
 [test_818584-discard-duplicate-backups.js]
 [test_818587_compress-bookmarks-backups.js]
 [test_818593-store-backup-metadata.js]
 [test_992901-backup-unsorted-hierarchy.js]
 [test_997030-bookmarks-html-encode.js]
 [test_1129529.js]
+[test_1303405.js]
 [test_async_observers.js]
 [test_bmindex.js]
 [test_bookmarkstree_cache.js]
 [test_bookmarks.js]
 [test_bookmarks_eraseEverything.js]
 [test_bookmarks_fetch.js]
 [test_bookmarks_getRecent.js]
 [test_bookmarks_insert.js]