From e3b58c02928b22d6ae07ce4ee140cba45762cffe Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Fri, 23 Feb 2018 17:27:43 +0100 Subject: [PATCH] Bug 656936 - Remove Bookmarks::GetRemoveFolderTransaction. r=standard8 MozReview-Commit-ID: DLyjzC7ODyB --HG-- extra : rebase_source : 4d71ab1909ca6e1065e31dc1824de3b119e05e7c --- .../sync/tests/unit/test_bookmark_tracker.js | 20 ++- .../places/nsINavBookmarksService.idl | 22 --- toolkit/components/places/nsNavBookmarks.cpp | 133 ++++++------------ toolkit/components/places/nsNavBookmarks.h | 73 ---------- .../test_removeFolderTransaction_reinsert.js | 48 ++++--- .../places/tests/unit/test_452777.js | 43 ------ .../components/places/tests/unit/xpcshell.ini | 1 - 7 files changed, 81 insertions(+), 259 deletions(-) delete mode 100644 toolkit/components/places/tests/unit/test_452777.js diff --git a/services/sync/tests/unit/test_bookmark_tracker.js b/services/sync/tests/unit/test_bookmark_tracker.js index b98c82d908c9..2a1bb7a5bc5e 100644 --- a/services/sync/tests/unit/test_bookmark_tracker.js +++ b/services/sync/tests/unit/test_bookmark_tracker.js @@ -9,6 +9,7 @@ ChromeUtils.import("resource://services-sync/service.js"); ChromeUtils.import("resource://services-sync/util.js"); ChromeUtils.import("resource://gre/modules/osfile.jsm"); ChromeUtils.import("resource:///modules/PlacesUIUtils.jsm"); +ChromeUtils.import("resource://gre/modules/PlacesTransactions.jsm"); let engine; let store; @@ -1230,30 +1231,27 @@ add_task(async function test_onItemDeleted_removeFolderTransaction() { await startTracking(); - let txn = PlacesUtils.bookmarks.getRemoveFolderTransaction(folder_id); + let txn = PlacesTransactions.Remove({guid: folder_guid}); // We haven't executed the transaction yet. await verifyTrackerEmpty(); _("Execute the remove folder transaction"); - txn.doTransaction(); + await txn.transact(); await verifyTrackedItems(["menu", folder_guid, fx_guid, tb_guid]); Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3); await resetTracker(); _("Undo the remove folder transaction"); - txn.undoTransaction(); + await PlacesTransactions.undo(); - // At this point, the restored folder has the same ID, but a different GUID. - let new_folder_guid = await PlacesUtils.promiseItemGuid(folder_id); - - await verifyTrackedItems(["menu", new_folder_guid]); - Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE); + await verifyTrackedItems(["menu", folder_guid, fx_guid, tb_guid]); + Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3); await resetTracker(); _("Redo the transaction"); - txn.redoTransaction(); - await verifyTrackedItems(["menu", new_folder_guid]); - Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE); + await PlacesTransactions.redo(); + await verifyTrackedItems(["menu", folder_guid, fx_guid, tb_guid]); + Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3); } finally { _("Clean up."); await cleanup(); diff --git a/toolkit/components/places/nsINavBookmarksService.idl b/toolkit/components/places/nsINavBookmarksService.idl index bbda69b3eb73..758149716ba2 100644 --- a/toolkit/components/places/nsINavBookmarksService.idl +++ b/toolkit/components/places/nsINavBookmarksService.idl @@ -401,28 +401,6 @@ interface nsINavBookmarksService : nsISupports [optional] in ACString aGuid, [optional] in unsigned short aSource); - /** - * Gets an undo-able transaction for removing a folder from the bookmarks - * tree. - * @param aItemId - * The id of the folder to remove. - * @param [optional] aSource - * The change source, forwarded to all bookmark observers. Defaults - * to SOURCE_DEFAULT. - * @return An object implementing nsITransaction that can be used to undo - * or redo the action. - * - * This method exists because complex delete->undo operations rely on - * recreated folders to have the same ID they had before they were deleted, - * so that any other items deleted in different transactions can be - * re-inserted correctly. This provides a safe encapsulation of this - * functionality without exposing the ability to recreate folders with - * specific IDs (potentially dangerous if abused by other code!) in the - * public API. - */ - nsITransaction getRemoveFolderTransaction(in long long aItemId, - [optional] in unsigned short aSource); - /** * Convenience function for container services. Removes * all children of the given folder. diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp index 3ba59a7c4000..4ff14fd1b5e2 100644 --- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -798,24 +798,55 @@ nsNavBookmarks::RemoveItem(int64_t aItemId, uint16_t aSource) NS_IMETHODIMP -nsNavBookmarks::CreateFolder(int64_t aParent, const nsACString& aName, +nsNavBookmarks::CreateFolder(int64_t aParent, const nsACString& aTitle, int32_t aIndex, const nsACString& aGUID, - uint16_t aSource, int64_t* aNewFolder) + uint16_t aSource, int64_t* aNewFolderId) { // NOTE: aParent can be null for root creation, so not checked - NS_ENSURE_ARG_POINTER(aNewFolder); - + NS_ENSURE_ARG_POINTER(aNewFolderId); + NS_ENSURE_ARG_MIN(aIndex, nsINavBookmarksService::DEFAULT_INDEX); if (!aGUID.IsEmpty() && !IsValidGUID(aGUID)) return NS_ERROR_INVALID_ARG; - // CreateContainerWithID returns the index of the new folder, but that's not - // used here. To avoid any risk of corrupting data should this function - // be changed, we'll use a local variable to hold it. The true argument - // will cause notifications to be sent to bookmark observers. - int32_t localIndex = aIndex; - nsresult rv = CreateContainerWithID(-1, aParent, aName, true, &localIndex, - aGUID, aSource, aNewFolder); + // Get the correct index for insertion. This also ensures the parent exists. + int32_t index = aIndex, folderCount; + int64_t grandParentId; + nsAutoCString folderGuid; + nsresult rv = FetchFolderInfo(aParent, &folderCount, folderGuid, &grandParentId); NS_ENSURE_SUCCESS(rv, rv); + + mozStorageTransaction transaction(mDB->MainConn(), false); + + if (aIndex == nsINavBookmarksService::DEFAULT_INDEX || aIndex >= folderCount) { + index = folderCount; + } else { + // Create space for the insertion. + rv = AdjustIndices(aParent, index, INT32_MAX, 1); + NS_ENSURE_SUCCESS(rv, rv); + } + + *aNewFolderId = -1; + PRTime dateAdded = RoundedPRNow(); + nsAutoCString guid(aGUID); + nsCString title; + TruncateTitle(aTitle, title); + + rv = InsertBookmarkInDB(-1, FOLDER, aParent, index, + title, dateAdded, 0, folderGuid, grandParentId, + nullptr, aSource, aNewFolderId, guid); + NS_ENSURE_SUCCESS(rv, rv); + + rv = transaction.Commit(); + NS_ENSURE_SUCCESS(rv, rv); + + int64_t tagsRootId = TagsRootId(); + + NOTIFY_BOOKMARKS_OBSERVERS(mCanNotify, mObservers, + SKIP_TAGS(aParent == tagsRootId), + OnItemAdded(*aNewFolderId, aParent, index, FOLDER, + nullptr, title, dateAdded, guid, + folderGuid, aSource)); + return NS_OK; } @@ -831,86 +862,6 @@ bool nsNavBookmarks::IsLivemark(int64_t aFolderId) return isLivemark; } -nsresult -nsNavBookmarks::CreateContainerWithID(int64_t aItemId, - int64_t aParent, - const nsACString& aTitle, - bool aIsBookmarkFolder, - int32_t* aIndex, - const nsACString& aGUID, - uint16_t aSource, - int64_t* aNewFolder) -{ - NS_ENSURE_ARG_MIN(*aIndex, nsINavBookmarksService::DEFAULT_INDEX); - - // Get the correct index for insertion. This also ensures the parent exists. - int32_t index, folderCount; - int64_t grandParentId; - nsAutoCString folderGuid; - nsresult rv = FetchFolderInfo(aParent, &folderCount, folderGuid, &grandParentId); - NS_ENSURE_SUCCESS(rv, rv); - - mozStorageTransaction transaction(mDB->MainConn(), false); - - if (*aIndex == nsINavBookmarksService::DEFAULT_INDEX || - *aIndex >= folderCount) { - index = folderCount; - } else { - index = *aIndex; - // Create space for the insertion. - rv = AdjustIndices(aParent, index, INT32_MAX, 1); - NS_ENSURE_SUCCESS(rv, rv); - } - - *aNewFolder = aItemId; - PRTime dateAdded = RoundedPRNow(); - nsAutoCString guid(aGUID); - nsCString title; - TruncateTitle(aTitle, title); - - rv = InsertBookmarkInDB(-1, FOLDER, aParent, index, - title, dateAdded, 0, folderGuid, grandParentId, - nullptr, aSource, aNewFolder, guid); - NS_ENSURE_SUCCESS(rv, rv); - - rv = transaction.Commit(); - NS_ENSURE_SUCCESS(rv, rv); - - int64_t tagsRootId = TagsRootId(); - - NOTIFY_BOOKMARKS_OBSERVERS(mCanNotify, mObservers, - SKIP_TAGS(aParent == tagsRootId), - OnItemAdded(*aNewFolder, aParent, index, FOLDER, - nullptr, title, dateAdded, guid, - folderGuid, aSource)); - - *aIndex = index; - return NS_OK; -} - - -NS_IMPL_ISUPPORTS(nsNavBookmarks::RemoveFolderTransaction, nsITransaction) - -NS_IMETHODIMP -nsNavBookmarks::GetRemoveFolderTransaction(int64_t aFolderId, uint16_t aSource, - nsITransaction** aResult) -{ - NS_ENSURE_ARG_MIN(aFolderId, 1); - NS_ENSURE_ARG_POINTER(aResult); - - // Create and initialize a RemoveFolderTransaction object that can be used to - // recreate the folder safely later. - - RemoveFolderTransaction* rft = - new RemoveFolderTransaction(aFolderId, aSource); - if (!rft) - return NS_ERROR_OUT_OF_MEMORY; - - NS_ADDREF(*aResult = rft); - return NS_OK; -} - - nsresult nsNavBookmarks::GetDescendantFolders(int64_t aFolderId, nsTArray& aDescendantFoldersArray) { diff --git a/toolkit/components/places/nsNavBookmarks.h b/toolkit/components/places/nsNavBookmarks.h index 49d17c3ba9d1..2d86fd04abc6 100644 --- a/toolkit/components/places/nsNavBookmarks.h +++ b/toolkit/components/places/nsNavBookmarks.h @@ -7,7 +7,6 @@ #define nsNavBookmarks_h_ #include "nsINavBookmarksService.h" -#include "nsITransaction.h" #include "nsNavHistory.h" #include "nsToolkitCompsCID.h" #include "nsCategoryCache.h" @@ -164,21 +163,6 @@ public: nsresult QueryFolderChildrenAsync(nsNavHistoryFolderResultNode* aNode, mozIStoragePendingStatement** _pendingStmt); - /** - * @return index of the new folder in aIndex, whether it was passed in or - * generated by autoincrement. - * - * @note If aFolder is -1, uses the autoincrement id for folder index. - * @note aTitle will be truncated to TITLE_LENGTH_MAX - */ - nsresult CreateContainerWithID(int64_t aId, int64_t aParent, - const nsACString& aTitle, - bool aIsBookmarkFolder, - int32_t* aIndex, - const nsACString& aGUID, - uint16_t aSource, - int64_t* aNewFolder); - /** * Fetches information about the specified id from the database. * @@ -400,63 +384,6 @@ private: nsresult GetBookmarksForURI(nsIURI* aURI, nsTArray& _bookmarks); - class RemoveFolderTransaction final : public nsITransaction { - public: - RemoveFolderTransaction(int64_t aID, uint16_t aSource) - : mID(aID) - , mSource(aSource) - {} - - NS_DECL_ISUPPORTS - - NS_IMETHOD DoTransaction() override { - nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); - NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY); - BookmarkData folder; - nsresult rv = bookmarks->FetchItemInfo(mID, folder); - // TODO (Bug 656935): store the BookmarkData struct instead. - mParent = folder.parentId; - mIndex = folder.position; - - rv = bookmarks->GetItemTitle(mID, mTitle); - NS_ENSURE_SUCCESS(rv, rv); - - return bookmarks->RemoveItem(mID, mSource); - } - - NS_IMETHOD UndoTransaction() override { - nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService(); - NS_ENSURE_TRUE(bookmarks, NS_ERROR_OUT_OF_MEMORY); - int64_t newFolder; - return bookmarks->CreateContainerWithID(mID, mParent, mTitle, true, - &mIndex, EmptyCString(), - mSource, &newFolder); - } - - NS_IMETHOD RedoTransaction() override { - return DoTransaction(); - } - - NS_IMETHOD GetIsTransient(bool* aResult) override { - *aResult = false; - return NS_OK; - } - - NS_IMETHOD Merge(nsITransaction* aTransaction, bool* aResult) override { - *aResult = false; - return NS_OK; - } - - private: - ~RemoveFolderTransaction() {} - - int64_t mID; - uint16_t mSource; - MOZ_INIT_OUTSIDE_CTOR int64_t mParent; - nsCString mTitle; - MOZ_INIT_OUTSIDE_CTOR int32_t mIndex; - }; - // Used to enable and disable the observer notifications. bool mCanNotify; diff --git a/toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js b/toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js index 4cbced81a47d..66cae56df6ba 100644 --- a/toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js +++ b/toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js @@ -1,6 +1,8 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ /** * This test ensures that reinserting a folder within a transaction gives it - * a different GUID, and passes the GUID to the observers. + * the same GUID, and passes it to the observers. */ add_task(async function test_removeFolderTransaction_reinsert() { @@ -9,19 +11,16 @@ add_task(async function test_removeFolderTransaction_reinsert() { parentGuid: PlacesUtils.bookmarks.menuGuid, title: "Test folder", }); - let folderId = await PlacesUtils.promiseItemId(folder.guid); let fx = await PlacesUtils.bookmarks.insert({ parentGuid: folder.guid, title: "Get Firefox!", url: "http://getfirefox.com", }); - let fxId = await PlacesUtils.promiseItemId(fx.guid); let tb = await PlacesUtils.bookmarks.insert({ parentGuid: folder.guid, title: "Get Thunderbird!", url: "http://getthunderbird.com", }); - let tbId = await PlacesUtils.promiseItemId(tb.guid); let notifications = []; function checkNotifications(expected, message) { @@ -30,6 +29,7 @@ add_task(async function test_removeFolderTransaction_reinsert() { } let observer = { + __proto__: NavBookmarkObserver.prototype, onItemAdded(itemId, parentId, index, type, uri, title, dateAdded, guid, parentGuid) { notifications.push(["onItemAdded", itemId, parentId, guid, parentGuid]); @@ -43,10 +43,14 @@ add_task(async function test_removeFolderTransaction_reinsert() { PlacesUtils.bookmarks.removeObserver(observer); }); - let transaction = PlacesUtils.bookmarks.getRemoveFolderTransaction(folderId); - deepEqual(notifications, [], "We haven't executed the transaction yet"); + let transaction = PlacesTransactions.Remove({guid: folder.guid}); + + let folderId = await PlacesUtils.promiseItemId(folder.guid); + let fxId = await PlacesUtils.promiseItemId(fx.guid); + let tbId = await PlacesUtils.promiseItemId(tb.guid); + + await transaction.transact(); - transaction.doTransaction(); checkNotifications([ ["onItemRemoved", tbId, folderId, tb.guid, folder.guid], ["onItemRemoved", fxId, folderId, fx.guid, folder.guid], @@ -54,17 +58,25 @@ add_task(async function test_removeFolderTransaction_reinsert() { 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 = await PlacesUtils.promiseItemGuid(folderId); - checkNotifications([ - ["onItemAdded", folderId, PlacesUtils.bookmarksMenuFolderId, newFolderGuid, - PlacesUtils.bookmarks.menuGuid], - ], "Undo should reinsert folder with same ID and different GUID"); + await PlacesTransactions.undo(); + + folderId = await PlacesUtils.promiseItemId(folder.guid); + fxId = await PlacesUtils.promiseItemId(fx.guid); + tbId = await PlacesUtils.promiseItemId(tb.guid); - transaction.redoTransaction(); checkNotifications([ - ["onItemRemoved", folderId, PlacesUtils.bookmarksMenuFolderId, - newFolderGuid, PlacesUtils.bookmarks.menuGuid], - ], "Redo should forward new GUID to observer"); + ["onItemAdded", folderId, PlacesUtils.bookmarksMenuFolderId, folder.guid, + PlacesUtils.bookmarks.menuGuid], + ["onItemAdded", fxId, folderId, fx.guid, folder.guid], + ["onItemAdded", tbId, folderId, tb.guid, folder.guid], + ], "Undo should reinsert folder with different id but same GUID"); + + await PlacesTransactions.redo(); + + checkNotifications([ + ["onItemRemoved", tbId, folderId, tb.guid, folder.guid], + ["onItemRemoved", fxId, folderId, fx.guid, folder.guid], + ["onItemRemoved", folderId, PlacesUtils.bookmarksMenuFolderId, folder.guid, + PlacesUtils.bookmarks.menuGuid], + ], "Redo should pass the GUID to observer"); }); diff --git a/toolkit/components/places/tests/unit/test_452777.js b/toolkit/components/places/tests/unit/test_452777.js deleted file mode 100644 index 5cc78ba475b6..000000000000 --- a/toolkit/components/places/tests/unit/test_452777.js +++ /dev/null @@ -1,43 +0,0 @@ -/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- - * vim:set ts=2 sw=2 sts=2 expandtab - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -/** - * This test ensures that when removing a folder within a transaction, undoing - * the transaction restores it with the same id (as received by the observers). - */ - -add_task(async function test_undo_folder_remove_within_transaction() { - const TITLE = "test folder"; - - // Create two test folders; remove the first one. This ensures that undoing - // the removal will not get the same id by chance (the insert id's can be - // reused in SQLite). - let folder = await PlacesUtils.bookmarks.insert({ - parentGuid: PlacesUtils.bookmarks.unfiledGuid, - title: TITLE, - type: PlacesUtils.bookmarks.TYPE_FOLDER, - }); - - let id = await PlacesUtils.promiseItemId(folder.guid); - - await PlacesUtils.bookmarks.insert({ - parentGuid: PlacesUtils.bookmarks.unfiledGuid, - title: "test folder 2", - type: PlacesUtils.bookmarks.TYPE_FOLDER, - }); - - let transaction = PlacesUtils.bookmarks.getRemoveFolderTransaction(id); - transaction.doTransaction(); - - // Now check to make sure it gets added with the right id - PlacesUtils.bookmarks.addObserver({ - onItemAdded(aItemId, aFolder, aIndex, aItemType, aURI, aTitle) { - Assert.equal(aItemId, id); - Assert.equal(aTitle, TITLE); - } - }); - transaction.undoTransaction(); -}); diff --git a/toolkit/components/places/tests/unit/xpcshell.ini b/toolkit/components/places/tests/unit/xpcshell.ini index 7594b9f031f3..603781af673a 100644 --- a/toolkit/components/places/tests/unit/xpcshell.ini +++ b/toolkit/components/places/tests/unit/xpcshell.ini @@ -42,7 +42,6 @@ skip-if = os == "linux" [test_429505_remove_shortcuts.js] [test_433317_query_title_update.js] [test_433525_hasChildren_crash.js] -[test_452777.js] [test_454977.js] [test_463863.js] [test_485442_crash_bug_nsNavHistoryQuery_GetUri.js]