Closed Bug 1346819 Opened 7 years ago Closed 7 years ago

Port SanitizeAsBCP47 to LocaleService

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 1346819 - Port SanitizeAsBCP47 to LocaleService.
59 bytes, text/x-review-board-request
jfkthame
: review+
baku
: review+
Details 
As we move app locale retrieval to LocaleService, we have to also move the SanitizeAsBCP47 method.

This is due to the double nature of our API - for internal use, we want the language tag code ("ja-JP-mac") but for passing to ICU/Intl APIs we want the bcp47 locale id ("ja-JP-x-lvariant-mac").

:jfkthame,

Do you prefer this as boolean param on GetAppLocale and GetAppLocales or as a separate function that we'll ask people to run before they pass result to Intl/ICU?

I feel like for convenience reasons separate function would be easier, but I'm worried about devs forgetting about it and not testing the result on "ja-JP-mac".
NI :jfkthame?

I'm leaning toward making it a non-optional param on GetAppLocale(s) to make sure that callers make a decision depending  on their use case.
Flags: needinfo?(jfkthame)
Or maybe split into parallel APIs with explicit names? Forcing callers to choose between GetAppLocaleAsBCP47() and GetAppLocaleAsLangTag().

If we have an extra param on GetAppLocale(), what would its type be? If it's a bool or an enum value, it seems to easy for people to just cargo-cult from an existing callsite without thinking about what the `true` or `0` or whatever actually means, whereas making it part of the API name means it's harder to overlook.
Flags: needinfo?(jfkthame)
sgtm! Patch incoming :)

We'll want to backport it to aurora.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> We'll want to backport it to aurora.

Why, out of curiosity? Are we backporting a bunch of other stuff that depends on this?
We migrated Services.locale and a few other callsites to LocaleService in 54 and some of those calls are used in ICU/Intl API, so without this backport those cases will break in ja-JP-mac (and break badly because it makes the Intl API throw).
Ah, makes sense -- thanks.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Ok, something like this?
Comment on attachment 8846724 [details]
Bug 1346819 - Port SanitizeAsBCP47 to LocaleService.

https://reviewboard.mozilla.org/r/119738/#review121608

