# HG changeset patch # User Mark Banner # Date 1509009254 -3600 # Node ID e33f91893d88b23184152223ca9a293f9215edb8 # Parent 42d311dccb3655338201034888c4d7ef6081de0e Bug 1411891 - Improve the performance of deleting bookmarks with async transactions. r=mak MozReview-Commit-ID: GL9nKfypie1 diff --git a/browser/components/extensions/ext-bookmarks.js b/browser/components/extensions/ext-bookmarks.js --- a/browser/components/extensions/ext-bookmarks.js +++ b/browser/components/extensions/ext-bookmarks.js @@ -312,30 +312,30 @@ this.bookmarks = class extends Extension remove: function(id) { let info = { guid: id, }; // The API doesn't give you the old bookmark at the moment try { - return PlacesUtils.bookmarks.remove(info, {preventRemovalOfNonEmptyFolders: true}).then(result => {}) + return PlacesUtils.bookmarks.remove(info, {preventRemovalOfNonEmptyFolders: true}) .catch(error => Promise.reject({message: error.message})); } catch (e) { return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`}); } }, removeTree: function(id) { let info = { guid: id, }; try { - return PlacesUtils.bookmarks.remove(info).then(result => {}) + return PlacesUtils.bookmarks.remove(info) .catch(error => Promise.reject({message: error.message})); } catch (e) { return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`}); } }, onCreated: new EventManager(context, "bookmarks.onCreated", fire => { let listener = (event, bookmark) => { diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js --- a/browser/components/places/content/controller.js +++ b/browser/components/places/content/controller.js @@ -868,27 +868,33 @@ PlacesController.prototype = { * Creates a set of transactions for the removal of a range of items. * A range is an array of adjacent nodes in a view. * @param [in] range * An array of nodes to remove. Should all be adjacent. * @param [out] transactions * An array of transactions. * @param [optional] removedFolders * An array of folder nodes that have already been removed. + * @return {Integer} The total number of items affected. */ async _removeRange(range, transactions, removedFolders) { NS_ASSERT(transactions instanceof Array, "Must pass a transactions array"); if (!removedFolders) removedFolders = []; + let bmGuidsToRemove = []; + let totalItems = 0; + for (var i = 0; i < range.length; ++i) { var node = range[i]; if (this._shouldSkipNode(node, removedFolders)) continue; + totalItems++; + if (PlacesUtils.nodeIsTagQuery(node.parent)) { // This is a uri node inside a tag container. It needs a special // untag transaction. var tagItemId = PlacesUtils.getConcreteItemId(node.parent); var uri = NetUtil.newURI(node.uri); if (PlacesUIUtils.useAsyncTransactions) { let tag = node.parent.title; if (!tag) { @@ -937,43 +943,47 @@ PlacesController.prototype = { } else { // This is a common bookmark item. if (PlacesUtils.nodeIsFolder(node)) { // If this is a folder we add it to our array of folders, used // to skip nodes that are children of an already removed folder. removedFolders.push(node); } if (PlacesUIUtils.useAsyncTransactions) { - transactions.push( - PlacesTransactions.Remove({ guid: node.bookmarkGuid })); + bmGuidsToRemove.push(node.bookmarkGuid); } else { let txn = new PlacesRemoveItemTransaction(node.itemId); transactions.push(txn); } } } + if (bmGuidsToRemove.length) { + transactions.push(PlacesTransactions.Remove({ guids: bmGuidsToRemove })); + } + return totalItems; }, /** * Removes the set of selected ranges from bookmarks. * @param txnName * See |remove|. */ async _removeRowsFromBookmarks(txnName) { - var ranges = this._view.removableSelectionRanges; - var transactions = []; - var removedFolders = []; + let ranges = this._view.removableSelectionRanges; + let transactions = []; + let removedFolders = []; + let totalItems = 0; for (let range of ranges) { - await this._removeRange(range, transactions, removedFolders); + totalItems += await this._removeRange(range, transactions, removedFolders); } if (transactions.length > 0) { if (PlacesUIUtils.useAsyncTransactions) { - await PlacesUIUtils.batchUpdatesForNode(this._view.result, transactions.length, async () => { + await PlacesUIUtils.batchUpdatesForNode(this._view.result, totalItems, async () => { await PlacesTransactions.batch(transactions); }); } else { var txn = new PlacesAggregatedTransaction(txnName, transactions); PlacesUtils.transactionManager.doTransaction(txn); } } }, diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -737,84 +737,100 @@ var Bookmarks = Object.freeze({ // Remove non-enumerable properties. delete updatedItem.source; return Object.assign({}, updatedItem); }); })(); }, /** - * Removes a bookmark-item. + * Removes one or more bookmark-items. * - * @param guidOrInfo - * The globally unique identifier of the item to remove, or an - * object representing it, as defined above. + * @param guidOrInfo This may be: + * - The globally unique identifier of the item to remove + * - an object representing the item, as defined above + * - an array of objects representing the items to be removed * @param {Object} [options={}] * Additional options that can be passed to the function. * Currently supports the following properties: * - preventRemovalOfNonEmptyFolders: Causes an exception to be * thrown when attempting to remove a folder that is not empty. * - source: The change source, forwarded to all bookmark observers. * Defaults to nsINavBookmarksService::SOURCE_DEFAULT. * - * @return {Promise} resolved when the removal is complete. - * @resolves to an object representing the removed bookmark. + * @return {Promise} + * @resolves when the removal is complete * @rejects if the provided guid doesn't match any existing bookmark. * @throws if the arguments are invalid. */ remove(guidOrInfo, options = {}) { - let info = guidOrInfo; - if (!info) + let infos = guidOrInfo; + if (!infos) throw new Error("Input should be a valid object"); - if (typeof(guidOrInfo) != "object") - info = { guid: guidOrInfo }; - - // Disallow removing the root folders. - if ([this.rootGuid, this.menuGuid, this.toolbarGuid, this.unfiledGuid, - this.tagsGuid, this.mobileGuid].includes(info.guid)) { - throw new Error("It's not possible to remove Places root folders."); + if (!Array.isArray(guidOrInfo)) { + if (typeof(guidOrInfo) != "object") { + infos = [{ guid: guidOrInfo }]; + } else { + infos = [guidOrInfo]; + } } if (!("source" in options)) { options.source = Bookmarks.SOURCES.DEFAULT; } - // Even if we ignore any other unneeded property, we still validate any - // known property to reduce likelihood of hidden bugs. - let removeInfo = validateBookmarkObject("Bookmarks.jsm: remove", info); + let removeInfos = []; + for (let info of infos) { + // Disallow removing the root folders. + if ([ + Bookmarks.rootGuid, Bookmarks.menuGuid, Bookmarks.toolbarGuid, + Bookmarks.unfiledGuid, Bookmarks.tagsGuid, Bookmarks.mobileGuid + ].includes(info.guid)) { + throw new Error("It's not possible to remove Places root folders."); + } + + // Even if we ignore any other unneeded property, we still validate any + // known property to reduce likelihood of hidden bugs. + let removeInfo = validateBookmarkObject("Bookmarks.jsm: remove", info); + removeInfos.push(removeInfo); + } return (async function() { - let item = await fetchBookmark(removeInfo); - if (!item) - throw new Error("No bookmarks found for the provided GUID."); + let removeItems = []; + for (let info of removeInfos) { + let item = await fetchBookmark(info); + if (!item) + throw new Error("No bookmarks found for the provided GUID."); - item = await removeBookmark(item, options); + removeItems.push(item); + } + + await removeBookmarks(removeItems, options); // Notify onItemRemoved to listeners. - let observers = PlacesUtils.bookmarks.getObservers(); - let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null; - let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; - notify(observers, "onItemRemoved", [ item._id, item._parentId, item.index, - item.type, uri, item.guid, - item.parentGuid, - options.source ], - { isTagging: isUntagging }); + for (let item of removeItems) { + let observers = PlacesUtils.bookmarks.getObservers(); + let uri = item.hasOwnProperty("url") ? PlacesUtils.toURI(item.url) : null; + let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; + notify(observers, "onItemRemoved", [ item._id, item._parentId, item.index, + item.type, uri, item.guid, + item.parentGuid, + options.source ], + { isTagging: isUntagging }); - if (isUntagging) { - for (let entry of (await fetchBookmarksByURL(item, true))) { - notify(observers, "onItemChanged", [ entry._id, "tags", false, "", - PlacesUtils.toPRTime(entry.lastModified), - entry.type, entry._parentId, - entry.guid, entry.parentGuid, - "", options.source ]); + if (isUntagging) { + for (let entry of (await fetchBookmarksByURL(item, true))) { + notify(observers, "onItemChanged", [ entry._id, "tags", false, "", + PlacesUtils.toPRTime(entry.lastModified), + entry.type, entry._parentId, + entry.guid, entry.parentGuid, + "", options.source ]); + } } } - - // Remove non-enumerable properties. - return Object.assign({}, item); })(); }, /** * Removes ALL bookmarks, resetting the bookmarks storage to an empty tree. * * Note that roots are preserved, only their children will be removed. * @@ -1509,17 +1525,17 @@ function insertBookmarkTree(items, sourc * * @param {Object} item Livemark item that need to be added. */ async function insertLivemarkData(item) { // Delete the placeholder but note the index of it, so that we can insert the // livemark item at the right place. let placeholder = await Bookmarks.fetch(item.guid); let index = placeholder.index; - await removeBookmark(item, {source: item.source}); + await removeBookmarks([item], {source: item.source}); let feedURI = null; let siteURI = null; item.annos = item.annos.filter(function(aAnno) { switch (aAnno.name) { case PlacesUtils.LMANNO_FEEDURI: feedURI = NetUtil.newURI(aAnno.value); return false; @@ -1786,80 +1802,93 @@ async function fetchBookmarksByParent(db ORDER BY b.position ASC `, { parentGuid: info.parentGuid }); return rowsToItemsArray(rows); } // Remove implementation. -function removeBookmark(item, options) { - return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: removeBookmark", +function removeBookmarks(items, options) { + return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: removeBookmarks", async function(db) { - let urls; - let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; + let urls = []; await db.executeTransaction(async function transaction() { - // If it's a folder, remove its contents first. - if (item.type == Bookmarks.TYPE_FOLDER) { - if (options.preventRemovalOfNonEmptyFolders && item._childCount > 0) { - throw new Error("Cannot remove a non-empty folder."); + let parentGuids = new Set(); + let syncChangeDelta = + PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source); + + for (let item of items) { + parentGuids.add(item.parentGuid); + + // If it's a folder, remove its contents first. + if (item.type == Bookmarks.TYPE_FOLDER) { + if (options.preventRemovalOfNonEmptyFolders && item._childCount > 0) { + throw new Error("Cannot remove a non-empty folder."); + } + urls = urls.concat(await removeFoldersContents(db, [item.guid], options)); } - urls = await removeFoldersContents(db, [item.guid], options); } - // Remove annotations first. If it's a tag, we can avoid paying that cost. - if (!isUntagging) { + for (let chunk of chunkArray(items, SQLITE_MAX_VARIABLE_NUMBER)) { // We don't go through the annotations service for this cause otherwise // we'd get a pointless onItemChanged notification and it would also // set lastModified to an unexpected value. - await removeAnnotationsForItem(db, item._id); + await removeAnnotationsForItems(db, chunk); + + // Remove the bookmarks. + await db.executeCached( + `DELETE FROM moz_bookmarks WHERE guid IN (${ + new Array(chunk.length).fill("?").join(",")})`, + chunk.map(item => item.guid) + ); } - // Remove the bookmark from the database. - await db.executeCached( - `DELETE FROM moz_bookmarks WHERE guid = :guid`, { guid: item.guid }); - - // Fix indices in the parent. - await db.executeCached( - `UPDATE moz_bookmarks SET position = position - 1 WHERE - parent = :parentId AND position > :index - `, { parentId: item._parentId, index: item.index }); + for (let item of items) { + // Fix indices in the parent. + await db.executeCached( + `UPDATE moz_bookmarks SET position = position - 1 WHERE + parent = :parentId AND position > :index + `, { parentId: item._parentId, index: item.index }); - let syncChangeDelta = - PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source); - - // Mark all affected separators as changed - await adjustSeparatorsSyncCounter(db, item._parentId, item.index, syncChangeDelta); + if (item._grandParentId == PlacesUtils.tagsFolderId) { + // If we're removing a tag entry, increment the change counter for all + // bookmarks with the tagged URL. + await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL( + db, item.url, syncChangeDelta); + } - if (isUntagging) { - // If we're removing a tag entry, increment the change counter for all - // bookmarks with the tagged URL. - await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL( - db, item.url, syncChangeDelta); + await adjustSeparatorsSyncCounter(db, item._parentId, item.index, syncChangeDelta); + } + + for (let guid of parentGuids) { + // Mark all affected parents as changed. + await setAncestorsLastModified(db, guid, new Date(), syncChangeDelta); } // Write a tombstone for the removed item. - await insertTombstone(db, item, syncChangeDelta); - - await setAncestorsLastModified(db, item.parentGuid, new Date(), - syncChangeDelta); + await insertTombstones(db, items, syncChangeDelta); }); - // If not a tag recalculate frecency... - if (item.type == Bookmarks.TYPE_BOOKMARK && !isUntagging) { - // ...though we don't wait for the calculation. - updateFrecency(db, [item.url]).catch(Cu.reportError); + // Update the frecencies outside of the transaction, so that the updates + // can progress in the background. + for (let item of items) { + let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; + + // If not a tag recalculate frecency... + if (item.type == Bookmarks.TYPE_BOOKMARK && !isUntagging) { + // ...though we don't wait for the calculation. + updateFrecency(db, [item.url]).catch(Cu.reportError); + } } - if (urls && urls.length && item.type == Bookmarks.TYPE_FOLDER) { + if (urls.length) { updateFrecency(db, urls, true).catch(Cu.reportError); } - - return item; }); } // Reorder implementation. function reorderChildren(parent, orderedChildrenGuids, options) { return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: reorderChildren", db => db.executeTransaction(async function() { @@ -2152,24 +2181,25 @@ var removeOrphanAnnotations = async func `); }; /** * Removes annotations for a given item. * * @param db * the Sqlite.jsm connection handle. - * @param itemId - * internal id of the item for which to remove annotations. + * @param items + * The items for which to remove annotations. */ -var removeAnnotationsForItem = async function(db, itemId) { +var removeAnnotationsForItems = async function(db, items) { + // Remove the annotations. + let ids = sqlList(items.map(item => item._id)); await db.executeCached( - `DELETE FROM moz_items_annos - WHERE item_id = :id - `, { id: itemId }); + `DELETE FROM moz_items_annos WHERE item_id IN (${ids})`, + ); await db.executeCached( `DELETE FROM moz_anno_attributes WHERE id IN (SELECT n.id from moz_anno_attributes n LEFT JOIN moz_annos a1 ON a1.anno_attribute_id = n.id LEFT JOIN moz_items_annos a2 ON a2.anno_attribute_id = n.id WHERE a1.id ISNULL AND a2.id ISNULL) `); }; @@ -2451,8 +2481,16 @@ function* chunkArray(array, chunkLength) yield array; return; } let startIndex = 0; while (startIndex < array.length) { yield array.slice(startIndex, startIndex += chunkLength); } } + +/** + * Convert a list of strings or numbers to its SQL + * representation as a string. + */ +function sqlList(list) { + return list.map(JSON.stringify).join(); +} diff --git a/toolkit/components/places/PlacesTransactions.jsm b/toolkit/components/places/PlacesTransactions.jsm --- a/toolkit/components/places/PlacesTransactions.jsm +++ b/toolkit/components/places/PlacesTransactions.jsm @@ -1482,39 +1482,43 @@ PT.SortByName.prototype = { /** * Transaction for removing an item (any type). * * Required Input Properties: guids. */ PT.Remove = DefineTransaction(["guids"]); PT.Remove.prototype = { async execute({ guids }) { - let promiseBookmarksTree = async function(guid) { - let tree; + let removedItems = []; + + for (let guid of guids) { try { - tree = await PlacesUtils.promiseBookmarksTree(guid); + // Although we don't strictly need to get this information for the remove, + // we do need it for the possibility of undo(). + removedItems.push(await PlacesUtils.promiseBookmarksTree(guid)); } catch (ex) { throw new Error("Failed to get info for the specified item (guid: " + guid + "): " + ex); } - return tree; - }; - let removedItems = []; - for (let guid of guids) { - removedItems.push(await promiseBookmarksTree(guid)); } + let removeThem = async function() { + let bmsToRemove = []; for (let info of removedItems) { if (info.annos && info.annos.some(anno => anno.name == PlacesUtils.LMANNO_FEEDURI)) { await PlacesUtils.livemarks.removeLivemark({ guid: info.guid }); } else { - await PlacesUtils.bookmarks.remove({ guid: info.guid }); + bmsToRemove.push({guid: info.guid}); } } + + if (bmsToRemove.length) { + await PlacesUtils.bookmarks.remove(bmsToRemove); + } }; await removeThem(); this.undo = async function() { for (let info of removedItems) { await createItemsFromBookmarksTree(info, true); } }; diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js @@ -225,34 +225,61 @@ add_task(async function update_move_diff add_task(async function remove_bookmark() { let bm = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK, parentGuid: PlacesUtils.bookmarks.unfiledGuid, url: new URL("http://remove.example.com/") }); let itemId = await PlacesUtils.promiseItemId(bm.guid); let parentId = await PlacesUtils.promiseItemId(bm.parentGuid); let observer = expectNotifications(); - bm = await PlacesUtils.bookmarks.remove(bm.guid); + await PlacesUtils.bookmarks.remove(bm.guid); // TODO (Bug 653910): onItemAnnotationRemoved notified even if there were no // annotations. observer.check([ { name: "onItemRemoved", arguments: [ itemId, parentId, bm.index, bm.type, bm.url, bm.guid, bm.parentGuid, Ci.nsINavBookmarksService.SOURCE_DEFAULT ] } ]); }); +add_task(async function remove_multiple_bookmarks() { + let bm1 = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid, + url: "http://remove.example.com/" }); + let bm2 = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: PlacesUtils.bookmarks.unfiledGuid, + url: "http://remove1.example.com/" }); + let itemId1 = await PlacesUtils.promiseItemId(bm1.guid); + let parentId1 = await PlacesUtils.promiseItemId(bm1.parentGuid); + let itemId2 = await PlacesUtils.promiseItemId(bm2.guid); + let parentId2 = await PlacesUtils.promiseItemId(bm2.parentGuid); + + let observer = expectNotifications(); + await PlacesUtils.bookmarks.remove([bm1, bm2]); + // TODO (Bug 653910): onItemAnnotationRemoved notified even if there were no + // annotations. + observer.check([ { name: "onItemRemoved", + arguments: [ itemId1, parentId1, bm1.index, bm1.type, bm1.url, + bm1.guid, bm1.parentGuid, + Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }, + { name: "onItemRemoved", + arguments: [ itemId2, parentId2, bm2.index, bm2.type, bm2.url, + bm2.guid, bm2.parentGuid, + Ci.nsINavBookmarksService.SOURCE_DEFAULT ] } + ]); +}); + add_task(async function remove_folder() { let bm = await PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER, parentGuid: PlacesUtils.bookmarks.unfiledGuid }); let itemId = await PlacesUtils.promiseItemId(bm.guid); let parentId = await PlacesUtils.promiseItemId(bm.parentGuid); let observer = expectNotifications(); - bm = await PlacesUtils.bookmarks.remove(bm.guid); + await PlacesUtils.bookmarks.remove(bm.guid); observer.check([ { name: "onItemRemoved", arguments: [ itemId, parentId, bm.index, bm.type, null, bm.guid, bm.parentGuid, Ci.nsINavBookmarksService.SOURCE_DEFAULT ] } ]); }); add_task(async function remove_bookmark_tag_notification() { diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js @@ -62,26 +62,25 @@ add_task(async function remove_roots_fai let guids = [PlacesUtils.bookmarks.rootGuid, PlacesUtils.bookmarks.unfiledGuid, PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid, PlacesUtils.bookmarks.tagsGuid, PlacesUtils.bookmarks.mobileGuid]; for (let guid of guids) { Assert.throws(() => PlacesUtils.bookmarks.remove(guid), - /It's not possible to remove Places root folders/); + /It's not possible to remove Places root folders\./); } }); add_task(async function remove_normal_folder_under_root_succeeds() { let folder = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.rootGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER }); checkBookmarkObject(folder); - let removed_folder = await PlacesUtils.bookmarks.remove(folder); - Assert.deepEqual(folder, removed_folder); + await PlacesUtils.bookmarks.remove(folder); Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder.guid)), null); }); add_task(async function remove_bookmark() { // When removing a bookmark we need to check the frecency. First we confirm // that there is a normal update when it is inserted. let frecencyChangedPromise = promiseFrecencyChanged("http://example.com/", UNVISITED_BOOKMARK_BONUS); @@ -91,42 +90,61 @@ add_task(async function remove_bookmark( title: "a bookmark" }); checkBookmarkObject(bm1); await frecencyChangedPromise; // This second one checks the frecency is changed when we remove the bookmark. frecencyChangedPromise = promiseFrecencyChanged("http://example.com/", 0); - let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid); - checkBookmarkObject(bm2); - - Assert.deepEqual(bm1, bm2); - Assert.equal(bm2.parentGuid, PlacesUtils.bookmarks.unfiledGuid); - Assert.equal(bm2.index, 0); - Assert.deepEqual(bm2.dateAdded, bm2.lastModified); - Assert.equal(bm2.type, PlacesUtils.bookmarks.TYPE_BOOKMARK); - Assert.equal(bm2.url.href, "http://example.com/"); - Assert.equal(bm2.title, "a bookmark"); + await PlacesUtils.bookmarks.remove(bm1.guid); await frecencyChangedPromise; }); +add_task(async function remove_multiple_bookmarks() { + // When removing a bookmark we need to check the frecency. First we confirm + // that there is a normal update when it is inserted. + let frecencyChangedPromise = promiseFrecencyChanged("http://example.com/", + UNVISITED_BOOKMARK_BONUS); + let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + url: "http://example.com/", + title: "a bookmark" }); + checkBookmarkObject(bm1); + + let frecencyChangedPromise1 = promiseFrecencyChanged("http://example1.com/", + UNVISITED_BOOKMARK_BONUS); + let bm2 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + url: "http://example1.com/", + title: "a bookmark" }); + checkBookmarkObject(bm2); + + await Promise.all([frecencyChangedPromise, frecencyChangedPromise1]); + + // This second one checks the frecency is changed when we remove the bookmark. + frecencyChangedPromise = promiseFrecencyChanged("http://example.com/", 0); + frecencyChangedPromise1 = promiseFrecencyChanged("http://example1.com/", 0); + + await PlacesUtils.bookmarks.remove([bm1, bm2]); + + await Promise.all([frecencyChangedPromise, frecencyChangedPromise1]); +}); add_task(async function remove_bookmark_orphans() { let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_BOOKMARK, url: "http://example.com/", title: "a bookmark" }); checkBookmarkObject(bm1); PlacesUtils.annotations.setItemAnnotation((await PlacesUtils.promiseItemId(bm1.guid)), "testanno", "testvalue", 0, 0); - let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid); - checkBookmarkObject(bm2); + await PlacesUtils.bookmarks.remove(bm1.guid); // Check there are no orphan annotations. let conn = await PlacesUtils.promiseDBConnection(); let annoAttrs = await conn.execute(`SELECT id, name FROM moz_anno_attributes`); // Bug 1306445 will eventually remove the mobile root anno. Assert.equal(annoAttrs.length, 1); Assert.equal(annoAttrs[0].getResultByName("name"), PlacesUtils.MOBILE_ROOT_ANNO); let annos = await conn.execute(`SELECT item_id, anno_attribute_id FROM moz_items_annos`); @@ -137,40 +155,28 @@ add_task(async function remove_bookmark_ add_task(async function remove_bookmark_empty_title() { let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_BOOKMARK, url: "http://example.com/", title: "" }); checkBookmarkObject(bm1); - let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid); - checkBookmarkObject(bm2); - - Assert.deepEqual(bm1, bm2); - Assert.equal(bm2.index, 0); - Assert.strictEqual(bm2.title, ""); + await PlacesUtils.bookmarks.remove(bm1.guid); + Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null); }); add_task(async function remove_folder() { let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, title: "a folder" }); checkBookmarkObject(bm1); - let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid); - checkBookmarkObject(bm2); - - Assert.deepEqual(bm1, bm2); - Assert.equal(bm2.parentGuid, PlacesUtils.bookmarks.unfiledGuid); - Assert.equal(bm2.index, 0); - Assert.deepEqual(bm2.dateAdded, bm2.lastModified); - Assert.equal(bm2.type, PlacesUtils.bookmarks.TYPE_FOLDER); - Assert.equal(bm2.title, "a folder"); - Assert.ok(!("url" in bm2)); + await PlacesUtils.bookmarks.remove(bm1.guid); + Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null); // No wait for onManyFrecenciesChanged in this test as the folder doesn't have // any children that would need updating. }); add_task(async function test_contents_removed() { let folder1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, @@ -241,39 +247,27 @@ add_task(async function test_nested_cont }); add_task(async function remove_folder_empty_title() { let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, title: "" }); checkBookmarkObject(bm1); - let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid); - checkBookmarkObject(bm2); - - Assert.deepEqual(bm1, bm2); - Assert.equal(bm2.index, 0); - Assert.strictEqual(bm2.title, ""); + await PlacesUtils.bookmarks.remove(bm1.guid); + Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null); }); add_task(async function remove_separator() { let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_SEPARATOR }); checkBookmarkObject(bm1); - let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid); - checkBookmarkObject(bm2); - - Assert.deepEqual(bm1, bm2); - Assert.equal(bm2.parentGuid, PlacesUtils.bookmarks.unfiledGuid); - Assert.equal(bm2.index, 0); - Assert.deepEqual(bm2.dateAdded, bm2.lastModified); - Assert.equal(bm2.type, PlacesUtils.bookmarks.TYPE_SEPARATOR); - Assert.ok(!("url" in bm2)); - Assert.strictEqual(bm2.title, ""); + await PlacesUtils.bookmarks.remove(bm1.guid); + Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null); }); add_task(async function test_nested_content_fails_when_not_allowed() { let folder1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, title: "a folder" }); await PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid, type: PlacesUtils.bookmarks.TYPE_FOLDER, diff --git a/toolkit/modules/NewTabUtils.jsm b/toolkit/modules/NewTabUtils.jsm --- a/toolkit/modules/NewTabUtils.jsm +++ b/toolkit/modules/NewTabUtils.jsm @@ -1141,18 +1141,17 @@ var ActivityStreamLinks = { }, /** * Removes a bookmark * * @param {String} aBookmarkGuid * The bookmark guid associated with the bookmark to remove * - * @returns {Promise} Returns a promise set to an object representing the - * removed bookmark + * @returns {Promise} Returns a promise at completion. */ deleteBookmark(aBookmarkGuid) { return PlacesUtils.bookmarks.remove(aBookmarkGuid); }, /** * Removes a history link and unpins the URL if previously pinned * diff --git a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js --- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js +++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js @@ -618,19 +618,19 @@ add_task(async function activityStream_d await PlacesUtils.bookmarks.insert(placeInfo); } bookmarksSize = await NewTabUtils.activityStreamProvider.getBookmarksSize(); Assert.equal(bookmarksSize, 2, "size 2 for 2 bookmarks added"); let bookmarkGuid = await new Promise(resolve => PlacesUtils.bookmarks.fetch( {url: bookmarks[0].url}, bookmark => resolve(bookmark.guid))); - let deleted = await provider.deleteBookmark(bookmarkGuid); - Assert.equal(deleted.guid, bookmarkGuid, "the correct bookmark was deleted"); - + await provider.deleteBookmark(bookmarkGuid); + Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bookmarkGuid)), null, + "the bookmark should no longer be found"); bookmarksSize = await NewTabUtils.activityStreamProvider.getBookmarksSize(); Assert.equal(bookmarksSize, 1, "size 1 after deleting"); }); add_task(async function activityStream_blockedURLs() { await setUpActivityStreamTest(); let provider = NewTabUtils.activityStreamLinks; @@ -677,9 +677,8 @@ TestProvider.prototype = { let args = Array.prototype.slice.call(arguments, 1); args.unshift(this); for (let obs of this._observers) { if (obs[observerMethodName]) obs[observerMethodName].apply(NewTabUtils.links, args); } }, }; -