--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -4,16 +4,17 @@
ChromeUtils.import("resource://services-common/utils.js");
ChromeUtils.import("resource://services-sync/constants.js");
ChromeUtils.import("resource://services-sync/engines/bookmarks.js");
ChromeUtils.import("resource://services-sync/engines.js");
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;
let tracker;
const DAY_IN_MS = 24 * 60 * 60 * 1000;
add_task(async function setup() {
@@ -1225,40 +1226,37 @@ add_task(async function test_onItemDelet
CommonUtils.makeURI("http://getthunderbird.com"),
PlacesUtils.bookmarks.DEFAULT_INDEX,
"Get Thunderbird!");
let tb_guid = await PlacesUtils.promiseItemGuid(tb_id);
_(`Thunderbird GUID: ${tb_guid}`);
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();
}
});
add_task(async function test_treeMoved() {
_("Moving an entire tree of bookmarks should track the parents");
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -397,38 +397,16 @@ interface nsINavBookmarksService : nsISu
* @throws if aGuid is malformed.
*/
long long createFolder(in long long aParentFolder, in AUTF8String name,
in long index,
[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.
* @param aItemId
* The id of the folder to remove children from.
* @param [optional] aSource
* The change source, forwarded to all bookmark observers. Defaults
* to SOURCE_DEFAULT.
*/
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -793,130 +793,81 @@ nsNavBookmarks::RemoveItem(int64_t aItem
}
return NS_OK;
}
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;
}
bool nsNavBookmarks::IsLivemark(int64_t aFolderId)
{
nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
NS_ENSURE_TRUE(annosvc, false);
bool isLivemark;
nsresult rv = annosvc->ItemHasAnnotation(aFolderId,
FEED_URI_ANNO,
&isLivemark);
NS_ENSURE_SUCCESS(rv, false);
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<int64_t>& aDescendantFoldersArray) {
nsresult rv;
// New descendant folders will be added from this index on.
uint32_t startIndex = aDescendantFoldersArray.Length();
{
nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
"SELECT id "
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -2,17 +2,16 @@
/* 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/. */
#ifndef nsNavBookmarks_h_
#define nsNavBookmarks_h_
#include "nsINavBookmarksService.h"
-#include "nsITransaction.h"
#include "nsNavHistory.h"
#include "nsToolkitCompsCID.h"
#include "nsCategoryCache.h"
#include "nsTHashtable.h"
#include "nsWeakReference.h"
#include "mozilla/Attributes.h"
#include "prtime.h"
@@ -160,31 +159,16 @@ public:
* @param _pendingStmt
* The Storage pending statement that will be used to control async
* execution.
*/
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.
*
* @param aItemId
* Id of the item to fetch information for.
* @param aBookmark
* BookmarkData to store the information.
*/
nsresult FetchItemInfo(int64_t aItemId,
@@ -395,73 +379,16 @@ private:
nsIURI* aURI,
uint16_t aSource,
int64_t* _itemId,
nsACString& _guid);
nsresult GetBookmarksForURI(nsIURI* aURI,
nsTArray<BookmarkData>& _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;
// Tracks whether we are in batch mode.
// Note: this is only tracking bookmarks batches, not history ones.
bool mBatching;
};
--- a/toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js
+++ b/toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js
@@ -1,70 +1,82 @@
+/* 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() {
let folder = await PlacesUtils.bookmarks.insert({
type: PlacesUtils.bookmarks.TYPE_FOLDER,
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) {
deepEqual(notifications, expected, message);
notifications.length = 0;
}
let observer = {
+ __proto__: NavBookmarkObserver.prototype,
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);
PlacesUtils.registerShutdownFunction(function() {
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});
- transaction.doTransaction();
+ let folderId = await PlacesUtils.promiseItemId(folder.guid);
+ let fxId = await PlacesUtils.promiseItemId(fx.guid);
+ let tbId = await PlacesUtils.promiseItemId(tb.guid);
+
+ await transaction.transact();
+
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 = await PlacesUtils.promiseItemGuid(folderId);
+ await PlacesTransactions.undo();
+
+ folderId = await PlacesUtils.promiseItemId(folder.guid);
+ fxId = await PlacesUtils.promiseItemId(fx.guid);
+ tbId = await PlacesUtils.promiseItemId(tb.guid);
+
checkNotifications([
- ["onItemAdded", folderId, PlacesUtils.bookmarksMenuFolderId, newFolderGuid,
+ ["onItemAdded", folderId, PlacesUtils.bookmarksMenuFolderId, folder.guid,
PlacesUtils.bookmarks.menuGuid],
- ], "Undo should reinsert folder with same ID and different GUID");
+ ["onItemAdded", fxId, folderId, fx.guid, folder.guid],
+ ["onItemAdded", tbId, folderId, tb.guid, folder.guid],
+ ], "Undo should reinsert folder with different id but same GUID");
- transaction.redoTransaction();
+ await PlacesTransactions.redo();
+
checkNotifications([
- ["onItemRemoved", folderId, PlacesUtils.bookmarksMenuFolderId,
- newFolderGuid, PlacesUtils.bookmarks.menuGuid],
- ], "Redo should forward new GUID to observer");
+ ["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");
});
deleted file mode 100644
--- 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();
-});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -37,17 +37,16 @@ skip-if = os == "linux"
[test_415460.js]
[test_415757.js]
[test_419731.js]
[test_419792_node_tags_property.js]
[test_425563.js]
[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]
[test_486978_sort_by_date_queries.js]
[test_536081.js]
[test_1085291.js]
[test_1105208.js]
[test_1105866.js]
