Closed Bug 1113175 Opened 6 years ago Closed 6 years ago

Implement clear() in History.jsm

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.1 - 26 Jan

People

(Reporter: mak, Assigned: ttaubert)

References

Details

Attachments

(1 file, 2 obsolete files)

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)
Blocks: 1113178
Converted a few tests to use it (i.e. bug 1113178) and it seems to do fine.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8545285 - Flags: review?(mak77)
Iteration: --- → 37.3 - 12 Jan
Points: --- → 3
Flags: qe-verify-
Flags: firefox-backlog+
This should probably wait for bug 1101553.
Depends on: 1101553
Using history.getObservers() now.
Attachment #8545285 - Attachment is obsolete: true
Attachment #8545285 - Flags: review?(mak77)
Attachment #8548119 - Flags: review?(mak77)
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Yay for tests. Forgot to notify onManyFrecenciesChanged() observers.
Attachment #8548119 - Attachment is obsolete: true
Attachment #8548119 - Flags: review?(mak77)
Attachment #8548808 - Flags: review?(mak77)
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
Attachment #8548808 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/f2058d853176
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.