From 4a667fe56cb6547a2050ec263a27ec924ed68b18 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Tue, 20 Dec 2016 22:49:17 +0000 Subject: [PATCH] Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality, r=mak This patch allows History.removeVisitsByFilter to take `limit` and `url` options restricting which visits are removed. MozReview-Commit-ID: KJpSnMSJnUW --HG-- extra : rebase_source : 03e47bcca21b951f5402c7335a91e601cb0cabfd --- toolkit/components/places/History.jsm | 57 +++++-- .../history/test_removeVisitsByFilter.js | 148 +++++++++++++----- 2 files changed, 154 insertions(+), 51 deletions(-) diff --git a/toolkit/components/places/History.jsm b/toolkit/components/places/History.jsm index f903d19d4e63..6978b48072d1 100644 --- a/toolkit/components/places/History.jsm +++ b/toolkit/components/places/History.jsm @@ -328,6 +328,9 @@ this.History = Object.freeze({ * been added since this date (inclusive). * - endDate: (Date) Remove visits that have * been added before this date (inclusive). + * - limit: (Number) Limit the number of visits + * we remove to this number + * - url: (URL) Only remove visits to this URL * If both `beginDate` and `endDate` are specified, * visits between `beginDate` (inclusive) and `end` * (inclusive) are removed. @@ -352,6 +355,8 @@ this.History = Object.freeze({ let hasBeginDate = "beginDate" in filter; let hasEndDate = "endDate" in filter; + let hasURL = "url" in filter; + let hasLimit = "limit" in filter; if (hasBeginDate) { ensureDate(filter.beginDate); } @@ -361,10 +366,22 @@ this.History = Object.freeze({ if (hasBeginDate && hasEndDate && filter.beginDate > filter.endDate) { throw new TypeError("`beginDate` should be at least as old as `endDate`"); } - if (!hasBeginDate && !hasEndDate) { + if (!hasBeginDate && !hasEndDate && !hasURL && !hasLimit) { throw new TypeError("Expected a non-empty filter"); } + if (hasURL && !(filter.url instanceof URL) && typeof filter.url != "string" && + !(filter.url instanceof Ci.nsIURI)) { + throw new TypeError("Expected a valid URL for `url`"); + } + + if (hasLimit && + (typeof filter.limit != "number" || + filter.limit <= 0 || + !Number.isInteger(filter.limit))) { + throw new TypeError("Expected a non-zero positive integer as a limit"); + } + if (onResult && typeof onResult != "function") { throw new TypeError("Invalid function: " + onResult); } @@ -780,27 +797,43 @@ var removeVisitsByFilter = Task.async(function*(db, filter, onResult = null) { // 1. Determine visits that took place during the interval. Note // that the database uses microseconds, while JS uses milliseconds, // so we need to *1000 one way and /1000 the other way. - let dates = { - conditions: [], - args: {}, - }; + let conditions = []; + let args = {}; if ("beginDate" in filter) { - dates.conditions.push("visit_date >= :begin * 1000"); - dates.args.begin = Number(filter.beginDate); + conditions.push("v.visit_date >= :begin * 1000"); + args.begin = Number(filter.beginDate); } if ("endDate" in filter) { - dates.conditions.push("visit_date <= :end * 1000"); - dates.args.end = Number(filter.endDate); + conditions.push("v.visit_date <= :end * 1000"); + args.end = Number(filter.endDate); } + if ("limit" in filter) { + args.limit = Number(filter.limit); + } + + let optionalJoin = ""; + if ("url" in filter) { + let url = filter.url; + if (url instanceof Ci.nsIURI) { + url = filter.url.spec; + } else { + url = new URL(url).href; + } + optionalJoin = `JOIN moz_places h ON h.id = v.place_id`; + conditions.push("h.url_hash = hash(:url)", "h.url = :url"); + args.url = url; + } + let visitsToRemove = []; let pagesToInspect = new Set(); let onResultData = onResult ? [] : null; yield db.executeCached( - `SELECT id, place_id, visit_date / 1000 AS date, visit_type FROM moz_historyvisits - WHERE ${ dates.conditions.join(" AND ") }`, - dates.args, + `SELECT v.id, place_id, visit_date / 1000 AS date, visit_type FROM moz_historyvisits v + ${optionalJoin} + WHERE ${ conditions.join(" AND ") }${ args.limit ? " LIMIT :limit" : "" }`, + args, row => { let id = row.getResultByName("id"); let place_id = row.getResultByName("place_id"); diff --git a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js index cda75bb2c283..699420e432a4 100644 --- a/toolkit/components/places/tests/history/test_removeVisitsByFilter.js +++ b/toolkit/components/places/tests/history/test_removeVisitsByFilter.js @@ -24,15 +24,25 @@ add_task(function* test_removeVisitsByFilter() { // Populate the database. // Create `SAMPLE_SIZE` visits, from the oldest to the newest. - let bookmarks = new Set(options.bookmarks); + let bookmarkIndices = new Set(options.bookmarks); let visits = []; - let visitByURI = new Map(); + let frecencyChangePromises = new Map(); + let uriDeletePromises = new Map(); + let getURL = options.url ? + i => "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/byurl/" + Math.floor(i / (SAMPLE_SIZE / 5)) + "/" : + i => "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/" + i + "/" + Math.random(); for (let i = 0; i < SAMPLE_SIZE; ++i) { - let spec = "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/" + i + "/" + Math.random(); + let spec = getURL(i); let uri = NetUtil.newURI(spec); let jsDate = new Date(Number(referenceDate) + 3600 * 1000 * i); let dbDate = jsDate * 1000; - let hasBookmark = bookmarks.has(i); + let hasBookmark = bookmarkIndices.has(i); + let hasOwnBookmark = hasBookmark; + if (!hasOwnBookmark && options.url) { + // Also mark as bookmarked if one of the earlier bookmarked items has the same URL. + hasBookmark = + options.bookmarks.filter(n => n < i).some(n => visits[n].uri.spec == spec && visits[n].test.hasBookmark); + } do_print("Generating " + uri.spec + ", " + dbDate); let visit = { uri, @@ -53,8 +63,7 @@ add_task(function* test_removeVisitsByFilter() { }, }; visits.push(visit); - visitByURI.set(spec, visit); - if (hasBookmark) { + if (hasOwnBookmark) { do_print("Adding a bookmark to visit " + i); yield PlacesUtils.bookmarks.insert({ url: uri, @@ -83,14 +92,41 @@ add_task(function* test_removeVisitsByFilter() { filter.endDate = new Date(ms); endIndex = options.end; } - for (let i = beginIndex; i <= endIndex; ++i) { - let test = visits[i].test; - do_print("Marking visit " + i + " as expecting removal"); + if ("limit" in options) { + endIndex = beginIndex + options.limit - 1; // -1 because the start index is inclusive. + filter.limit = options.limit; + } + let removedItems = visits.slice(beginIndex); + endIndex -= beginIndex; + if (options.url) { + let rawURL = ""; + switch (options.url) { + case 1: + filter.url = new URL(removedItems[0].uri.spec); + rawURL = filter.url.href; + break; + case 2: + filter.url = removedItems[0].uri; + rawURL = filter.url.spec; + break; + case 3: + filter.url = removedItems[0].uri.spec; + rawURL = filter.url; + break; + } + endIndex = Math.min(endIndex, removedItems.findIndex((v, index) => v.uri.spec != rawURL) - 1); + } + removedItems.splice(endIndex + 1); + let remainingItems = visits.filter(v => !removedItems.includes(v)); + for (let i = 0; i < removedItems.length; i++) { + let test = removedItems[i].test; + do_print("Marking visit " + (beginIndex + i) + " as expecting removal"); test.toRemove = true; - if (test.hasBookmark) { - test.onFrecencyChanged = PromiseUtils.defer(); - } else { - test.onDeleteURI = PromiseUtils.defer(); + if (test.hasBookmark || + (options.url && remainingItems.some(v => v.uri.spec == removedItems[i].uri.spec))) { + frecencyChangePromises.set(removedItems[i].uri.spec, PromiseUtils.defer()); + } else if (!options.url || i == 0) { + uriDeletePromises.set(removedItems[i].uri.spec, PromiseUtils.defer()); } } @@ -112,23 +148,21 @@ add_task(function* test_removeVisitsByFilter() { }, onFrecencyChanged: function(aURI) { do_print("onFrecencyChanged " + aURI.spec); - let visit = visitByURI.get(aURI.spec); - Assert.ok(!!visit.test.onFrecencyChanged, "Observing onFrecencyChanged"); - visit.test.onFrecencyChanged.resolve(); + let deferred = frecencyChangePromises.get(aURI.spec); + Assert.ok(!!deferred, "Observing onFrecencyChanged"); + deferred.resolve(); }, onManyFrecenciesChanged: function() { do_print("Many frecencies changed"); - for (let visit of visits) { - if (visit.onFrecencyChanged) { - visit.onFrecencyChanged.resolve(); - } + for (let [, deferred] of frecencyChangePromises) { + deferred.resolve(); } }, onDeleteURI: function(aURI) { do_print("onDeleteURI " + aURI.spec); - let visit = visitByURI.get(aURI.spec); - Assert.ok(!!visit.test.onDeleteURI, "Observing onDeleteURI"); - visit.test.onDeleteURI.resolve(); + let deferred = uriDeletePromises.get(aURI.spec); + Assert.ok(!!deferred, "Observing onDeleteURI"); + deferred.resolve(); }, onDeleteVisits: function(aURI) { // Not sure we can test anything. @@ -166,9 +200,10 @@ add_task(function* test_removeVisitsByFilter() { for (let i = 0; i < visits.length; ++i) { let visit = visits[i]; do_print("Controlling the results on visit " + i); + let remainingVisitsForURI = remainingItems.filter(v => visit.uri.spec == v.uri.spec).length; Assert.equal( - visits_in_database(visit.uri) == 0, - visit.test.toRemove, + visits_in_database(visit.uri), + remainingVisitsForURI, "Visit is still present iff expected"); if (options.useCallback) { Assert.equal( @@ -176,26 +211,18 @@ add_task(function* test_removeVisitsByFilter() { visit.test.announcedByOnRow, "Visit removal has been announced by onResult iff expected"); } - if (visit.test.hasBookmark || !visit.test.toRemove) { - Assert.notEqual(page_in_database(visit.uri), 0, "The page is should still appear in the db"); + if (visit.test.hasBookmark || remainingVisitsForURI) { + Assert.notEqual(page_in_database(visit.uri), 0, "The page should still appear in the db"); } else { Assert.equal(page_in_database(visit.uri), 0, "The page should have been removed from the db"); } } // Make sure that the observer has been called wherever applicable. - for (let visit of visits) { - do_print("Making sure that the observer has been called for " + visit.uri.spec); - let test = visit.test; - do_print("Checking onFrecencyChanged"); - if (test.onFrecencyChanged) { - yield test.onFrecencyChanged.promise; - } - do_print("Checking onDeleteURI"); - if (test.onDeleteURI) { - yield test.onDeleteURI.promise; - } - } + do_print("Checking URI delete promises."); + yield Promise.all(Array.from(uriDeletePromises.values())); + do_print("Checking frecency change promises."); + yield Promise.all(Array.from(frecencyChangePromises.values())); PlacesUtils.history.removeObserver(observer); }); @@ -205,6 +232,8 @@ add_task(function* test_removeVisitsByFilter() { {end: 19}, {begin: 0, end: 10}, {begin: 3, end: 4}, + {begin: 5, end: 8, limit: 2}, + {begin: 10, end: 18, limit: 5}, ]) { for (let bookmarks of [[], [5, 6]]) { let options = { @@ -217,6 +246,15 @@ add_task(function* test_removeVisitsByFilter() { if ("end" in range) { options.end = range.end; } + if ("limit" in range) { + options.limit = range.limit; + } + yield remover(options); + options.url = 1; + yield remover(options); + options.url = 2; + yield remover(options); + options.url = 3; yield remover(options); } } @@ -253,6 +291,38 @@ add_task(function* test_error_cases() { () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date(1000), endDate: new Date(0)}), /TypeError: `beginDate` should be at least as old/ ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({limit: {}}), + /Expected a non-zero positive integer as a limit/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({limit: -1}), + /Expected a non-zero positive integer as a limit/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({limit: 0.1}), + /Expected a non-zero positive integer as a limit/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({limit: Infinity}), + /Expected a non-zero positive integer as a limit/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({url: {}}), + /Expected a valid URL for `url`/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({url: 0}), + /Expected a valid URL for `url`/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date(1000), endDate: new Date(0)}), + /TypeError: `beginDate` should be at least as old/ + ); + Assert.throws( + () => PlacesUtils.history.removeVisitsByFilter({beginDate: new Date(1000), endDate: new Date(0)}), + /TypeError: `beginDate` should be at least as old/ + ); }); add_task(function* test_orphans() {