зеркало из https://github.com/mozilla/gecko-dev.git
Bug 656936 - Remove Bookmarks::GetRemoveFolderTransaction. r=standard8
MozReview-Commit-ID: DLyjzC7ODyB --HG-- extra : rebase_source : 4d71ab1909ca6e1065e31dc1824de3b119e05e7c
This commit is contained in:
Родитель
2b1d67d232
Коммит
e3b58c0292
|
@ -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();
|
||||
|
|
|
@ -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.
|
||||
|
|
|
@ -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<int64_t>& aDescendantFoldersArray) {
|
||||
|
|
|
@ -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<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;
|
||||
|
||||
|
|
|
@ -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");
|
||||
});
|
||||
|
|
|
@ -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();
|
||||
});
|
|
@ -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]
|
||||
|
|
Загрузка…
Ссылка в новой задаче