523453 - optionally allow loading engines from chrome:// URIs

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

Fennec would like this functionality for multi-locale builds, so that we can package the search engines in the locale JARs. I'm going to make this opt-in using a pref.

Comment 1

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

Attachment: patch

Similar to the patch we discussed on IRC. I made one change to preserve the [app] _ids for JAR-loaded plugins, otherwise the engine sorting was broken for existing Fennec users (since they'd have the useDBForOrder pref set, and the engine IDs would all change). Slightly risky, but it's pretty unlikely that we'll ever get a conflict in practice, there.

Comment 2

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

Comment on attachment 407389 [details] [diff] [review]
patch

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
>+__defineGetter__("gChromeReg", function () {

nit: prevailing style is function()

>   __id: null,
>   get _id() {
>     if (!this.__id) {

Invert and return early here?

>+      // This means we're vulnerable to conflicts if a file loaded from a JAR
>+      // has the same filename as a file loaded from the app dir, but with a
>+      // different engine name. People using the JAR functionality should be
>+      // careful not to do that!
>+      if (this._isInAppDir || this._isInJAR) {
>+        let leafName;
>+        if (this._file)
>+          leafName = this._file.leafName;
>+        else {
>+          // If we've reached this point, we must be loaded from a JAR, which
>+          // also means we should have a URL.

Since the app dir vs. jar distinction is already being made, can't you use [jar]+leafName to avoid the conflicts?

>   _loadEnginesFromCache: function SRCH_SVC__loadEnginesFromCache(aDir) {
>+        if (json.filePath)
>+          engine = new Engine({cached: true, value: json.filePath}, json._dataType,
>+                                  json._readOnly);

nit: indent here is a little funky.

>+        else if (json._url)
>+          engine = new Engine(makeURI(json._url), json._dataType, json._readOnly);

Instead of creating a URI, this should just pass in the URL, and like _file, _url should be a getter that can convert to a URI when needed. You can just turn |cached| into a type string and work with that in the Engine constructor.

When creating a trunk version of this, note that gIoSvc is now NetUtil.ioService.

Looks good!

Comment 3

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

(In reply to comment #2)
> Since the app dir vs. jar distinction is already being made, can't you use
> [jar]+leafName to avoid the conflicts?

Er, right. Totally missed comment 1...

Comment 4

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

Attachment: updated to comments (trunk version)

Comment 5

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

Attachment: as landed

Forgot to replace one gIoSvc in the previous patch.

Comment 6

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

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

Comment 7

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

Attachment: 192 version

Comment 8

[Johnathan Nightingale [:johnath]]

Blocks a fennec blocker. This one makes me nervous; even preffed off, I wish we had better tests, here.

Comment 9

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

https://hg.mozilla.org/releases/mozilla-1.9.2/rev/d4f44d679884