Closed Bug 1426063 Opened 6 years ago Closed 6 years ago

Add PLATFORM builtin to Fluent in Gecko

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

For the migration of preferences chrome (bug 1424682) we'll need the PLATFORM built-in.
Blocks: 1424682
Priority: -- → P3
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8938168 [details]
Bug 1426063 - Add PLATFORM built-in to Fluent in Gecko.

https://reviewboard.mozilla.org/r/208882/#review214790

We'll need tests and docs for this. I'm not sure how/where we do either, though. firefox-source-docs for gecko-specific GLOBALS? Also, not sure what the right test framework for this would be.

General question, this is chrome-only, right? How do we implement the same thing for content?
Attachment #8938168 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] from comment #2)
> We'll need tests and docs for this. I'm not sure how/where we do either,
> though. firefox-source-docs for gecko-specific GLOBALS? Also, not sure what
> the right test framework for this would be.

That probably depends what kind of test you'd expect.
If you need a test that verifies correct return for the current platform, that's easy. If you want to test returns for all platforms then I'd probably override AppConstants.platform  in xpcshell-test.

I'll work on that.

I'm not sure about docs. :flod - can you help here?

> General question, this is chrome-only, right? How do we implement the same
> thing for content?

Hah! So, here me and :stas disagree. I believe that MessageContext should live on the privileged side and thus have access to mozIntl.* and things like PLATFORM.
If we go this way (which is what I'm currently planning for in bug 1407418, we get those for free.

Stas wants MessageContext to live on the unprivileged side eventually which would mean that we have to use Intl.* and we don't have access to PLATFORM in those localizations.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> I'm not sure about docs. :flod - can you help here?

This seems quite out of my depth, but I guess I could help once it's clear where and how to document it.
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)

> Stas wants MessageContext to live on the unprivileged side eventually which
> would mean that we have to use Intl.* and we don't have access to PLATFORM
> in those localizations.

I hypothesize that having MessageContext on the content side will be as fast as having it on the privileged side for static UIs and faster for UIs with a lot of dynamic elements which require frequent retranslation. The reason why I suspect it will be the case is that we'd avoid crossing the process boundary every time formatValues is called.

With MessageContext on the content side we can also define the language fallback chain as an iterable of MessageContexts. This would allow Fluent's DOM bindings to iterate only once over elements with data-l10n-id, which in turn could bring more perf gains.

I need to verify these hypotheses and I'll report back with data once I have my laptop fixed.

Content-side MessageContext also brings us closer to how the Web works in general. In the future, I imagine Intl.MessageContext being available to localization libraries on the content-side.

For PLATFORM specifically, since its value doesn't change on runtime, we could expose its value somehow via the L10nRegistry and use it to create MessageContext instances on the content side.
I separated out https://github.com/mozilla-l10n/localizer-documentation/issues/103 for documentation discussion. This bug depends on that.

> Content-side MessageContext also brings us closer to how the Web works in general. In the future, I imagine Intl.MessageContext being available to localization libraries on the content-side.

I believe for Gecko we will still want to use mozIntl.MessageContext to enable Gecko-specific behavior. Notice that already unprivileged chrome content is using mozIntl [0]. If we limit, say DateTimeFormat inside MessageContext in unprivileged content, I expect folks to try to get mozIntl.MessageContext exposed via IntlUtils to close the gap between intl of privileged and unprivileged content.

> For PLATFORM specifically, since its value doesn't change on runtime, we could expose its value somehow via the L10nRegistry and use it to create MessageContext instances on the content side.

Sure, but that's just a dirty hack that trickles down onto other things. I believe that we should take a firm stance and either accept that we want full l10n experience in unprivileged content or that we want unprivileged content to only do what regular Intl can (and no PLATFORM then).



[0] https://searchfox.org/mozilla-central/source/dom/base/IntlUtils.cpp
Comment on attachment 8938168 [details]
Bug 1426063 - Add PLATFORM built-in to Fluent in Gecko.

https://reviewboard.mozilla.org/r/208882/#review216688

High-level, I hear the comments about content, but I fail to map those to a concrete idea on how to implement that. Maybe that's something to clarify outside of a bug?

If we fail to get this global into content, I don't think we should have it in chrome. Having it in just one of the two would be toxic, I think.

More technical nits on the tests, too.

