Closed Bug 1249263 Opened 8 years ago Closed 7 years ago

Introduce History.jsm::removeByFilter that accepts a filter object for host and timeframe

Categories

(Toolkit :: Places, defect, P2)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox55 --- fixed

People

(Reporter: mak, Assigned: milindl, Mentored)

References

Details

Attachments

(3 files)

Bug 1249263 - add a `removeByFilter` method to filter by host and time,
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1249263 - add tests for History.jsm::`removeByFilter`,
59 bytes, text/x-review-board-request
mak
: review+
Details
fix test_removeByFilter.js to wait for the assertInDB and assertNotInDB promises
8.28 KB, patch
mak
: review+
Details | Diff | Splinter Review 
Currently history.remove only accepts an url or an array of urls.
We should also support a filter object.
properties for the filter should be:
beginDate (Date, see RemoveVisitsByFilter)
endDate (Date, see RemoveVisitsByFilter)
host (string, should accept a subhost wildcard)
Priority: -- → P2
Rather than adding to remove(), let's just add a new removeByFilter API similar to removeVisitsByFilter, taking beginDate, endDate and host properties.
removeVisitsByFilter can be used as an example for the implementation.
Mentor: mak77
Summary: History.jsm::remove should accept a filter object for host and timeframe → Introduce History.jsm::removeByFilter that accepts a filter object for host and timeframe
Hi Marco, I'd like to work upon this bug. I'm getting my first patch ready, I have a doubt before I can complete it for a review.

