From 0b94d0fe3eb02cedf23910277c4b6996e2e3b1cf Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Thu, 3 Aug 2017 18:32:42 +0100 Subject: [PATCH] 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 --HG-- extra : rebase_source : 0b4029905dc76a0ca49d16a7e71c85f1f07b8e2d --- .../places/content/bookmarkProperties.js | 21 +-- ...okmarkProperties_addFolderDefaultButton.js | 5 + .../components/places/PlacesTransactions.jsm | 84 ++++++++--- .../tests/unit/test_async_transactions.js | 141 +++++++++++++++--- 4 files changed, 195 insertions(+), 56 deletions(-) diff --git a/browser/components/places/content/bookmarkProperties.js b/browser/components/places/content/bookmarkProperties.js index 1074439d23ae..79f5d1af2849 100644 --- a/browser/components/places/content/bookmarkProperties.js +++ b/browser/components/places/content/bookmarkProperties.js @@ -552,16 +552,10 @@ var BookmarkPropertiesPanel = { _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; @@ -665,14 +659,11 @@ var BookmarkPropertiesPanel = { 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}`); } diff --git a/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js b/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js index b065f1c01425..94eef0e6bc11 100644 --- a/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js +++ b/browser/components/places/tests/browser/browser_bookmarkProperties_addFolderDefaultButton.js @@ -45,6 +45,11 @@ add_task(async function() { }); 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); } diff --git a/toolkit/components/places/PlacesTransactions.jsm b/toolkit/components/places/PlacesTransactions.jsm index c1f56273e300..a936effbbbbd 100644 --- a/toolkit/components/places/PlacesTransactions.jsm +++ b/toolkit/components/places/PlacesTransactions.jsm @@ -726,19 +726,19 @@ function isPrimitive(v) { return v === null || (typeof(v) != "object" && typeof(v) != "function"); } +function checkProperty(obj, prop, required, checkFn) { + if (prop in obj) + return checkFn(obj[prop]); + + return !required; +} + DefineTransaction.annotationObjectValidate = function(obj) { - let checkProperty = (prop, required, checkFn) => { - if (prop in obj) - return checkFn(obj[prop]); - - return !required; - }; - 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))) @@ -747,6 +747,19 @@ DefineTransaction.annotationObjectValidate = function(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); @@ -763,7 +776,7 @@ DefineTransaction.defineInputProps = function(names, validateFn, 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}`); } }, @@ -898,10 +911,13 @@ DefineTransaction.defineInputProps(["index", "newIndex"], 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"); @@ -1102,35 +1118,61 @@ PT.NewBookmark.prototype = Object.seal({ * 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; } }); diff --git a/toolkit/components/places/tests/unit/test_async_transactions.js b/toolkit/components/places/tests/unit/test_async_transactions.js index 10806d09d045..f883b057e38a 100644 --- a/toolkit/components/places/tests/unit/test_async_transactions.js +++ b/toolkit/components/places/tests/unit/test_async_transactions.js @@ -9,6 +9,7 @@ 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"]); @@ -161,11 +162,17 @@ function ensureUndoState(aExpectedEntries = [], aExpectedUndoPosition = 0) { } 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]); @@ -173,22 +180,36 @@ function ensureItemsAdded(...items) { 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) { @@ -245,8 +266,13 @@ function ensureTagsForURI(aURI, aTags) { 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) { @@ -259,20 +285,42 @@ async function ensureLivemarkCreatedByAddLivemark(aLivemarkGuid) { 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); @@ -318,6 +366,14 @@ async function ensureBookmarksTreeRestoredCorrectly(...aOriginalBookmarksTrees) } } +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); @@ -384,7 +440,7 @@ add_task(async function test_new_folder_with_annotation() { if (aRedo) { // Ignore lastModified in the comparison, for performance reasons. originalInfo.lastModified = null; - await ensureBookmarksTreeRestoredCorrectly(originalInfo); + await ensureBookmarksTreeRestoredCorrectlyExceptDates(originalInfo); } observer.reset(); }; @@ -408,6 +464,51 @@ add_task(async function test_new_folder_with_annotation() { 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"), @@ -659,7 +760,7 @@ add_task(async function test_remove_folder() { 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" @@ -1463,9 +1564,9 @@ add_task(async function test_copy() { 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(); @@ -1603,7 +1704,7 @@ add_task(async function test_invalid_uri_spec_throws() { }); 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()]; @@ -1645,7 +1746,7 @@ 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(); @@ -1655,7 +1756,7 @@ add_task(async function test_remove_multiple() { let bmGuid = await PT.NewBookmark({ url: new URL("http://test.bookmark.removed"), - parentGuid: rootGuid }).transact(); + parentGuid: menuGuid }).transact(); guids.push(bmGuid); }); @@ -1679,7 +1780,7 @@ add_task(async function test_remove_multiple() { // Redo it. await PT.redo(); - await ensureBookmarksTreeRestoredCorrectly(...originalInfos); + await ensureBookmarksTreeRestoredCorrectlyExceptDates(...originalInfos); // Redo remove. await PT.redo();