Last Comment Bug 1076775 - Implement History.removeVisitsByFilter
: Implement History.removeVisitsByFilter
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla40
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 834545 1043863 1072364 1090157
Blocks: 739219 871908 1089691 1089695 1105492
  Show dependency treegraph
 
Reported: 2014-10-02 03:51 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2015-04-14 01:48 PDT (History)
8 users (show)
mmucci: firefox‑backlog+
ryanvm: in‑testsuite+
ttaubert: qe‑verify-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 40.1 - 13 Apr
Points: 8
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Possible API (3.57 KB, patch)
2014-10-28 04:57 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Implementation (20.45 KB, patch)
2014-11-18 14:09 PST, David Teller [:Yoric] (please use "needinfo")
mak77: feedback+
Details | Diff | Splinter Review
Implementation, v2 (24.69 KB, patch)
2014-11-24 07:30 PST, David Teller [:Yoric] (please use "needinfo")
mak77: review+
Details | Diff | Splinter Review
2. Cumulative patch (8.17 KB, patch)
2014-12-09 09:12 PST, David Teller [:Yoric] (please use "needinfo")
mak77: review+
Details | Diff | Splinter Review
Folded patch (25.62 KB, patch)
2014-12-10 04:38 PST, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Implementation, v3 (25.91 KB, patch)
2014-12-11 07:58 PST, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
MozReview Request: bz://1076775/Yoric (39 bytes, text/x-review-board-request)
2015-02-18 07:10 PST, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Review
MozReview Request: bz://1076775/Yoric (39 bytes, text/x-review-board-request)
2015-04-08 08:53 PDT, David Teller [:Yoric] (please use "needinfo")
mak77: review+
dteller: review+
Details | Review
Implementation (29.70 KB, patch)
2015-04-09 10:13 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Implementation (31.92 KB, patch)
2015-04-10 05:37 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Implementation (31.79 KB, patch)
2015-04-10 08:39 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description User image David Teller [:Yoric] (please use "needinfo") 2014-10-02 03:51:33 PDT
Spinoff from bug 834545.
Comment 1 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-10-27 07:41:34 PDT
Is this API described anywhere? What is its purpose?
Comment 2 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-10-27 07:42:47 PDT
More specifically: is this the replacement for removeVisitsByTimeframe?
Comment 3 User image Marco Bonardo [::mak] 2014-10-27 07:47:36 PDT
(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.
Comment 4 User image David Teller [:Yoric] (please use "needinfo") 2014-10-27 08:52:54 PDT
So, yes, it is a superset of `removeVisitsByTimeFrame`.
Comment 5 User image David Teller [:Yoric] (please use "needinfo") 2014-10-28 04:57:45 PDT
Created attachment 8512599 [details] [diff] [review]
Possible API
Comment 6 User image Marco Bonardo [::mak] 2014-10-30 14:59:44 PDT
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.
Comment 7 User image David Teller [:Yoric] (please use "needinfo") 2014-10-31 04:55:16 PDT
(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.
Comment 8 User image Marco Bonardo [::mak] 2014-10-31 06:10:53 PDT
(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.
Comment 9 User image David Teller [:Yoric] (please use "needinfo") 2014-10-31 06:14:59 PDT
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?
Comment 10 User image Marco Bonardo [::mak] 2014-10-31 06:17:28 PDT
I think we should not even take an array. our use-cases are much simpler than that.
why not just take a filter object?
Comment 11 User image David Teller [:Yoric] (please use "needinfo") 2014-10-31 06:19:03 PDT
Ok, fair enough.
Comment 12 User image David Teller [:Yoric] (please use "needinfo") 2014-11-18 14:09:16 PST
Created attachment 8524882 [details] [diff] [review]
Implementation

Here we go.
Comment 13 User image David Teller [:Yoric] (please use "needinfo") 2014-11-20 19:41:43 PST
Review ping?
Comment 14 User image Marco Bonardo [::mak] 2014-11-21 09:25:46 PST
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
Comment 15 User image David Teller [:Yoric] (please use "needinfo") 2014-11-21 12:13:32 PST
(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.
Comment 16 User image Marco Bonardo [::mak] 2014-11-21 14:16:20 PST
(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.
Comment 17 User image David Teller [:Yoric] (please use "needinfo") 2014-11-24 07:30:05 PST
Created attachment 8527732 [details] [diff] [review]
Implementation, v2

Applied feedback.
Comment 18 User image Marco Bonardo [::mak] 2014-11-26 07:48:35 PST
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.
Comment 19 User image David Teller [:Yoric] (please use "needinfo") 2014-12-09 09:12:28 PST
Created attachment 8533790 [details] [diff] [review]
2. Cumulative patch

I decided to go for the using your `notify` function, which required a few changes.
Comment 20 User image Marco Bonardo [::mak] 2014-12-10 02:41:11 PST
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 :)
Comment 21 User image David Teller [:Yoric] (please use "needinfo") 2014-12-10 04:35:27 PST
Will do :)

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=64d923a92e3a
Comment 22 User image David Teller [:Yoric] (please use "needinfo") 2014-12-10 04:38:51 PST
Created attachment 8534320 [details] [diff] [review]
Folded patch
Comment 25 User image David Teller [:Yoric] (please use "needinfo") 2014-12-11 07:53:32 PST
https://tbpl.mozilla.org/?tree=Try&rev=eff7d81cd745
Comment 26 User image David Teller [:Yoric] (please use "needinfo") 2014-12-11 07:58:37 PST
Created attachment 8535023 [details] [diff] [review]
Implementation, v3

Fixed a typo. This won't change the linux32 results.
Comment 27 User image David Teller [:Yoric] (please use "needinfo") 2015-02-18 07:10:22 PST
Created 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
Comment 28 User image David Teller [:Yoric] (please use "needinfo") 2015-02-18 07:10:56 PST
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
Comment 29 User image David Teller [:Yoric] (please use "needinfo") 2015-02-19 02:36:04 PST
https://reviewboard.mozilla.org/r/3983/#review3185

Ship It!
Comment 30 User image David Teller [:Yoric] (please use "needinfo") 2015-02-25 06:01:00 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e70c90ef407
Comment 31 User image David Teller [:Yoric] (please use "needinfo") 2015-02-25 08:52:02 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=baaaf71520be
Comment 32 User image David Teller [:Yoric] (please use "needinfo") 2015-03-08 01:30:29 PST
https://reviewboard.mozilla.org/r/3981/#review4027

This has been replaced by https://reviewboard.mozilla.org/r/4975/.
Comment 33 User image David Teller [:Yoric] (please use "needinfo") 2015-04-08 08:53:37 PDT
Created attachment 8589703 [details]
MozReview Request: bz://1076775/Yoric

/r/6715 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak

Pull down this commit:

hg pull -r f055e0581135a67fc1897b6fb0ff754d9c05976f https://reviewboard-hg.mozilla.org/gecko/
Comment 34 User image Marco Bonardo [::mak] 2015-04-09 07:02:29 PDT
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 35 User image Marco Bonardo [::mak] 2015-04-09 07:02:41 PDT
Comment on attachment 8589703 [details]
MozReview Request: bz://1076775/Yoric

https://reviewboard.mozilla.org/r/6713/#review5663
Comment 36 User image David Teller [:Yoric] (please use "needinfo") 2015-04-09 07:48:50 PDT
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.
Comment 37 User image David Teller [:Yoric] (please use "needinfo") 2015-04-09 07:49:00 PDT
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 38 User image David Teller [:Yoric] (please use "needinfo") 2015-04-09 07:50:41 PDT
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 39 User image David Teller [:Yoric] (please use "needinfo") 2015-04-09 07:51:13 PDT
Comment on attachment 8589703 [details]
MozReview Request: bz://1076775/Yoric

https://reviewboard.mozilla.org/r/6713/#review5677

Ship It!
Comment 40 User image David Teller [:Yoric] (please use "needinfo") 2015-04-09 07:51:33 PDT
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d6ec548e184
Comment 41 User image Marco Bonardo [::mak] 2015-04-09 08:05:57 PDT
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).
Comment 42 User image Marco Bonardo [::mak] 2015-04-09 08:06:10 PDT
https://reviewboard.mozilla.org/r/6713/#review5681

> No deep reason, I just find the control and error flows much more readable.

OK, whatever
Comment 43 User image David Teller [:Yoric] (please use "needinfo") 2015-04-09 10:13:37 PDT
Created attachment 8590356 [details] [diff] [review]
Implementation

Applied feedback. Let's land this.
Comment 44 User image Ryan VanderMeulen [:RyanVM] 2015-04-09 12:22:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff04a9792eb2
Comment 46 User image Marco Bonardo [::mak] 2015-04-10 03:40:44 PDT
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".
Comment 47 User image David Teller [:Yoric] (please use "needinfo") 2015-04-10 05:37:39 PDT
Created attachment 8590806 [details] [diff] [review]
Implementation

Added some cleanup code in test_remove.js
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c893729e1b
Comment 48 User image Carsten Book [:Tomcat] 2015-04-10 06:15:39 PDT
https://hg.mozilla.org/integration/fx-team/rev/c426c7eb3b43
Comment 49 User image Carsten Book [:Tomcat] 2015-04-10 08:12:51 PDT
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2636489&repo=fx-team
Comment 50 User image David Teller [:Yoric] (please use "needinfo") 2015-04-10 08:14:37 PDT
Strange. It passes tests locally and on try. I'll investigate.
Comment 51 User image David Teller [:Yoric] (please use "needinfo") 2015-04-10 08:36:10 PDT
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.
Comment 52 User image Marco Bonardo [::mak] 2015-04-10 08:37:25 PDT
we are mostly passing it in for API coherence, in future we might decide to wait for the update, but it might be expensive...
Comment 53 User image David Teller [:Yoric] (please use "needinfo") 2015-04-10 08:39:59 PDT
Created attachment 8590895 [details] [diff] [review]
Implementation

Deactivated check.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffbc0f842a06
Comment 54 User image Carsten Book [:Tomcat] 2015-04-13 06:37:36 PDT
https://hg.mozilla.org/integration/fx-team/rev/b894541dc70f
Comment 55 User image Wes Kocher (:KWierso) 2015-04-13 16:32:57 PDT
https://hg.mozilla.org/mozilla-central/rev/b894541dc70f

Note You need to log in before you can comment on or make changes to this bug.