::: intl/locale/LocaleService.cpp:134
(Diff revision 1)
> +LocaleService::GetAppLocalesAsBCP47(nsTArray<nsCString>& aRetVal)
> +{
> +  if (mAppLocales.IsEmpty()) {
> +    ReadAppLocales(mAppLocales);
> +  }
> +  nsTArray<nsCString> locales;

Don't create a local array, just append them directly to `aRetVal` in the loop.

::: intl/locale/LocaleService.cpp:136
(Diff revision 1)
> +  if (mAppLocales.IsEmpty()) {
> +    ReadAppLocales(mAppLocales);
> +  }
> +  nsTArray<nsCString> locales;
> +  for (uint32_t i = 0; i < mAppLocales.Length(); i++) {
> +    nsCString locale = mAppLocales[i];

Use nsAutoCString here, to avoid heap allocation.

::: intl/locale/LocaleService.cpp:402
(Diff revision 1)
>  NS_IMETHODIMP
> -LocaleService::GetAppLocale(nsACString& aRetVal)
> +LocaleService::GetAppLocalesAsBCP47(uint32_t* aCount, char*** aOutArray)
> +{
> +  if (mAppLocales.IsEmpty()) {
> +    ReadAppLocales(mAppLocales);
> +  }
> +
> +  *aCount = mAppLocales.Length();
> +
> +  nsTArray<nsCString> locales;
> +  for (uint32_t i = 0; i < mAppLocales.Length(); i++) {
> +    nsCString locale = mAppLocales[i];
> +    SanitizeForBCP47(locale);
> +    locales.AppendElement(locale);
> +  }
> +  *aOutArray = CreateOutArray(locales);
> +
> +  return NS_OK;
> +}

This method repeats enough of the C++-callable version above that I think it's worth sharing the implementation. Something like
```
NS_IMETHODIMP
LocaleService::GetAppLocalesAsBCP47(...)
{
  nsTArray<nsCString> locales;
  GetAppLocalesAsBCP47(locales);

  *aCount = locales.Length();
  *aOutArray = CreateOutArray(locales);

  return NS_OK;
}
```
should work, I think.

(What's a likely maximum number of app locales that might be present? If it's not too big, might be worth using an `AutoTArray<nsCString,N>` for some reasonable N.)

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:229
(Diff revision 1)
>    }
>    return (new Boolean(aValue)).valueOf();
>  }
>  
>  /**
>   * Get the current browser.

While you're here, please fix this comment (add "locale").
Thanks! Updated to your feedback.
Comment on attachment 8846724 [details]
Bug 1346819 - Port SanitizeAsBCP47 to LocaleService.

https://reviewboard.mozilla.org/r/119738/#review121730

LGTM, assuming try is happy with it.

::: intl/locale/LocaleService.cpp:135
(Diff revisions 1 - 2)
>    if (mAppLocales.IsEmpty()) {
>      ReadAppLocales(mAppLocales);
>    }
> -  nsTArray<nsCString> locales;
>    for (uint32_t i = 0; i < mAppLocales.Length(); i++) {
> -    nsCString locale = mAppLocales[i];
> +    nsAutoCString locale(mAppLocales[i]);

Gecko style guide says to initialize with

    nsAutoCString locale = mAppLocales[i];

in a case like this.

::: commit-message-7b19a:1
(Diff revision 2)
> +Bug 1346819 - Port SanitnizeAsBCP47 to LocaleService. r?jfkthame

Typo, "SanitizeAsBCP47".
Attachment #8846724 - Flags: review?(jfkthame) → review+
Comment on attachment 8846724 [details]
Bug 1346819 - Port SanitizeAsBCP47 to LocaleService.

https://reviewboard.mozilla.org/r/119738/#review121730

> Gecko style guide says to initialize with
> 
>     nsAutoCString locale = mAppLocales[i];
> 
> in a case like this.

That errors with:

 0:02.29 In file included from /home/zbraniecki/projects/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/intl/locale/Unified_cpp_intl_locale0.cpp:11:0:
 0:02.29 /home/zbraniecki/projects/mozilla/mozilla-unified/intl/locale/LocaleService.cpp: In member function ‘void mozilla::intl::LocaleService::GetAppLocalesAsBCP47(nsTArray<nsCString>&)’:
 0:02.29 /home/zbraniecki/projects/mozilla/mozilla-unified/intl/locale/LocaleService.cpp:135:41: error: conversion from ‘nsTArray_Impl<nsCString, nsTArrayInfallibleAllocator>::elem_type {aka nsCString}’ to non-scalar type ‘nsAutoCString’ requested
 0:02.29      nsAutoCString locale = mAppLocales[i];
 0:02.29                                          ^
 0:02.29 
 0:02.29 In the directory  /home/zbraniecki/projects/mozilla/mozilla-unified/obj-x86_64-pc-linux-gnu/intl/locale
Can I land without changing the nsAutoCString initialization style?
Flags: needinfo?(jfkthame)
Ah, ok... sorry, I too easily forget exactly which operations are/aren't supported. Yes, leave it as-is.
Flags: needinfo?(jfkthame)
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 14 changes to 14 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: WebIDL file dom/webidl/Window.webidl altered in changeset 7311fca7b3bf without DOM peer review
remote: 
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote
Comment on attachment 8846724 [details]
Bug 1346819 - Port SanitizeAsBCP47 to LocaleService.

https://reviewboard.mozilla.org/r/119738/#review121944

r+ for the webidl changes
Attachment #8846724 - Flags: review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b45d664c0a7b
Port SanitizeAsBCP47 to LocaleService. r=baku,jfkthame
https://hg.mozilla.org/mozilla-central/rev/b45d664c0a7b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: