Closed Bug 1129859 Opened 5 years ago Closed 5 years ago

Wrong dictionary chosen when lang attribute is en-US on some Linux distros

Categories

(Core :: Spelling checker, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jhorak, Assigned: jhorak)

Details

Attachments

(1 file, 1 obsolete file)

Some Linux distros using '_' as separator in dictionary name between language and region. When page specifies region in lang attribute like lang="en-US" then it fails to find correct dictionary because system dictionary name is actually named "en_US".
Attached patch Fix for Linux dicts v1 (obsolete) — Splinter Review
Please have a look.
Assignee: nobody → jhorak
Attachment #8559725 - Flags: review?(roc)
Comment on attachment 8559725 [details] [diff] [review]
Fix for Linux dicts v1

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

::: editor/composer/nsEditorSpellCheck.cpp
@@ +794,5 @@
> +    if (NS_FAILED(rv)) {
> +      int32_t dashIdx = dictName.FindChar('-');
> +      if (dashIdx != -1) {
> +        nsString dictNameUnderscored(dictName);
> +        dictNameUnderscored.SetCharAt('_', dashIdx);

This doesn't seem like the right way to fix this bug:

-- This is platform-specific so shouldn't really show up in cross-platform code.
-- SetCurrentDictionary (and maybe other code) looks for a '-' and this will break that.
-- This gets stored in page-specific prefs that could potentially be shared between platforms.

Seems to me there should be a lower-level place where we can replace - with _, when actually loading a dictionary.

But I don't really understand this bug. Can you explain in more detail?
Attachment #8559725 - Flags: review?(roc) → review-
Problem is that you are using '-' separator, while Fedora or RHEL is using '_' separator in dictionary names. They (Fedora) use it to match witch locale and changing it would break others apps like LibreOffice, so we can't change dictionary names in Fedora. So in Fedora's Firefox fallback code is called when page has something like lang='en-US' in source and so first of dictionary in dictionary list is picked, which is usually wrong because user have usually a lot of English dictionaries for various regions installed by default.

Changing it in lower level (ie. replace '_' with '-' in dictionary name) breaks some test, like test with base_utf8 dictionary.
With this change: https://hg.mozilla.org/try/rev/8a0da54a68e7
and modified tests (ie. replacing all '_' character in test dictionary' filenames  by '-') looks fine. 
Corresponding try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0da54a68e7
Issue in Linux is solve by this. Could you please give me a feedback to this change? Thank you.
Flags: needinfo?(roc)
> Problem is that you are using '-' separator, while Fedora or RHEL is using '_' separator in dictionary
> names.

FWIW the BCP47 spec says '-': http://www.ietf.org/rfc/bcp/bcp47.txt

(In reply to Jan Horak from comment #5)
> With this change: https://hg.mozilla.org/try/rev/8a0da54a68e7
> and modified tests (ie. replacing all '_' character in test dictionary'
> filenames  by '-') looks fine. 
> Corresponding try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0da54a68e7
> Issue in Linux is solve by this. Could you please give me a feedback to this
> change? Thank you.

This looks good to me!
Flags: needinfo?(roc)
Attaching fix for this bug, replace all '_' by '-' in dictionary names. Due to this change all '_' characters in dictionary filenames used for testing has been replaced by '-'. I also removed some code in nsEditorSpellcheck.cpp which is no longer necessary.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ca377de5ab
Attachment #8559725 - Attachment is obsolete: true
Attachment #8563984 - Flags: review?(roc)
Thanks for the review.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/226ad2adf361
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.