1126722 - Overridden search engine order is not as expected

Status: RESOLVED WONTFIX

(Firefox :: Search, defect, P4)

Description

[Georg Koppen]

Suppose I want to have Bing as my default search engine and as the first one in the search engine list, Google at the second place and Yahoo at the third place. In order to get that I write an extension (attached) containing essentially the following preference (overwrites):

pref("browser.search.defaultenginename", "Bing");
pref("browser.search.order.extra.1", "Bing");
pref("browser.search.order.extra.2", "Google");
pref("browser.search.order.extra.3", "Yahoo");

Surprisingly that does not give me the desired order after installing the extension:

In ESR 31 I get (across platform and OS): Yahoo, Bing and Google + the remaining search engines and in Firefox 35+ I get Bing, Yahoo, Google + the remaining search engines (across platform and OS as well).

The expected result is: Bing, Google, Yahoo + the remaining search engines.

Note: This is working well if one just specifies browser.search.order.extra.1 and browser.search.order.extra.2 just when the third place should be specified, too, things go weird.

Comment 1

[Georg Koppen]

Attachment: searchengineoverride.xpi

Comment 2

[Florian Quèze [:florian]]

Are you in the US? If so, you likely need to also set the browser.search.order.US.* prefs, see http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#402

Comment 3

[Georg Koppen]

No. Or do you mean whether I used an en-US bundle? Maybe. I could try to reproduce the behavior with an ESR45 based browser. Would we need to set browser.search.order.DE.* prefs etc. for the other bundles then, too?

Comment 4

[Florian Quèze [:florian]]

I mean based on your location, detected from the IP you had at the time the profile was created. I assume with Tor that's probably a random country. Or have you disabled the code doing the geo-ip location check?

What values do you have for these prefs:
browser.search.isUS
browser.search.region
browser.search.countryCode

Comment 5

[Mike Kaply [:mkaply]]

The reason this is happening is because getChildList is not guaranteed to return the preferences in alphabetical order.

When I tested this, I got back:

browser.search.order.extra.1
browser.search.order.extra.3
browser.search.order.extra.2

I think we should be alphabetizing the results from getChildList in this case.

Comment 6

[Mike Kaply [:mkaply]]

Attachment: Sort the result of getChildList

I was testing browser.search.order.extra independently and had just found this problem.

The original design of this pref was for one offs:

browser.search.order.extra.google="Google" 

to move Google to the top, but I think the numeric usage is valid and we should handle it.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8747123 [details] [diff] [review]
Sort the result of getChildList

Review of attachment 8747123 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable, thanks for investigating this! How much effort would it be to add test coverage for this behavior?

::: toolkit/components/search/nsSearchService.js
@@ +4224,5 @@
>  
>        // ... or engines sorted by default near the top of the list.
>        if (!sendSubmissionURL) {
>          let extras =
> +          Services.prefs.getChildList(BROWSER_SEARCH_PREF + "order.extra.").sort();

Looks like the sort() doesn't make any difference here, as the result of the computation we are doing here isn't an ordered list of engines but just a boolean indicating whether the order for the current default engine has been customized.

Comment 8

[Mike Kaply [:mkaply]]

> How much effort would it be to add test coverage for this behavior?

Great question. I was thinking about that this weekend. It looks like order.extra isn't tested at all, so I'll put together a test.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8747123 [details] [diff] [review]
Sort the result of getChildList

Removing the pending review request until there's a test then :-).

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Sort the result of getChildList and add test

Patch with extra sort removed and with test added.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8747913 [details] [diff] [review]
Sort the result of getChildList and add test

Looks like you forgot to |hg add| the test_order.js file.

Comment 12

[Mike Kaply [:mkaply]]

Attachment: Proper patch

Proper patch (I hope)

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8748170 [details] [diff] [review]
Proper patch

Review of attachment 8748170 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/tests/xpcshell/test_order.js
@@ +6,5 @@
> +
> +"use strict";
> +
> +function run_test() {
> +  Services.prefs.setCharPref(BROWSER_SEARCH_PREF + "order.extra.1", "Twitter");

Tests in toolkit/ should avoid relying on the search plugins shipped by browser/.

Most (if not all) of the tests here install test search plugins and only rely on these being around.

Comment 14

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 15

[Mike Kaply [:mkaply]]

This isn't something worth fixing at this point. We're moving away from these prefs, and this was a legacy extension.

You can reorder the search engine in preferences.