::: intl/l10n/test/test_localization.js:75
(Diff revision 2)
> +  }
> +
> +  const source = new FileSource('test', ['en-US'], '/localization/{locale}');
> +  L10nRegistry.registerSource(source);
> +
> +  async function * generateMessages(resIds) {

This should break eslint, but doesn't. I filed bug 1428769 to get eslint support for our files here.

This is going to complain about `function * foo` instead of `function* foo` (and possibly yield, too, didn't read that far), and single-quote strings.

::: intl/l10n/test/test_localization.js:85
(Diff revision 2)
> +    '/test.ftl'
> +  ], generateMessages);
> +
> +  let values = await l10n.formatValues([['key']]);
> +
> +  ok(values[0].includes(`${ AppConstants.platform.toUpperCase() } Value`));

Why does this test for a partial value instead of equality?
Attachment #8938168 - Flags: review?(l10n) → review-
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)

> Sure, but that's just a dirty hack that trickles down onto other things. I
> believe that we should take a firm stance and either accept that we want
> full l10n experience in unprivileged content or that we want unprivileged
> content to only do what regular Intl can (and no PLATFORM then).

I don't think it's a dirty hack. In fact, I think of this as a feature. The question I'd like to ask is: 'Do we want the information about the platform to be available to unprivileged code?'.  If the answer is yes, then it makes sense to have a dedicated API for it. If the answer is no, then by exposing it only to translations formatted with MessageContext, we'd leak that information to unprivileged code through the localization layer. While it's probably okay to do so in this case, we should consider the wider implications of this approach.
> High-level, I hear the comments about content, but I fail to map those to a concrete idea on how to implement that. Maybe that's something to clarify outside of a bug?

That's being discussed in bug 1407418.

> If we fail to get this global into content, I don't think we should have it in chrome. Having it in just one of the two would be toxic, I think.

Hmm, I try to think of PLATFORM as just one example of a class of data that we can easily expose to localizers in privileged content, and need to decide what to do in unprivileged case.

More on this below.

> Why does this test for a partial value instead of equality?

Because we use FSI/PDI around placeables.

> I don't think it's a dirty hack. In fact, I think of this as a feature. The question I'd like to ask is: 'Do we want the information about the platform to be available to unprivileged code?'.  If the answer is yes, then it makes sense to have a dedicated API for it. If the answer is no, then by exposing it only to translations formatted with MessageContext, we'd leak that information to unprivileged code through the localization layer. While it's probably okay to do so in this case, we should consider the wider implications of this approach.

Linking a response to this statenment to response to Axel's take on that above (If we don't expose it to unpriv, we shouldn't have it for chrome):

I believe we have three classes of localizable UIs around Firefox UI and Fluent:

1) Chrome, privileged UI (Preferences, Tabbar etc.)
2) Chrome, unprivileged UI (WebPayments, Date/time picker etc.)
3) Content, unprivileged UI (Regular, unprivileged websites like AMO in About:addons)

We also have two classes of localization features we can use:

a) Web platform features that can power localization (ECMA402 JS API, Fetch API, DOM spec for events etc.)
b) Gecko specific features that are adding to (a)

I believe that PLATFORM is part of (b) together with things like:
 - OS regional preferences sensitive DateTimeFormat
 - mozIntl.getLocaleInfo, mozIntl.getDisplayNames, mozIntl.getCalendarInfo, mozIntl.RelativeTimeFormat (which adds bestFit unit selection)
 - There are probably others that will come as we start looking into font preferences, DOM CPP APIs, and enable Rust XPCOM modules.


It seems to me that Axel is making the point that could be generalized to - we should not expose those features to (1), if we can't expose them to (2).
Stas is making a point that we should treat (2) just like we treat (3).

I think I disagree with Stas and believe that we should aim to provide in (2) a functionality matching (1). I believe that that's the expectation from Firefox UX team, front end team and doing differently will hinder the adoption of Fluent in Firefox and later potentially hinder the transition of UIs from (1) to (2) because they'd be losing features.

The other point Stas makes is `If the answer is yes, then it makes sense to have a dedicated API for it.` - I'd like to understand better this point. My initial take is that that would mean we will be adding a special API for each and every case of a feature we get in (1) that we also want in (2).
If I'm correct that the expectation for (2) is to match (1), then we'd basically be adding the same feature twice every time.

Alternative approach, which I proposed in bug 1407418 is to place the MessageContext for (2) in (1), and facilitate the IPC between them.

That gives us the (1)==(2) feature parity for free and guarantees we won't "forget" or have to maintain it separately resulting in developers hitting cases where the behavior in (2) doesn't match the behavior (1) when they port their code to unprivileged.
Actually, I poked around on #fx-team and got some feedback from Gijs and MattN:

