Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: jfkthame, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Comment 1

2 years ago
The mozIntl API to match that will be bug 1312053.
See Also: → bug 1312053
(Reporter)

Updated

2 years ago
Mentor: jfkthame, gandalf
Keywords: good-first-bug
(Assignee)

Comment 2

2 years ago
Created attachment 8803477 [details] [diff] [review]
Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft

Here's a simplistic implementation that should be adequate, I think. It's slightly inefficient in that we end up copying the locale string both here and within SanitizeForBCP47 (because the interfaces use nsACString, which IIRC is not guaranteed to be null-terminated), but I doubt that's really an issue here.
Attachment #8803477 - Flags: review?(gandalf)
(Reporter)

Updated

2 years ago
Attachment #8803477 - Flags: review?(gandalf) → review+
(Assignee)

Comment 3

2 years ago
Note that the patch here depends on the "part 0" patch from bug 1301655, so that will need to land first.
Depends on: 1301655
(Assignee)

Comment 4

2 years ago
Created attachment 8803804 [details] [diff] [review]
Remove/disable tests that depend on setting prefs to override the UI locale directionality

We have a few existing chrome mochitests that depend on using intl.uidirection.* prefs to set directionality, so removing the prefs will prevent these tests working. We may eventually want to bring back similar tests using on-the-fly locale switching, but for now I think we'll just have to disable them.
Attachment #8803804 - Flags: review?(gandalf)
(Reporter)

Comment 5

2 years ago
Comment on attachment 8803804 [details] [diff] [review]
Remove/disable tests that depend on setting prefs to override the UI locale directionality

Yeah, ICU is well tested and we're switching to it.
Attachment #8803804 - Flags: review?(gandalf) → review+
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3198f930b755ff943d1ab0e1f5c422b0801db625
Bug 1312049 - Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft. r=gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/686282bd2f19b5f0ab249cde34caea04fc5c17a1
Bug 1312049 - Remove/disable tests that depend on setting prefs to override the UI locale directionality. r=gandalf

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3198f930b755
https://hg.mozilla.org/mozilla-central/rev/686282bd2f19
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → jfkthame
status-firefox52: fixed → ---
Target Milestone: mozilla52 → ---

Comment 8

2 years ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/950739a54a07
Backed out changeset 686282bd2f19 
https://hg.mozilla.org/mozilla-central/rev/969c3295d3aa
Backed out changeset 3198f930b755 for frequent timeouts in M(oth) toolkit/content/tests/chrome/test_tooltip.xul on Windows opt and pgo. r=backout a=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out for frequent timeouts in M(oth) toolkit/content/tests/chrome/test_tooltip.xul on Windows opt and pgo:

https://hg.mozilla.org/mozilla-central/rev/969c3295d3aa77931cca26eddb047d9d74bd9858
https://hg.mozilla.org/mozilla-central/rev/950739a54a07f38d8659da430429892a65967983

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=686282bd2f19b5f0ab249cde34caea04fc5c17a1
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38421958&repo=mozilla-inbound

02:56:55     INFO - TEST-START | toolkit/content/tests/chrome/test_tooltip.xul
03:01:03     INFO -  JavaScript error: resource://gre/modules/FormHistory.jsm, line 375: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
03:01:03     INFO -  JavaScript error: resource://gre/modules/PlacesUtils.jsm, line 1958: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]
03:02:09     INFO - TEST-INFO | started process screenshot
03:02:09     INFO - TEST-INFO | screenshot: exit 0
03:02:09     INFO - must wait for load
03:02:09     INFO - must wait for focus
03:02:09     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/chrome/test_tooltip.xul | Test timed out. 
03:02:09     INFO - reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:114:7
03:02:09     INFO - TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:135:7
03:02:09     INFO - setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
Flags: needinfo?(jfkthame)

Comment 10

2 years ago
These patches will break the "Force RTL" add-on as written, I suspect. Can we ensure there's some kind of override when this relands?
(In reply to :Gijs Kruitbosch from comment #10)
> These patches will break the "Force RTL" add-on as written, I suspect. Can
> we ensure there's some kind of override when this relands?

It will and I agree - preferably we'd have some method to override the locale as detected. It'd be fine to have a single, hidden, boolean pref 'intl.uidirection' instead of the entire pref branch.
I'm also not sure whether I agree with simply disabling tests. If this is going to be the default, why not remove them? If they're not testing something useless, why not fix it by providing a method to override things?
(Reporter)

Comment 13

2 years ago
Jonathan, can we land it again? We'd like to enable mozIntl.getLocaleInfo which will use ICU for RTL info and it would be good to stay in sync with this.
(Assignee)

Comment 14

2 years ago
I'd like to get this re-landed, but haven't had time to figure out why it led to the (intermittent) timeouts in comment 9. I don't think the patch itself was necessarily faulty, more likely it affected timing in some way that upset a fragile test, but we can't have that level of orange going on. :(

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> I'm also not sure whether I agree with simply disabling tests. If this is
> going to be the default, why not remove them? If they're not testing
> something useless, why not fix it by providing a method to override things?

AIUI, the intention is that in future there will be a way to test such things by overriding the locale code with a custom testing locale that specifies RTL directionality; so when that mechanism is in place, we'd re-enable these tests.

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > These patches will break the "Force RTL" add-on as written, I suspect. Can
> > we ensure there's some kind of override when this relands?
> 
> It will and I agree - preferably we'd have some method to override the
> locale as detected. It'd be fine to have a single, hidden, boolean pref
> 'intl.uidirection' instead of the entire pref branch.

Given that we don't yet have the ability to override the locale for tests, afaik, maybe it's worth doing this for now. :gandalf, wdyt?
Flags: needinfo?(jfkthame)
(Reporter)

Comment 15

2 years ago
> Given that we don't yet have the ability to override the locale for tests, afaik, maybe it's worth doing this for now. :gandalf, wdyt?

Sure, once we switch ourselves to l20n.js, we'll want to introduce full pseudolocales like we did in Firefox OS [0], Windows [1] and like Android does [2].

> I'd like to get this re-landed, but haven't had time to figure out why it led to the (intermittent) timeouts in comment 9. I don't think the patch itself was necessarily faulty, more likely it affected timing in some way that upset a fragile test, but we can't have that level of orange going on. :(

Would you like me to try to help with this? I'd like this patch to land soon so that we can land bug 1321004 and be consistent between the two.


[0] https://bug900182.bmoattachments.org/attachment.cgi?id=8418596
    https://bugzilla.mozilla.org/show_bug.cgi?id=900182
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd319106(v=vs.85).aspx
[2] https://androidbycode.wordpress.com/2015/04/19/pseudo-localization-testing-in-android/
(Reporter)

Updated

2 years ago
Flags: needinfo?(jfkthame)
(Assignee)

Comment 16

2 years ago
Sure, if you're able to track down anything about the test failures that'd be great.
Flags: needinfo?(jfkthame)
(Reporter)

Comment 17

2 years ago
Ehh, so, it's windows only and I don't have windows machine.

:enn, you're the last person to touch this test. Do you have any idea why this patch might cause the perma-orange in `toolkit/content/tests/chrome/test_tooltip.xul` ?

If not, is there any chance you can help us find someone who might be able to help us find the cause?

I tried to look at the test and there's nothing related to the scope of this patch that I can see as a correlation so I'm totally lost :(
Flags: needinfo?(enndeakin)

Comment 18

2 years ago
The log in comment 9 suggests the window isn't being refocused, so it could be some previous test that is the issue. Does it only fail when run with the rest of the tests?

But I share the concern's raised by Gijs and Mike above. Won't this patch make it so RTL testing by developers isn't possible? Ideally, this change should check the preferences and then call uloc_isRightToLeft only if the preference isn't present.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 19

2 years ago
I believe that until we have pseudo-locales, I agree that we should add a preference that enforces direction.

Jonathan, do you have an idea what to do next with this test? Do you have a windows machine you can reproduce it on?

I'm concerned that this blocks bug 1312053, and we need it for date/time picker RTL work.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 20

2 years ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #19)
> I believe that until we have pseudo-locales, I agree that we should add a
> preference that enforces direction.

Yes, I think we could have a simple "intl.uidirection" as proposed in comment 11.

> Jonathan, do you have an idea what to do next with this test? Do you have a
> windows machine you can reproduce it on?

I do have access to a windows machine (though running lots of tests there is a pain).... after the holiday break, I'll try to spend some time on this and see if I can reproduce. Leaving n-i? flag as a reminder.
(Reporter)

Comment 21

2 years ago
Any update on this? :waldo is blocking me in bug 1312053 on this (for a good reason!) :)
(Assignee)

Comment 22

2 years ago
Created attachment 8832900 [details] [diff] [review]
Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft instead of a list of known-rtl locales, and replace the now-obsolete intl.uidirection.* prefs with a single override intl.uidirection for testing purposes

Updated the patch to introduce a single intl.uidirection pref that can be used to override the locale's directionality for testing purposes, and updated tests to use this instead of disabling them. (So this replaces both the earlier patches.) Re-requesting r? for the new pref, which wasn't part of the earlier reviewed patch.
Attachment #8832900 - Flags: review?(gandalf)
(Assignee)

Updated

2 years ago
Attachment #8803477 - Attachment is obsolete: true
Flags: needinfo?(jfkthame)
(Assignee)

Updated

2 years ago
Attachment #8803804 - Attachment is obsolete: true
(Reporter)

Comment 23

2 years ago
Comment on attachment 8832900 [details] [diff] [review]
Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft instead of a list of known-rtl locales, and replace the now-obsolete intl.uidirection.* prefs with a single override intl.uidirection for testing purposes

lgtm!
Attachment #8832900 - Flags: review?(gandalf) → review+
(Assignee)

Comment 24

2 years ago
Oops....in test_ext_i18n_css.html, we need to keep the old code that tweaks intl.uidirection.en, in addition to the new version, so that it'll still pass on Android while we don't yet have the ICU-based locale support there. I'll restore that to the test, and double-check that try is happy with it before landing.
(Assignee)

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e75593853170fa531322517064ac7c29eba9ca24
Bug 1312049 - Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft instead of a list of known-rtl locales, and replace the now-obsolete intl.uidirection.* prefs with a single override intl.uidirection for testing purposes. r=gandalf

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e75593853170
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.