Closed Bug 1126722 Opened 5 years ago Closed Last year

Overridden search engine order is not as expected

Categories

(Firefox :: Search, defect, P4)

31 Branch
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: gk, Assigned: mkaply)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files, 2 obsolete files)

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.
Priority: -- → P4
Whiteboard: [fxsearch]
Rank: 45
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
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?
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
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.
Attached patch Sort the result of getChildList (obsolete) — Splinter Review
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.
Attachment #8747123 - Flags: review?(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.
Attachment #8747123 - Flags: feedback+
> 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 on attachment 8747123 [details] [diff] [review]
Sort the result of getChildList

Removing the pending review request until there's a test then :-).
Attachment #8747123 - Flags: review?(florian)
Patch with extra sort removed and with test added.
Attachment #8747123 - Attachment is obsolete: true
Attachment #8747913 - Flags: review?(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.
Attachment #8747913 - Flags: review?(florian)
Attached patch Proper patchSplinter Review
Proper patch (I hope)
Attachment #8747913 - Attachment is obsolete: true
Attachment #8748170 - Flags: review?(Flore)
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.
Attachment #8748170 - Flags: review?(Flore) → review-
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.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---
Assignee: nobody → mozilla
Status: REOPENED → ASSIGNED
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.
Status: ASSIGNED → RESOLVED
Closed: Last yearLast year
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.