Spinoff from bug 834545.

Is this API described anywhere? What is its purpose?

More specifically: is this the replacement for removeVisitsByTimeframe?

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1) > Is this API described anywhere? What is its purpose? We have splitted the original remove into 2 methods. history.remove will just remove single uris/guids or array of uris/guids history.removeByFilter will replace the other removal methods like removePagesByHost, removeVisitsByTimeframe and so on. we had to do the splitting cause otherwise the input/output was becoming too much error-prone and we would have got spaghetti-code pretty soon.

So, yes, it is a superset of `removeVisitsByTimeFrame`.

Comment on attachment 8512599 [details] [diff] [review] Possible API Review of attachment 8512599 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.jsm @@ +326,5 @@ > + * @param filter: (string) > + * A hostname. Remove all visits to this host. If the hosts > + * starts with a wildcard (e.g. "*.mozilla.org"), also > + * remove visits to subdomains, i.e. "support.mozilla.org", > + * "nightly.mozilla.org", etc. In this case, I'd honestly prefer if all options would be named. Since basically this API has 2 behaviors, pages or visits, and host removals are pages, I think we could make filters be { pages: { begin: , end: , host: } } or { visits: { begin:, end: } } only one of these 2 filters can be provided in a call, or we should throw. that will reduce risk of usage mistakes, since we are putting many APIs in one. The documentation could say we accept pages or visits filters, valid options for pages are... while valid options for visits are... options are ANDed @@ +350,5 @@ > + * all pages that have been visited at least once until `end`. If `end` > + * is omitted, remove all pages that have been visited at least once > + * since `begin`. > + * > + * or: (Array) We don't have use-cases for this now, so I think we should start with a simpler API, as I explained above. In future we might always expand this, but for now we are keeping the APIs strict to our use-cases (one of the big mistakes in the original Places design, was trying to make it too much generic). @@ +360,5 @@ > + * @param onResult: (function(PageInfo)) > + * A callback invoked for each page found. > + * > + * @return (Promise) > + * A promise resoled once the operation is complete. typo: resoled fwiw, I think it's not useful to document "resolved once the operation is complete", that's the purpose of a Promise in general, I'd just say return {Promise} @@ +377,5 @@ > + * > + * @return (Promise) > + * A promise resoled once the operation is complete. > + * @resolve (bool) > + * `true` if at least one page was removed, `false` otherwise. This may be expensive to know, since we may not need to fetch removed pages on clear and we also likely don't have a use-case for that. I'd say to not resolve to anything.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6) > Since basically this API has 2 behaviors, pages or visits, and host removals > are pages, I think we could make filters be > > { pages: { begin: , end: , host: } } > or > { visits: { begin:, end: } } Sounds good. > only one of these 2 filters can be provided in a call, or we should throw. > that will reduce risk of usage mistakes, since we are putting many APIs in > one. So you want the options to be ORed? That's certainly possible. > The documentation could say we accept pages or visits filters, valid options > for pages are... while valid options for visits are... Ok.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7) > > only one of these 2 filters can be provided in a call, or we should throw. > > that will reduce risk of usage mistakes, since we are putting many APIs in > > one. > > So you want the options to be ORed? That's certainly possible. not sure if I'd call that ORed, what I want is that if one passed in an object that has both pages and visits properties, we will throw. the consumer should choose between pages or visits.

What I mean is that if you have an array of several `pages` (respectively several `visits`), we remove all the pages (respectively visits) that match any of the filters, e.g. removeByFilter([ { pages: {host: "*.microsoft.com", begin: someDate} }, { pages: {host: "*.apple.com", begin: someDate} } ]) will remove pages on both microsoft.com and apple.com starting at `someDate`. Right?

I think we should not even take an array. our use-cases are much simpler than that. why not just take a filter object?

Ok, fair enough.

Here we go.

Review ping?

Comment on attachment 8524882 [details] [diff] [review] Implementation Review of attachment 8524882 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.jsm @@ +304,5 @@ > }); > }, > > /** > + * Remove visits from the database. s/from the database/matching specific characteristics./ (the fact there is a db below should ideally be an implementation detail) please add * Any change may be observed through nsINavHistoryObserver. @@ +307,5 @@ > /** > + * Remove visits from the database. > + * > + * @param filter: (object) > + * Remove a set of visits from the database. duped comment, please remove this line @@ +309,5 @@ > + * > + * @param filter: (object) > + * Remove a set of visits from the database. > + * The `object` may contain some of the following > + * fields: nit: s/fields/properties/ @@ +313,5 @@ > + * fields: > + * - begin: (Date) Remove visits that have been > + * visited since this date (inclusive). > + * - end: (Date) Remove visits that have been > + * visited before this date (inclusive). I'd prefer these to be more specific like beginDate, endDate. Begin, end could indeed refer to anything, not just dates. @@ +315,5 @@ > + * visited since this date (inclusive). > + * - end: (Date) Remove visits that have been > + * visited before this date (inclusive). > + * If both `begin` and `end` are specified, visits > + * between `begin` and `end` are removed. nit: I'd repeat (for the sake of clarity) that extremes are included. @@ +469,5 @@ > throw new TypeError("Invalid url or guid: " + key); > } > > /** > + * Throw if an object is not a string. s/a string/a Date object/ @@ +532,5 @@ > + WHERE id IN ( ${ sqlList(idList) } )`); > +}); > + > +/** > + * Clean up pages, notify observers. please expand this a little bit explaining this invalidates frecency for remaining pages, removes orphan pages and then notifies. @@ +538,5 @@ > + * @param db: (Sqlite connection) > + * The database. > + * @param pages: (Array of objects) > + * Pages that have been touched and that need cleaning up. > + * The fields: Each object has the following properties: @@ +565,5 @@ > + if (++notifiedCount % NOTIFICATION_CHUNK_SIZE == 0) { > + // Every few notifications, yield time back to the main > + // thread to avoid jank. > + yield Promise.resolve(); > + } General comment: unfortunately we cannot yet batch here (see UpdateBatchScoper here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#2729) we must pay attention when converting consumers, removing thousands visits while an history view is open could become a perf nightmare.Bug 937560 might help here. Should be fine for the forget button, but we should wait before converting date containers in the Library/Sidebar. @@ +608,5 @@ > + let transition = row.getResultByName("visit_type"); > + let info = { > + date: date, > + transition: transition, > + }; please use ES6 shorthands: info = { date, transition }; @@ +610,5 @@ > + date: date, > + transition: transition, > + }; > + try { > + yield onResult(info); Hm... I actually don't think we should yield on the callback. And likely we are doing the same in remove. I don't see any reason to wait for the callback before proceeding, it just sounds like a recipe for jank... If the consumer wants to enqueue its work, it should probably implement its own queue. There's another problem here that I just noticed, and it's also valid in remove(), we are notifying BEFORE the removal has happened, while the old APIs were notifying after the fact. This subtle change in the behavior could become a nightmare when we start converting consumers, we should try to preserve the existing behavior if possible, both here and in remove. @@ +616,5 @@ > + // Errors should be reported but should not stop `remove`. > + Promise.reject(ex); > + } > + } > + }) missing semicolon @@ +622,5 @@ > + > + if (visitsToRemove.length == 0) { > + // Nothing to do > + return false; > + } you forgot to call PlacesUtils.history.clearEmbedVisits(); before exiting. That should be done regardless, also if there's no visits to remove. @@ +633,5 @@ > + // 3. Find out which pages have been orphaned > + let pages = []; > + yield db.execute( > + `SELECT id, url, guid, > + (foreign_count == 0 AND last_visit_date ISNULL) AS orphaned there is no double "=" in SQL standard as far as i remember (And this seems to confirm my recalls https://mariadb.com/kb/en/sql-99/29-simple-search-conditions/predicates/). Sqlite accepts == and forgives developers, but it's very uncommon to see it used in SQL. Please use the more common single equal. @@ +645,5 @@ > + let url = row.getResultByName("url"); > + let page = { > + id: id, > + guid: guid, > + toRemove: toRemove, please use shorthands: { id, guid, toRemove, uri: new URL(url) } would work @@ +687,5 @@ > let page = { > id: id, > guid: guid, > toRemove: toRemove, > + uri: new URL(url), If this changes from URI to url, please also rename the property to url, for code clarity. ::: toolkit/components/places/tests/head_common.js @@ +303,5 @@ > + do_print("page_in_database: " + stmt.getInt64(0)); > + } > + return found; > + > +/* if (!stmt.executeStep()) I don't understand the changes here, only one page can be found for a given url, so a while doesn't make sense, an if is more than enough I'm fine with returning a bool... so you could just return stmt.executeStep() @@ +937,5 @@ > aResultCode); > deferred.reject(ex); > }, > + handleResult: function () { > + }, why this change? (fwiw I'd be happy with an "handleResult() {},") ::: toolkit/components/places/tests/unit/history/test_removeVisitsByFilter.js @@ +1,5 @@ > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim:set ts=2 sw=2 sts=2 et: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * 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/. */ please use PD header in tests @@ +20,5 @@ > + deferred.resolve = resolve; > + deferred.reject = reject; > + }); > + return deferred; > +} use PromiseUtils.defer() instead? @@ +159,5 @@ > + if (Math.abs(visit.test.jsDate - info.date) < 100) { // Assume rounding errors > + Assert.ok(!visit.test.announcedByOnRow, > + "This is the first time we announce the removal of this visit"); > + Assert.ok(visit.test.toRemove, > + "This is a visit we intended to remove"); there is a bit of confusion around visit/page here, but I understand is a confusion that exists in history... @@ +177,5 @@ > + "Correct result"); > + > + // Make sure that we have eliminated exactly the entries we expected > + // to eliminate. > + visits.forEach((visit, i) => { nit: I'd prefer a common for loop (its more yield friendly) @@ +240,5 @@ > + > +// Test the various error cases > +add_task(function* test_error_cases() { > + Assert.throws( > + () => PlacesUtils.history.removeVisitsByFilter(), please also test the case you pass into something that is not an object, and an object with no properties ::: toolkit/components/places/tests/unit/history/xpcshell.ini @@ +6,1 @@ > [test_remove.js] please keep the tests lists in alphabetical order

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #14) > General comment: unfortunately we cannot yet batch here (see > UpdateBatchScoper here > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/ > nsNavHistory.cpp#2729) we must pay attention when converting consumers, > removing thousands visits while an history view is open could become a perf > nightmare.Bug 937560 might help here. > Should be fine for the forget button, but we should wait before converting > date containers in the Library/Sidebar. I don't follow. Does this mean you want me to remove the yields? > @@ +610,5 @@ > > + date: date, > > + transition: transition, > > + }; > > + try { > > + yield onResult(info); > > Hm... I actually don't think we should yield on the callback. > > And likely we are doing the same in remove. > > I don't see any reason to wait for the callback before proceeding, it just > sounds like a recipe for jank... If the consumer wants to enqueue its work, > it should probably implement its own queue. Iirc, I was following the behavior of Sqlite.jsm. > There's another problem here that I just noticed, and it's also valid in > remove(), we are notifying BEFORE the removal has happened, while the old > APIs were notifying after the fact. > This subtle change in the behavior could become a nightmare when we start > converting consumers, we should try to preserve the existing behavior if > possible, both here and in remove. Ok, will do. > > @@ +616,5 @@ > > + // Errors should be reported but should not stop `remove`. > > + Promise.reject(ex); > > + } > > + } > > + }) > > missing semicolon Told you I work better when I don't break my js2-mode :) > ::: toolkit/components/places/tests/head_common.js > @@ +303,5 @@ > > + do_print("page_in_database: " + stmt.getInt64(0)); > > + } > > + return found; > > + > > +/* if (!stmt.executeStep()) > > I don't understand the changes here, only one page can be found for a given > url, so a while doesn't make sense, an if is more than enough > > I'm fine with returning a bool... so you could just return stmt.executeStep() Oops, sorry, that was debugging code. > @@ +937,5 @@ > > aResultCode); > > deferred.reject(ex); > > }, > > + handleResult: function () { > > + }, > > why this change? (fwiw I'd be happy with an "handleResult() {},") Oops, sorry, leftover from debugging. > use PromiseUtils.defer() instead? Sure, now that it has landed.

(In reply to David Rajchenbach-Teller [:Yoric] (away until November 17th - use "needinfo") from comment #15) > (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #14) > > General comment: unfortunately we cannot yet batch here (see > > UpdateBatchScoper here > > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/ > > nsNavHistory.cpp#2729) we must pay attention when converting consumers, > > removing thousands visits while an history view is open could become a perf > > nightmare.Bug 937560 might help here. > > Should be fine for the forget button, but we should wait before converting > > date containers in the Library/Sidebar. > > I don't follow. Does this mean you want me to remove the yields? no, it was a general comment just to annotate things, there's nothing actionable here yet.. when we will convert views, we should pay more attention to the perf issues.

Applied feedback.

Comment on attachment 8527732 [details] [diff] [review] Implementation, v2 Review of attachment 8527732 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.jsm @@ +91,4 @@ > * may emit before we yield. > */ > const NOTIFICATION_CHUNK_SIZE = 300; > +const ONRESULT_CHUNK_SIZE = 300; fwiw, I think it's not worth to distinguish these result callbacks are still a sort of notification @@ +327,5 @@ > + * > + * @return (Promise) > + * @resolve (bool) > + * `true` if at least one visit was removed, `false` > + * otherwise. nit: "whether at least one visit was removed." @@ +518,5 @@ > }); > > > +/** > + * Remove a list of pages from `moz_places` from their id. s/from/by/ @@ +563,5 @@ > + yield removePagesById(db, [p.id for (p of pages) if (p.toRemove)]); > + > + let notifiedCount = 0; > + for (let {guid, url, toRemove} of pages) { > + gNotifier.notifyOnPageExpired( I know this comes late in the game, but I just added a notify() helper in History.jsm and I'd be happy if you'd use that now since we want to remove notifyOnPageExpired. basically instead of going through notifyOnPageExpired you directly notify to the observers: let observers = PlacesUtils.history.getObservers(); notify(observers, notification_name, arguments); note that you'll have to split your pages into those to remove and those not, and notify onDeleteURI or onDeleteVisits to the 2 groups. If you prefer to do this in a follow-up bug/patch I'm fine with that provided it happens soon. @@ +598,5 @@ > + for (let info of data) { > + try { > + onResult(info); > + } catch (ex) { > + // Errors should be reported but should not stop the operaiton. typo: operaiton @@ +670,5 @@ > + `SELECT id, url, guid, > + (foreign_count = 0 AND last_visit_date ISNULL) AS orphaned > + FROM moz_places > + WHERE id IN (${ sqlList([...pagesToInspect]) })`, > + null, please indent like `SELECT ... `, null, @@ +671,5 @@ > + (foreign_count = 0 AND last_visit_date ISNULL) AS orphaned > + FROM moz_places > + WHERE id IN (${ sqlList([...pagesToInspect]) })`, > + null, > + (row) => { nit: no need for the rounded parenthesis @@ +685,5 @@ > + // 4. Clean up and notify > + yield cleanupPagesAndNotify(db, pages); > + }); > + > + /*don't wait*/ notifyOnResult(onResultData, onResult); please don't use multiline comments like this, since for debugging purposes is often useful to comment out parts of code and this kind of comments makes that a nightmare. I'm fine with a suffix // line comment @@ +687,5 @@ > + }); > + > + /*don't wait*/ notifyOnResult(onResultData, onResult); > + } finally { > + PlacesUtils.history.clearEmbedVisits(); please add a comment like "// Ensure we cleanup embed visits in any case." @@ +750,1 @@ > `); splinter is confusing here, please ensure the indentation is right @@ +755,2 @@ > > + /* don't wait */ notifyOnResult(onResultData, onResult); ditto on multiline comment ::: toolkit/components/places/tests/history/test_remove.js @@ +1,4 @@ > /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > /* vim:set ts=2 sw=2 sts=2 et: */ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ based on a recent policy update, we can stop adding license headers to test files, and they will default to PD. So, it's fine to NOT have a header.

I decided to go for the using your `notify` function, which required a few changes.

Comment on attachment 8533790 [details] [diff] [review] 2. Cumulative patch Review of attachment 8533790 [details] [diff] [review]: ----------------------------------------------------------------- The tests disappeared from this one, but I guess you did that on purpose to help the review process. Just ensure to land those :)

Will do :) Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=64d923a92e3a

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/36679c5f568e for https://treeherder.mozilla.org/logviewer.html#?job_id=4529864&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=4527142&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=4527129&repo=mozilla-inbound. Equally possible that only Linux32 hates you, or that the other platforms that also do haven't gotten around to finishing yet.

Fixed a typo. This won't change the linux32 results.

/r/3983 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak /r/3985 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak /r/3987 - Bug 1089695 - Sanitize now uses PlacesUtils.history.removeVisitsByFilter Pull down these commits: hg pull review -r 32cb9c7b31d0eb6334df45e614e5131fcf7e3371

Comment on attachment 8565980 [details] MozReview Request: bz://1076775/Yoric /r/3983 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak /r/3985 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak /r/3987 - Bug 1089695 - Sanitize now uses PlacesUtils.history.removeVisitsByFilter Pull down these commits: hg pull review -r 32cb9c7b31d0eb6334df45e614e5131fcf7e3371

https://reviewboard.mozilla.org/r/3983/#review3185 Ship It!

https://reviewboard.mozilla.org/r/3981/#review4027 This has been replaced by https://reviewboard.mozilla.org/r/4975/.

/r/6715 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak Pull down this commit: hg pull -r f055e0581135a67fc1897b6fb0ff754d9c05976f https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8589703 [details] MozReview Request: bz://1076775/Yoric https://reviewboard.mozilla.org/r/6713/#review5657 I think we're good enough to go! ::: toolkit/components/places/History.jsm (Diff revision 1) > - () => new Promise((resolve) => { > + Task.spawn(function*() { I'm not sure there's a good reason to use Task.spawn vs XPCOMUtils.defineLazyGetter(this, "DBConnPromised", () => PlacesUtils.promiseWrappedConnection().then(db => { }) ); ::: toolkit/components/places/History.jsm (Diff revision 1) > + * been visited since this date (inclusive). "visits that have been visited" Maybe "visits that have been added", or "inserted" This is repeated twice. ::: toolkit/components/places/History.jsm (Diff revision 1) > + * Note that this `VisitInfo` does NOT contain the referrer. "Note that the referrer property of this `visitInfo` is NOT populated." ::: toolkit/components/places/History.jsm (Diff revision 1) > + if ("beginDate" in filter) { you could define let hasBeginDate = "beginDate" in filter; let hasEndDate = "endDate" in filter; this would make code a bit more readable and avoid hasArgs (since would be a simple !hasBeginDate && !hasEndDate) ::: toolkit/components/places/History.jsm (Diff revision 1) > + * are no more bookmarks) or updating their frecency Not just bookmarks. let's say "no more foreign keys" or "foreign references" ::: toolkit/components/places/History.jsm (Diff revision 1) > + * (otherwise). nit: (otherwise) ca nprobably be removed, "or" is clear enough ::: toolkit/components/places/History.jsm (Diff revision 1) > + * updated. nit: oneline ::: toolkit/components/places/History.jsm (Diff revision 1) > + throw new TypeError("`beginDate` should be at least as old as `endDate`"); didn't you check this on the input already? IS there any case where input is valid but we might arrive here with invalid values? ::: toolkit/components/places/History.jsm (Diff revision 1) > + Task.async(function*(row) { will executeCached actually wait for the promise returned here? To me doesn't look like we allow to pass generator functions or promises to Sqlite.jsm http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm#642 On the other side, this works because you are not yielding... ::: toolkit/components/places/tests/head_common.js (Diff revision 1) > +function promiseClearHistory() Use PlacesTestUtils.clearHistory() instead, we just finished removing custom clear history helpers from the codebase! ::: toolkit/components/places/tests/history/test_remove.js (Diff revision 1) > - * License, v. 2.0. If a copy of the MPL was not distributed with this > + * http://creativecommons.org/publicdomain/zero/1.0/ */ Just remove the license, and it will be PD. ::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js (Diff revision 1) > + * http://creativecommons.org/publicdomain/zero/1.0/ */ remove license header ::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js (Diff revision 1) > +add_task(function* init() { please test on Try without this hack? If it passes on Try, you might still keep it locally, but it's likely we don't need it. Ah, I actually have a theory, the clearDB() call could be bogus... Mostly due to OS file locks. Try removing it! What do you need it for? if it's just to remove bookmarks and history, use the APIs (PTU.clearHistory and PU.bookmarks.eraseEverything)

Comment on attachment 8589703 [details] MozReview Request: bz://1076775/Yoric https://reviewboard.mozilla.org/r/6713/#review5663

https://reviewboard.mozilla.org/r/6713/#review5669 ::: toolkit/components/places/History.jsm (Diff revision 1) > - () => new Promise((resolve) => { > + Task.spawn(function*() { No deep reason, I just find the control and error flows much more readable.

https://reviewboard.mozilla.org/r/6713/#review5675 > will executeCached actually wait for the promise returned here? > To me doesn't look like we allow to pass generator functions or promises to Sqlite.jsm > http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm#642 > > On the other side, this works because you are not yielding... Oops. > please test on Try without this hack? If it passes on Try, you might still keep it locally, but it's likely we don't need it. > > Ah, I actually have a theory, the clearDB() call could be bogus... Mostly due to OS file locks. Try removing it! > > What do you need it for? if it's just to remove bookmarks and history, use the APIs (PTU.clearHistory and PU.bookmarks.eraseEverything) Ok, `clearDB` was the culprit. Replacing it with calls to `PlacesUtils` seems to do the trick.

Comment on attachment 8589703 [details] MozReview Request: bz://1076775/Yoric /r/6715 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak Pull down this commit: hg pull -r 6794715ba1ce9c53226090ed00c876b1991b4187 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8589703 [details] MozReview Request: bz://1076775/Yoric https://reviewboard.mozilla.org/r/6713/#review5677 Ship It!

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d6ec548e184

Comment on attachment 8589703 [details] MozReview Request: bz://1076775/Yoric https://reviewboard.mozilla.org/r/6713/#review5679 Inviala! ::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js (Diff revisions 1 - 2) > - clearDB(); > + yield PlacesUtils.history.clear(); Please use yield PlacesTestUtils.clearHistory(); it will also wait for expiration (for now that's needed).

https://reviewboard.mozilla.org/r/6713/#review5681 > No deep reason, I just find the control and error flows much more readable. OK, whatever

Applied feedback. Let's land this.

Backed out for xpcshell failures. https://hg.mozilla.org/mozilla-central/rev/80b261ea54fc https://treeherder.mozilla.org/logviewer.html#?job_id=8683444&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=8680850&repo=mozilla-inbound

175 vs 100 frecency sounds to me like there is a bookmark more/less than expected. Frecency is updates asynchronously, we don't wait for it, so either it's better to not test for it, or you need a promiseAsyncUpdates() "somewhere".

Added some cleanup code in test_remove.js Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c893729e1b

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2636489&repo=fx-team

Strange. It passes tests locally and on try. I'll investigate.

Well, if frecency is updated asynchronously, perhaps it doesn't make sense to pass frecency to the callback in History.remove? Regardless, removing that check.

we are mostly passing it in for API coherence, in future we might decide to wait for the update, but it might be expensive...

Deactivated check. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffbc0f842a06