Bug 1451352 - Rework copying bookmarks (via paste or dnd) to use insertTree to improve performance.
MozReview-Commit-ID: 5oC0ASheHf7
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -1248,55 +1248,51 @@ async function getTransactionsForMove(it
* @param {Array} items A list of unwrapped nodes to get transactions for.
* @param {Integer} insertionIndex The requested index for insertion.
* @param {String} insertionParentGuid The guid of the parent folder to insert
* or move the items to.
* @return {Array} Returns an array of created PlacesTransactions.
*/
async function getTransactionsForCopy(items, insertionIndex,
insertionParentGuid) {
- let transactions = [];
let index = insertionIndex;
+ let children = [];
for (let item of items) {
- let transaction;
let guid = item.itemGuid;
if (PlacesUIUtils.PLACES_FLAVORS.includes(item.type) &&
// For anything that is comming from within this session, we do a
// direct copy, otherwise we fallback and form a new item below.
"instanceId" in item && item.instanceId == PlacesUtils.instanceId &&
// If the Item doesn't have a guid, this could be a virtual tag query or
// other item, so fallback to inserting a new bookmark with the URI.
guid &&
// For virtual root items, we fallback to creating a new bookmark, as
// we want a shortcut to be created, not a full tree copy.
!PlacesUtils.bookmarks.isVirtualRootItem(guid) &&
!PlacesUtils.isVirtualLeftPaneItem(guid)) {
- transaction = PlacesTransactions.Copy({
- excludingAnnotation: "Places/SmartBookmark",
- guid,
- newIndex: index,
- newParentGuid: insertionParentGuid,
- });
+ // XXX Can't promiseBookmarksTree has the wrong style of data for insertTree,
+ // is it time for fetchTree?
+ // children.push(await PlacesUtils.promiseBookmarksTree(guid));
} else if (item.type == PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) {
- transaction = PlacesTransactions.NewSeparator({
+ children.push({
index,
- parentGuid: insertionParentGuid,
+ type: PlacesUtils.bookmarks.TYPE_SEPARATOR
});
} else {
let title = item.type != PlacesUtils.TYPE_UNICODE ? item.title : item.uri;
- transaction = PlacesTransactions.NewBookmark({
+ children.push({
index,
- parentGuid: insertionParentGuid,
title,
url: item.uri,
});
}
- transactions.push(transaction);
-
if (index != -1) {
index++;
}
}
- return transactions;
+ return [PlacesTransactions.InsertTree({
+ guid: insertionParentGuid,
+ children
+ })];
}
--- a/browser/components/places/tests/browser/browser_paste_bookmarks.js
+++ b/browser/components/places/tests/browser/browser_paste_bookmarks.js
@@ -207,17 +207,17 @@ add_task(async function paste_from_diffe
let data = {
"title": "test",
"id": 32,
"instanceId": "FAKEFAKEFAKE",
"itemGuid": "ZBf_TYkrYGvW",
"parent": 452,
"dateAdded": 1464866275853000,
"lastModified": 1507638113352000,
- "type": "text/x-moz-place",
+ "type": PlacesUtils.TYPE_X_MOZ_PLACE,
"uri": TEST_URL1
};
data = JSON.stringify(data);
xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
xferable.setTransferData(PlacesUtils.TYPE_X_MOZ_PLACE, PlacesUtils.toISupportsString(data),
data.length * 2);
@@ -235,8 +235,127 @@ add_task(async function paste_from_diffe
"Should be one bookmark in the unfiled folder.");
Assert.equal(tree.children[0].title, "test",
"Should have the correct title");
Assert.equal(tree.children[0].uri, TEST_URL1,
"Should have the correct URL");
await PlacesUtils.bookmarks.remove(tree.children[0].guid);
});
+
+add_task(async function paste_separator_from_different_instance() {
+ let xferable = Cc["@mozilla.org/widget/transferable;1"]
+ .createInstance(Ci.nsITransferable);
+ xferable.init(null);
+
+ // Fake data on the clipboard to pretend this is from a different instance
+ // of Firefox.
+ let data = {
+ "title": "test",
+ "id": 32,
+ "instanceId": "FAKEFAKEFAKE",
+ "itemGuid": "ZBf_TYkrYGvW",
+ "parent": 452,
+ "dateAdded": 1464866275853000,
+ "lastModified": 1507638113352000,
+ "type": PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR
+ };
+ data = JSON.stringify(data);
+
+ xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
+ xferable.setTransferData(PlacesUtils.TYPE_X_MOZ_PLACE, PlacesUtils.toISupportsString(data),
+ data.length * 2);
+
+ Services.clipboard.setData(xferable, null, Ci.nsIClipboard.kGlobalClipboard);
+
+ info("Selecting UnfiledBookmarks in the left pane");
+ PlacesOrganizer.selectLeftPaneBuiltIn("UnfiledBookmarks");
+
+ info("Pasting clipboard");
+ await ContentTree.view.controller.paste();
+
+ let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
+
+ Assert.equal(tree.children.length, 1,
+ "Should be one bookmark in the unfiled folder.");
+ Assert.equal(tree.children[0].type, PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR,
+ "Should have the correct type");
+
+ await PlacesUtils.bookmarks.remove(tree.children[0].guid);
+});
+
+// XXX fixme
+add_task(async function paste_copy_check_indexes() {
+ info("Selecting BookmarksToolbar in the left pane");
+ PlacesOrganizer.selectLeftPaneBuiltIn("BookmarksToolbar");
+
+ let copyChildren = [];
+ let targetChildren = [];
+ for (let i = 0; i < 10; i++) {
+ copyChildren.push({
+ url: `${TEST_URL}${i}`,
+ title: `Copy ${i}`
+ });
+ targetChildren.push({
+ url: `${TEST_URL1}${i}`,
+ title: `Target ${i}`
+ });
+ }
+
+ let copyBookmarks = await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.toolbarGuid,
+ children: copyChildren
+ });
+
+ let targetBookmarks = await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ children: targetChildren
+ });
+
+ ContentTree.view.selectItems([
+ copyBookmarks[0].guid,
+ copyBookmarks[3].guid,
+ copyBookmarks[6].guid,
+ copyBookmarks[9].guid,
+ ]);
+
+ await promiseClipboard(() => {
+ info("Cutting multiple selection");
+ ContentTree.view.controller.copy();
+ }, PlacesUtils.TYPE_X_MOZ_PLACE);
+
+ info("Selecting UnfiledBookmarks in the left pane");
+ PlacesOrganizer.selectLeftPaneBuiltIn("UnfiledBookmarks");
+
+ ContentTree.view.selectItems([targetBookmarks[4].guid]);
+
+ info("Pasting clipboard");
+ await ContentTree.view.controller.paste();
+
+ let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid);
+
+ const expectedBookmarkOrder = [
+ targetBookmarks[0].guid,
+ targetBookmarks[1].guid,
+ targetBookmarks[2].guid,
+ targetBookmarks[3].guid,
+ copyBookmarks[0].guid,
+ copyBookmarks[3].guid,
+ copyBookmarks[6].guid,
+ copyBookmarks[9].guid,
+ targetBookmarks[4].guid,
+ targetBookmarks[5].guid,
+ targetBookmarks[6].guid,
+ targetBookmarks[7].guid,
+ targetBookmarks[8].guid,
+ targetBookmarks[9].guid,
+ ];
+
+ Assert.equal(tree.children.length, expectedBookmarkOrder.length,
+ "Should be the expected amount of bookmarks in the unfiled folder.");
+
+ for (let i = 0; i < expectedBookmarkOrder.length; ++i) {
+ Assert.equal(tree.children[i].guid, expectedBookmarkOrder[i],
+ `Should be the expected item at index ${i}`);
+ }
+
+ await PlacesUtils.bookmarks.eraseEverything();
+});
--- a/toolkit/components/places/PlacesTransactions.jsm
+++ b/toolkit/components/places/PlacesTransactions.jsm
@@ -178,16 +178,22 @@ const TRANSACTIONS_QUEUE_TIMEOUT_MS = 24
ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
ChromeUtils.import("resource://gre/modules/Services.jsm");
ChromeUtils.defineModuleGetter(this, "PlacesUtils",
"resource://gre/modules/PlacesUtils.jsm");
Cu.importGlobalProperties(["URL"]);
+const BOOKMARK_TYPES = [
+ PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ PlacesUtils.bookmarks.TYPE_FOLDER,
+ PlacesUtils.bookmarks.TYPE_SEPARATOR,
+];
+
function setTimeout(callback, ms) {
let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
timer.initWithCallback(callback, ms, timer.TYPE_ONE_SHOT);
}
class TransactionsHistoryArray extends Array {
constructor() {
super();
@@ -744,22 +750,23 @@ DefineTransaction.annotationObjectValida
}
}
throw new Error("Invalid annotation object");
};
DefineTransaction.childObjectValidate = function(obj) {
if (obj &&
checkProperty(obj, "title", false, v => typeof(v) == "string") &&
- !("type" in obj && obj.type != PlacesUtils.bookmarks.TYPE_BOOKMARK)) {
- obj.url = DefineTransaction.urlValidate(obj.url);
- let validKeys = ["title", "url"];
- if (Object.keys(obj).every(k => validKeys.includes(k))) {
- return obj;
+ checkProperty(obj, "type", false, v => BOOKMARK_TYPES.includes(v))) {
+ if ("type" in obj && obj.type == PlacesUtils.bookmarks.TYPE_FOLDER) {
+ obj.children = DefineTransaction.validatePropertyValue("children", obj, true);
}
+ // Note: we don't validate all items here - we let the bookmarks API validate
+ // them for us to save duplication.
+ return obj;
}
throw new Error("Invalid child object");
};
DefineTransaction.urlValidate = function(url) {
if (url instanceof Ci.nsIURI)
return new URL(url.spec);
return new URL(url);
@@ -1744,8 +1751,63 @@ PT.Copy.prototype = {
};
this.redo = async function() {
await createItemsFromBookmarksTree(newItemInfo, true);
};
return newItemGuid;
}
};
+
+/**
+ * Transaction for inserting a tree of items.
+ *
+ * Required Input Properties: guid, children
+ */
+PT.InsertTree = DefineTransaction(["guid", "children"]);
+PT.InsertTree.prototype = {
+ async execute({guid, children}) {
+ let bms = await PlacesUtils.bookmarks.insertTree({
+ guid, children
+ });
+
+ this.undo = async function() {
+ for (let bm of bms) {
+ try {
+ await PlacesUtils.bookmarks.remove(bm.guid);
+ } catch (ex) {
+ if (!ex.message.includes("No bookmarks found for the provided GUID.")) {
+ throw ex;
+ }
+ }
+ }
+ };
+ this.redo = async function() {
+ // For re-inserting, we have to remap the bookmarks tree so that children
+ // are in the correct place.
+ if (bms.length > 1) {
+ let knownBms = new Map();
+ // We'll filter out non-top level items from the array.
+ bms = bms.filter(bm => {
+ knownBms.set(bm.guid, bm);
+ // If the item has a known parent, then we re-parent it within the
+ // tree.
+ if ("parentGuid" in bm && knownBms.has(bm.parentGuid)) {
+ let parent = knownBms.get(bm.parentGuid);
+ if ("children" in parent) {
+ parent.children.push(bm);
+ } else {
+ parent.children = [bm];
+ }
+ return false;
+ }
+ return true;
+ });
+ }
+
+ bms = await PlacesUtils.bookmarks.insertTree({
+ guid, children: bms
+ });
+ };
+
+ return bms.map(bm => bm.guid);
+ }
+};
--- a/toolkit/components/places/tests/unit/test_async_transactions.js
+++ b/toolkit/components/places/tests/unit/test_async_transactions.js
@@ -537,18 +537,18 @@ add_task(async function test_new_bookmar
parentGuid: bm_info.parentGuid,
index: bmStartIndex });
observer.reset();
};
await ensureDo();
await PT.undo();
ensureUndo();
- await PT.redo(true);
- await ensureDo();
+ await PT.redo();
+ await ensureDo(true);
await PT.undo();
ensureUndo();
await PT.clearTransactionsHistory();
ensureUndoState();
});
add_task(async function test_merge_create_folder_and_item() {
@@ -1918,8 +1918,130 @@ add_task(async function test_renameTag()
await PT.undo();
ensureTagsForURI(url, []);
await PT.clearTransactionsHistory();
ensureUndoState();
await PlacesUtils.bookmarks.eraseEverything();
});
+
+add_task(async function test_insertTree_singleBookmark() {
+ observer.reset();
+
+ let bmData = [{
+ url: "http://example.com",
+ title: "135246",
+ }];
+
+ let guids = await PT.InsertTree({
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ children: bmData
+ }).transact();
+
+ Assert.equal(guids.length, bmData.length,
+ "Should have inserted the correct amount of items");
+
+ ensureItemsAdded({guid: guids[0], parentGuid: PlacesUtils.bookmarks.unfiledGuid, ...bmData[0]});
+
+ let originalInfos = [];
+ for (let guid of guids) {
+ originalInfos.push(await PlacesUtils.promiseBookmarksTree(guid));
+ }
+ await PT.undo();
+ await ensureItemsRemoved(...guids);
+ await ensureNonExistent(...guids);
+ await PT.redo();
+ await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
+ ensureItemsAdded({guid: guids[0], parentGuid: PlacesUtils.bookmarks.unfiledGuid, ...bmData[0]});
+
+ await PlacesUtils.bookmarks.eraseEverything();
+});
+
+add_task(async function test_insertTree_multipleItems() {
+ observer.reset();
+
+ let bmData = [{
+ url: "http://example.com",
+ title: "135246",
+ }, {
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "142536",
+ children: [{
+ url: "http://example.com/2",
+ title: "531246",
+ }],
+ }, {
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "13572468",
+ children: [{
+ url: "http://example.com/3",
+ title: "15263748",
+ }, {
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ title: "12563478",
+ children: [{
+ url: "http://example.com/4",
+ title: "75312468",
+ }],
+ }],
+ }];
+
+ let guids = await PT.InsertTree({
+ guid: PlacesUtils.bookmarks.unfiledGuid,
+ children: bmData
+ }).transact();
+
+ Assert.equal(guids.length, 7,
+ "Should have inserted the correct amount of items");
+
+ const expectedAddedData = [{
+ guid: guids[0],
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+ url: bmData[0].url,
+ title: bmData[0].title,
+ }, {
+ guid: guids[1],
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+ title: bmData[1].title,
+ type: bmData[1].type,
+ }, {
+ guid: guids[2],
+ parentGuid: guids[1],
+ url: bmData[1].children[0].url,
+ title: bmData[1].children[0].title,
+ }, {
+ guid: guids[3],
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+ title: bmData[2].title,
+ type: bmData[2].type,
+ }, {
+ guid: guids[4],
+ parentGuid: guids[3],
+ url: bmData[2].children[0].url,
+ title: bmData[2].children[0].title,
+ }, {
+ guid: guids[5],
+ parentGuid: guids[3],
+ title: bmData[2].children[1].title,
+ type: bmData[2].children[1].type,
+ }, {
+ guid: guids[6],
+ parentGuid: guids[5],
+ url: bmData[2].children[1].children[0].url,
+ title: bmData[2].children[1].children[0].title,
+ }];
+
+ ensureItemsAdded(...expectedAddedData);
+
+ let originalInfos = [];
+ for (let guid of guids) {
+ originalInfos.push(await PlacesUtils.promiseBookmarksTree(guid));
+ }
+ await PT.undo();
+ await ensureItemsRemoved(...guids);
+ await ensureNonExistent(...guids);
+ await PT.redo();
+ await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
+ ensureItemsAdded(...expectedAddedData);
+
+ await PlacesUtils.bookmarks.eraseEverything();
+});