Move ChromeRegistry::IsLocaleRTL to LocaleService::IsLocaleRTL.

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Centralizing IsLocaleRTL in LocaleService will make it easier to remove locale related bits from ChromeRegistry and prepare us for pseudolocales.
(Assignee)

Updated

2 years ago
Blocks: 1346877
(Assignee)

Updated

2 years ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 8

2 years ago
> 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?
(Assignee)

Comment 9

2 years ago
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)
(Assignee)

Comment 10

2 years ago
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)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8847246 [details]
Bug 1347272 - Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL.

Ok, I'll update the patch.
Attachment #8847246 - Flags: review?(jfkthame)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1350102
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
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?
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
Feedback applied. I  like the isAppLocaleRTL as an attribute :)

re-requesting review.
Flags: needinfo?(jfkthame)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

2 years ago
mozreview-review
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+

Comment 24

2 years ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7174ac72d14
Move ChromeRegistry::IsLocaleRTL to LocaleService::IsAppLocaleRTL. r=jfkthame
(Assignee)

Comment 26

2 years ago
I don't understand why the try on Android is green. Sorry for that!

Fixing.
Flags: needinfo?(gandalf)
Comment hidden (mozreview-request)

Comment 28

2 years ago
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.
(Assignee)

Comment 30

2 years ago
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.

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a1b3649b2a5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.