Closed Bug 1400006 Opened 2 years ago Closed 2 years ago

Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped

Categories

(Core :: Internationalization, defect, P1)

Product:
Core
Component:
Internationalization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox55 --- wontfix
firefox56 blocking fixed
firefox57 + fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped.
59 bytes, text/x-review-board-request
Pike
: review+
lizzard
: approval-mozilla-release+
Details 
Bug 1399203 and bug 1389709 found out a limitation of our current language negotiation algorithm that affects our users.

Basically, if the user specifies the locale with region, and we do not have a direct for that region, we're going to pick all locales for the same language and other regions in no order.

The example of where it returns suboptimal results:

1) Requested locale "en-CA"
2) Available locales ["en-ZA", "en-GB", "en-US"]
3) Negotiated locales ["en-ZA", "en-GB", "en-US"]

That's a correct behavior in the light of our negotiation strategy, because en-CA is considered equally distant from all three available locales.

Same for de, fr, it, es, etc.:

1) Requested locale "de-LU"
2) Available locales ["de-AT", "de-CH", "de-DE"]
3) Negotiated locales ["de-AT", "de-CH", "de-DE"]

This would not happen, if the user requested a generic "de", "en" etc.:

1) Requested locale "en"
2) Available locales ["en-ZA", "en-GB", "en-US"]
3) Negotiated locales ["en-US", "en-ZA", "en-GB"]

because after not finding a direct match, we would use likelySubtags to extend "en" to "en-Latn-US" and then find the priority match in "en-US".

We can extend this logic to "en-US" or "de-LU" by adding a step which strips the region tag and then applies likelySubtag on the result.

This is not a perfect solution, since logically we know that for example for "es-CL" a closer match would be "es-MX" but the likelySubtags will fallback on "es-ES", but I think it's an acceptable choice and closer to what users would expect.

This means that in absence of direct match the following fallbacks would happen:

"de-LU" -> "de-DE"
"es-CL" -> "es-ES"
"en-CA" -> "en-US"

Notice that this will not affect languages that use multiple scripts, so ar, sr and zh are not affected.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Depends on: 1399203, 1389709
Blocks: 1389709, 1399203
No longer depends on: 1389709, 1399203
Priority: -- → P3
Why mark this P3?  We need to fix the issue blocked by this bug for the 56 release, which is next week.
Flags: needinfo?(djripper)
Sorry, I set n-i to the wrong Makoto Kato
Flags: needinfo?(djripper) → needinfo?(m_kato)
Comment on attachment 8908384 [details]
Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped.

https://reviewboard.mozilla.org/r/180000/#review186066

Only got comments on comments and commit message, r=me with those fixed.

The intent of the bug is OK, let's get this unblocked.

::: commit-message-9517e:1
(Diff revision 1)
> +Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped. r?jfkthame

Can you add a bit more to the commit message?

The initial comment of the bug would be good, maybe stripped just a bit.

As we're talking uplift, I'd like to make this understandable as a commit.

::: intl/locale/LocaleService.cpp:353
(Diff revision 1)
>  
>  /**
>   * This is the raw algorithm for language negotiation based roughly
>   * on RFC4647 language filtering, with changes from LDML language matching.
>   *
>   * The exact algorithm is custom, and consist of 5 level strategy:

6 by now.

Also, do we need to modify the top-level comment?
Attachment #8908384 - Flags: review+
Attachment #8909430 - Flags: review?(jfkthame) → review?(l10n)
Jet asked me, for expediency, to push a version of this patch after addressing the review in comment 4.
Comment on attachment 8909430 [details]
Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped.

https://reviewboard.mozilla.org/r/180922/#review186130

Brad, thanks for trying to help. Not exactly easy to review the increment, given that this ended up being a separate commit ID.

Please reformat the commit message to have the MozReview-Commit-ID as last line, with an empty line as separator.

::: commit-message-ffe6c:3
(Diff revision 2)
> +Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped. r?jfkthame
> +
> +MozReview-Commit-ID: BR1WrgXSf6a

This should be trailing lines, that is,

... full commit message ...
<emtpy line>
MozReview-Commit-ID
Attachment #8909430 - Flags: review?(l10n) → review-
Comment on attachment 8909430 [details]
Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped.

https://reviewboard.mozilla.org/r/180922/#review186138
Attachment #8909430 - Flags: review+
Attachment #8908384 - Flags: review?(jfkthame)
Attachment #8909430 - Flags: review?(jfkthame)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3265d380358
Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped. r=Pike
https://hg.mozilla.org/mozilla-central/rev/e3265d380358
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Can you request uplift to m-r ?  Thanks.  

I am going to delay the mobile release candidate build (which in theory should be today) so that we can make sure to get in all the necessary fixes that block the release.
Flags: needinfo?(gandalf)
Marking this a blocker since it seems to be necessary for bug 1389709 , which is a 56 release blocker
tracking-firefox56: + → blocking
Zibi is traveling so we may need someone else to do the uplift request. thanks
Flags: needinfo?(bwerth)
Flags: needinfo?(bugs)
Comment on attachment 8909430 [details]
Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped.

Approval Request Comment
[Feature/Bug causing the regression]: Locale partial matching provides suboptimal match if direct match not available.
[User impact if declined]: Blocks Bug 1389709, which causes search engine listings to be incorrect for some locales in Fennec.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Bug 1389709
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds one step to the middle of an already multi-step algorith, giving the possibility of a more correct result before the final fallback.
[String changes made/needed]: None
Flags: needinfo?(bwerth)
Flags: needinfo?(bugs)
Attachment #8909430 - Flags: approval-mozilla-release?
Attachment #8908384 - Attachment is obsolete: true
clear ni because this is already fixed and requesting uplift
Flags: needinfo?(m_kato)
Priority: P3 → P1
Comment on attachment 8909430 [details]
Bug 1400006 - Extend language negotiation in LocaleService to support looking for the best likelySubtag for the locale with region stripped.

Fix for underlying locale problems affecting search and maybe other issues.
Attachment #8909430 - Flags: approval-mozilla-release? → approval-mozilla-release+
This change should be in an upcoming 56 RC2 build.
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.