Closed Bug 653078 Opened 10 years ago Closed 9 years ago

Init the livemarks service delayed in PlacesCategoriesStarter rather than relying on browser.js

Categories

(Toolkit :: Places, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mak, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

While we wait for a overhaul of the service, we should move its initialization in toolkit and use PlacesCategoriesStarter rather then relying on browser.js initing it for us.

Also check if we can make the service init mostly a no-op and delay fetch of all the livemarks to the first update, Sync uses the service for isLivemark and it's possible it's bypassing our delay.
Attached patch patch v1.0Splinter Review
No perf advantage here, but I think makes sense to move this to Places code, bot from a browser cleanup point of view and to avoid Places users having to figure out how to startup it. In future we may remove start() and handle startup more gracefully.
FWIW, I already fixed Sync code that may init lm service earlier in other patches.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #538381 - Flags: review?(dietrich)
Comment on attachment 538381 [details] [diff] [review]
patch v1.0

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

profile-after-change occurs how long before delayedStartup() is called?
(In reply to comment #2)
> profile-after-change occurs how long before delayedStartup() is called?

I don't have timing sadly, but it's a good question. Usually I'd expect it to be ignorable, but clearly it may change if FF starts on a busy system. I guess if it would be safer to bump up delay to 10s.
I'm not really ok making this change without a more deterministic approach. Especially since there's not a material benefit (ie: perf) making it worth the risk. Yeah, it's nice for other Places consumers, but we know those people, and they know where to find us, so it's not a critical problem.
Comment on attachment 538381 [details] [diff] [review]
patch v1.0

yeah, considering the no-benefit I'm putting this on sleep for now.
Attachment #538381 - Flags: review?(dietrich)
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
this is no more needed
Status: NEW → RESOLVED
Closed: 9 years ago
Depends on: livemarksIO
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.