1347801 - Land MessageContext in Gecko

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Fluent is a localization API that we're aiming to use in place of DTD/properties.

You can read more at http://projectfluent.io/

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review commit: https://reviewboard.mozilla.org/r/120904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120904/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/1-2/

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/2-3/

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/3-4/

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/4-5/

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

:mossop,

I'd like to start planning the landing of the first two bits - MessageContext here and L10nRegistry in bug 1333980.

The main file here - MessageContext.jsm - has three sections:

1) Parser (lines 38 - 804)
2) Resolver (lines 934 - 1377)
3) MessageContext class (lines 1392 - 1582)

The Parser is responsible for handling the Fluent syntax:
 - https://github.com/projectfluent/fluent/tree/master/spec
 - http://projectfluent.io/fluent/guide/

The resolver is responsible for formatting the messages, so actually getting the parsed messages and variables formatted into a string.

The MessageContext is the only public class that is shaped to be similar to ECMA402 Intl APIs and takes Fluent syntax strings, parses them using the parser and allows for formatting using the Resolver.

We have a lot of tests in our main repo [0] but I added some basic xpcshell tests here as well.

[0] https://github.com/projectfluent/fluent.js/tree/master/fluent/test

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

It's not a review request yet, just feedback for now.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Makoto, are you ok with us placing the localization API in ./intl/l10n directory? I'd like to use a new directory so that people working on l10n can recompile just that when doing changes.

You can see the whole planned structure here: https://bitbucket.org/zbraniecki/mozilla-unified/src/d05bfec2ae8910e745e6768341fddd89f93187e6/intl/l10n/?at=fennec

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> Makoto, are you ok with us placing the localization API in ./intl/l10n
> directory? I'd like to use a new directory so that people working on l10n
> can recompile just that when doing changes.

Yes.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

https://reviewboard.mozilla.org/r/120904/#review143658

::: intl/l10n/MessageContext.jsm:38
(Diff revision 5)
> + * This parser is optimized for runtime performance.
> + *
> + * There is an equivalent of this parser in syntax/parser which is
> + * generating full AST which is useful for FTL tools.
> + */
> +class RuntimeParser {

I'd like to see more documentation for each method on this class.

::: intl/l10n/MessageContext.jsm:38
(Diff revision 5)
> + * This parser is optimized for runtime performance.
> + *
> + * There is an equivalent of this parser in syntax/parser which is
> + * generating full AST which is useful for FTL tools.
> + */
> +class RuntimeParser {

Many of the method names in thsi class look problematic. While prefixed with "get" they don't appear to return anything. Perhaps "find" or "skip" would be a better prefix based on what it look like they are doing?

::: intl/l10n/MessageContext.jsm:839
(Diff revision 5)
> +   * instance passed as an argument.
> +   *
> +   * @param   {MessageContext} [ctx]
> +   * @returns {string}
> +   */
> +  valueOf() {

valueOf feels like the wrong choice here since it always returns a string. Would toString be better?

::: intl/l10n/MessageContext.jsm:842
(Diff revision 5)
> +   * @returns {string}
> +   */
> +  valueOf() {
> +    throw new Error('Subclasses of FluentType must implement valueOf.');
> +  }
> +}

Please also document the match method

::: intl/l10n/MessageContext.jsm:1011
(Diff revision 5)
> +  return new FluentNone();
> +}
> +
> +
> +/**
> + * Resolve a reference to a message to the message object.

I don't follow this comment.

::: intl/l10n/MessageContext.jsm:1161
(Diff revision 5)
> + *
> + * @param   {Object} expr
> + * @returns {FluentType}
> + * @private
> + */
> +function Type(env, expr) {

Please document the env argument and for both arguments give a brief description.

::: intl/l10n/MessageContext.jsm:1369
(Diff revision 5)
> + * The return value must be unwrapped via `valueOf` by the caller.
> + *
> + * @param   {MessageContext} ctx
> + * @param   {Object}         args
> + * @param   {Object}         message
> + * @param   {Array}          errors

Here and throughout please give brief descriptions of the arguments.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Stas, would you have time to apply :mossop's feedback on this? I built the last MessageContext.jsm from fluent-gecko and it doesn't seem to contain any of the changes suggested here (and it has the two comments for content removed with DCE).

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/5-6/

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Addressed the early feedback and updated to Fluent 0.4.1. :mossop volunteered to review it for us.

I believe it's time to land it so that we can land L10nRegistry in bug 1333980 and start working on binding AddonsManager (bug 1365709), Fennec DLC (bug 1347802) and our new Localization API (bug 1347800) into it.

Comment 14

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #10)
> Comment on attachment 8847963 [details]
> ::: intl/l10n/MessageContext.jsm:839
> (Diff revision 5)
> > +   * instance passed as an argument.
> > +   *
> > +   * @param   {MessageContext} [ctx]
> > +   * @returns {string}
> > +   */
> > +  valueOf() {
> 
> valueOf feels like the wrong choice here since it always returns a string.
> Would toString be better?

This comment was never addressed. Is there a reason that valueOf is better?

Comment 15

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #14)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > Comment on attachment 8847963 [details]
> > ::: intl/l10n/MessageContext.jsm:839
> > (Diff revision 5)
> > > +   * instance passed as an argument.
> > > +   *
> > > +   * @param   {MessageContext} [ctx]
> > > +   * @returns {string}
> > > +   */
> > > +  valueOf() {
> > 
> > valueOf feels like the wrong choice here since it always returns a string.
> > Would toString be better?
> 
> This comment was never addressed. Is there a reason that valueOf is better?

Never mind, I see the extra comment now.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

https://reviewboard.mozilla.org/r/120904/#review161428

This is looking really good, there are just a bunch of comments because there is a lot of code here. I'm likely to have a little more to say after I look over the other patch too.

A couple of general comments:

I generally dislike it when a function returns different kinds of things that callers then have to differentiate between. So returning a string in one case and an object in another is asking for confusion. If we could standardise return types that would be great. I think the JIT is more effective when a variable's type remains the same too.

Speaking about returns, rather than returning arrays of things from a function would it be better to return objects so that callers don't have to remember specific indexes into the array for the things they care about. It tends to make the code more readable.

In some cases you get the current character and compare to other characters, in other cases you get the character code and compare to codes requiring comments explaining the codes. Why not just always get the current character? < and > still work on characters.

::: intl/l10n/MessageContext.jsm:64
(Diff revision 6)
> +        this.getEntry();
> +      } catch (e) {
> +        if (e instanceof SyntaxError) {
> +          errors.push(e);
> +
> +          const nextEntity = this.findNextEntryStart();

It's a little strange that all the other functions here advance this._index but this one returns a new index. Can we just call this skipToNextEntryStart and do the manipulation in there?

::: intl/l10n/MessageContext.jsm:89
(Diff revision 6)
> +  getEntry() {
> +    // The index here should either be at the beginning of the file
> +    // or right after new line.
> +    if (this._index !== 0 &&
> +        this._source[this._index - 1] !== '\n') {
> +      throw this.error('Expected new line and a new entry');

This error message could be more explicit:

    Expected an entry to begin at the beginning of the file or on a new line.

::: intl/l10n/MessageContext.jsm:105
(Diff revision 6)
> +    if (ch === '[') {
> +      this.skipSection();
> +      return;
> +    }
> +
> +    if (ch !== '\n') {

Since we always skipWS before calling getEntry when would we ever be positioned at a \n character?

::: intl/l10n/MessageContext.jsm:128
(Diff revision 6)
> +    this.skipInlineWS();
> +    this.getSymbol();
> +    this.skipInlineWS();
> +
> +    if (this._source[this._index] !== ']' ||
> +        this._source[this._index + 1] !== ']') {

Should we also check that the following character is a newline?

::: intl/l10n/MessageContext.jsm:135
(Diff revision 6)
> +    }
> +
> +    this._index += 2;
> +
> +    // sections are ignored in the runtime ast
> +    return undefined;

No need for this as nothing ever does anything with the return.

::: intl/l10n/MessageContext.jsm:161
(Diff revision 6)
> +    } else {
> +      this.skipWS();

If we have any other character shouldn't we be throwing an error? I guess we will after cascading through the rest of the method but might be clearer to just thow here.

::: intl/l10n/MessageContext.jsm:185
(Diff revision 6)
> +      }
> +      tags = this.getTags();
> +    }
> +
> +    if (tags === null && attrs === null && typeof val === 'string') {
> +      this.entries[id] = val;

For consistency I think this should be the same data structure as elsewhere, i.e. { val }

::: intl/l10n/MessageContext.jsm:218
(Diff revision 6)
> +      cc = this._source.charCodeAt(++this._index);
> +    }
> +  }
> +
> +  /**
> +   * Skip whitespace except for \n.

and \r

::: intl/l10n/MessageContext.jsm:272
(Diff revision 6)
> +    const start = this._index;
> +    let cc = this._source.charCodeAt(this._index);
> +
> +    if ((cc >= 97 && cc <= 122) || // a-z
> +        (cc >= 65 && cc <= 90) ||  // A-Z
> +        cc === 95 || cc === 32) {  //  _

And space

::: intl/l10n/MessageContext.jsm:274
(Diff revision 6)
> +
> +    if ((cc >= 97 && cc <= 122) || // a-z
> +        (cc >= 65 && cc <= 90) ||  // A-Z
> +        cc === 95 || cc === 32) {  //  _
> +      cc = this._source.charCodeAt(++this._index);
> +    } else if (name.length === 0) {

When can name.length be anything other than 0?

::: intl/l10n/MessageContext.jsm:285
(Diff revision 6)
> +           (cc >= 48 && cc <= 57) ||  // 0-9
> +           cc === 95 || cc === 45 || cc === 32) {  //  _-
> +      cc = this._source.charCodeAt(++this._index);
> +    }
> +
> +    // If we encountered the end of name, we want to test is the last

s/test is/test if/

::: intl/l10n/MessageContext.jsm:318
(Diff revision 6)
> +
> +      if (ch === '\n') {
> +        break;
> +      }
> +
> +      value += ch;

You might get slightly better performance by doing a substring after the look rather than appending character at a time.

::: intl/l10n/MessageContext.jsm:382
(Diff revision 6)
> +    if (ch === '\\' &&
> +      (this._source[this._index + 1] === '"' ||
> +       this._source[this._index + 1] === '{' ||
> +       this._source[this._index + 1] === '\\')) {
> +      buffer += this._source[this._index + 1];
> +      this._index += 2;
> +      ch = this._source[this._index];
> +    }

Why isn't this just handled in the loop below?

::: intl/l10n/MessageContext.jsm:392
(Diff revision 6)
> +      this._index += 2;
> +      ch = this._source[this._index];
> +    }
> +
> +    while (this._index < this._length) {
> +      // This block handles multi-line strings combining strings seaprated

s/seaprated/separated/

There should also be a comma before combining.

::: intl/l10n/MessageContext.jsm:401
(Diff revision 6)
> +        if (
> +            this._source[this._index] === '}' ||
> +            this._source[this._index] === '[' ||
> +            this._source[this._index] === '*' ||
> +            this._source[this._index] === '#' ||
> +            this._source[this._index] === '.'
> +        ) {

Weird style, no newline after or before the brackets would be my preference.

::: intl/l10n/MessageContext.jsm:416
(Diff revision 6)
> +      } else if (ch === '\\') {
> +        const ch2 = this._source[this._index + 1];
> +        if (ch2 === '"' || ch2 === '{') {
> +          ch = ch2;
> +          this._index++;
> +        }

If I'm reading this right then there is no way to have a \ immediately before a placeable.

::: intl/l10n/MessageContext.jsm:500
(Diff revision 6)
> +
> +    // If the expression is followed by `->` we're going to collect
> +    // its members and return it as a select expression.
> +    if (ch !== '}') {
> +      if (ch !== '-' || this._source[this._index + 1] !== '>') {
> +        throw this.error('Expected "}", "," or "->"');

There doesn't seem to be a check for , here

::: intl/l10n/MessageContext.jsm:520
(Diff revision 6)
> +      if (variants[0].length === 0) {
> +        throw this.error('Expected members for the select expression');
> +      }
> +    }
> +
> +    if (variants === undefined) {

This is just the else of the above if statement no?

::: intl/l10n/MessageContext.jsm:876
(Diff revision 6)
> +  /**
> +   * Skips an FTL comment.
> +   *
> +   * @private
> +   */
> +  skipComment() {

Should this check that the next next character is also / and throw an error if not?

::: intl/l10n/MessageContext.jsm:1180
(Diff revision 6)
> + * @private
> + */
> +function PlaceableLength(env, parts) {
> +  const { ctx } = env;
> +  return parts.reduce(
> +    (sum, part) => sum + part.valueOf(ctx).length,

What if valueOf doesn't return a string?

::: intl/l10n/MessageContext.jsm:1573
(Diff revision 6)
> +    } else {
> +      posargs.push(Type(env, arg));
> +    }
> +  }
> +
> +  // XXX functions should also report errors

File a bug for this follow-up

::: intl/l10n/MessageContext.jsm:1582
(Diff revision 6)
> +/**
> + * Resolve a pattern (a complex string with placeables).
> + *
> + * @param   {Object} env
> + *    Resolver environment object.
> + * @param   {Object} ptn

This is an array, not an object.

::: intl/l10n/MessageContext.jsm:1810
(Diff revision 6)
> +   * @param   {Object | string}    message
> +   * @param   {Object | undefined} args
> +   * @param   {Array}              errors
> +   * @returns {?Array<FluentType>}
> +   */
> +  formatToParts(message, args, errors) {

Maybe make errors optional so callers can just ignore it if they like?

::: intl/l10n/MessageContext.jsm:1892
(Diff revision 6)
> +    const cache = this._intls.get(ctor) || {};
> +    const id = JSON.stringify(opts);
> +
> +    if (!cache[id]) {
> +      cache[id] = new ctor(this.locales, opts);
> +      this._intls.set(ctor, cache);

This is unnecessary in some cases and you might save some perf by not doing it then.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Dave Townsend [:mossop] from comment #16)
> I generally dislike it when a function returns different kinds of things
> that callers then have to differentiate between. So returning a string in
> one case and an object in another is asking for confusion. If we could
> standardise return types that would be great. I think the JIT is more
> effective when a variable's type remains the same too.

The reasons for this in this code is usually the GC overhead threat.

Around 90-95% of messages that we have are simple strings, while the remaining cases are more complex (objects).

Creating the object just to return it and then extract the string out of it and discard it has been resulting in an overhead that we decided to kill in runtime with those optimizations.

All our non-runtime parsers, and non-GC ports of Fluent (like Rust) are consistent in what they return. But since here we're talking about startup time, we try to squeeze every bit of optimization we can hope for.

Do you want us to give up on that?

> In some cases you get the current character and compare to other characters,
> in other cases you get the character code and compare to codes requiring
> comments explaining the codes. Why not just always get the current
> character? < and > still work on characters.

When I have a choice, I go for character code. But in cases where for some reason I already have the
character, instead of getting the code out of it, I'm just comparing to the character.

> ::: intl/l10n/MessageContext.jsm:185
> (Diff revision 6)
> > +      }
> > +      tags = this.getTags();
> > +    }
> > +
> > +    if (tags === null && attrs === null && typeof val === 'string') {
> > +      this.entries[id] = val;
> 
> For consistency I think this should be the same data structure as elsewhere,
> i.e. { val }

That's the primary optimization we're doing here to save on creating objects for 90% of cases.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> (In reply to Dave Townsend [:mossop] from comment #16)
> > I generally dislike it when a function returns different kinds of things
> > that callers then have to differentiate between. So returning a string in
> > one case and an object in another is asking for confusion. If we could
> > standardise return types that would be great. I think the JIT is more
> > effective when a variable's type remains the same too.
> 
> The reasons for this in this code is usually the GC overhead threat.
> 
> Around 90-95% of messages that we have are simple strings, while the
> remaining cases are more complex (objects).
> 
> Creating the object just to return it and then extract the string out of it
> and discard it has been resulting in an overhead that we decided to kill in
> runtime with those optimizations.
> 
> All our non-runtime parsers, and non-GC ports of Fluent (like Rust) are
> consistent in what they return. But since here we're talking about startup
> time, we try to squeeze every bit of optimization we can hope for.
> 
> Do you want us to give up on that?

I'd love to have cleaner code but if that's what we have to do to keep performance then I guess that's what we have to do.

> > In some cases you get the current character and compare to other characters,
> > in other cases you get the character code and compare to codes requiring
> > comments explaining the codes. Why not just always get the current
> > character? < and > still work on characters.
> 
> When I have a choice, I go for character code. But in cases where for some
> reason I already have the
> character, instead of getting the code out of it, I'm just comparing to the
> character.

Any particular reason for preferring character codes? Using and comparing against characters directly is self documenting so I'd lean that way unless there is a performance reason I'm missing.

> > ::: intl/l10n/MessageContext.jsm:185
> > (Diff revision 6)
> > > +      }
> > > +      tags = this.getTags();
> > > +    }
> > > +
> > > +    if (tags === null && attrs === null && typeof val === 'string') {
> > > +      this.entries[id] = val;
> > 
> > For consistency I think this should be the same data structure as elsewhere,
> > i.e. { val }
> 
> That's the primary optimization we're doing here to save on creating objects
> for 90% of cases.

Ick, but ok.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Dave Townsend [:mossop] from comment #18)
> Any particular reason for preferring character codes? Using and comparing
> against characters directly is self documenting so I'd lean that way unless
> there is a performance reason I'm missing.

I made an experiment. I replaced all 'cc' with 'ch' in a hot function in the parser - `getIdentifier`.
The branch is here: https://github.com/projectfluent/fluent.js/compare/master...zbraniecki:parser-ch?expand=1

I ran the parser microbenchmark we use against jsshell from today. The result:

./tools/perf/test.js -e jsshell -s 30 -c ref.json
parseFTL: 
  mean:   19860.77 (+1%)
  stdev:  938.3
  sample: 30
parseFTLEntries: 
  mean:   4312.27 (+9%)
  stdev:  458.88
  sample: 30
format: 
  mean:   1739.6 (+2%)
  stdev:  207.91
  sample: 30

The `parseFTL` is the time it takes for the real AST parser, parseFTLEntries is the runtime parser, and format is the resolver.
There's some noise in this, but the regression in entries parser is noticable.

Notice - because of the way parser usually operates (it runs several times, but not enough to get hot and JIT), our benchmarking tool is firing a new shell for each sample instead of looping over N times.
This gives us performance characteristic that is more representative of how Fluent parser is used at runtime, but the results may go against some logic that comes from microbenchmarks that heat the code until it gets JITted.

My conclusion is that it still doesn't make much sense to switch from code points to characters.

p.s.

Also, it seems that node doesn't have this penalty, so there may be some bug against SpiderMonkey to be filed:

▶ ./tools/perf/test.js -e node -s 30 -c ref.json
parseFTL: 
  mean:   7683.23 (0%)
  stdev:  320.59
  sample: 30
parseFTLEntries: 
  mean:   2036.97 (0%)
  stdev:  73.49
  sample: 30
format: 
  mean:   935.57 (0%)
  stdev:  34.8
  sample: 30

but I'd prefer to not distract SpiderMonkey people now, and instead start optimizing SM codepaths used by Fluent only when we prove the technology, since Parser is not currently the bottleneck wrt. performance impact of Fluent in Firefox.

Comment 20

[Staś Małolepszy :stas]

Hi Dave, thanks for your thorough review and comments.  I'm the other author of this code and I'm mostly responsible for the resolution and formatting part.  Zibi has been doing great work answering your questions so I'll chime in just in a few places that touch the code I wrote.

(In reply to Dave Townsend [:mossop] from comment #16)
> Comment on attachment 8847963 [details]
> Bug 1347801 - Land Fluent MessageContext (Parser, Resolved, Context).
> 
> https://reviewboard.mozilla.org/r/120904/#review161428

> Speaking about returns, rather than returning arrays of things from a
> function would it be better to return objects so that callers don't have to
> remember specific indexes into the array for the things they care about. It
> tends to make the code more readable.

+1. We were trying to move away from returning arrays but I still see them in a few places. It makes sense to me to change them to objects.

> ::: intl/l10n/MessageContext.jsm:1180
> (Diff revision 6)
> > + * @private
> > + */
> > +function PlaceableLength(env, parts) {
> > +  const { ctx } = env;
> > +  return parts.reduce(
> > +    (sum, part) => sum + part.valueOf(ctx).length,
> 
> What if valueOf doesn't return a string?

Good catch, thanks. I filed bug 1382161.

> ::: intl/l10n/MessageContext.jsm:1573
> (Diff revision 6)
> > +    } else {
> > +      posargs.push(Type(env, arg));
> > +    }
> > +  }
> > +
> > +  // XXX functions should also report errors
> 
> File a bug for this follow-up

Bug 1382164.

> ::: intl/l10n/MessageContext.jsm:1810
> (Diff revision 6)
> > +   * @param   {Object | string}    message
> > +   * @param   {Object | undefined} args
> > +   * @param   {Array}              errors
> > +   * @returns {?Array<FluentType>}
> > +   */
> > +  formatToParts(message, args, errors) {
> 
> Maybe make errors optional so callers can just ignore it if they like?

If the caller doesn't pass the errors array to format/formatToParts, the errors will be effectively ignored, although the array will be created and errors will be pushed to it before it's GC'ed.  If I understand correctly, when the errors argument is left undefined by the caller we could leave it as such and avoid creating the array and pushing errors to it at the cost of a few more ifs here and there.

> ::: intl/l10n/MessageContext.jsm:1892
> (Diff revision 6)
> > +    const cache = this._intls.get(ctor) || {};
> > +    const id = JSON.stringify(opts);
> > +
> > +    if (!cache[id]) {
> > +      cache[id] = new ctor(this.locales, opts);
> > +      this._intls.set(ctor, cache);
> 
> This is unnecessary in some cases and you might save some perf by not doing
> it then.

Sorry, I don't understand this comment. In which cases is this unnecessary?

Comment 21

[Dave Townsend [:mossop]]

(In reply to Staś Małolepszy :stas (back on Jul 31) from comment #20)
> > ::: intl/l10n/MessageContext.jsm:1810
> > (Diff revision 6)
> > > +   * @param   {Object | string}    message
> > > +   * @param   {Object | undefined} args
> > > +   * @param   {Array}              errors
> > > +   * @returns {?Array<FluentType>}
> > > +   */
> > > +  formatToParts(message, args, errors) {
> > 
> > Maybe make errors optional so callers can just ignore it if they like?
> 
> If the caller doesn't pass the errors array to format/formatToParts, the
> errors will be effectively ignored, although the array will be created and
> errors will be pushed to it before it's GC'ed.  If I understand correctly,
> when the errors argument is left undefined by the caller we could leave it
> as such and avoid creating the array and pushing errors to it at the cost of
> a few more ifs here and there.

Sorry, I see now that because this passes through to resolve where errors is optional that makes it effectively optional here already. Maybe just document that in the javadoc above or make it clearer by putting the default of [] in the function arguments.

> > ::: intl/l10n/MessageContext.jsm:1892
> > (Diff revision 6)
> > > +    const cache = this._intls.get(ctor) || {};
> > > +    const id = JSON.stringify(opts);
> > > +
> > > +    if (!cache[id]) {
> > > +      cache[id] = new ctor(this.locales, opts);
> > > +      this._intls.set(ctor, cache);
> > 
> > This is unnecessary in some cases and you might save some perf by not doing
> > it then.
> 
> Sorry, I don't understand this comment. In which cases is this unnecessary?

In the case where cache is already in the Map but didn't have cache[id] defined on it. It makes the code slightly more complicated but saves a call to set. Does that make sense?

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/6-7/

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/7-8/

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Dave, I applied almost all of your feedback.

The only piece left is the one related to caching intl ctors, since I don't fully understand your suggestion, I'd like to wait for Stas.

I'm re-requesting review because if we can get it down to this one item, then we'd be ready to land once you and stas resolve it.

Since interdiff is not reliable, you can see the patch on fluent.js here: https://github.com/projectfluent/fluent.js/pull/51/files

Comment 25

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #21)
> (In reply to Staś Małolepszy :stas (back on Jul 31) from comment #20)
> > > ::: intl/l10n/MessageContext.jsm:1892
> > > (Diff revision 6)
> > > > +    const cache = this._intls.get(ctor) || {};
> > > > +    const id = JSON.stringify(opts);
> > > > +
> > > > +    if (!cache[id]) {
> > > > +      cache[id] = new ctor(this.locales, opts);
> > > > +      this._intls.set(ctor, cache);
> > > 
> > > This is unnecessary in some cases and you might save some perf by not doing
> > > it then.
> > 
> > Sorry, I don't understand this comment. In which cases is this unnecessary?
> 
> In the case where cache is already in the Map but didn't have cache[id]
> defined on it. It makes the code slightly more complicated but saves a call
> to set. Does that make sense?

Here is an example of the code you'd use to avoid the extra set:

    let cache;
    const id = JSON.stringify(opts);

    if (this._intls.has(ctor)) {
        cache = this._intls.get(ctor);

        if (!cache[id]) {
          cache[id] = new ctor(this.locales, opts);
        }
    } else {
        cache = {
            [id]: new ctor(this.locales, opts);
        }
        this._intls.set(ctor, cache);
    }
    return cache[id];

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

https://reviewboard.mozilla.org/r/120904/#review164866

I'd still like to see a way, either in here or in L10nRegistry or something in between that allows for async parsing of the ftl files. Can we spin it out into a worker thread perhaps?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdf9ccb0b9ab
Land Fluent MessageContext (Parser, Resolver, Context). r=mossop

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Also needed to back this one out: https://hg.mozilla.org/integration/autoland/rev/60735a337ae3f5d6c13c05a620e400cf5ed3c784

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8847963 [details]
Bug 1347801 - Land Fluent MessageContext (Parser, Resolver, Context).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120904/diff/8-9/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a19ff151e785
Land Fluent MessageContext (Parser, Resolver, Context). r=mossop

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a19ff151e785