Closed Bug 475289 Opened 16 years ago Closed 15 years ago

Lazily initialize engineMetadataService


(Firefox :: Search, defect)

Not set



Firefox 3.7a1


(Reporter: rflint, Assigned: rflint)


(Keywords: perf, Whiteboard: [ts])


(2 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
The engineMetadataService currently does two things for us on startup: it provides the engine order if it's changed from the default and allows us to determine if an engine is hidden or not.
Our current caching of the hidden attribute doesn't help us because we're not setting the actual value in most cases, which causes us to always check the eMS and the selected engine is set without using the order, so we can delay building the engine order until the user changes or manages their engines.
Attachment #358794 - Flags: review?(
Whiteboard: [TStartup]
Ryan - now that we are landing things on central again - is this something we should revive?  It doesn't have tests, but if you think there's a real Ts win here, it is worth landing, right?

I know it's waiting on a Gavin review, but I rather suspect this fell off-radar, and probably needs unrotting?
Minus the CACHE_VERSION change, this patch should still apply. It was a fairly decent win when I wrote it and I doubt the situation has changed much since - I'll have to get new numbers for it.
Test coverage is provided by the general suite for the search service (bug 458810), which actually did fall off the radar :)
Whiteboard: [TStartup] → [ts]
Attached patch Branch versionSplinter Review
Attachment #358794 - Attachment is obsolete: true
Attachment #395767 - Flags: review?(
Attachment #358794 - Flags: review?(
Attachment #395767 - Attachment description: Update CACHE_VERSION → Branch version
Attachment #395767 - Flags: review?(
Attached patch PatchSplinter Review
I think we've been sitting on this win long enough - Paul, roping you in since you last touched most of what this patch is fiddling with (sorry!). Please feel free to grab me on IRC if you've got any questions. Thanks!
Attachment #403185 - Flags: review?(paul)
Flags: wanted-firefox3.6?
Comment on attachment 403185 [details] [diff] [review]

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>+  set _sortedEngines(aValue) {
>+    this.__sortedEngines = aValue;
>+  },

Just change buildSortedEngines list to set __sortedEngines directly, and omit this setter?

>+      if (this.__sortedEngines) {
>         this._sortedEngines.push(aEngine);

I'd use __sortedEngines directly here as well.

>     this._sortedEngines = this._sortedEngines.concat(alphaEngines);
>+    return this._sortedEngines;

could collapse the assignment/return here.
Attachment #403185 - Flags: review+
Attachment #403185 - Flags: review?(paul)
Gavin, does this ease our story for multi-locale builds?
I don't think it has any impact on that. It doesn't touch engine loading at all.
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.6a1 → Firefox 3.7a1
Gavin - would this help us in Fennec too? If so, I think we should look for 192 approval.
Don't think it would help in Fennec, since it doesn't initialize the search service on startup.
Flags: wanted-firefox3.6?
You need to log in before you can comment on or make changes to this bug.