Closed Bug 570387 Opened 10 years ago Closed 10 years ago

PlacesDBUtils should register itself in the idle-daily category

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: Gavin, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
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.
Depends on: 598966
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(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.
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
Summary: Places should register its PlacesDBUtils timer using the category manager → PlacesDBUtils should register itself in the idle-daily category
Attached patch patch v1.1Splinter Review
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).
Attachment #481062 - Attachment is obsolete: true
Attachment #481494 - Flags: review?(sdwilsh)
"minor" in the sense that they are small sized.
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.
Attachment #481494 - Flags: review?(sdwilsh) → review?(dietrich)
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?)
Depends on: 602871
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.
Attachment #481494 - Flags: review?(dietrich) → review+
yep bug 602871 is about that, I can't push this till that is fixed
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.
Attachment #481494 - Flags: approval2.0?
Attachment #481494 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/84abb3a647d9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.