Closed Bug 1347272 Opened 8 years ago Closed 8 years ago

Move ChromeRegistry::IsLocaleRTL to LocaleService::IsLocaleRTL.

Categories

(Core :: Internationalization, enhancement)

Product:
Core
Component:
Internationalization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1347272 - Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Centralizing IsLocaleRTL in LocaleService will make it easier to remove locale related bits from ChromeRegistry and prepare us for pseudolocales.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Except of bunch of trivial changes, the non-trivial change here is that we're dropping support for different UI direction depending on the chrome package. I think it's fair to assume that there are no users of that feature except of this one test that I had to fix. With the change, the pref intl.uidirection is the only way to alter the direction except of changing the UI locale to an RTL one.
I actually think that lots of users use this feature in the following way: A RTL-localized browser and addons that don't support that locale, and fall back to en-US. Those addons are currently displayed in LTR mode.
> A RTL-localized browser and addons that don't support that locale, and fall back to en-US. Interesting. Thanks! Is that going to die with 57? Should we also add "packages" to LocaleService - much like our "browser" vs "devtools" which could have different requested, available, supported locales and in result RTL status?
Three thoughts: 1) I do think we'll want to add packages/contexts or whatever we name it which will be a virtual space for different language negotiation setups. 2) I can also rework this patch to not touch the `-moz-locale-dir`. Basically I'd not remove IsLocaleRTL from ChromeRegistry, add it to LocaleService and use it everywhere where 'global' package has been used before. This will leave one use, in XULDocument, getting IsLocaleRTL directly from ChromeRegistry so that `-moz-locale-dir` works. 3) But ultimately. I think I believe we should get rid of `-moz-locale-dir`. The `dir` should be just set on the Window object the way we do this in HTML <window dir="rtl"> <window dir="ltr">. Then CSS will pick it the same way as it does in HTML. I could even see us adding a way to recognize the `lang` attribute on the Window Element, and adapting the JS context to use it, and `dir` to be taken from `lang` if not set directly. :pike, :jfkthame - thoughts?
Flags: needinfo?(l10n)
Flags: needinfo?(jfkthame)
Forgot to add: > 1) I do think we'll want to add packages/contexts or whatever we name it which will be a virtual space for different language negotiation setups. But I don't think we should do this now yet, and instead I'd prefer to do (2) or (3).
AFAIK, xul addons are going to stay around for system addons for a bit longer, so we might need to have a way to do this for the chrome:// protocol for a while. I have no idea how this plays out with web extensions, tbh. My hope is that we'll have a lang attr on the documentElement? I guess 2) would work for now, if I re-read your comment to say "I'm going to convert all the call sites with "global" to use the new API, and XULDocument is gonna ask chrome registry for package directly". Not sure what chrome registry code would do precisely in that scenario?
Flags: needinfo?(l10n)
Comment on attachment 8847246 [details] Bug 1347272 - Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL. Ok, I'll update the patch.
Attachment #8847246 - Flags: review?(jfkthame)
Patch updated. Sad to lose the ChromeRegistry cuts for now, but at least if we establish IsLocaleRTL in LocaleService we should be good and I have my hope we'll get rid of it this year.
I'm also thinking that maybe the method name should be "IsAppRTL" since we don't accept a param and we also may return an overridden value in case of a pref.
Comment on attachment 8847246 [details] Bug 1347272 - Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL. https://reviewboard.mozilla.org/r/120250/#review126462 ::: intl/locale/LocaleService.cpp:383 (Diff revision 8) > } > return true; > } > > +bool > +LocaleService::IsLocaleRTL() Maybe call this IsAppLocaleRTL(), to make it clearer what it's returning? ::: intl/locale/LocaleService.cpp:399 (Diff revision 8) > +#else > + // first check the intl.uidirection.<locale> preference, and if that is not > + // set, check the same preference but with just the first two characters of > + // the locale. If that isn't set, default to left-to-right. > + nsAutoCString prefString = NS_LITERAL_CSTRING("intl.uidirection.") + locale; > + nsXPIDLCString dir = Preferences::GetCharPref(prefString.get()); This won't build, `GetCharPref()` is not a static method. And a few lines later, `prefBranch` is undefined. I think you can just do something like nsAutoCString dir; Preferences::GetCString(prefString.get(), &dir); if (dir.IsEmpty()) { int32_t hyphen = prefString.FindChar('-'); if (hyphen >= 1) { prefString.Truncate(hyphen); Preferences::GetCString(prefString.get(), &dir); } } return dir.EqualsLiteral("rtl"); ::: intl/locale/mozILocaleService.idl:179 (Diff revision 8) > [retval, array, size_is(aCount)] out string aLocales); > + > + /** > + * Returns whether the current app locale is RTL. > + */ > + boolean isLocaleRTL(); isAppLocaleRTL? Any reason not to make this a read-only attribute?
Feedback applied. I like the isAppLocaleRTL as an attribute :) re-requesting review.
Flags: needinfo?(jfkthame)
Comment on attachment 8847246 [details] Bug 1347272 - Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL. https://reviewboard.mozilla.org/r/120250/#review126716
Attachment #8847246 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c7174ac72d14 Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL. r=jfkthame
I don't understand why the try on Android is green. Sorry for that! Fixing.
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3a1b3649b2a5 Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL. r=jfkthame
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #26) > I don't understand why the try on Android is green. Sorry for that! > > Fixing. I guess try doesn't do a build without ENABLE_INTL_API, so the legacy codepath doesn't get compiled.
Well, it does hazard builds and android builds. Both without INTL. I don't get it. Anyhoo, landed with the fix. Sorry for the mess.
https://hg.mozilla.org/mozilla-central/rev/3a1b3649b2a5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See Also: → 1634631
See Also: 1634631 →
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: