Last Comment Bug 475289 - Lazily initialize engineMetadataService
: Lazily initialize engineMetadataService
: perf
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 3.7a1
Assigned To: Ryan Flint [:rflint] (ping via IRC for reviews)
: Florian Quèze [:florian] [:flo]
Depends on:
  Show dependency treegraph
Reported: 2009-01-25 19:34 PST by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2010-01-25 16:30 PST (History)
16 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (11.41 KB, patch)
2009-01-25 19:34 PST, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Splinter Review
Branch version (10.27 KB, patch)
2009-08-20 20:44 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details | Diff | Splinter Review
Patch (10.02 KB, patch)
2009-09-27 21:48 PDT, Ryan Flint [:rflint] (ping via IRC for reviews) review+
Details | Diff | Splinter Review

Description User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-01-25 19:34:00 PST
Created attachment 358794 [details] [diff] [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.
Comment 1 User image Johnathan Nightingale [:johnath] 2009-06-16 12:54:41 PDT
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?
Comment 2 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-06-18 06:33:28 PDT
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 :)
Comment 3 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-08-20 20:44:41 PDT
Created attachment 395767 [details] [diff] [review]
Branch version
Comment 4 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-09-27 21:48:45 PDT
Created attachment 403185 [details] [diff] [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!
Comment 5 User image :Gavin Sharp [email:] 2009-09-28 15:09:45 PDT
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.
Comment 6 User image Axel Hecht [:Pike] 2009-09-29 11:37:44 PDT
Gavin, does this ease our story for multi-locale builds?
Comment 7 User image :Gavin Sharp [email:] 2009-09-29 12:08:25 PDT
I don't think it has any impact on that. It doesn't touch engine loading at all.
Comment 8 User image Ryan Flint [:rflint] (ping via IRC for reviews) 2009-10-02 20:49:40 PDT
Comment 9 User image Mark Finkle (:mfinkle) (use needinfo?) 2009-10-11 17:36:39 PDT
Gavin - would this help us in Fennec too? If so, I think we should look for 192 approval.
Comment 10 User image :Gavin Sharp [email:] 2009-10-18 07:53:05 PDT
Don't think it would help in Fennec, since it doesn't initialize the search service on startup.

Note You need to log in before you can comment on or make changes to this bug.