475289 - Lazily initialize engineMetadataService

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

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

[Johnathan Nightingale [:johnath]]

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

[Ryan Flint [:rflint] (ping via IRC for reviews)]

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

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Branch version

Comment 4

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

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

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 403185 [details] [diff] [review]
Patch

>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

[Axel Hecht [:Pike]]

Gavin, does this ease our story for multi-locale builds?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I don't think it has any impact on that. It doesn't touch engine loading at all.

Comment 8

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/mozilla-central/rev/fbf106437925

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Gavin - would this help us in Fennec too? If so, I think we should look for 192 approval.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Don't think it would help in Fennec, since it doesn't initialize the search service on startup.