570387 - PlacesDBUtils should register itself in the idle-daily category

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

It would be nice if the PlacesDBUtils timer were registered using the category manager rather than by manually invoking registerTimer in the startup path. That requires it to be registered by a component that's garanteed to be instantiated sometime during startup. I don't know whether any of the existing Places JS components fit the bill (both nsPlacesDBFlush.js and nsPlacesExpiration.js seem to be instantiated by history observer events or something), but even if they don't, maybe we could convert it into a JS component rather than a module?

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

actually on the daily timer we start listening for idle, I guess if we should add an idle-daily category for components that is invoked on daily idle notification instead of using the timer manager category.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

This is a possible solution.  I've added a PlacesCategoriesStarter.js component that can be registered to categories and used as a starter for modules or any other stuff. It's pretty small.
I don't want to change the module to a component because it is a known module for users and QA, there is also some article explaining how to vacuum and do maintenance with it. So changing it would break all of that.
With my patch for bug 598966 (under testing) it can startup at idle-daily and launch maintenance, or any other future target.

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> I don't want to change the module to a component because it is a known module
> for users and QA, there is also some article explaining how to vacuum and do
> maintenance with it. So changing it would break all of that.
> With my patch for bug 598966 (under testing) it can startup at idle-daily and
> launch maintenance, or any other future target.
We could make the module do a lazy getter for the component, if push came to shove.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I prefer a centralized component able to satisfy any future need, rather than a module+component for each case. it's small and starts only on the category, its cost is visible only at the first cached registration

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Shawn, sorry if i'm overloading you with reviews, but dietrich's not around iirc.

The changes to PlacesDBUtils are mostly cleanup to use modules and removal of timers, this module is well tested by test_preventive_maintenance.js and test_preventive_maintenance_checkAndFixDatabase.js, so it should just require a quick look.
Other changes are minor (mostly the component addition).

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

"minor" in the sense that they are small sized.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 481494 [details] [diff] [review]
patch v1.1

moving review to dietrich since Shawn is going away for some days and is unlikely to find time soon.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm I thought of a possible problem here, xpcshell tests don't share prefs, so each test could notify idle-daily and run these tasks, becoming much slower, in future we would also run vacuum for each test...
We already have similar issues because tinderboxes always report high idle times.

we should probably make xpcshell test harness do something to reset idle time (could synthesize keyboard event work?)

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 481494 [details] [diff] [review]
patch v1.1

r=me on these changes, if the idle timer isn't hitting during test runs. maybe talk to releng to see if there's something to prevent? i'm not sure, but kinda doubt that synthesized events would un-idle. iirc it's tied into OS-level notifications from hardware.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yep bug 602871 is about that, I can't push this till that is fixed

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 481494 [details] [diff] [review]
patch v1.1

Asking approval to land once the underlying issue with idle and xpcshell tests is fixed.
This removes some unneeded work at startup and the module is already well tested.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/84abb3a647d9