Basically, it seems that I was wrong above on a couple of expectations:
 - the unprivileged content is not going to become more popular. It's reserved for unsafe features and will stay as such
 - currently, localization of unprivileged content uses only DTD and if any JS l10n or substitution (sic!) is needed, they'd send the message ID to a privileged script
 - For PLATFORM case, we could in theory use `navigator.oscpu`

So, my take is that the issue of supporting (b) in (2) is less of an issue than I expected. That makes me less concerned about going the route proposed by :stas.

My only concern is that if we want PLATFORM in content then, and we'd use `navigator.oscpu` for that, then we'd have to make sure that it aligns well between what privileged and unprivileged returns.

Axel, would you be ok with that plan?
Flags: needinfo?(l10n)
I like the idea of using navigator.oscpu for content, as https://searchfox.org/mozilla-central/source/dom/base/Navigator.cpp#447 comes with spoofing against fingerprinting built in, so we don't need to do anything.

Also, https://searchfox.org/mozilla-central/source/toolkit/components/resistfingerprinting/nsRFPService.h#14 says what we're getting, and it will still return OS, so that's nice.

lgtm.
Flags: needinfo?(l10n)
Updated the patch. Tried to test what happens when I add linting for `intl/**` but there's a lot of work to do, including probably waiting for an update to eslint (currently it fails to parse generators).

I'll aim to add intl/l10n/* to eslint after fluent 0.5.
In bug 1424682, comment 11, :stas wrote:
> I would prefer if PLATFORM() returned longer names: windows, macos, linux, android, ios, etc,

I'm a bit torn on this. On one hand, I like to be more expressive. On the other, AppConstants.platform uses a particular naming scheme which makes it easier to find platform-specific elements in Gecko and keeping in sync with that feels like an easy win.

The current scheme in the patch is: "win" | "macosx" | "linux" | "android" | "other".

Pike, Stas - can you decide between the two of you (maybe check with :mossop as well?) the scheme we should use and I'll update the patches?
Flags: needinfo?(stas)
Flags: needinfo?(l10n)
Comment on attachment 8938168 [details]
Bug 1426063 - Add PLATFORM built-in to Fluent in Gecko.

https://reviewboard.mozilla.org/r/208882/#review219634

I don't have a strong preference on the option names. I can see both the argument for consistency with AppConstants for gecko/firefox devs, but also the opportunity to use today's platform names, aka, macos instead of mac os/x.
Attachment #8938168 - Flags: review?(l10n) → review+
Flags: needinfo?(l10n)
The main target of those keys are localizers. We also have an opportunity to fix the legacy naming of the platforms for the purpose of localization. I vote for:

    "windows" | "macos" | "linux" | "android" | "other".
Flags: needinfo?(stas)
Pike, Flod - how does it sound to you?
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
As I said, I don't have a strong opinion either way.

I strongly believe that localizers are not the target audience here, though. In the bulk of all cases, the selector will be coming from the engineers of the original code.

It's a tad sad that we can't do case-insensitive checking here. Then we wouldn't need to decide if it's "windows" or "Windows", or "macOS" ;-)

Anywho, I'm rambling, I don't think it matters much either way. Just file a bug with the result of this for compare-locales to have a check? And then I'll just need to think about how to encode that in project config, so that I can check between known and unknown functions.
Flags: needinfo?(l10n)
(In reply to Staś Małolepszy :stas from comment #19)
> The main target of those keys are localizers. We also have an opportunity to
> fix the legacy naming of the platforms for the purpose of localization. I
> vote for:
> 
>     "windows" | "macos" | "linux" | "android" | "other".

I'm OK with this proposal, in particular for macos.
Flags: needinfo?(francesco.lodolo)
Pike - updated to :stas proposal. Can you confirm you're ok with the patch?
Flags: needinfo?(l10n)
yep, lgtm.
Flags: needinfo?(l10n)
Comment on attachment 8938168 [details]
Bug 1426063 - Add PLATFORM built-in to Fluent in Gecko.

https://reviewboard.mozilla.org/r/208882/#review223826

::: commit-message-1624b:1
(Diff revision 9)
> +Bug 1426063 - Add PLATFORM builtin to Fluent in Gecko. r?pike

übernit: if you have to touch this patch again, use "built-in" consistently?
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b7d42d527af
Add PLATFORM built-in to Fluent in Gecko. r=Pike
https://hg.mozilla.org/mozilla-central/rev/2b7d42d527af
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: