From 908c8f81e1add8566569591a4b9b7f1603f80bc7 Mon Sep 17 00:00:00 2001 From: Asaf Romano Date: Fri, 12 Dec 2014 13:02:05 +0200 Subject: [PATCH] Bug 1110101 - Bookmarks.remove doesn't remove folder contents properly. r=mak. --- toolkit/components/places/Bookmarks.jsm | 168 ++++++++++-------- .../bookmarks/test_bookmarks_notifications.js | 64 +++++-- .../tests/bookmarks/test_bookmarks_remove.js | 14 ++ 3 files changed, 158 insertions(+), 88 deletions(-) diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index 75e2a0d6de1c..70137511d1b5 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -456,83 +456,15 @@ let Bookmarks = Object.freeze({ */ eraseEverything: Task.async(function* () { let db = yield DBConnPromised; - yield db.executeTransaction(function* () { - let rows = yield db.executeCached( - `WITH RECURSIVE - descendants(did) AS ( - SELECT b.id FROM moz_bookmarks b - JOIN moz_bookmarks p ON b.parent = p.id - WHERE p.guid IN ( :toolbarGuid, :menuGuid, :unfiledGuid ) - UNION ALL - SELECT id FROM moz_bookmarks - JOIN descendants ON parent = did - ) - SELECT b.id AS _id, b.parent AS _parentId, b.position AS 'index', - b.type, url, b.guid, p.guid AS parentGuid, b.dateAdded, - b.lastModified, b.title, p.parent AS _grandParentId, - NULL AS _childCount, NULL AS keyword - FROM moz_bookmarks b - JOIN moz_bookmarks p ON p.id = b.parent - LEFT JOIN moz_places h ON b.fk = h.id - WHERE b.id IN descendants - `, { menuGuid: this.menuGuid, toolbarGuid: this.toolbarGuid, - unfiledGuid: this.unfiledGuid }); - let items = rowsToItemsArray(rows); - - yield db.executeCached( - `WITH RECURSIVE - descendants(did) AS ( - SELECT b.id FROM moz_bookmarks b - JOIN moz_bookmarks p ON b.parent = p.id - WHERE p.guid IN ( :toolbarGuid, :menuGuid, :unfiledGuid ) - UNION ALL - SELECT id FROM moz_bookmarks - JOIN descendants ON parent = did - ) - DELETE FROM moz_bookmarks WHERE id IN descendants - `, { menuGuid: this.menuGuid, toolbarGuid: this.toolbarGuid, - unfiledGuid: this.unfiledGuid }); - - // Clenup orphans. - yield removeOrphanAnnotations(db); - yield removeOrphanKeywords(db); - - // TODO (Bug 1087576): this may leave orphan tags behind. - - // Update roots' lastModified. - yield db.executeCached( - `UPDATE moz_bookmarks SET lastModified = :time - WHERE id IN (SELECT id FROM moz_bookmarks - WHERE guid IN ( :rootGuid, :toolbarGuid, :menuGuid, :unfiledGuid )) - `, { time: toPRTime(new Date()), rootGuid: this.rootGuid, - menuGuid: this.menuGuid, toolbarGuid: this.toolbarGuid, - unfiledGuid: this.unfiledGuid }); - - let urls = [for (item of items) if (item.url) item.url]; - updateFrecency(db, urls).then(null, Cu.reportError); - - // Send onItemRemoved notifications to listeners. - // TODO (Bug 1087580): this should send a single clear bookmarks - // notification rather than notifying for each bookmark. - - // Notify listeners in reverse order to serve children before parents. - let observers = PlacesUtils.bookmarks.getObservers(); - for (let item of items.reverse()) { - let uri = item.hasOwnProperty("url") ? toURI(item.url) : null; - notify(observers, "onItemRemoved", [ item._id, item._parentId, - item.index, item.type, uri, - item.guid, item.parentGuid ]); - - let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; - if (isUntagging) { - for (let entry of (yield fetchBookmarksByURL(item))) { - notify(observers, "onItemChanged", [ entry._id, "tags", false, "", - toPRTime(entry.lastModified), - entry.type, entry._parentId, - entry.guid, entry.parentGuid ]); - } - } + const folderGuids = [this.toolbarGuid, this.menuGuid, this.unfiledGuid]; + yield removeFoldersContents(db, folderGuids); + const time = toPRTime(new Date()); + for (let folderGuid of folderGuids) { + yield db.executeCached( + `UPDATE moz_bookmarks SET lastModified = :time + WHERE id IN (SELECT id FROM moz_bookmarks WHERE guid = :folderGuid ) + `, { folderGuid, time }); } }.bind(this)); }), @@ -1026,6 +958,10 @@ function* removeBookmark(item) { let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; yield db.executeTransaction(function* transaction() { + // If it's a folder, remove its contents first. + if (item.type == Bookmarks.TYPE_FOLDER) + yield removeFoldersContents(db, [item.guid]); + // Remove annotations first. If it's a tag, we can avoid paying that cost. if (!isUntagging) { // We don't go through the annotations service for this cause otherwise @@ -1414,3 +1350,83 @@ let setAncestorsLastModified = Task.async(function* (db, folderGuid, time) { `, { guid: folderGuid, type: Bookmarks.TYPE_FOLDER, time: toPRTime(time) }); }); + +/** + * Remove all descendants of one or more bookmark folders. + * + * @param db + * the Sqlite.jsm connection handle. + * @param folderGuids + * array of folder guids. + */ +let removeFoldersContents = +Task.async(function* (db, folderGuids) { + let itemsRemoved = []; + for (let folderGuid of folderGuids) { + let rows = yield db.executeCached( + `WITH RECURSIVE + descendants(did) AS ( + SELECT b.id FROM moz_bookmarks b + JOIN moz_bookmarks p ON b.parent = p.id + WHERE p.guid = :folderGuid + UNION ALL + SELECT id FROM moz_bookmarks + JOIN descendants ON parent = did + ) + SELECT b.id AS _id, b.parent AS _parentId, b.position AS 'index', + b.type, url, b.guid, p.guid AS parentGuid, b.dateAdded, + b.lastModified, b.title, p.parent AS _grandParentId, + NULL AS _childCount, NULL AS keyword + FROM moz_bookmarks b + JOIN moz_bookmarks p ON p.id = b.parent + LEFT JOIN moz_places h ON b.fk = h.id + WHERE b.id IN descendants`, { folderGuid }); + + itemsRemoved = itemsRemoved.concat(rowsToItemsArray(rows)); + + yield db.executeCached( + `WITH RECURSIVE + descendants(did) AS ( + SELECT b.id FROM moz_bookmarks b + JOIN moz_bookmarks p ON b.parent = p.id + WHERE p.guid = :folderGuid + UNION ALL + SELECT id FROM moz_bookmarks + JOIN descendants ON parent = did + ) + DELETE FROM moz_bookmarks WHERE id IN descendants`, { folderGuid }); + } + + // Cleanup orphans. + yield removeOrphanAnnotations(db); + yield removeOrphanKeywords(db); + + // TODO (Bug 1087576): this may leave orphan tags behind. + + let urls = [for (item of itemsRemoved) if (item.url) item.url]; + updateFrecency(db, urls).then(null, Cu.reportError); + + // Send onItemRemoved notifications to listeners. + // TODO (Bug 1087580): for the case of eraseEverything, this should send a + // single clear bookmarks notification rather than notifying for each + // bookmark. + + // Notify listeners in reverse order to serve children before parents. + let observers = PlacesUtils.bookmarks.getObservers(); + for (let item of itemsRemoved.reverse()) { + let uri = item.hasOwnProperty("url") ? toURI(item.url) : null; + notify(observers, "onItemRemoved", [ item._id, item._parentId, + item.index, item.type, uri, + item.guid, item.parentGuid ]); + + let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId; + if (isUntagging) { + for (let entry of (yield fetchBookmarksByURL(item))) { + notify(observers, "onItemChanged", [ entry._id, "tags", false, "", + toPRTime(entry.lastModified), + entry.type, entry._parentId, + entry.guid, entry.parentGuid ]); + } + } + } +}); diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js index cff76962b411..b4390ae2cf76 100644 --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js @@ -315,6 +315,46 @@ add_task(function* remove_bookmark_tag_notification() { ]); }); +add_task(function* remove_folder_notification() { + let folder1 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER, + parentGuid: PlacesUtils.bookmarks.unfiledGuid }); + let folder1Id = yield PlacesUtils.promiseItemId(folder1.guid); + let folder1ParentId = yield PlacesUtils.promiseItemId(folder1.parentGuid); + + let bm = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: folder1.guid, + url: new URL("http://example.com/") }); + let bmItemId = yield PlacesUtils.promiseItemId(bm.guid); + + let folder2 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER, + parentGuid: folder1.guid }); + let folder2Id = yield PlacesUtils.promiseItemId(folder2.guid); + + let bm2 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK, + parentGuid: folder2.guid, + url: new URL("http://example.com/") }); + let bm2ItemId = yield PlacesUtils.promiseItemId(bm2.guid); + + let observer = expectNotifications(); + yield PlacesUtils.bookmarks.remove(folder1.guid); + + observer.check([ { name: "onItemRemoved", + arguments: [ bm2ItemId, folder2Id, bm2.index, bm2.type, + bm2.url, bm2.guid, bm2.parentGuid ] }, + { name: "onItemRemoved", + arguments: [ folder2Id, folder1Id, folder2.index, + folder2.type, null, folder2.guid, + folder2.parentGuid ] }, + { name: "onItemRemoved", + arguments: [ bmItemId, folder1Id, bm.index, bm.type, + bm.url, bm.guid, bm.parentGuid ] }, + { name: "onItemRemoved", + arguments: [ folder1Id, folder1ParentId, folder1.index, + folder1.type, null, folder1.guid, + folder1.parentGuid ] } + ]); +}); + add_task(function* eraseEverything_notification() { // Let's start from a clean situation. yield PlacesUtils.bookmarks.eraseEverything(); @@ -348,19 +388,9 @@ add_task(function* eraseEverything_notification() { let menuBmParentId = yield PlacesUtils.promiseItemId(menuBm.parentGuid); let observer = expectNotifications(); - let removed = yield PlacesUtils.bookmarks.eraseEverything(); + yield PlacesUtils.bookmarks.eraseEverything(); observer.check([ { name: "onItemRemoved", - arguments: [ menuBmId, menuBmParentId, - menuBm.index, menuBm.type, - menuBm.url, menuBm.guid, - menuBm.parentGuid ] }, - { name: "onItemRemoved", - arguments: [ toolbarBmId, toolbarBmParentId, - toolbarBm.index, toolbarBm.type, - toolbarBm.url, toolbarBm.guid, - toolbarBm.parentGuid ] }, - { name: "onItemRemoved", arguments: [ folder2Id, folder2ParentId, folder2.index, folder2.type, null, folder2.guid, folder2.parentGuid ] }, @@ -370,7 +400,17 @@ add_task(function* eraseEverything_notification() { { name: "onItemRemoved", arguments: [ folder1Id, folder1ParentId, folder1.index, folder1.type, null, folder1.guid, - folder1.parentGuid ] } + folder1.parentGuid ] }, + { name: "onItemRemoved", + arguments: [ menuBmId, menuBmParentId, + menuBm.index, menuBm.type, + menuBm.url, menuBm.guid, + menuBm.parentGuid ] }, + { name: "onItemRemoved", + arguments: [ toolbarBmId, toolbarBmParentId, + toolbarBm.index, toolbarBm.type, + toolbarBm.url, toolbarBm.guid, + toolbarBm.parentGuid ] } ]); }); diff --git a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js index 54048df4e4f6..a553c3b40e6f 100644 --- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js +++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js @@ -147,6 +147,20 @@ add_task(function* remove_folder() { Assert.ok(!("keyword" in bm2)); }); +add_task(function* test_nested_contents_removed() { + let folder1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, + type: PlacesUtils.bookmarks.TYPE_FOLDER, + title: "a folder" }); + let folder2 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid, + type: PlacesUtils.bookmarks.TYPE_FOLDER, + title: "a folder" }); + let sep = yield PlacesUtils.bookmarks.insert({ parentGuid: folder2.guid, + type: PlacesUtils.bookmarks.TYPE_SEPARATOR }); + yield PlacesUtils.bookmarks.remove(folder1); + Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder1.guid)), null); + Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder2.guid)), null); + Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(sep.guid)), null); +}); add_task(function* remove_folder_empty_title() { let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER,