this needs bug 1249263 first.

And this is now actionable! The only thing to take into account is that the new API is asynchronous, so the call points must be adapted to use promises

http://searchfox.org/mozilla-central/search?q=%5C.removePages%28By%7CFromH%29&regexp=true&path=

can i work on this?

Sure, do you have a working build already? An artifact build should be enough. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Hi I am looking at working on this. https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/browser/components/places/content/controller.js#1015 So I should make that function "function PC__removeHistoryContainer(aContainerNode)" async and return await removeByFilter?

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #6) > Hi I am looking at working on this. Great, thanks! > https://searchfox.org/mozilla-central/rev/ > f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/browser/components/places/content/ > controller.js#1015 > > So I should make that function "function > PC__removeHistoryContainer(aContainerNode)" async and return await > removeByFilter? Note that `return await <foo>` in an async function is unnecessary. You only need to `return foo`, as the async part will cope with that. That said, I think we can make this simpler. _removeHistoryContainer can simply return the result of removeByFilter (this will be a promise), and the callers can then `await this._removeHistoryContainer(...)`. In the _removeRowsFromHistory, I think it is probably best to change that to an async function, and await where it is called, and await when it calls _removeHistoryContainer.

So am I on the right track? Not sure what to do with aHostContainer.title, since the original function includes all subdomains as well. But if I concat a "*." to the front of it, wont that break it on what aHostContainer.title actually looks like? Also what tests should to make sure I haven't broken anything?

I mean aContainerNode.title.

Let me clarify what I mean. How am I dealing with the second boolean parameter in removeHistoryContainer, which is a boolean that enables different subdomains.

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #8) > So am I on the right track? I think you could directly return the result from PlacesUtils.history.removeByFilter rather than assigning it to removed for this case. Don't forget to add the `await` to the callers as well. For beginDate/endDate, I think you need to pass them through a function to get the right date format for the new filter: beginDate: PlacesUtils.toDate(beginTime + 1), endDate: PlacesUtils.toDate(endDate) Also, it looks like you've got tabs in your patch. We normally use spaces instead. > Not sure what to do with aHostContainer.title, since the original function > includes all subdomains as well. But if I concat a "*." to the front of it, > wont that break it on what aHostContainer.title actually looks like? I'm not sure here. I think doing the concat might be right, but it is probably worth dumping aHostContainer.title out in an experiment, or capturing it in the browser console. > Also what tests should to make sure I haven't broken anything? The bit of code that you're changing here is in the library window (History -> Show All History). Most of the tests for this are in one folder, and you can run those at the same time with: ./mach mochitest browser/components/places/tests/browser/ It'll provide a summary of test failures/passes. You can always intentionally break the function to attempt to get a failure and prove that it is being tested (some functions are called directly from tests, but I think this one is hidden away a bit). If you then have one test that you're looking into, you can run something like: ./mach mochitest browser/components/places/tests/browser/browser_library_commands.js to run just the one test file. Note, it is also worth running ESLint to check for errors - this provides static analysis of the javascript code, and needs to pass before we'll accept a patch. You can run in similar ways: ./mach eslint browser/components/places There's also editor integrations available: https://developer.mozilla.org/docs/Mozilla/Developer_guide/ESLint#Editor_Integration and hooks for when you commit/push: https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook

> I'm not sure here. I think doing the concat might be right, but it is > probably worth dumping aHostContainer.title out in an experiment, or > capturing it in the browser console. How do I do this? The console.log isn't printing anywhere when I delete stuff from "Show all History". I did a build after I added console.log and ran that.

Marco, any idea how we hit the _removeHistoryContainer function, or could it be dead code? https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/browser/components/places/content/controller.js#1012 The first call point is gated on: ``` if (node.itemId == -1 && PlacesUtils.nodeIsQuery(node) && PlacesUtils.asQuery(node).queryOptions.queryType == Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) ``` I'm not sure we can have an itemId == -1 can we now? The second call point doesn't have the itemId check, but I'm not sure how we'd hit it. I tried creating a bookmark with uri "place:queryType=0&sort=4&maxResults=42&excludeQueries=1" but that gets treated as a bookmark more than a query container...

