There has been a minor update to Fluent, and I'd like to sync us with the upstream to make future transition to 0.5 easier.

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). Stas, can you skim through if this looks like you'd expect? There's still some manual work to do between what fluent-gecko produces and this, but it mostly comes out of limitations of rollup and acron. I hope to keep the manual work low and we can try to even optimize it further, but for now, I just would like to get the 0.4.2 changes into m-c.

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). Dave - this is a minor fluent update and a couple alignments between upstream and our deployment based on https://github.com/projectfluent/fluent.js/blob/master/fluent/CHANGELOG.md#fluent-042-november-27-2017 How should we approach reviewing such landings? Is it enough if :stas reviews them, or do you want to continue reviewing them?

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). https://reviewboard.mozilla.org/r/206864/#review212986 ::: intl/l10n/MessageContext.jsm:1069 (Diff revision 4) > - const nf = ctx._memoizeIntlObject( > + const nf = ctx._memoizeIntlObject( > - Intl.NumberFormat, this.opts > + Intl.NumberFormat, this.opts > - ); > + ); > - return nf.format(this.value); > + return nf.format(this.value); > + } catch (e) { > + // XXX Report the error. I filed https://github.com/projectfluent/fluent.js/issues/106 to track this. ::: intl/l10n/MessageContext.jsm:1645 (Diff revision 4) > } > > - // XXX functions should also report errors > + try { > - return callee(posargs, keyargs); > + return callee(posargs, keyargs); > + } catch (e) { > + // XXX Report errors. I filed https://github.com/projectfluent/fluent.js/issues/107 to track this.

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). Apart from updating fluent to 0.4.2, this also updates fluent-dom which hasn't had official releases since 0.0.1. Should we tag a new version of it?

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). Yep, we should release fluent-dom 0.5 aligned with fluent 0.5! Setting r? on stas and f? on mossop for the final patch.

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). bad MozReview.

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). https://reviewboard.mozilla.org/r/206864/#review213320

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). https://reviewboard.mozilla.org/r/206864/#review213658 ::: intl/l10n/DOMLocalization.jsm:633 (Diff revision 6) > + constructor(windowElement, resourceIds, generateMessages = defaultGenerateMessages) { > + super(windowElement, resourceIds, generateMessages); > + } > +} > + > +this.DOMLocalization = GeckoDOMLocalization; This indirection is a bit confusing. Without spotting this line I'd expect the DOMLocalization class to be the thing that was exported. Can we rename that class to something else to avoid this? ::: intl/l10n/Localization.jsm:28 (Diff revision 6) > const Cc = Components.classes; > const Ci = Components.interfaces; > > const { L10nRegistry } = Cu.import("resource://gre/modules/L10nRegistry.jsm", {}); > -const LocaleService = Cc["@mozilla.org/intl/localeservice;1"].getService(Ci.mozILocaleService); > const ObserverService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); Once you're importing Services you ould use Services.obs instead of this.

Stas, can you take another look? I realized that the way Gecko importing works, I only need the extension on the Localization class, so I named the base `FluentLocalization` and the one specific `Localization`. Then, in DOMLocalization, I extend what Localization.jsm exports, which is, in this case, a specialized class already, so I was able to just extend it without any specialization there!

Comment on attachment 8936063 [details] Bug 1424449 - Update Fluent to master (f722d99). https://reviewboard.mozilla.org/r/206864/#review214460 ::: intl/l10n/DOMLocalization.jsm:22 (Diff revision 7) > > > -/* fluent@0.4.1 */ > +/* fluent@0.4.2 */ > > const { Localization } = > Components.utils.import("resource://gre/modules/Localization.jsm", {}); It's hard for me to review the way you import classes between JSM without knowing how much of it happens automatically when you build fluent-gecko, and how much is done by you manually. I built fluent-gecko locally and instead of Component.utils.import I see the following here: import Localization from '../../fluent-dom/src/localization.js'; So, I'm assuming this is one of the places where you post-edit by hand. You probably also need to edit `Localization.jsm` then to rename the vanilla `Localization` class to `FluentLocalization`. IUC, instead of exporting the custom `Localization` from `Localization.jsm` you could continue to export `GeckoLocalization` and it would take a hand-edit here and in `DOMLocalization extends GeckoLocalization` to make it work. In both cases, `DOMLocalization` is different from the vanialla fluent-dom's `DOMLocalization` because it extends the custom `(Gecko)Localization` rathen than the vanilla `Localization`. If this is confusing, it's because you end up with many different `Localization` subclasses with very similar names. The solution you propose in the updated patch is: `DOMLocalization extends Localization extends FluentLocalization`, out of which `FluentLocalization` is the vanilla one, and others are custom, and importantly they are not the same as `DOMLocalization` and `Localization` from http://projectfluent.org/fluent.js/fluent-dom/. I preferred the previous solution which was more explicit: `GeckoLocalization extends Localization` and `GeckoDOMLocalization extends DOMLocalization`. In both cases the only thing that the `Gecko*` subclass adds is the ability to talk to the `L10nRegistry`.

> It's hard for me to review the way you import classes between JSM without knowing how much of it happens automatically when you build fluent-gecko, and how much is done by you manually. > I built fluent-gecko locally and instead of Component.utils.import I see the following here: Our code to export fluent.js into JSMs is imperfect. It's very hard to make it perfect. We have a choice of either paying for that imperfection in Firefox codebase, or on the boundary between mozilla-central and fluent.js by having to do some manual work on what fluent.js/fluent-gecko produces. In this particular case, rollup does not allow for the es6 imports to be turned into C.u.imports. I very strongly oppose making the m-c codebase store code with artifacts of our build system imperfections, thus I prefer me (and in a couple cases you) to pay the price. If you believe otherwise, we'll have to resolve this before we move forward. > In both cases, `DOMLocalization` is different from the vanialla fluent-dom's `DOMLocalization` because it extends the custom `(Gecko)Localization` rathen than the vanilla `Localization`. > If this is confusing, it's because you end up with many different `Localization` subclasses with very similar names. We have one Localization and one DOMLocalization in Gecko. I'm trying (as above) to optimize the naming for the Firefox front-end engineers population. They will interact with exactly 2 classes, and naming them with specializations seems like an artifact of our build system rather than a feature. I'm trying to avoid such artifacts creeping onto their experience of using Fluent. Once we have es6 modules in Gecko, I'd like to do: ``` import { Localization as FluentLocalization } from './intl/l10n/fluent-dom/localization; export class Localization extends FluentLocalization {}; ``` style, as you suggested, which means that the public API names are `Localization` and `DOMLocalization`. Is that something you can accept or would you prefer me to negotiate that with Gecko module owners?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18) > I very strongly oppose making the m-c codebase store code with artifacts of > our build system imperfections, thus I prefer me (and in a couple cases you) > to pay the price. > > If you believe otherwise, we'll have to resolve this before we move forward. I agree with you. I wasn't being critical towards the process and I appreciate the work you're doing between building fluent-gecko and proposing the patch here to work around the imperfections of fluent's build system. I wanted to explain why it's hard to review a patch like this without seeing the corresponding changes in the fluent.js repo. > (...) the public API names are `Localization` and `DOMLocalization`. But they are not the same Localization and DOMLocalization as those exported by fluent-dom. My concern is that engineers will Google them and will find docs which are not true. fluent is an external library and it's reasonable to me that any modifications to it that are Gecko-specific would be named accordingly. If we bundled a modified version of React in mozilla-central which changes the API slightly, I assume we'd call it GeckoReact.

> But they are not the same Localization and DOMLocalization as those exported by fluent-dom. My concern is that engineers will Google them and will find docs which are not true. I guess it all loops back to the issue of naming. `Localization` is as of a generic name as possible. You might as well call it "Class". Maybe Fluent shouldn't use generic names then? :) In Gecko we have an API that matches functionality of Fluent's `Localization` class in 95%. It's the only class that we expose and the only people should use. Naming it `Localization` is the same for me as naming `Services.intl.DateTimeFormat` which is 95% compatible with `Intl.DateTimeFormat`. > fluent is an external library and it's reasonable to me that any modifications to it that are Gecko-specific would be named accordingly. If we bundled a modified version of React in mozilla-central which changes the API slightly, I assume we'd call it GeckoReact. If React decided to expose a class "View", and Mozilla wanted to expose a class named "View" that used React, I believe we'd name it "View".

This will be resolved in bug 1452734