Closed Bug 1312049 Opened 8 years ago Closed 7 years ago

Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: zbraniecki, Assigned: jfkthame, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

The mozIntl API to match that will be bug 1312053.
See Also: → 1312053
Mentor: jfkthame, gandalf
Keywords: good-first-bug
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)
Attachment #8803477 - Flags: review?(gandalf) → review+
Note that the patch here depends on the "part 0" patch from bug 1301655, so that will need to land first.
Depends on: 1301655
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/3198f930b755
https://hg.mozilla.org/mozilla-central/rev/686282bd2f19
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → jfkthame
Target Milestone: mozilla52 → ---
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)
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?
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.
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)
> 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/
Flags: needinfo?(jfkthame)
Sure, if you're able to track down anything about the test failures that'd be great.
Flags: needinfo?(jfkthame)
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)
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)
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)
(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.
Any update on this? :waldo is blocking me in bug 1312053 on this (for a good reason!) :)
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)
Attachment #8803477 - Attachment is obsolete: true
Flags: needinfo?(jfkthame)
Attachment #8803804 - Attachment is obsolete: true
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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/e75593853170
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.