PlacesDBUtils should register itself in the idle-daily category

RESOLVED FIXED in mozilla2.0b7

Status

()

Toolkit
Places
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Gavin, Assigned: mak)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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?
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Updated

7 years ago
Depends on: 598966
(Assignee)

Comment 2

7 years ago
Created attachment 481062 [details] [diff] [review]
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.
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.
(Assignee)

Comment 4

7 years ago
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
(Assignee)

Updated

7 years ago
Summary: Places should register its PlacesDBUtils timer using the category manager → PlacesDBUtils should register itself in the idle-daily category
(Assignee)

Comment 5

7 years ago
Created attachment 481494 [details] [diff] [review]
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).
Attachment #481062 - Attachment is obsolete: true
Attachment #481494 - Flags: review?(sdwilsh)
(Assignee)

Comment 6

7 years ago
"minor" in the sense that they are small sized.
(Assignee)

Comment 7

7 years ago
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)
(Assignee)

Comment 8

7 years ago
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?)
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 10

7 years ago
yep bug 602871 is about that, I can't push this till that is fixed
(Assignee)

Comment 11

7 years ago
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+
(Assignee)

Comment 12

7 years ago
http://hg.mozilla.org/mozilla-central/rev/84abb3a647d9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Blocks: 605503

Updated

7 years ago
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.