1312049 - Switch nsChromeRegistry::GetDirectionForLocale to use uloc_isRightToLeft

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

ICU provides with directionality information:

http://icu-project.org/apiref/icu4c/uloc_8h.html#aaa54a35dd7e24e2af83243b96f1ecdc7

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

The mozIntl API to match that will be bug 1312053.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 3

[Jonathan Kew [:jfkthame]]

Note that the patch here depends on the "part 0" patch from bug 1301655, so that will need to land first.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 5

[Zibi Braniecki [:zbraniecki][: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.

Comment 6

[Jonathan Kew [:jfkthame]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3198f930b755
https://hg.mozilla.org/mozilla-central/rev/686282bd2f19

Comment 8

[Pulsebot]

Backout by [e-mail]:
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

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[:Gijs (he/him)]

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?

Comment 11

[Mike de Boer [:mikedeboer]]

(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.

Comment 12

[Mike de Boer [:mikedeboer]]

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?

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 14

[Jonathan Kew [:jfkthame]]

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?

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

> 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/

Comment 16

[Jonathan Kew [:jfkthame]]

Sure, if you're able to track down anything about the test failures that'd be great.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 :(

Comment 18

[Neil Deakin]

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.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 20

[Jonathan Kew [: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.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

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!

Comment 24

[Jonathan Kew [:jfkthame]]

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.

Comment 25

[Jonathan Kew [:jfkthame]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e75593853170