* How do I add beginDates and endDates (since those aren't stored in the moz_places table)? Should I also query moz_historyvisits for the latest visit date? Or do something like filtering it in the deletion itself (adding a clause to WHERE - however that does not seem to follow the general pattern of querying first and then deleting purely by ID).

Also, does it make sense to add a few tests (after some reasonable work has been done in adding the function)?

Thanks!
(In reply to milindl from comment #4)
> Comment on attachment 8856522 [details]
> Bug 1249263 - add a `removeByFilter` method to filter by host[WIP],
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/128472/diff/1-2/

I've filtered the dates using the SQLite query itself (I JOINed), but I am open to any other suggestions for this (my SQL knowledge is limited). It also passes everything in test_remove.js.
(In reply to milindl from comment #2)
> * How do I add beginDates and endDates (since those aren't stored in the
> moz_places table)? Should I also query moz_historyvisits for the latest
> visit date?

yes, you should look at how the old API does it (removePagesFromHost and removePagesByTimeframe)
You basically need to find all the pages that have at least one visit in the timeframe.

 Or do something like filtering it in the deletion itself (adding
> a clause to WHERE - however that does not seem to follow the general pattern
> of querying first and then deleting purely by ID).

The problem is that we need to select first to be able to properly notify later.

> Also, does it make sense to add a few tests (after some reasonable work has
> been done in adding the function)?

Absolutely yes, you should add a test_removeByFilter.js to toolkit/components/places/tests/history, similar to test_removeVisitsByFilter.js
Comment on attachment 8856522 [details]
Bug 1249263 - add a `removeByFilter` method to filter by host and time,

https://reviewboard.mozilla.org/r/128472/#review131904

::: toolkit/components/places/History.jsm:393
(Diff revision 2)
> +   *      The full URI of the page.
> +   *             or (string)
> +   *      Either the full URI of the page or the GUID of the page.
> +   *             or (Array<URL|nsIURI|string>)
> +   *      An array of the above, to batch requests.
> +   * @param onResult: (function(PageInfo))

need to document the filter argument

::: toolkit/components/places/History.jsm:407
(Diff revision 2)
> +   *       is neither a valid GUID nor a valid URI or if `pages`
> +   *       is an empty array; or
> +   *       if `filter` does not have the expected type, in particular
> +   *       if the `object` is empty
> +   */
> +  removeByFilter(pages, filter, onResult) {

This should not take a pages array, see the APIs this is replacing. The scope of this is to be able to remove "unknown" number of pages provided they ssatisfy a filter, for example all the pages having a visit today, or all the pages having the given host.

::: toolkit/components/places/History.jsm:443
(Diff revision 2)
> +    }
> +
> +    if (hasHost && !(filter.host instanceof URL) && typeof filter.host != "string" &&
> +        !(filter.host instanceof Ci.nsIURI)) {
> +      throw new TypeError("Expected a valid URL for `host`");
> +    }

I think that host should be a non-empty string, and should contain at a maximum a single wildcard (*). see comment 0

::: toolkit/components/places/History.jsm:444
(Diff revision 2)
> +
> +    if (hasHost && !(filter.host instanceof URL) && typeof filter.host != "string" &&
> +        !(filter.host instanceof Ci.nsIURI)) {
> +      throw new TypeError("Expected a valid URL for `host`");
> +    }
> +    // Use the inner remove itself, since we share most code with it

this won't likely be true anymore since we won't filter on urls

::: toolkit/components/places/History.jsm:1020
(Diff revision 2)
> +    conditions.push("v.visit_date <= :end * 1000");
> +    args.end = Number(aFilter.endDate);
> +  }
> +
> +  if (conditions.length > 0) {
> +    optionalJoinFilter = `JOIN moz_historyvisits v ON v.place_id = h.id`;

So, you may want to do some SQL perf testing, maybe using Sqlite browser. IT may be faster to use a subquery to check if a visit exists in the given timeframe, rather than doing a full join, because joining pages with visits will generate a giant table in memory, that is not very efficient.

See what RemovePagesByTimeframe does http://searchfox.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#2695

For host, you must filter on rev_host. that means you must reverse the passed in host and fix it to match again rev_host (that is the reversed host plus a ".", for example for google.com the rev_host is moc.elgoog.). This is because in SQL is faster to do queries like rev_host BETWEEN "moc.elgoog." AND "moc.elgoog," and select everything inside that host (also somethingelse.google.com)
Attachment #8856522 - Flags: review?(mak77)
Thanks for the review, Marco. 
The references given to the old API are extremely useful. 

I've rewritten most stuff, however, I'm not sure how to filter by host. In your example, you've reversed google.com to filter for *.google.com - so should I assume that if I'm given a URL x, should I search for *.x? I am reading the old API, and from what I can understand, they've done the same thing.

As of now I am using a regexp for host which accepts *.x or x, where x can be www.google.com, subhost.google.com, .org, google.com etc (some examples) - currently I don't accept things of the form www.*.org or www.google.* and stuff like that.

Please tell me what approach is reasonable regarding the wildcard (if it's to be included implicitly, is there a point to having it in the regex?)

Also, if we have something like `localhost`, how do we select that (since it is not between tsohlacol. and tsohlacol,)?
(In reply to milindl from comment #8)
> As of now I am using a regexp for host which accepts *.x or x, where x can
> be www.google.com, subhost.google.com, .org, google.com etc (some examples)

probably .org is too wide, we should enforce having an host name (so one dot with chars before and after it).
There is also a special case that is the empty string. It should map to local files. The old API was mapping a special string "(local files)" for that, but I think we can go with the empty string (no host).

> - currently I don't accept things of the form www.*.org or www.google.* and
> stuff like that.

sounds good.

> Also, if we have something like `localhost`, how do we select that (since it
> is not between tsohlacol. and tsohlacol,)?

it is still stored in the db as tsohlacol. even if not particularly useful.

Based on the previous API (RemovePagesFromHost), you can either pass "google.com" and then only pages having rev_host = "moc.elgoog." will be selected, or you can pass "*.google.com" and then pages having rev_host between "moc.elgoog." and "moc.elgoog/" will be selected (I said "," before, but I was wrong, the next ascii after "." is "/").
did you intend to remove the review request, during WIP, or was that by mistake?
Assignee: nobody → i.milind.luthra
Status: NEW → ASSIGNED
I did intend to remove it, because I noticed some errors while writing test (basically a WIP remove).

I tried to put it back again, but MozReview says 'mak(r? cleared)' and I'm unsure how to deal with that (I can't change it).

Please do review it, it follows the new approach you suggested.

Thanks, and sorry for any issues
Attachment #8856522 - Flags: review?(mak77)
I think we hit bug 1283338.
Yes, it does seem like that.
However, now I see that we can simply use the Details section of the attachment to set flags, as you have done here (in the future I should not have this problem)

Thanks!
Comment on attachment 8856522 [details]
Bug 1249263 - add a `removeByFilter` method to filter by host and time,

https://reviewboard.mozilla.org/r/128472/#review132544

Great job here, it looks very promising!
I think it's time to start working on the test, that could help you finding more issues than the ones I could find visually.

::: toolkit/components/places/History.jsm:403
(Diff revision 5)
> +   *
> +   *
> +   * @param filter: An object containing a non empty subset of the following
> +   * properties:
> +   * - host: (string)
> +   *     Hostname with subhost wildcard (at most one *)

or an empty string for local files. Should also explain where the wildcard can and cannot be used.

::: toolkit/components/places/History.jsm:405
(Diff revision 5)
> +   * @param filter: An object containing a non empty subset of the following
> +   * properties:
> +   * - host: (string)
> +   *     Hostname with subhost wildcard (at most one *)
> +   * - beginDate: (Date)
> +   *     The first time the page was visited

append " (inclusive)."

::: toolkit/components/places/History.jsm:407
(Diff revision 5)
> +   * - host: (string)
> +   *     Hostname with subhost wildcard (at most one *)
> +   * - beginDate: (Date)
> +   *     The first time the page was visited
> +   * - endDate: (Date)
> +   *     The last time the page was visited

ditto

::: toolkit/components/places/History.jsm:409
(Diff revision 5)
> +   * - beginDate: (Date)
> +   *     The first time the page was visited
> +   * - endDate: (Date)
> +   *     The last time the page was visited
> +   * @param onResult: (function(PageInfo))
> +   *      A callback invoked for each page found.

@param [optional] onResult...

::: toolkit/components/places/History.jsm:410
(Diff revision 5)
> +   *     The first time the page was visited
> +   * - endDate: (Date)
> +   *     The last time the page was visited
> +   * @param onResult: (function(PageInfo))
> +   *      A callback invoked for each page found.
> +   *

please add a

@note This will remove any page that have *at least* one visit satisfying the criteria. If the page has also visits outside the specifed timeframe, they will also be removed along with the page.

::: toolkit/components/places/History.jsm:421
(Diff revision 5)
> +   *       if `filter` does not have the expected type, in particular
> +   *       if the `object` is empty, or its components do not satisfy the
> +   *       criteria given above
> +   */
> +  removeByFilter(filter, onResult) {
> +

please no newlines after braces

::: toolkit/components/places/History.jsm:422
(Diff revision 5)
> +   *       if the `object` is empty, or its components do not satisfy the
> +   *       criteria given above
> +   */
> +  removeByFilter(filter, onResult) {
> +
> +    if (!filter || typeof filter !== "object") {

please no newlines after braces

::: toolkit/components/places/History.jsm:427
(Diff revision 5)
> +    if (!filter || typeof filter !== "object") {
> +      throw new TypeError("Expected a filter object");
> +    }
> +
> +    let hasBeginDate = "beginDate" in filter;
> +    let hasEndDate = "endDate" in filter;

move these just before their first use (just before the if (hasBeginDate))

::: toolkit/components/places/History.jsm:429
(Diff revision 5)
> +    }
> +
> +    let hasBeginDate = "beginDate" in filter;
> +    let hasEndDate = "endDate" in filter;
> +    let hasHost = "host" in filter;
> +

and remove this newline

::: toolkit/components/places/History.jsm:454
(Diff revision 5)
> +    // The first one matches `localhost` or any other custom set in hostsfile
> +    // The second one matches *.google.com or google.com etc
> +    // The third one is for local files (special)
> +    if (hasHost &&
> +        !(filter.host.match(/^[a-z0-9]+$/) ||
> +         filter.host.match(/^(\*\.)?([a-z0-9]+)(\.[a-z0-9]+)+$/) ||

I think you may wish to extend the charset here, at least with "-".

::: toolkit/components/places/History.jsm:455
(Diff revision 5)
> +    // The second one matches *.google.com or google.com etc
> +    // The third one is for local files (special)
> +    if (hasHost &&
> +        !(filter.host.match(/^[a-z0-9]+$/) ||
> +         filter.host.match(/^(\*\.)?([a-z0-9]+)(\.[a-z0-9]+)+$/) ||
> +         filter.host === "")) {

!(a || b) is the same as !a && !b. I think the latter would be more readable in this case.

::: toolkit/components/places/History.jsm:977
(Diff revision 5)
>    return visitsToRemove.length != 0;
>  });
>  
> +// Inner implementation of History.removeByFilter
> +var removeByFilter = Task.async(function*(db, filter, onResult = null) {
> +  // 1. Create statement for date filteration

typo? I assume you want "filtering" or "filtration"?

::: toolkit/components/places/History.jsm:979
(Diff revision 5)
>  
> +// Inner implementation of History.removeByFilter
> +var removeByFilter = Task.async(function*(db, filter, onResult = null) {
> +  // 1. Create statement for date filteration
> +  // We need to multiply/divide by 1000 since JS uses milliseconds and SQLite
> +  // uses microseconds

It's actually not Sqlite, it's Places itself that decided to use microseconds years ago :(

::: toolkit/components/places/History.jsm:980
(Diff revision 5)
> +// Inner implementation of History.removeByFilter
> +var removeByFilter = Task.async(function*(db, filter, onResult = null) {
> +  // 1. Create statement for date filteration
> +  // We need to multiply/divide by 1000 since JS uses milliseconds and SQLite
> +  // uses microseconds
> +  let existsMozHistoryVisitsQuery = "";

this is not a complete query, it's a SQL fragment.
I'd probably name it dateFilterSQLFragment or something like that.
And the same for hostFilterSQLFragment.

::: toolkit/components/places/History.jsm:982
(Diff revision 5)
> +  // 1. Create statement for date filteration
> +  // We need to multiply/divide by 1000 since JS uses milliseconds and SQLite
> +  // uses microseconds
> +  let existsMozHistoryVisitsQuery = "";
> +  let conditions = [];
> +  let args = {};

we call these "params" usually (bound parameters in full)

::: toolkit/components/places/History.jsm:985
(Diff revision 5)
> +  let existsMozHistoryVisitsQuery = "";
> +  let conditions = [];
> +  let args = {};
> +  if ("beginDate" in filter) {
> +    conditions.push("v.visit_date >= :begin * 1000");
> +    args.begin = Number(filter.beginDate);

Let's not put the * 1000 in the condition, let's instead set the parameter to PlacesUtils.toPRTime(filter.beginDate)

::: toolkit/components/places/History.jsm:989
(Diff revision 5)
> +    conditions.push("v.visit_date >= :begin * 1000");
> +    args.begin = Number(filter.beginDate);
> +  }
> +  if ("endDate" in filter) {
> +    conditions.push("v.visit_date <= :end * 1000");
> +    args.end = Number(filter.endDate);

ditto

::: toolkit/components/places/History.jsm:1004
(Diff revision 5)
> +
> +  // 2. Create statement for host and subhost filtering
> +  let hostQuery = "";
> +  if (filter.host) {
> +    // There are three cases that we need to consider,
> +    // google.com, *.google.com, localhost

it's actually 4, with local files.
Also, please use mozilla.org instead of google.com :p

::: toolkit/components/places/History.jsm:1009
(Diff revision 5)
> +    // google.com, *.google.com, localhost
> +
> +    if (filter.host.match(/^([a-z0-9]+)$/) || filter.host === "") {
> +      // Case 1: localhost and hostfile-specific names
> +      if (filter.host === "") {
> +        filter.host = "localhost";

this looks wrong?
local files don't have localhost as rev_host.
You can open places.sqlite with sqlite browser and check there.
From what I see we use "." as rev_host for local files, and "tsohlacol." for localhost

::: toolkit/components/places/History.jsm:1024
(Diff revision 5)
> +        `h.rev_host between :revHostStart and :revHostEnd`;
> +      args.revHostStart = revHost + ".";
> +      args.revHostEnd = revHost + "/";
> +    } else {
> +      // Case 3: single url, without wildcard
> +      let revHost = filter.host.split("").reverse().join("") + ".";

this can probably be merged with the first if case?

::: toolkit/components/places/History.jsm:1037
(Diff revision 5)
> +  let query =
> +      `SELECT h.id, url, rev_host, guid, title, frecency
> +      FROM moz_places h WHERE
> +      (${ existsMozHistoryVisitsQuery })
> +      ${ existsMozHistoryVisitsQuery !== "" && hostQuery !== "" ?" AND ":"" }
> +      (${ hostQuery })`;

you could mabe do something like:
${ [fragment1, fragment2].join(" AND ") }

::: toolkit/components/places/History.jsm:1075
(Diff revision 5)
> +
> +  try {
> +    if (pages.length === 0) {
> +      // Nothing to do
> +      return false;
> +    }

this is unlikely to throw, could be moved out of the try
Attachment #8856522 - Flags: review?(mak77)
> please no newlines after braces
by this I mean "do not leave an empty line after an opening brace", not that you should write everything on one line!
Sorry for the confusing comment.
Comment on attachment 8856522 [details]
Bug 1249263 - add a `removeByFilter` method to filter by host and time,

https://reviewboard.mozilla.org/r/128472/#review132544

Thanks for the review, will start on tests now (and over weekend). I am thinking of two tests, first `test_removeByFilter` which 
-> adds some URLs with random subhosts and the same host (for * checking)
-> adds some URLs for exact matching
-> adds some URLs and corresponding visits in the other table for timeframe filtration
I'm a bit confused by the whole `hasBookmark` thing in the code for remove and removeVisitsByFilter tests, and whether I need to include it. I'll investigate that.

The second test which I've already written is somewhat simple `test_error_cases`.
(In reply to milindl from comment #21)
> Thanks for the review, will start on tests now (and over weekend). I am
> thinking of two tests, first `test_removeByFilter` which 
> -> adds some URLs with random subhosts and the same host (for * checking)
> -> adds some URLs for exact matching
> -> adds some URLs and corresponding visits in the other table for timeframe
> filtration

It should also test input validation. But maybe that's your second test?
See the other tests http://searchfox.org/mozilla-central/search?q=history%2Ftest_remove&path=

> I'm a bit confused by the whole `hasBookmark` thing in the code for remove
> and removeVisitsByFilter tests, and whether I need to include it. I'll
> investigate that.

Basically foreign_count is a poor-man's solution to implement a foreign key. We don't want to remove a page from the database if it has a positive foreign_count because something else is referencing that page entry.
we have 2 notifications, onDeleteURI and onDeleteVisits. The former indicates a page (and all of its visits) has been removed from the database, the latter indicated only the visits have been removed, but the page remained in the database.
It's not great and we have plans to change the whole notifications system, but it may take a while and in the meanwhile we have to stick to that.
I think it makes sense to also check this in your test.
In your case onDeleteVisits should probably always have aVisitTime set to 0, since all the visits are removed.
Comment on attachment 8856522 [details]
Bug 1249263 - add a `removeByFilter` method to filter by host and time,

https://reviewboard.mozilla.org/r/128472/#review134832

it looks good, the only blocking issue is lack of a test.

::: commit-message-73163:1
(Diff revision 8)
> +Bug 1249263 - add a `removeByFilter` method to filter by host,r?mak

by time and host

::: toolkit/components/places/History.jsm:395
(Diff revision 8)
>        db => removeVisitsByFilter(db, filter, onResult)
>      );
>    },
>  
>    /**
> +   * Remove pages from the database based on the filter.

s/the/a/

::: toolkit/components/places/History.jsm:431
(Diff revision 8)
> +      throw new TypeError("Expected a filter object");
> +    }
> +
> +    let hasHost = "host" in filter;
> +    if (hasHost) {
> +      filter.host = filter.host.toLowerCase();

you should maybe first check that typeof filter.host is "string", throw otherwise. The test can check the thrown error message.

::: toolkit/components/places/History.jsm:457
(Diff revision 8)
> +    // Host should follow one of these formats
> +    // The first one matches `localhost` or any other custom set in hostsfile
> +    // The second one matches *.mozilla.org or mozilla.com etc
> +    // The third one is for local files
> +    if (hasHost &&
> +        !(filter.host.match(/^[a-z0-9\-]+$/)) &&

nit: IIRC if the - char is at the end of a character class, it doesn't need to be escaped (since there's nothing after it)

::: toolkit/components/places/History.jsm:458
(Diff revision 8)
> +    // The first one matches `localhost` or any other custom set in hostsfile
> +    // The second one matches *.mozilla.org or mozilla.com etc
> +    // The third one is for local files
> +    if (hasHost &&
> +        !(filter.host.match(/^[a-z0-9\-]+$/)) &&
> +        !(filter.host.match(/^(\*\.)?([a-z0-9\-]+)(\.[a-z0-9\-]+)+$/)) &&

please use the regex .test() method, instead of the string .match(). It should be faster since we don't need the matches.

::: toolkit/components/places/History.jsm:460
(Diff revision 8)
> +    // The third one is for local files
> +    if (hasHost &&
> +        !(filter.host.match(/^[a-z0-9\-]+$/)) &&
> +        !(filter.host.match(/^(\*\.)?([a-z0-9\-]+)(\.[a-z0-9\-]+)+$/)) &&
> +        (filter.host !== "")) {
> +      throw new TypeError("Expected well formed url string for `host` with atmost 1 *");

s/1 */one wildcard./

::: toolkit/components/places/History.jsm:983
(Diff revision 8)
>  
> +// Inner implementation of History.removeByFilter
> +var removeByFilter = Task.async(function*(db, filter, onResult = null) {
> +  // 1. Create fragment for date filtration
> +  // We need to multiply/divide by 1000 since JS uses milliseconds and Places
> +  // uses microseconds

the comment is probably pointless now that the code uses toPRTime, it seems self-explanatory

::: toolkit/components/places/History.jsm:1010
(Diff revision 8)
> +  let hostFilterSQLFragment = "";
> +  if (filter.host) {
> +    // There are four cases that we need to consider,
> +    // mozilla.org, *.mozilla.org, localhost, and local files
> +
> +    if (filter.host.indexOf("*.") === 0) {

filter.host.startsWith("*") (it can only eventually start with *. since you check that on input validation).

::: toolkit/components/places/History.jsm:1031
(Diff revision 8)
> +  // 3. Find out what needs to be removed
> +  let fragmentArray = [hostFilterSQLFragment, dateFilterSQLFragment];
> +  let query =
> +      `SELECT h.id, url, rev_host, guid, title, frecency
> +      FROM moz_places h WHERE
> +      (${ fragmentArray.filter(f => f !== "").join(") AND (") })`;

nit: please indent at the query contents, not at the backticks
Attachment #8856522 - Flags: review?(mak77)
I'll fix the nits - I'm finding small issues as I am writing a test (which is good, because I am able to fix them).

More importantly, I have final exams right now, so I will get back on this on May 3 (which is about 2 weeks from today) and which is when my finals end.

Thanks!
Sure, no hurry, take your time.
Hi Marco:

I think I've fixed most of the nits, but I'd like to you take a look at the test code to make sure what I am testing is what I should be testing, and if there are any other things I should test.

Thanks!
PS: I am left with the part where I add a bookmark of the same page, and then expect it _not_ to get removed when I call the method because of its foreign count. Except for that, I have covered the cases mentioned in the file.
Now I am done with testing for the bookmarks as well :)

I also have this other issue where review board shows one open issue in the review summary but none in the issue pane when I open it up - could it be a problem to land it later?

Thanks!
Comment on attachment 8856522 [details]
Bug 1249263 - add a `removeByFilter` method to filter by host and time,

https://reviewboard.mozilla.org/r/128472/#review140002
Attachment #8856522 - Flags: review?(mak77) → review+
(In reply to milindl from comment #36)
> I also have this other issue where review board shows one open issue in the
> review summary but none in the issue pane when I open it up - could it be a
> problem to land it later?

I don't know if it's just a visual glitch or an actual issue. If it's an actual issue, it will disallow autolanding. We could still land manually
Probably worth filing a bug in the mozreview component for it.
(In reply to Marco Bonardo [::mak] from comment #39)
> (In reply to milindl from comment #36)
> > I also have this other issue where review board shows one open issue in the
> > review summary but none in the issue pane when I open it up - could it be a
> > problem to land it later?
> 
> I don't know if it's just a visual glitch or an actual issue. If it's an
> actual issue, it will disallow autolanding. We could still land manually
> Probably worth filing a bug in the mozreview component for it.

There is already a similar issue, so I have simply commented on that here - https://bugzilla.mozilla.org/show_bug.cgi?id=1198570

Also, the test commit is now suitable for review (I'll remove the WIP from the commit message if it looks good), I think I have tested most of the cases I could come up with.

Thanks!
Comment on attachment 8864506 [details]
Bug 1249263 - add tests for History.jsm::`removeByFilter`,

https://reviewboard.mozilla.org/r/136192/#review140036

::: toolkit/components/places/tests/history/test_removeByFilter.js:3
(Diff revision 5)
> +"use strict";
> +
> +Cu.importGlobalProperties(["URL"]);

This should not be needed because it's already done in toolkit/components/places/tests/head_common.js

I know some of the tests are still doing this, and that's where you copied it from, but it's still unnecessary.

::: toolkit/components/places/tests/history/test_removeByFilter.js:31
(Diff revision 5)
> +  yield PlacesUtils.bookmarks.eraseEverything();
> +
> +  // Adding a witness URI
> +  let witnessURI = NetUtil.newURI("http://witnessmozilla.org/test_browserhistory/test_removeByFilter" + Math.random());
> +  yield PlacesTestUtils.addVisits(witnessURI);
> +  Assert.ok(page_in_database(witnessURI), "Witness URI is in database");

please use yield PlacesTestUtils.isPageInDB instead of page_in_database. I should probably file a bug to remove page_in_database.
In general utils in PlacesTestUtils are preferred over equivalent utils in head files, unfortunately cleaning up old tests is expensive.

::: toolkit/components/places/tests/history/test_removeByFilter.js:37
(Diff revision 5)
> +
> +  let removeByFilterTester = Task.async(function*(visits, filter, checkBeforeRemove, checkAfterRemove, useCallback, bookmarkedUri) {
> +    // Add visits for URIs
> +    yield PlacesTestUtils.addVisits(visits);
> +    if (bookmarkedUri !== null && visits.map(v => v.uri).includes(bookmarkedUri)) {
> +      PlacesUtils.bookmarks.insertBookmark(

please use the new async bookmarking API instead, in this case yield PlacesUtils.bookmarks.insert({
  parentGuid: PlacesUtils.bookmarks.unfiledGuid,
  url: bookmarkedUri,
  title: "test bookmark"
});

::: toolkit/components/places/tests/history/test_removeByFilter.js:56
(Diff revision 5)
> +    let removed = false;
> +    if (useCallback) {
> +      // The amount of callbacks will be the unique URIs to remove from the database
> +      let netCallbacksRequired = (new Set(visits.map(v => v.uri))).size;
> +      removed = yield PlacesUtils.history.removeByFilter(filter, pageInfo => {
> +        Assert.ok(netCallbacksRequired > 0, "Callback called as many times as required");

may be worth checking that pageInfo is valid. off-hand I couldn't find an helper for that (are the other tests just assuming it's good? probably yes.)

You could probably add a pageInfo validator to the head file in history/ and use it here. It should at least check this is an object and has the basic expected properties.
Another possibility I'd support, would be to add a validator to PlacesUtils, like we did for bookmarks:
http://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#534 and replace History.jsm::validatePageInfo with that new helper. Then we would use the same validator both in tests and code, that would be nice.

::: toolkit/components/places/tests/history/test_removeByFilter.js:247
(Diff revision 5)
> +    () => PlacesUtils.history.removeByFilter({beginDate: new Date(1000), endDate: new Date(0)}),
> +      /TypeError: `beginDate` should be at least as old/
> +  );
> +  Assert.throws(
> +    () => PlacesUtils.history.removeByFilter({host: "#"}),
> +      /TypeError: Expected well formed url string for/

url string could make the consumer think he needs to also pass a scheme, maybe we should change the error to "hostname string"

::: toolkit/components/places/tests/history/test_removeByFilter.js:297
(Diff revision 5)
> +      },
> +      onTitleChanged(aUri) {
> +        reject(new Error("Unexpected call to onTitleChanged"));
> +      },
> +      onClearHistory() {
> +        reject("Unexpected call to onClearHistory");

all the others are "new Error", why not this one?

::: toolkit/components/places/tests/history/test_removeByFilter.js:306
(Diff revision 5)
> +      },
> +      onFrecencyChanged(aURI) {
> +        try {
> +          Assert.ok(bookmarkedUri, "Observing onFrecencyChanged");
> +        } finally {
> +          resolve();

I'm not sure I understand why you are using frecency notifications to move one, shouldn't we just use onDeleteURI or onDeleteVisits (and while there, we could check that the appropriate one is invoked, depending on bookmarked status).

If that adds too much complication, please at least add a comment about why we are doing this.
Attachment #8864506 - Flags: review?(mak77)
> may be worth checking that pageInfo is valid. off-hand I couldn't find an
> helper for that (are the other tests just assuming it's good? probably yes.)
> 
> You could probably add a pageInfo validator to the head file in history/ and
> use it here. It should at least check this is an object and has the basic
> expected properties.
> Another possibility I'd support, would be to add a validator to PlacesUtils,
> like we did for bookmarks:
> http://searchfox.org/mozilla-central/source/toolkit/components/places/
> PlacesUtils.jsm#534 and replace History.jsm::validatePageInfo with that new
> helper. Then we would use the same validator both in tests and code, that
> would be nice.

`test_remove.js` checks the pageInfo for specific properties like url and guid, however does not perform all the checks that the validatePageInfo method does. It is a good idea to shift that to PlacesUtils, since that may save on repetition of code. 

Do you want me to make a third commit for that, or do it along with the first commit? I will also need to shift the method `normalizeToURLOrGUID` along with it, but that should not prove to be a problem, since it is used only 3 times, 2 times in `validatePageInfo` itself and once in another part of History.jsm. (http://searchfox.org/mozilla-central/search?q=normalizeToURLOrGUID&path=)

Thanks!
(In reply to milindl from comment #42)
> Do you want me to make a third commit for that, or do it along with the
> first commit? I will also need to shift the method `normalizeToURLOrGUID`
> along with it, but that should not prove to be a problem, since it is used
> only 3 times, 2 times in `validatePageInfo` itself and once in another part
> of History.jsm.
> (http://searchfox.org/mozilla-central/search?q=normalizeToURLOrGUID&path=)

It's the same for me, what you feel that would make your life easier.
Comment on attachment 8856522 [details]
Bug 1249263 - add a `removeByFilter` method to filter by host and time,

https://reviewboard.mozilla.org/r/128472/#review141518

::: toolkit/components/places/PlacesUtils.jsm:989
(Diff revision 12)
> +   *
> +   * @param pageInfo: (PageInfo)
> +   * @return (PageInfo)
> +   */
> +  validatePageInfo:
> +  function PU_validatePageInfo(pageInfo, validateVisits = true) {

please use ES shorthands, thus just: 
validatePageInfo(pageInfo, ...

It is no more needed to name each function, it was needed time ago, we didn't fix all the old code yet.

::: toolkit/components/places/PlacesUtils.jsm:1049
(Diff revision 12)
> +   *
> +   * @throws (TypeError)
> +   *         If the key is neither a valid guid nor a valid url.
> +   */
> +  normalizeToURLOrGUID:
> +  function PU_normalizeToURLOrGUID(key) {

ditto

::: toolkit/components/places/PlacesUtils.jsm:1073
(Diff revision 12)
> +   *
> +   * @param transitionType: (String)
> +   * @return (Boolean)
> +   */
> +  isValidTransitionType:
> +  function PU_isValidTransitionType(transitionType) {

I'd rather expose this from History.jsm, it seems more relevant as a history API than a generic one.
I'd also short its name to isValidTransition. So in the end it will be PlacesUtils.history.isValidTransition

::: toolkit/components/places/PlacesUtils.jsm:1081
(Diff revision 12)
> +
> +  /**
> +   * Throw if an object is not a Date object.
> +   */
> +  ensureDate:
> +  function PU_ensureDate(arg) {

ditto
Comment on attachment 8864506 [details]
Bug 1249263 - add tests for History.jsm::`removeByFilter`,

https://reviewboard.mozilla.org/r/136192/#review141528

::: toolkit/components/places/tests/history/test_removeByFilter.js:31
(Diff revision 7)
> +  // Adding a witness URI
> +  let witnessURI = NetUtil.newURI("http://witnessmozilla.org/test_browserhistory/test_removeByFilter" + Math.random());
> +  yield PlacesTestUtils.addVisits(witnessURI);
> +  Assert.ok((yield PlacesTestUtils.isPageInDB(witnessURI)), "Witness URI is in database");
> +
> +  let removeByFilterTester = Task.async(function*(visits, filter, checkBeforeRemove, checkAfterRemove, useCallback, bookmarkedUri) {

heads-up: tomorrow we are going to land a patch that will convert all the code from Task.jsm to async/await (new ES construct).
If this lands before it won't be a big deal, otherwise you may have to change the code to async/away and stop using Task.jsm.
In general from now on we will stick to async/await instead of Task and yield.
See Bug 1353542 for further details.

Basically:
myvar = Task.async(function*(
becomes
myvar = async function(
and yield becomes await.
Attachment #8864506 - Flags: review?(mak77) → review+
Comment on attachment 8864506 [details]
Bug 1249263 - add tests for History.jsm::`removeByFilter`,

https://reviewboard.mozilla.org/r/136192/#review141534

::: toolkit/components/places/tests/history/test_removeByFilter.js:133
(Diff revision 7)
> +  ];
> +  let assertInDB = function(aUri) {
> +      Assert.ok((yield PlacesTestUtils.isPageInDB(aUri)));
> +  };
> +  let assertNotInDB = function(aUri) {
> +    Assert.ok(!(yield PlacesTestUtils.isPageInDB(aUri)));

yield cannot be used in normal functions. You can check your code using ./mach eslint for these errors
Comment on attachment 8864506 [details]
Bug 1249263 - add tests for History.jsm::`removeByFilter`,

https://reviewboard.mozilla.org/r/136192/#review141528

> heads-up: tomorrow we are going to land a patch that will convert all the code from Task.jsm to async/await (new ES construct).
> If this lands before it won't be a big deal, otherwise you may have to change the code to async/away and stop using Task.jsm.
> In general from now on we will stick to async/await instead of Task and yield.
> See Bug 1353542 for further details.
> 
> Basically:
> myvar = Task.async(function*(
> becomes
> myvar = async function(
> and yield becomes await.

I've fixed the issues, so I think we will be able to land as of now, if the 1-error-remaining issue does not actually prevent the code from landing.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8840f2afc5f9
add a `removeByFilter` method to filter by host and time,r=mak
I landed directly on inbound, since we can't autoland for the ghost issue bug.
Okay, I'm a bit unclear on the whole trees thing(I get the branches thing, but not the 'push to a closed tree' thing), but from what I understand, I should not mark this as bug as fixed till it is merged into central by a sherrif.

Anyway, thanks!
(In reply to milindl from comment #55)
> Okay, I'm a bit unclear on the whole trees thing(I get the branches thing,
> but not the 'push to a closed tree' thing), but from what I understand, I
> should not mark this as bug as fixed till it is merged into central by a
> sherrif.

Until time ago we had various integration trees: inbound, fx-team, ... and the main one mozilla-central. Things were landed on one of the integration trees, and then later merged into mozilla-central. bugs are fixed when the patch lands in mozilla-central (it's a partially-automated task nowadays so you don't usually care about it.).
with mozreview and autoland the process changed a bit, so most integration trees were closed in favor of autoland. Things are automatically landed on autoland and later merged into mozilla-central.

inbound is still around for edge cases, like this one :)

Trees can be CLOSED or APPROVAL REQUIRED for various reasons (from infrastructure problems to deliberate decisions) and in those cases in general it's not allowed to push to them unless told by a sheriff (one of the nice peoples taking care of the trees).

Thank you for the work here, it's very much appreciated, we're one step closer to fully-asynchronous history!
https://hg.mozilla.org/mozilla-central/rev/8840f2afc5f9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
This test fails after converting Task.jsm to async/await, here's a fix as discussed with Marco on IRC.
Attachment #8867139 - Flags: review?(mak77)
Assignee: i.milind.luthra → florian
Assignee: florian → i.milind.luthra
Attachment #8867139 - Flags: review?(mak77) → review+
Thanks for fixing this, sorry, I was not available on IRC today.
no worries, I rushed a little bit the test review to land sooner, and that's my fault. We fixed it, it's all good!
Pushed by florian@queze.net:
https://hg.mozilla.org/mozilla-central/rev/1178b701781d
fix test_removeByFilter.js to wait for the assertInDB and assertNotInDB promises, r=mak. a=tomcat
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: