Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs. r?adw
Optimise adding a folder with child bookmarks for transactions by allowing PlacesTransactions.NewFolder to take children details and use insertTree rather than needing separate NewFolder and then multiple NewBookmark transactions.
MozReview-Commit-ID: 6s9j0pbsiUB
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -547,26 +547,20 @@ var BookmarkPropertiesPanel = {
/**
* Returns a childItems-transactions array representing the URIList with
* which the dialog has been opened.
*/
_getTransactionsForURIList: function BPP__getTransactionsForURIList() {
var transactions = [];
for (let uri of this._URIs) {
- // uri should be an object in the form { uri, title }. Though add-ons
- // could still use the legacy form, where it's an nsIURI.
- // TODO: Remove This from v57 on.
- let [_uri, _title] = uri instanceof Ci.nsIURI ?
- [uri, this._getURITitleFromHistory(uri)] : [uri.uri, uri.title];
-
let createTxn =
- new PlacesCreateBookmarkTransaction(_uri, -1,
+ new PlacesCreateBookmarkTransaction(uri.uri, -1,
PlacesUtils.bookmarks.DEFAULT_INDEX,
- _title);
+ uri.title);
transactions.push(createTxn);
}
return transactions;
},
/**
* Returns a transaction for creating a new folder item representing the
* various fields and opening arguments of the dialog.
@@ -660,24 +654,21 @@ var BookmarkPropertiesPanel = {
itemGuid = await PlacesTransactions.NewBookmark(info).transact();
} else if (this._itemType == LIVEMARK_CONTAINER) {
info.feedUrl = this._feedURI;
if (this._siteURI)
info.siteUrl = this._siteURI;
itemGuid = await PlacesTransactions.NewLivemark(info).transact();
} else if (this._itemType == BOOKMARK_FOLDER) {
+ // NewFolder requires a url rather than uri.
+ info.children = this._URIs.map(item => {
+ return { url: item.uri, title: item.title };
+ });
itemGuid = await PlacesTransactions.NewFolder(info).transact();
- // URIs is an array of objects in the form { uri, title }. It is still
- // named URIs because for backwards compatibility it could also be an
- // array of nsIURIs. TODO: Fix the property names from v57.
- for (let { uri: url, title } of this._URIs) {
- await PlacesTransactions.NewBookmark({ parentGuid: itemGuid, url, title })
- .transact();
- }
} else {
throw new Error(`unexpected value for _itemType: ${this._itemType}`);
}
this._itemGuid = itemGuid;
this._itemId = await PlacesUtils.promiseItemId(itemGuid);
return Object.freeze({
itemId: this._itemId,
--- a/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js
+++ b/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js
@@ -40,14 +40,19 @@ add_task(async function() {
await promiseTitleChangeNotification;
let newFolder = await PlacesUtils.bookmarks.fetch({
parentGuid: PlacesUtils.bookmarks.unfiledGuid,
index: insertionIndex,
});
is(newFolder.title, "n", "folder name has been edited");
+
+ let bm = await PlacesUtils.bookmarks.fetch(newBookmark.guid);
+ Assert.equal(bm.index, insertionIndex + 1,
+ "Bookmark should have been shifted to the next index");
+
await PlacesUtils.bookmarks.remove(newFolder);
await PlacesUtils.bookmarks.remove(newBookmark);
}
);
});
});
--- a/toolkit/components/places/PlacesTransactions.jsm
+++ b/toolkit/components/places/PlacesTransactions.jsm
@@ -721,54 +721,67 @@ DefineTransaction.indexValidate =
v >= PlacesUtils.bookmarks.DEFAULT_INDEX);
DefineTransaction.guidValidate =
simpleValidateFunc(v => /^[a-zA-Z0-9\-_]{12}$/.test(v));
function isPrimitive(v) {
return v === null || (typeof(v) != "object" && typeof(v) != "function");
}
-DefineTransaction.annotationObjectValidate = function(obj) {
- let checkProperty = (prop, required, checkFn) => {
- if (prop in obj)
- return checkFn(obj[prop]);
+function checkProperty(obj, prop, required, checkFn) {
+ if (prop in obj)
+ return checkFn(obj[prop]);
+
+ return !required;
+}
- return !required;
- };
-
+DefineTransaction.annotationObjectValidate = function(obj) {
if (obj &&
- checkProperty("name", true, v => typeof(v) == "string" && v.length > 0) &&
- checkProperty("expires", false, Number.isInteger) &&
- checkProperty("flags", false, Number.isInteger) &&
- checkProperty("value", false, isPrimitive) ) {
+ checkProperty(obj, "name", true, v => typeof(v) == "string" && v.length > 0) &&
+ checkProperty(obj, "expires", false, Number.isInteger) &&
+ checkProperty(obj, "flags", false, Number.isInteger) &&
+ checkProperty(obj, "value", false, isPrimitive) ) {
// Nothing else should be set
let validKeys = ["name", "value", "flags", "expires"];
if (Object.keys(obj).every(k => validKeys.includes(k)))
return obj;
}
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;
+ }
+ }
+ throw new Error("Invalid child object");
+};
+
DefineTransaction.urlValidate = function(url) {
if (url instanceof Ci.nsIURI)
return new URL(url.spec);
return new URL(url);
};
DefineTransaction.inputProps = new Map();
DefineTransaction.defineInputProps = function(names, validateFn, defaultValue) {
for (let name of names) {
this.inputProps.set(name, {
validateValue(value) {
if (value === undefined)
return defaultValue;
try {
return validateFn(value);
} catch (ex) {
- throw new Error(`Invalid value for input property ${name}`);
+ throw new Error(`Invalid value for input property ${name}: ${ex}`);
}
},
validateInput(input, required) {
if (required && !(name in input))
throw new Error(`Required input property is missing: ${name}`);
return this.validateValue(input[name]);
},
@@ -893,20 +906,23 @@ DefineTransaction.defineInputProps(["tit
DefineTransaction.defineInputProps(["keyword", "oldKeyword", "postData", "tag",
"excludingAnnotation"],
DefineTransaction.strValidate, "");
DefineTransaction.defineInputProps(["index", "newIndex"],
DefineTransaction.indexValidate,
PlacesUtils.bookmarks.DEFAULT_INDEX);
DefineTransaction.defineInputProps(["annotation"],
DefineTransaction.annotationObjectValidate);
+DefineTransaction.defineInputProps(["child"],
+ DefineTransaction.childObjectValidate);
DefineTransaction.defineArrayInputProp("guids", "guid");
DefineTransaction.defineArrayInputProp("urls", "url");
DefineTransaction.defineArrayInputProp("tags", "tag");
DefineTransaction.defineArrayInputProp("annotations", "annotation");
+DefineTransaction.defineArrayInputProp("children", "child");
DefineTransaction.defineArrayInputProp("excludingAnnotations",
"excludingAnnotation");
/**
* Creates items (all types) from a bookmarks tree representation, as defined
* in PlacesUtils.promiseBookmarksTree.
*
* @param tree
@@ -1097,45 +1113,71 @@ PT.NewBookmark.prototype = Object.seal({
return info.guid;
}
});
/**
* Transaction for creating a folder.
*
* Required Input Properties: title, parentGuid.
- * Optional Input Properties: index, annotations.
+ * Optional Input Properties: index, annotations, children
*
* When this transaction is executed, it's resolved to the new folder's GUID.
*/
PT.NewFolder = DefineTransaction(["parentGuid", "title"],
- ["index", "annotations"]);
+ ["index", "annotations", "children"]);
PT.NewFolder.prototype = Object.seal({
- async execute({ parentGuid, title, index, annotations }) {
- let info = { type: PlacesUtils.bookmarks.TYPE_FOLDER,
- parentGuid, index, title };
+ async execute({ parentGuid, title, index, annotations, children }) {
+ let folderGuid;
+ let info = {
+ children: [{
+ title,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ }],
+ // insertTree uses guid as the parent for where it is being inserted
+ // into.
+ guid: parentGuid,
+ };
+
+ if (children && children.length > 0) {
+ info.children[0].children = children;
+ }
async function createItem() {
- info = await PlacesUtils.bookmarks.insert(info);
+ // Note, insertTree returns an array, rather than the folder/child structure.
+ // For simplicity, we only get the new folder id here. This means that
+ // an undo then redo won't retain exactly the same information for all
+ // the child bookmarks, but we believe that isn't important at the moment.
+ let bmInfo = await PlacesUtils.bookmarks.insertTree(info);
+ // insertTree returns an array, but we only need to deal with the folder guid.
+ folderGuid = bmInfo[0].guid;
+
+ // Bug 1388097: insertTree doesn't handle inserting at a specific index for the folder,
+ // therefore we update the bookmark manually afterwards.
+ if (index != PlacesUtils.bookmarks.DEFAULT_INDEX) {
+ bmInfo[0].index = index;
+ bmInfo = await PlacesUtils.bookmarks.update(bmInfo[0]);
+ }
+
if (annotations.length > 0) {
- let itemId = await PlacesUtils.promiseItemId(info.guid);
+ let itemId = await PlacesUtils.promiseItemId(folderGuid);
PlacesUtils.setAnnotationsForItem(itemId, annotations);
}
}
await createItem();
this.undo = async function() {
- await PlacesUtils.bookmarks.remove(info);
+ await PlacesUtils.bookmarks.remove(folderGuid);
};
this.redo = async function() {
await createItem();
// See the reasoning in CreateItem for why we don't care
// about precisely resetting the lastModified value.
};
- return info.guid;
+ return folderGuid;
}
});
/**
* Transaction for creating a separator.
*
* Required Input Properties: parentGuid.
* Optional Input Properties: index.
--- a/toolkit/components/places/tests/unit/test_async_transactions.js
+++ b/toolkit/components/places/tests/unit/test_async_transactions.js
@@ -4,16 +4,17 @@
* 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/. */
const bmsvc = PlacesUtils.bookmarks;
const tagssvc = PlacesUtils.tagging;
const annosvc = PlacesUtils.annotations;
const PT = PlacesTransactions;
const rootGuid = PlacesUtils.bookmarks.rootGuid;
+const menuGuid = PlacesUtils.bookmarks.menuGuid;
Components.utils.importGlobalProperties(["URL"]);
// Create and add bookmarks observer.
var observer = {
__proto__: NavBookmarkObserver.prototype,
tagRelatedGuids: new Set(),
@@ -156,44 +157,64 @@ function ensureUndoState(aExpectedEntrie
function checkEqualEntries(aExpectedEntry, aActualEntry) {
do_check_eq(aExpectedEntry.length, aActualEntry.length);
aExpectedEntry.forEach( (t, i) => do_check_eq(t, aActualEntry[i]) );
}
aExpectedEntries.forEach( (e, i) => checkEqualEntries(e, actualEntries[i]) );
}
function ensureItemsAdded(...items) {
- Assert.equal(observer.itemsAdded.size, items.length);
+ let expectedResultsCount = items.length;
+
for (let item of items) {
- Assert.ok(observer.itemsAdded.has(item.guid));
+ if ("children" in item) {
+ expectedResultsCount += item.children.length;
+ }
+ Assert.ok(observer.itemsAdded.has(item.guid),
+ `Should have the expected guid ${item.guid}`);
let info = observer.itemsAdded.get(item.guid);
- Assert.equal(info.parentGuid, item.parentGuid);
+ Assert.equal(info.parentGuid, item.parentGuid,
+ "Should have notified the correct parentGuid");
for (let propName of ["title", "index", "itemType"]) {
if (propName in item)
Assert.equal(info[propName], item[propName]);
}
if ("url" in item)
Assert.ok(info.url.equals(item.url));
}
+
+ Assert.equal(observer.itemsAdded.size, expectedResultsCount,
+ "Should have added the correct number of items");
}
function ensureItemsRemoved(...items) {
- Assert.equal(observer.itemsRemoved.size, items.length);
+ let expectedResultsCount = items.length;
+
for (let item of items) {
// We accept both guids and full info object here.
if (typeof(item) == "string") {
- Assert.ok(observer.itemsRemoved.has(item));
+ Assert.ok(observer.itemsRemoved.has(item),
+ `Should have removed the expected guid ${item}`);
} else {
- Assert.ok(observer.itemsRemoved.has(item.guid));
+ if ("children" in item) {
+ expectedResultsCount += item.children.length;
+ }
+
+ Assert.ok(observer.itemsRemoved.has(item.guid),
+ `Should have removed expected guid ${item.guid}`);
let info = observer.itemsRemoved.get(item.guid);
- Assert.equal(info.parentGuid, item.parentGuid);
+ Assert.equal(info.parentGuid, item.parentGuid,
+ "Should have notified the correct parentGuid");
if ("index" in item)
Assert.equal(info.index, item.index);
}
}
+
+ Assert.equal(observer.itemsRemoved.size, expectedResultsCount,
+ "Should have removed the correct number of items");
}
function ensureItemsChanged(...items) {
for (let item of items) {
do_check_true(observer.itemsChanged.has(item.guid));
let changes = observer.itemsChanged.get(item.guid);
do_check_true(changes.has(item.property));
let info = changes.get(item.property);
@@ -240,44 +261,71 @@ function ensureTimestampsUpdated(aGuid,
}
function ensureTagsForURI(aURI, aTags) {
let tagsSet = tagssvc.getTagsForURI(aURI);
do_check_eq(tagsSet.length, aTags.length);
do_check_true(aTags.every( t => tagsSet.includes(t)));
}
-function createTestFolderInfo(aTitle = "Test Folder") {
- return { parentGuid: rootGuid, title: aTitle };
+function createTestFolderInfo(title = "Test Folder", parentGuid = menuGuid,
+ children = undefined) {
+ let info = { parentGuid, title };
+ if (children) {
+ info.children = children;
+ }
+ return info;
}
function isLivemarkTree(aTree) {
return !!aTree.annos &&
aTree.annos.some( a => a.name == PlacesUtils.LMANNO_FEEDURI );
}
async function ensureLivemarkCreatedByAddLivemark(aLivemarkGuid) {
// This throws otherwise.
await PlacesUtils.livemarks.getLivemark({ guid: aLivemarkGuid });
}
+function removeAllDatesInTree(tree) {
+ if ("lastModified" in tree) {
+ delete tree.lastModified;
+ }
+ if ("dateAdded" in tree) {
+ delete tree.dateAdded;
+ }
+
+ if (!tree.children) {
+ return;
+ }
+
+ for (let child of tree.children) {
+ removeAllDatesInTree(child);
+ }
+}
+
// Checks if two bookmark trees (as returned by promiseBookmarksTree) are the
// same.
// false value for aCheckParentAndPosition is ignored if aIsRestoredItem is set.
async function ensureEqualBookmarksTrees(aOriginal,
aNew,
aIsRestoredItem = true,
- aCheckParentAndPosition = false) {
+ aCheckParentAndPosition = false,
+ aIgnoreAllDates = false) {
// Note "id" is not-enumerable, and is therefore skipped by Object.keys (both
// ours and the one at deepEqual). This is fine for us because ids are not
// restored by Redo.
if (aIsRestoredItem) {
- // Ignore lastModified for newly created items, for performance reasons.
- if (!aOriginal.lastModified)
+ if (aIgnoreAllDates) {
+ removeAllDatesInTree(aOriginal);
+ removeAllDatesInTree(aNew);
+ } else if (!aOriginal.lastModified) {
+ // Ignore lastModified for newly created items, for performance reasons.
aNew.lastModified = aOriginal.lastModified;
+ }
Assert.deepEqual(aOriginal, aNew);
if (isLivemarkTree(aNew))
await ensureLivemarkCreatedByAddLivemark(aNew.guid);
return;
}
for (let property of Object.keys(aOriginal)) {
if (property == "children") {
@@ -313,16 +361,24 @@ async function ensureEqualBookmarksTrees
async function ensureBookmarksTreeRestoredCorrectly(...aOriginalBookmarksTrees) {
for (let originalTree of aOriginalBookmarksTrees) {
let restoredTree =
await PlacesUtils.promiseBookmarksTree(originalTree.guid);
await ensureEqualBookmarksTrees(originalTree, restoredTree);
}
}
+async function ensureBookmarksTreeRestoredCorrectlyExceptDates(...aOriginalBookmarksTrees) {
+ for (let originalTree of aOriginalBookmarksTrees) {
+ let restoredTree =
+ await PlacesUtils.promiseBookmarksTree(originalTree.guid);
+ await ensureEqualBookmarksTrees(originalTree, restoredTree, true, false, true);
+ }
+}
+
async function ensureNonExistent(...aGuids) {
for (let guid of aGuids) {
Assert.strictEqual((await PlacesUtils.promiseBookmarksTree(guid)), null);
}
}
add_task(async function test_recycled_transactions() {
async function ensureTransactThrowsFor(aTransaction) {
@@ -379,17 +435,17 @@ add_task(async function test_new_folder_
let originalInfo = await PlacesUtils.promiseBookmarksTree(folder_info.guid);
let ensureDo = async function(aRedo = false) {
ensureUndoState([[txn]], 0);
await ensureItemsAdded(folder_info);
ensureAnnotationsSet(folder_info.guid, [ANNO]);
if (aRedo) {
// Ignore lastModified in the comparison, for performance reasons.
originalInfo.lastModified = null;
- await ensureBookmarksTreeRestoredCorrectly(originalInfo);
+ await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);
}
observer.reset();
};
let ensureUndo = () => {
ensureUndoState([[txn]], 1);
ensureItemsRemoved({ guid: folder_info.guid,
parentGuid: folder_info.parentGuid,
@@ -403,16 +459,61 @@ add_task(async function test_new_folder_
await PT.redo();
await ensureDo(true);
await PT.undo();
ensureUndo();
await PT.clearTransactionsHistory();
ensureUndoState();
});
+add_task(async function test_new_folder_with_children() {
+ let folder_info = createTestFolderInfo("Test folder", PlacesUtils.bookmarks.menuGuid, [{
+ url: "http://test_create_item.com",
+ title: "Test creating an item"
+ }]);
+ ensureUndoState();
+ let txn = PT.NewFolder(folder_info);
+ folder_info.guid = await txn.transact();
+ let originalInfo = await PlacesUtils.promiseBookmarksTree(folder_info.guid);
+ let ensureDo = async function(aRedo = false) {
+ ensureUndoState([[txn]], 0);
+ ensureItemsAdded(folder_info);
+ if (aRedo) {
+ // Ignore lastModified in the comparison, for performance reasons.
+ originalInfo.lastModified = null;
+ await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);
+ }
+ observer.reset();
+ };
+
+ let ensureUndo = () => {
+ ensureUndoState([[txn]], 1);
+ ensureItemsRemoved({
+ guid: folder_info.guid,
+ parentGuid: folder_info.parentGuid,
+ index: bmStartIndex,
+ children: [{
+ title: "Test creating an item",
+ url: "http://test_create_item.com",
+ }]
+ });
+ observer.reset();
+ };
+
+ await ensureDo();
+ await PT.undo();
+ await ensureUndo();
+ await PT.redo();
+ await ensureDo(true);
+ await PT.undo();
+ ensureUndo();
+ await PT.clearTransactionsHistory();
+ ensureUndoState();
+});
+
add_task(async function test_new_bookmark() {
let bm_info = { parentGuid: rootGuid,
url: NetUtil.newURI("http://test_create_item.com"),
index: bmStartIndex,
title: "Test creating an item" };
ensureUndoState();
let txn = PT.NewBookmark(bm_info);
@@ -654,17 +755,17 @@ add_task(async function test_remove_fold
await ensureItemsRemoved(folder_level_2_info, folder_level_1_info);
observer.reset();
// Redo the creation of both folders
await PT.redo();
ensureUndoState([ [remove_folder_2_txn],
[folder_level_2_txn_result, folder_level_1_txn_result] ], 1);
await ensureItemsAdded(folder_level_1_info, folder_level_2_info);
- await ensureBookmarksTreeRestoredCorrectly(original_folder_level_1_tree);
+ await ensureBookmarksTreeRestoredCorrectlyExceptDates(original_folder_level_1_tree);
observer.reset();
// Redo Remove "Folder Level 2"
await PT.redo();
ensureUndoState([ [remove_folder_2_txn],
[folder_level_2_txn_result, folder_level_1_txn_result] ]);
await ensureItemsRemoved(folder_level_2_info);
observer.reset();
@@ -1458,19 +1559,19 @@ add_task(async function test_copy() {
let txn = PT.Copy({ guid: aOriginalGuid, newParentGuid: rootGuid });
let duplicateGuid = await txn.transact();
let originalInfo = await PlacesUtils.promiseBookmarksTree(aOriginalGuid);
let duplicateInfo = await PlacesUtils.promiseBookmarksTree(duplicateGuid);
await ensureEqualBookmarksTrees(originalInfo, duplicateInfo, false);
async function redo() {
await PT.redo();
- await ensureBookmarksTreeRestoredCorrectly(originalInfo);
+ await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo);
await PT.redo();
- await ensureBookmarksTreeRestoredCorrectly(duplicateInfo);
+ await ensureBookmarksTreeRestoredCorrectlyExceptDates(duplicateInfo);
}
async function undo() {
await PT.undo();
// also undo the original item addition.
await PT.undo();
await ensureNonExistent(aOriginalGuid, duplicateGuid);
}
@@ -1598,17 +1699,17 @@ add_task(async function test_invalid_uri
PT.Tag({ tag: "TheTag",
urls: ["invalid uri spec"] }));
Assert.throws(() =>
PT.Tag({ tag: "TheTag",
urls: ["about:blank", "invalid uri spec"] }));
});
add_task(async function test_annotate_multiple_items() {
- let parentGuid = rootGuid;
+ let parentGuid = menuGuid;
let guids = [
await PT.NewBookmark({ url: "about:blank", parentGuid }).transact(),
await PT.NewFolder({ title: "Test Folder", parentGuid }).transact()];
let annotation = { name: "TestAnno", value: "TestValue" };
await PT.Annotate({ guids, annotation }).transact();
async function ensureAnnoSet() {
@@ -1640,27 +1741,27 @@ add_task(async function test_annotate_mu
await PT.clearTransactionsHistory();
observer.reset();
});
add_task(async function test_remove_multiple() {
let guids = [];
await PT.batch(async function() {
let folderGuid = await PT.NewFolder({ title: "Test Folder",
- parentGuid: rootGuid }).transact();
+ parentGuid: menuGuid }).transact();
let nestedFolderGuid =
await PT.NewFolder({ title: "Nested Test Folder",
parentGuid: folderGuid }).transact();
await PT.NewSeparator(nestedFolderGuid).transact();
guids.push(folderGuid);
let bmGuid =
await PT.NewBookmark({ url: new URL("http://test.bookmark.removed"),
- parentGuid: rootGuid }).transact();
+ parentGuid: menuGuid }).transact();
guids.push(bmGuid);
});
let originalInfos = [];
for (let guid of guids) {
originalInfos.push(await PlacesUtils.promiseBookmarksTree(guid));
}
@@ -1674,17 +1775,17 @@ add_task(async function test_remove_mult
await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
// Undo the New* transactions batch.
await PT.undo();
await ensureNonExistent(...guids);
// Redo it.
await PT.redo();
- await ensureBookmarksTreeRestoredCorrectly(...originalInfos);
+ await ensureBookmarksTreeRestoredCorrectlyExceptDates(...originalInfos);
// Redo remove.
await PT.redo();
await ensureNonExistent(...guids);
// Cleanup
await PT.clearTransactionsHistory();
observer.reset();