1113175 - Implement clear() in History.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

we need an async replacement for removeAllPages(), it should basically be a js port of the current cpp code in History.jsm
it should not take any argument, and the first implementation should also likely not return anything (in future we'll evaluate whether we need it to return a bool)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1113175-Implement-async-History.clear.patch

Converted a few tests to use it (i.e. bug 1113178) and it seems to do fine.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

This should probably wait for bug 1101553.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1113175-Implement-async-History.clear.patch, v2

Using history.getObservers() now.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1113175-Implement-async-History.clear.patch, v3

Yay for tests. Forgot to notify onManyFrecenciesChanged() observers.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8548808 [details] [diff] [review]
0001-Bug-1113175-Implement-async-History.clear.patch, v3

Review of attachment 8548808 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/History.jsm
@@ +342,5 @@
>    /**
> +   * Clear all history.
> +   *
> +   * @return (Promise)
> +   *      A promise resoled once the operation is complete.

typo: resoled

@@ +480,5 @@
> +  let db = yield DBConnPromised;
> +
> +  yield db.executeTransaction(function* () {
> +    // Remove all history.
> +    yield db.execute("DELETE FROM moz_historyvisits");

I think for now we don't need a transaction here, most of the job is done by this single query, single queries already run in their own transaction.

We will re-evaluate in future if we decide to merge back some stuff here from places expiration. Indeed the primary reason the job was split to expiration, is because removeAllPages was synchronous, now that it is not anymore we could simplify expiration and do all the cleanup here. But I'd suggest leaving that to a separate bug (that we should probably file to simplify intra-components dependencies)

It's instead ok to use execute instead of executeCached since I don't expect a user to run clear() very often.

@@ +493,5 @@
> +    // Invalidate frecencies for the remaining places. This must happen
> +    // after the notification to ensure it runs enqueued to expiration.
> +    yield db.execute(
> +      `UPDATE moz_places SET frecency =
> +       (CASE WHEN url BETWEEN 'place:' AND 'place;' THEN 0 ELSE -1 END)

please indent this once more

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/f2058d853176

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f2058d853176