1097542 - Unclear order logic for searchplugins in Firefox for Android

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Francesco Lodolo [:flod]]

When not explicitly ordered in region.properties, the remaining searchplugins should be displayed in alphabetical order of ShortName
http://hg.mozilla.org/mozilla-central/file/default/toolkit/components/search/nsSearchService.js#l3803

I noticed the strange ordering on a multilocale Nightly build set to Italian, but it reproduces on a single locale build of Aurora for both French and Italian.

On desktop eBay is displayed before Wikipedia, on Android it's the opposite (Twitter->Wikipedia->eBay).

Files have the same ShortName attribute on both /browser and /mobile:
eBay-it.xml -> eBay
wikipedia-it.xml -> Wikipedia (it)

This happens on a multi-locale build, but I also tested French and Italian on single-locale Aurora builds with the same results.

No clue why there is such a different behavior.

Comment 1

[Francesco Lodolo [:flod]]

Actually, I might have found something.

var alphaEngines = ['Twitter', 'Wikipedia (it)', 'eBay'];
alphaEngines = alphaEngines.sort(function (a, b) {
  return a.localeCompare(b);
});
alert(alphaEngines);

http://jsfiddle.net/hy4f8tsg/
On desktop returns "eBay,Twitter,Wikipedia (it)".
On mobile returns "Twitter,Wikipedia (it),eBay".

Comment 2

[:Margaret Leibovic]

(In reply to Francesco Lodolo [:flod] from comment #1)
> Actually, I might have found something.
> 
> var alphaEngines = ['Twitter', 'Wikipedia (it)', 'eBay'];
> alphaEngines = alphaEngines.sort(function (a, b) {
>   return a.localeCompare(b);
> });
> alert(alphaEngines);
> 
> http://jsfiddle.net/hy4f8tsg/
> On desktop returns "eBay,Twitter,Wikipedia (it)".
> On mobile returns "Twitter,Wikipedia (it),eBay".

That is very weird, I could also reproduce that. I wonder if the localeCompare implementation be doing something different in Fennec as opposed to desktop.

Jeff, could this be a problem with the JS engine?

Comment 3

[Richard Newman [:rnewman]]

If the JS engine is relying on a libc function for locale awareness, it might well be wrong on Android (cf Bug 864753).

Comment 4

[Jeff Walden [:Waldo]]

On desktop, using localeCompare relies upon our Intl.Collator implementation for its behavior, which in turn depends upon what ICU does.  ICU will order strings in whatever way it thinks is sensible for the system-chosen locale (not sure how we pick that on mobile systems).  (Which is exactly what the Intl spec requires it to do: locale-sensitively compare the two strings "in an implementation-defined fashion".)  In my testing, this seems to perform a case-insensitive ordering of strings, basically.

On mobile, however, we don't have Intl or ICU yet (bug 864843, bug 866301).  What we use there is a hairy series of calls into the locale service, through a collation factory, to create an nsICollation object to do the comparison.  I think that eventually hits http://mxr.mozilla.org/mozilla-central/source/intl/locale/unix/nsCollationUnix.cpp#103 which does a strcoll() call to collate the strings.  I believe the passed-in strength is case-sensitive, so we'll get whatever strcoll() does on mixed-case strings.  Which may well be to sort all uppers before all lowers, because codepoint order.

The quick, stupid hackaround would be to use |a.toUpperCase().localeCompare(b.toUpperCase())|.  If you have actual XPCOM access here you could create an nsICollator and pass in case-insensitive yourself.  If you don't have XPCOM access, you don't have any nice, clean options, short of waiting for Intl/ICU to be enabled on mobile, or at least none I'm aware of.

Comment 5

[Richard Newman [:rnewman]]

Thanks for the excellent writeup!

Comment 6

[Richard Newman [:rnewman]]

Attachment: Use explicit collation in nsSearchService. v1

Untested patch; I'm procrastinating, so I might as well get something useful out of it.

I also discovered a typo:

http://dxr.mozilla.org/mozilla-central/source/intl/locale/nsICollation.idl#37

Sadface.

Comment 7

[Richard Newman [:rnewman]]

(bzexport automatically assigned this.)

Comment 8

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

Richard - Want to run with that patch?

Comment 9

[Richard Newman [:rnewman]]

I can take it, but I can't guarantee 35. Will give it a shot.

Comment 10

[Richard Newman [:rnewman]]

With this change applied, desktop search xpcshell tests pass, and search engines are ordered alphabetically.

Testing on Android now.

Comment 11

[Richard Newman [:rnewman]]

Well, it doesn't break, but my hacky multilocale build seems to only have en-US search engines, so I can't actually test this.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8521950 [details] [diff] [review]
Use explicit collation in nsSearchService. v1

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

Interesting.  Hardcoding ASCII insensitivity makes sense to me, but hardcoding accent insensitivity worries me a little... maybe there are some languages whose collations commonly ignore accents but others whose don't?  According to Jeff, localeCompare would automatically handle those differences per user, on desktop at least.  In other words, this potentially slightly changes behavior on desktop.

(In reply to Richard Newman [:rnewman] from comment #6)
> I also discovered a typo:

And the first S in "insensitive" is capitalized in kCollationCaseInSensitive: http://mxr.mozilla.org/mozilla-central/source/intl/locale/nsICollation.idl#43  O_o

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8521950 [details] [diff] [review]
Use explicit collation in nsSearchService. v1

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

I assume sorting the engine list is a rare operation, such that it's not worth creating a single collator and putting it to ongoing use.  (Note that the previous localeCompare did cache collator across calls, tho it's not obvious from this code.)

::: toolkit/components/search/nsSearchService.js
@@ +3802,5 @@
>      }
> +
> +    let locale = Cc["@mozilla.org/intl/nslocaleservice;1"]
> +                   .getService(Ci.nsILocaleService)
> +                   .newLocale(getLocale());

