Closed Bug 1097542 Opened 10 years ago Closed 10 years ago

Unclear order logic for searchplugins in Firefox for Android

Categories

(Firefox :: Search, defect)

All
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: flod, Assigned: rnewman)

Details

Attachments

(1 file)

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.
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".
Summary: Unclear order logic for searchplugin in Firefox for Android → Unclear order logic for searchplugins in Firefox for Android
tracking-fennec: --- → ?
(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?
Flags: needinfo?(jwalden+bmo)
If the JS engine is relying on a libc function for locale awareness, it might well be wrong on Android (cf Bug 864753).
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.
Flags: needinfo?(jwalden+bmo)
Thanks for the excellent writeup!
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(bzexport automatically assigned this.)
Assignee: rnewman → nobody
Status: ASSIGNED → NEW
Richard - Want to run with that patch?
tracking-fennec: ? → 35+
Flags: needinfo?(rnewman)
I can take it, but I can't guarantee 35. Will give it a shot.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Component: General → Search
Flags: needinfo?(rnewman)
OS: All → Android
Product: Firefox for Android → Firefox
Version: unspecified → Trunk
With this change applied, desktop search xpcshell tests pass, and search engines are ordered alphabetically.

Testing on Android now.
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.
Attachment #8521950 - Flags: review?(jwalden+bmo)
Attachment #8521950 - Flags: review?(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
Attachment #8521950 - Flags: review?(adw) → review+
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).
Attachment #8521950 - Flags: review?(jwalden+bmo) → review+
(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!
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.)
https://hg.mozilla.org/mozilla-central/rev/7b527ee5c7dc
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
flod, could you please verify this when it makes it into Nightly?
Flags: needinfo?(francesco.lodolo)
Firefox 36.0a1 (2014-11-18)

Order is now as expected: eBay, Twitter, Wikipedia.
Status: RESOLVED → VERIFIED
Flags: needinfo?(francesco.lodolo)
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.
Attachment #8521950 - Flags: approval-mozilla-aurora?
Attachment #8521950 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
tracking-fennec: 35+ → ---
You need to log in before you can comment on or make changes to this bug.