Bug 1303405 - Ensure RemoveFolderTransaction::UndoTransaction passes the reinserted GUID to observers. r=mak

MozReview-Commit-ID: 5HpDKEmsjRW

--HG--
extra : rebase_source : ac15fa1b1e1aa6b520a13ab29decf371c66b73e3

services/sync/tests/unit/test_bookmark_tracker.js

@@ -1140,13 +1140,14 @@ add_task(function* test_onItemDeleted_removeFolderTransaction() {
 
     _("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]);

toolkit/components/places/SQLFunctions.cpp

@@ -842,7 +842,15 @@ namespace places {
 
     int64_t lastInsertedId = aArgs->AsInt64(1);
 
-    nsNavHistory::StoreLastInsertedId(table, lastInsertedId);
+    MOZ_ASSERT(table.EqualsLiteral("moz_places") ||
+               table.EqualsLiteral("moz_historyvisits") ||
+               table.EqualsLiteral("moz_bookmarks"));
+
+    if (table.EqualsLiteral("moz_bookmarks")) {
+      nsNavBookmarks::StoreLastInsertedId(table, lastInsertedId);
+    } else {
+      nsNavHistory::StoreLastInsertedId(table, lastInsertedId);
+    }
 
     RefPtr<nsVariant> result = new nsVariant();
     rv = result->SetAsInt64(lastInsertedId);

toolkit/components/places/nsNavBookmarks.cpp

@@ -148,6 +148,17 @@ NS_IMPL_ISUPPORTS(nsNavBookmarks
 )
 
 
+Atomic<int64_t> nsNavBookmarks::sLastInsertedItemId(0);
+
+
+void // static
+nsNavBookmarks::StoreLastInsertedId(const nsACString& aTable,
+                                    const int64_t aLastInsertedId) {
+  MOZ_ASSERT(aTable.EqualsLiteral("moz_bookmarks"));
+  sLastInsertedItemId = aLastInsertedId;
+}
+
+
 nsresult
 nsNavBookmarks::Init()
 {
@@ -346,7 +357,7 @@ nsNavBookmarks::InsertBookmarkInDB(int64_t aPlaceId,
        "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);
@@ -395,34 +406,22 @@ nsNavBookmarks::InsertBookmarkInDB(int64_t aPlaceId,
   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 "
-      "FROM moz_bookmarks "
-      "ORDER BY ROWID DESC "
-      "LIMIT 1"
-    );
-    NS_ENSURE_STATE(lastInsertIdStmt);
-    mozStorageStatementScoper lastInsertIdScoper(lastInsertIdStmt);
-
-    bool hasResult;
-    rv = lastInsertIdStmt->ExecuteStep(&hasResult);
-    NS_ENSURE_SUCCESS(rv, rv);
-    NS_ENSURE_TRUE(hasResult, NS_ERROR_UNEXPECTED);
-    rv = lastInsertIdStmt->GetInt64(0, _itemId);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = lastInsertIdStmt->GetUTF8String(1, _guid);
-    NS_ENSURE_SUCCESS(rv, rv);
+    *_itemId = sLastInsertedItemId;
   }
 
   if (aParentId > 0) {

toolkit/components/places/nsNavBookmarks.h

@@ -216,6 +216,10 @@ public:
   static const int32_t kGetChildrenIndex_Type;
   static const int32_t kGetChildrenIndex_PlaceID;
 
+  static mozilla::Atomic<int64_t> sLastInsertedItemId;
+  static void StoreLastInsertedId(const nsACString& aTable,
+                                  const int64_t aLastInsertedId);
+
 private:
   static nsNavBookmarks* gBookmarksService;
 

toolkit/components/places/nsPlacesTriggers.h

@@ -210,6 +210,7 @@
   "CREATE TEMP TRIGGER moz_bookmarks_foreign_count_afterinsert_trigger " \
   "AFTER INSERT ON moz_bookmarks FOR EACH ROW " \
   "BEGIN " \
+    "SELECT store_last_inserted_id('moz_bookmarks', NEW.id); " \
     "UPDATE moz_places " \
     "SET foreign_count = foreign_count + 1 " \
     "WHERE id = NEW.fk;" \

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

@@ -0,0 +1,70 @@
+/**
+ * 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 folder = yield PlacesUtils.bookmarks.insert({
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+    parentGuid: PlacesUtils.bookmarks.menuGuid,
+    title: "Test folder",
+  });
+  let folderId = yield PlacesUtils.promiseItemId(folder.guid);
+  let fx = yield PlacesUtils.bookmarks.insert({
+    parentGuid: folder.guid,
+    title: "Get Firefox!",
+    url: "http://getfirefox.com",
+  });
+  let fxId = yield PlacesUtils.promiseItemId(fx.guid);
+  let tb = yield PlacesUtils.bookmarks.insert({
+    parentGuid: folder.guid,
+    title: "Get Thunderbird!",
+    url: "http://getthunderbird.com",
+  });
+  let tbId = yield PlacesUtils.promiseItemId(tb.guid);
+
+  let notifications = [];
+  function checkNotifications(expected, message) {
+    deepEqual(notifications, expected, message);
+    notifications.length = 0;
+  }
+
+  let observer = {
+    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]);
+    },
+  };
+  PlacesUtils.bookmarks.addObserver(observer, false);
+  PlacesUtils.registerShutdownFunction(function() {
+    PlacesUtils.bookmarks.removeObserver(observer);
+  });
+
+  let transaction = PlacesUtils.bookmarks.getRemoveFolderTransaction(folderId);
+  deepEqual(notifications, [], "We haven't executed the transaction yet");
+
+  transaction.doTransaction();
+  checkNotifications([
+    ["onItemRemoved", tbId, folderId, tb.guid, folder.guid],
+    ["onItemRemoved", fxId, folderId, fx.guid, folder.guid],
+    ["onItemRemoved", folderId, PlacesUtils.bookmarksMenuFolderId, folder.guid,
+      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");
+});

toolkit/components/places/tests/bookmarks/xpcshell.ini

@@ -45,5 +45,6 @@ skip-if = toolkit == 'android' || toolkit == 'gonk'
 [test_keywords.js]
 [test_nsINavBookmarkObserver.js]
 [test_protectRoots.js]
+[test_removeFolderTransaction_reinsert.js]
 [test_removeItem.js]
 [test_savedsearches.js]