I figured out how we hit it. On the left sidebar of the Library window, History is separated by time. On mine I have "Today" and "Yesterday" under History. If you right click "Today" and delete it, it will call removeHistoryContainer. So I found that... aContainerNode.title = "Today" Which doesn't make sense to me in the function removePagesFromHost("Today", true). How is "Today" a host? I cannot find where removePagesFromHost is defined. I found it's IDL but that's it.

(In reply to Mark Banner (:standard8) from comment #13) > I'm not sure we can have an itemId == -1 can we now? Yes, in the query-returning-queries case, that is for example History->[today, yesterday...] or the other history groupings (by site, by date and site). Those are not bookmarks, they are queries returned by a query, as such they have itemId == -1. (In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #14) > So I found that... > aContainerNode.title = "Today" > > Which doesn't make sense to me in the function removePagesFromHost("Today", > true). > How is "Today" a host? note there's an if condition there: if (PlacesUtils.nodeIsHost(aContainerNode)) { // Site container. PlacesUtils.history.removePagesFromHost(aContainerNode.title, true); "Today" should fail the nodeIsHost check. (In reply to Mark Banner (:standard8) from comment #11) > For beginDate/endDate, I think you need to pass them through a function to > get the right date format for the new filter: > > beginDate: PlacesUtils.toDate(beginTime + 1), > endDate: PlacesUtils.toDate(endDate) Considering beginTime and endTime are microseconds and toDate rounds to milliseconds, the +1 there sounds like a no-op. Based on the code comment, this could actually be a problem to evaluate, I'm not sure if it's still true that containers exclude the lower bound, after all we started rounding to milliseconds some time ago... There may be a bug here to figure out, maybe in a follow-up. I'm not sure I understand the question about adding "*." in front of container.title, that won't change container.title, a new string is generated by the concat.

So how do you find a history grouping by site? The only groupings I'm seeing are by date. I want to see if concatenating "*." to the host is appropriate, and for that I need nodeIsHost(aContainerNode) to equal true.

The History Sidebar has various groupings among which "by site", "by date", "by date and site".

(In reply to Marco Bonardo [::mak] from comment #3) > http://searchfox.org/mozilla-central/search?q=%5C. > removePages%28By%7CFromH%29&regexp=true&path= Am I fixing tests too?

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #18) > Am I fixing tests too? It'd be G R E A T!

I have a question about fixing the tests in test_browserhistory.js. https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/unit/test_browserhistory.js#90 One test is called "test_removePagesFromHost", and another is called "test_removePagesFromHost_keepSubdomains". But removePagesFromHost isn't the name of the function being tested anymore. It's testing different functionalities of removeByFilter now. So do I rename those tests?

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #20) > But removePagesFromHost isn't the name of the function being tested anymore. > It's testing different functionalities of removeByFilter now. So do I rename > those tests? Yes please. The test names are not critical, so you can go as fancy as you wish on them, but I suppose something generic like remove_pages_from_host() would also do.

Would you rather me do this as the mozreview thing rather than just a diff? How is this going to remove the jank mentioned in the issue that depends on this one? If it's just awaiting at every call point, what is even the point of it being async? To controller.js, should _removeHistoryContainer return the promise result of removeByFilter, when before it returned nothing?

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #22) > Created attachment 8947775 [details] [diff] [review] > bug1089691-2.diff > > Would you rather me do this as the mozreview thing rather than just a diff? mozreview is simpler to deal with, since we can land to Try and autoland it automatically, in general it's preferred. It depends on how much effort requires on your side to convert it, shouldn't be much. > How is this going to remove the jank mentioned in the issue that depends on > this one? If it's just awaiting at every call point, what is even the point > of it being async? It won't likely solve the problem by itself, but it will move I/O from the main-thread to another thread, that in general can help with Jank. > To controller.js, should _removeHistoryContainer return the promise result > of removeByFilter, when before it returned nothing? yes, I think it's fine to return the promise.

Comment on attachment 8948079 [details] Bug 1089691 - Convert consumers of removePagesByTimeframe and https://reviewboard.mozilla.org/r/217702/#review223666 Thanks, a few mistakes to fix yet ::: browser/components/places/content/controller.js:988 (Diff revision 1) > /** > * Removes the set of selected ranges from history, asynchronously. > * > * @note history deletes are not undoable. > */ > - _removeRowsFromHistory: function PC__removeRowsFromHistory() { > + async _removeRowsFromHistory() { there is a single call to _removeRowsFromHistory in controller.js::remove, that should await now ::: browser/components/places/content/controller.js:1002 (Diff revision 1) > Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) { > - this._removeHistoryContainer(node); > + await this._removeHistoryContainer(node); > } > } > > PlacesUtils.history.remove([...URIs]).catch(Components.utils.reportError); And this can likely await instead of .catch ::: browser/components/places/content/controller.js:1029 (Diff revision 1) > // We want to exclude beginTime from the removal because > // removePagesByTimeframe includes both extremes, while date containers > // exclude the lower extreme. So, if we would not exclude it, we would > // end up removing more history than requested. > - PlacesUtils.history.removePagesByTimeframe(beginTime + 1, endTime); > + const filter = { > + beginDate: PlacesUtils.toDate(beginTime + 1), This is likely a bug, the + 1 here doesn't do anything because to Date will convert these microseconds into milliseconds. Probably here we should add 1000 instead of 1. It didn't matter for the old API since it was taking microseconds. ::: browser/components/places/content/controller.js:1032 (Diff revision 1) > // end up removing more history than requested. > - PlacesUtils.history.removePagesByTimeframe(beginTime + 1, endTime); > + const filter = { > + beginDate: PlacesUtils.toDate(beginTime + 1), > + endDate: PlacesUtils.toDate(endTime) > + }; > + return PlacesUtils.history.removeByFilter(filter); nit: you don't necessarily need to build a temp filter object ahead of the call, you could also just do : PlacesUtils.history.removeByFilter({ your_filter_options }); ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:137 (Diff revision 1) > */ > - Delete(item, usSinceEpoch) { > + async Delete(item, usSinceEpoch) { > if ("uri" in item) { > Async.promiseSpinningly(PlacesUtils.history.remove(item.uri)); > } else if ("host" in item) { > - PlacesUtils.history.removePagesFromHost(item.host, false); > + await PlacesUtils.history.removeByFilter({ host: item.host }); probably this needs a rebase, I see that the code changed very recently and it's already awaiting ::: toolkit/components/places/tests/favicons/test_root_icons.js:69 (Diff revision 1) > rootIconURI.spec, "Should get the smallest icon"); > Assert.equal(await getFaviconUrlForPage("http://www.places.test/old/"), > rootIconURI.spec, "Should get the root icon"); > > - PlacesUtils.history.removePagesByTimeframe( > - PlacesUtils.toPRTime(Date.now() - 14400000), > + let filter = { > + beginDate: Date.now() - 14400000, I don't think this works, did you run this test? beginDate should be a Date object ::: toolkit/components/places/tests/favicons/test_root_icons.js:86 (Diff revision 1) > Assert.equal(rows.length, 0, "There should be no page entry"); > rows = await db.execute("SELECT * FROM moz_icons_to_pages"); > Assert.equal(rows.length, 0, "There should be no relation entry"); > > - PlacesUtils.history.removePagesByTimeframe(0, PlacesUtils.toPRTime(new Date())); > + filter = { > + beginDate: 0, ditto ::: toolkit/components/places/tests/unit/test_history_sidebar.js:376 (Diff revision 1) > // Remove "Today". > - hs.removePagesByTimeframe(midnight.getTime() * 1000, Date.now() * 1000); > + const filter = { > + beginDate: midnight.getTime(), > + endDate: Date.now() > + }; > + await hs.removeByFilter(filter); This is unlikely to work because hs is the old history service, it should use PlacesUtils.history. You can run this test as ./mach xpcshell-test relative_path_to_test ::: toolkit/components/places/tests/unit/test_history_sidebar.js:413 (Diff revision 1) > var dateContainer = root.getChild(0).QueryInterface(Ci.nsINavHistoryContainerResultNode); > dateContainer.containerOpen = true; > > do_check_eq(dateContainer.childCount, visibleContainers.length); > // Remove "Today". > - hs.removePagesByTimeframe(midnight.getTime() * 1000, Date.now() * 1000); > + await hs.removeByFilter(filter); ditto

Stylistically, when should I build a temporary filter object ahead of the call?

if it's a non-trivial object, like if the properties include getters, or have very long names, or it has lots of properties. There isn't a strict rule and it's left to the dev judgement. In this case with just 2 short properties, it doesn't seem very necessary.

Hi, are you still working on this issue?

(In reply to Marco Bonardo [::mak] from comment #28) > Hi, are you still working on this issue? I am. I should have another patch for review this weekend.

I convert from: PlacesUtils.history.removePagesByTimeframe( PlacesUtils.toPRTime(Date.now() - 14400000), PlacesUtils.toPRTime(new Date()) ); to: await PlacesUtils.history.removeByFilter({ beginDate: PlacesUtils.toDate(Date.now() - 14400000), endDate: new Date() }); Not sure why the test is failing.

https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/favicons/test_root_icons.js#68 This is the test that is failing. > Assert.equal(rows.length, 1, "There should only be 1 icon entry");

Looking at this more, I see toDate takes microseconds, and Date.now() returns milliseconds. So then I have to do this, which looks overly complicated. > beginDate: PlacesUtils.toPRTime(Date.now() * 100 - 14400000000),

> beginDate: PlacesUtils.toDate(Date.now() * 100 - 14400000000), I mean.

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #33) > > beginDate: PlacesUtils.toDate(Date.now() * 100 - 14400000000), You don't need to use toDate() for this case, just use new Date(milliseconds). toDate is for when you have a PRTime.

I attached the change I made to test_nsINavHistoryViewer.js. Now the test is failing. I don't think I did the promise wrong so I don't know what's up. This is the command I ran: > ./mach xpcshell-test toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js This is the error message: https://pastebin.mozilla.org/9078272

I think the problem is that RemovePagesFromHost was sending a batch notification here https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/components/places/nsNavHistory.cpp#2678 that caused a host query to completely refresh, instead of removing entries one by one from itself. The new API doesn't send batching notifications, and that means we have a perf hit. But that's out of scope to be solved in this bug, likely we'll need new notifications from bug 1340498 to be able to solve that. For now, you can comment out the invalidated container check and file a follow-up bug where we'll deal with that somehow.

The same thing his happening in test_frecency_observers.js. Comment out line 43? https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/unit/test_frecency_observers.js#43

(In reply to Jacob Harrow-Mortelliti [:jakehm] from comment #37) > The same thing his happening in test_frecency_observers.js. > > Comment out line 43? > https://searchfox.org/mozilla-central/source/toolkit/components/places/tests/ > unit/test_frecency_observers.js#43 uh no, probably the test should store the promise from onFrecencyChanged BEFORE the call to removeByFilter, and then await for it after.

Comment on attachment 8948079 [details] Bug 1089691 - Convert consumers of removePagesByTimeframe and https://reviewboard.mozilla.org/r/217702/#review228576 It looks good, just a couple things to fix and we should be good to run it on the Try server. ::: toolkit/components/places/tests/favicons/test_root_icons.js:86 (Diff revision 2) > rows = await db.execute("SELECT * FROM moz_icons_to_pages"); > Assert.equal(rows.length, 0, "There should be no relation entry"); > > - PlacesUtils.history.removePagesByTimeframe(0, PlacesUtils.toPRTime(new Date())); > + await PlacesUtils.history.removeByFilter({ > + beginDate: new Date(0), > + endDt: new Date() typo (endDt) ::: toolkit/components/places/tests/unit/test_frecency_observers.js:44 (Diff revision 2) > url, > title: "test" > }); > - PlacesUtils.history.removePagesFromHost(url.host, false); > - await onFrecencyChanged(url); > + await PlacesUtils.history.removeByFilter({ host: url.host }); > + // Removed for bug 1089691. This is failing bcause the new API doesn't send batching notifications to nsNavHistory.cpp. > + // await onFrecencyChanged(url); try my suggestion here, and then we'll see :) But surely this case is not related to batching.

Based on the Try run, looks like there's a problem when passing things like "localhost" to removeByFilter, indeed we end up passing *.localhost and we fail at "Expected well formed hostname string for `host` with atmost 1 wildcard." because the regx expects something like "*.test.com". This needs a solution and a test in test_removeByFilter.js, possibly in a separate bug blocking this one. To simplify consumers we may likely allow the slightly broken "*.localhost" form

Comment on attachment 8948079 [details] Bug 1089691 - Convert consumers of removePagesByTimeframe and https://reviewboard.mozilla.org/r/217702/#review229498 The patch looks good, but we can't land it until we fix the previous comment (handling single word hosts)

I was thinking that we could avoid the "*", we can just use an initial dot as wildcard, thus "host.com" removes from "host.com", but ".host.com" removes from all subdomains. When parsing the string in removeByFilter, we could just remove the initial point (if present) and store a clearSubdomains bool based on that. Then the host should always be valid.

* is a common wildcard. Just a dot is going to seem random. localhost is a special case so why not have a special rule?

I don't think it's a big deal to just use a prefix dot for hosts, if it's properly documented. "*" has also the defect that one may try do "*host" instead of "*.host". That would require in any case to check the RemoveByFilter javadoc, so the difference seems unimportant.

So is there any further action I should take on this bug at this point?

If you wish to work on the bug you found, you can file that bug, set it as blocking this one, and provide a patch/test for it. Otherwise, we'll likely do that and then rebase your patch here on top of that fix, when possible, probably in the Firefox 61 timeframe.

Alright I will work on that.

(In reply to Marco Bonardo [::mak] from comment #48) > If you wish to work on the bug you found, you can file that bug, set it as > blocking this one, and provide a patch/test for it. > Otherwise, we'll likely do that and then rebase your patch here on top of > that fix, when possible, probably in the Firefox 61 timeframe. To be clear, the bug to fix is comment 42, we are not going to touch batching for now.

(In reply to Marco Bonardo [::mak] from comment #50) > (In reply to Marco Bonardo [::mak] from comment #48) > > If you wish to work on the bug you found, you can file that bug, set it as > > blocking this one, and provide a patch/test for it. > > Otherwise, we'll likely do that and then rebase your patch here on top of > > that fix, when possible, probably in the Firefox 61 timeframe. > > To be clear, the bug to fix is comment 42, we are not going to touch > batching for now. Oh so I totally named the bug the wrong thing even.

So you want me to go with the changing the wilcard to "." approach? In which case all the usages of "*." have to be changed. And the definition of the function has to be changed as well. Is there a way to change the title of that bug I made, or should I just invalidate it and make a new one, or does it not even matter?

The bug you filed is ok for future reference, just file another one for this other problem. also, in theory you should be able to edit a bug you filed, but maybe I'm wrong (don't recall all the basic permissions in bugzilla)

Comment on attachment 8952579 [details] [diff] [review] nsINavHistoryViewer.diff I'm working on the dependency and have an updated version of this patch that should be ready to land.

Comment on attachment 8973788 [details] Bug 1089691 - Convert consumers of removePagesByTimeframe and removePagesFromHost to PlacesUtils.history.removeByFilter. https://reviewboard.mozilla.org/r/242160/#review248530

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/5bd6b0a28492 Convert consumers of removePagesByTimeframe and removePagesFromHost to PlacesUtils.history.removeByFilter. r=mak