Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers. r=mak
MozReview-Commit-ID: 5HpDKEmsjRW
--- 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/SQLFunctions.cpp
+++ b/toolkit/components/places/SQLFunctions.cpp
@@ -837,17 +837,25 @@ namespace places {
MOZ_ASSERT(numArgs == 2);
nsAutoCString table;
rv = aArgs->GetUTF8String(0, table);
NS_ENSURE_SUCCESS(rv, rv);
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);
NS_ENSURE_SUCCESS(rv, rv);
result.forget(_result);
return NS_OK;
}
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -143,16 +143,27 @@ NS_IMPL_ISUPPORTS(nsNavBookmarks
, nsINavBookmarksService
, nsINavHistoryObserver
, nsIAnnotationObserver
, nsIObserver
, nsISupportsWeakReference
)
+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()
{
mDB = Database::GetDatabase();
NS_ENSURE_STATE(mDB);
nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
if (os) {
@@ -341,17 +352,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,44 +401,32 @@ 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 "
- "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) {
// Update last modified date of the ancestors.
// TODO (bug 408991): Doing this for all ancestors would be slow without a
// nested tree, so for now update only the parent.
rv = SetItemDateInternal(LAST_MODIFIED, aParentId, aDateAdded);
NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -211,16 +211,20 @@ public:
nsresult GetDescendantFolders(int64_t aFolderId,
nsTArray<int64_t>& aDescendantFoldersArray);
static const int32_t kGetChildrenIndex_Guid;
static const int32_t kGetChildrenIndex_Position;
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;
~nsNavBookmarks();
/**
* Checks whether or not aFolderId points to a live bookmark.
*
--- a/toolkit/components/places/nsPlacesTriggers.h
+++ b/toolkit/components/places/nsPlacesTriggers.h
@@ -205,16 +205,17 @@
"WHERE id = OLD.fk;" \
"END" \
)
#define CREATE_BOOKMARKS_FOREIGNCOUNT_AFTERINSERT_TRIGGER NS_LITERAL_CSTRING( \
"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;" \
"END" \
)
#define CREATE_BOOKMARKS_FOREIGNCOUNT_AFTERUPDATE_TRIGGER NS_LITERAL_CSTRING( \
"CREATE TEMP TRIGGER moz_bookmarks_foreign_count_afterupdate_trigger " \
new file mode 100644
--- /dev/null
+++ b/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");
+});
--- a/toolkit/components/places/tests/bookmarks/xpcshell.ini
+++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ -40,10 +40,11 @@ skip-if = toolkit == 'android' || toolki
[test_bookmarks_reorder.js]
[test_bookmarks_search.js]
[test_bookmarks_update.js]
[test_changeBookmarkURI.js]
[test_getBookmarkedURIFor.js]
[test_keywords.js]
[test_nsINavBookmarkObserver.js]
[test_protectRoots.js]
+[test_removeFolderTransaction_reinsert.js]
[test_removeItem.js]
[test_savedsearches.js]