587691 - need a way to get the default engine, even if it is hidden

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

For the functionality added in bug 586821, we want to always use the default engine, even if it's hidden.

Comment 1

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

One option is to change the defaultEngine getter to just always return the original default, even it if is hidden. That would mean context menu search would continue using that engine even though it "doesn't exist". It might also break other people's expectations.

Another option is to add some extra property, but I have no idea what we'd call it.

Comment 2

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

Attachment: add originalDefaultEngine

Comment 3

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

Attachment: patch

Comment 4

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

I'm not super pleased that we baked the "or fall back to the first visible engine" logic into the search service rather than in the frontend, because it kind of ties our hands a bit.

This patch just adds the additional property (originalDefaultEngine), and updates the docs to match, which I think is the simplest and safest choice at this point, even though it's sucky to have both "defaultEngine" and "originalDefaultEngine" properties...

Comment 5

[Marco Bonardo [:mak]]

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


>+  /**
>+   * The original default engine. This differs from the "defaultEngine"
>+   * attribute in that it always returns a given build's default engine,
>+   * regardless of whether it is hidden. May be null if there are no visible
>+   * search engines.
>+   */
>+  readonly attribute nsISearchEngine originalDefaultEngine;

can it really be null if there are no visible engines? the phrase "if there are no visible search engines" looks conflicting with "regardless of whether it is hidden" and the default engine pref should always exist.

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

>+  get originalDefaultEngine() {
>+    const defPref = BROWSER_SEARCH_PREF + "defaultenginename";
>+    return this.getEngineByName(getLocalizedPref(defPref, ""));
>+  },

looks like this could cache the value or we expect it to change during runtime? (the old code was looking like trying to cache it, but wrongly), even if the pref can be changed it's unlikely someone would do it and expect immediate effect.

Comment 6

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

(In reply to comment #5)
> can it really be null if there are no visible engines?

Well, it can be null if there are no installed engines, but I think the search service doesn't handle that case very well, so probably not worth covering that case here - I'll adjust the comment.

> looks like this could cache the value or we expect it to change during
> runtime?

Yeah, it could be cached. And this file could use Services.jsm and XPCOMUtils lazy getters, too - but search service doesn't have a test suite at the moment, so I really want to be conservative and avoid unnecessary changes. I'll file a followup.

Comment 7

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

Attachment: updated patch

Comment 8

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

Attachment: export for checkin

Comment 9

[Bas Schouten (:bas.schouten)]

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