getLocale() uses a completely different scheme to figure out locale, than the prior localeCompare did (which basically did getService(...).applicationLocale).  This is at least simpler to track down what's being used, maybe.  And it's what the rest of this file uses.  But it is sort of a behavior change, somewhat.

@@ +3806,5 @@
> +                   .newLocale(getLocale());
> +    let collation = Cc["@mozilla.org/intl/collation-factory;1"]
> +                      .createInstance(Ci.nsICollationFactory)
> +                      .CreateCollation(locale);
> +    const strength = Ci.nsICollation.kCollationCaseInSensitive;

o_O at that case

@@ +3808,5 @@
> +                      .createInstance(Ci.nsICollationFactory)
> +                      .CreateCollation(locale);
> +    const strength = Ci.nsICollation.kCollationCaseInSensitive;
> +    let comparator = (a, b) => collation.compareString(strength, a.name, b.name);
> +    alphaEngines = alphaEngines.sort(comparator);

arr.sort() mutates arr and returns arr, so no reason to have the assignment here (and a very slight reason not to, in that this adds an extra "dependency" on examining the result of the sort call, versus using the existing, already-retrieved value).

Comment 14

[Richard Newman [:rnewman]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)

> I assume sorting the engine list is a rare operation, such that it's not
> worth creating a single collator and putting it to ongoing use.

It should be infrequent enough, and grabbing it each time saves having to invalidate it on locale changes.


> getLocale() uses a completely different scheme to figure out locale, than
> the prior localeCompare did…

Yup, I went back and forth on that.

For the sake of documenting the reasoning, I opted for this for two reasons:

* This is pretty much the direction we're going for browser.js and the rest of Fennec -- the Java side owns the locale now, and we even have tentative plans to gut nsILocaleService (see Bug 1095298 Comment 29).

* This is the locale used for the list of search engines itself, so it makes sense to use the same collation. And if that differs from the rest of the browser, that's a legitimate bug!


> o_O at that case

inorite. Filed Bug 1100752.


> arr.sort() mutates arr and returns arr…

Good catch, thanks.


(In reply to Drew Willcoxon :adw from comment #12)

> Interesting.  Hardcoding ASCII insensitivity makes sense to me, but
> hardcoding accent insensitivity worries me a little... maybe there are some
> languages whose collations commonly ignore accents but others whose don't?

I think that's a legitimate concern. Given that I can't get a useful repro of this with a local build, and thus we'll be testing this the hard way, I think it makes sense to land with kCollationCaseInsensitiveAscii; if anyone screams about oddities that accent insensitivity will resolve, we can revisit with a one-line patch.


Thanks for the reviews, chaps!

Comment 15

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/7b527ee5c7dc

Comment 16

[Jeff Walden [:Waldo]]

For what it's worth, if we care about desktop particularly, we could always do:

  var cmp;
  if (typeof Intl !== "undefined) {
    var coll = new Intl.Collator(getLocale(),
                                 { sensitivity: "accent", ignorePunctuation: true });
    cmp = coll.compare;
  } else {
    ...current code...
  }

(The current code does effectively this but uses sensitivity determined by the locale.  I'm not sure what that is in general, or for any specific locale.  Also it considers punctuation, which may or may not matter but seems like something that's best ignored.)

(We're going super-far afield here, but I could also imagine the desired sort order being locale-sensitive even beyond this, say, to pick the sort order that would be used in a dictionary.  I know German has such a thing, but it appears ICU picks dictionary order as default, so no worry there.  Not sure about any other languages.  In any case, this would be rather trickier, because you have to specify this as part of the locale, not in the options -- which means you have to destructure the locale string into its components, insert the right annotation, then recombine it.  At least it'd be in JS and safe, but not a good/easy thing to do.  Still, if we must at some point, we must.)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b527ee5c7dc

Comment 18

[Richard Newman [:rnewman]]

flod, could you please verify this when it makes it into Nightly?

Comment 19

[Francesco Lodolo [:flod]]

Firefox 36.0a1 (2014-11-18)

Order is now as expected: eBay, Twitter, Wikipedia.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8521950 [details] [diff] [review]
Use explicit collation in nsSearchService. v1

Approval Request Comment
[Feature/regressing bug #]:
  Not a regression.

[User impact if declined]:
  Search engines appear in an unexpected order.

[Describe test coverage new/current, TBPL]:
  Existing search tests don't fail. Manually verified.

[Risks and why]: 
  Might alter sort orders on all platforms. Shouldn't do so -- designed so that the collation is broadly the same.

  Why: the JS engine relies on some stuff that isn't present on Android, so it can't sort strings in the way that desktop does. The new implementation is cross-platform. This is tracking 35.

[String/UUID change made/needed]:
  None.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d7623931ebc

Comment 22

[Richard Newman [:rnewman]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)

> On mobile, however, we don't have Intl or ICU yet (bug 864843, bug 866301). 

For the record, b2gdroid stole one of those bugs, so the general Fennec case is now Bug 1215247.