glen.little Reporter
	Description • 8 years ago

When calling browser.i18n.getMessage('myKey') in a Firefox WebExtension, an exception is shown in the background Console if 'myKey' does not exist:

   Unknown localization message myKey

In Chrome, it fails silently and returns an empty string.

At other times, I find that it returns a string of ??. This is not the same as Chome.

	Kris Maglione [:kmag] Assignee
	Comment 1 • 8 years ago

I'll change this to return "" rather than "??" for a missing key, since that's Chrome's documented behavior, but I'm planning to keep the console error.

	glen.little Reporter
	Comment 2 • 8 years ago

Can you at least not do the console error if the call is wrapped in a Try...Catch statement?  I know that some of the values are not going to be there... so I don't want to see console errors!

	Kris Maglione [:kmag] Assignee
	Comment 3 • 8 years ago

Nope, the method doesn't throw, so a try-catch block has no effect.

Why do you want this to fail silently for missing keys?

	glen.little Reporter
	Comment 4 • 8 years ago

Two answers, I guess:

- To match what Chrome does
- If I need to know, I can check the result and see if it is an empty string.

If I find an empty string when I didn't expect it, I can write my own log to the console to help find it. I'd rather not have the console filled with messages that I cannot turn off!

	Kris Maglione [:kmag] Assignee
	Comment 5 • 8 years ago

In general, a missing locale string is an error. It's true that you can check for an empty return value on every call, but most people won't, nor should they be expected to. But they should still get some clear indication that a failure occurred.

	glen.little Reporter
	Comment 6 • 8 years ago

As you know, the Chrome specs say:  "Gets the localized string for the specified message. If the message is missing, this method returns an empty string (''). If the format of the getMessage() call is wrong — for example, messageName is not a string or the substitutions array has more than 9 elements — this method returns undefined."

If you want to change what is being done, I'd throw an exception if the key is missing (and break the spec). But don't write messages to the console log - that doesn't help the code to know if the key is missing!

Thanks for working on this... it is great that Firefox moving to use WebExtensions!

	glen.little Reporter
	Comment 7 • 8 years ago

"But they should still get some clear indication that a failure occurred."

The developers will get a clear indication: there will be nothing showing where they expected some text!

:)

	Kris Maglione [:kmag] Assignee
	Comment 8 • 8 years ago

The documentation doesn't say that nothing will be reported, just that an empty string is returned. We can't change this to throw an error, since that may break existing code, whereas reporting an error does not. Also, in general, API methods are not expected to throw errors, but instead either set `lastError` in a callback, or simply report that an error occurred and wasn't checked.

Why exactly do you need to call `getMessage` with strings you expect to be missing?

	glen.little Reporter
	Comment 9 • 8 years ago

In one of the cases I'm working with, there are resources with keys like:

sample_1
sample_2
sample_3

etc.

The code doesn't know how many there are, so just tests  'sample_' + i  until it finds one that returns blank. So, I'm expecting a blank when I've gone past the last one. This allows someone on the team to add more "sample" resources without having to change the code to precisely match the number there.

And as the project evolves and other languages are added, some old samples are removed, but we can't easily renumber all the samples, so holes are left in the list. The code knows to check for a maximum of 20 possible samples. As a result, there will always be lots of console messages that cannot be avoided.

In another situation, as the project is being developed, the code will start calling for a resource before it is added to the resources file. The code knows when it gets a blank string to show a special flag on the screen. So no console messages are needed. In this case, the console message is not a show-stopper, since they will be eliminated eventually.

I consider getting a blank result a normal process, not an error. So I would not expect console messages everytime it happens.

Maybe if console messages are really needed, you can add a new property to  browser.i18n  and turn on the console messages when a resource key is missing!

	Kris Maglione [:kmag] Assignee
	Comment 10 • 8 years ago

Hm. I'm not sure that's a use case we want to support. There should be alternatives to probing for unknown keys, such as including a key with a count of available values, or storing all values in a single key, separated by newlines.

We may be able to handle this in some other way, though. I suppose we could set `lastError`, and require that it be checked if you're expecting a value to be missing. It would be a bit strange, though, since currently `lastError` is only set during callbacks, which `getMessage` doesn't support.

	glen.little Reporter
	Comment 11 • 8 years ago

By far the simplest would be to do what Chrome is doing... fail gracefully, return an empty string, and don't write a message to the console!

	Kris Maglione [:kmag] Assignee
	Comment 12 • 8 years ago

Failing gracefully and failing silently are not the same thing, and the simplest thing is often not the correct thing.

I think that failing silently is clearly the wrong behavior here. If I could get away with it, I'd add an additional argument for a default value, and/or throw an error for missing keys. But those are both too likely to cause compatibility problems, so reporting the failure is the best I think we can do for now.

	glen.little Reporter
	Comment 13 • 8 years ago

Again, I'd say that the simplest answer is to replicate what Chrome is doing, and not try to add anything to it!

But, I'll leave it in your capable hands!

	Kris Maglione [:kmag] Assignee
	Comment 14 • 8 years ago

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

	Bob Silverberg [:bsilverberg]
	Comment 15 • 8 years ago

Comment on attachment 8737689 [details]
MozReview Request: Bug 1258199: [webext] Return an empty string for missing keys in getMessage. r?bsilverberg

https://reviewboard.mozilla.org/r/44093/#review40657

::: toolkit/components/extensions/ExtensionUtils.jsm:412
(Diff revision 1)
>        }
>      }
>  
> -    Cu.reportError(`Unknown localization message ${message}`);
> -    return defaultValue;
> +    if (!this.missingKeys.has(message)) {
> +      let error = `Unknown localization message ${message}`;
> +      if (options.cloneScope) {

Why do we need this `if`? In what cases do we explicitly *not* want this to happen?

::: toolkit/components/extensions/ExtensionUtils.jsm:413
(Diff revision 1)
>      }
>  
> -    Cu.reportError(`Unknown localization message ${message}`);
> -    return defaultValue;
> +    if (!this.missingKeys.has(message)) {
> +      let error = `Unknown localization message ${message}`;
> +      if (options.cloneScope) {
> +        error = new options.cloneScope.Error(error);

Is this how `lastError` is set? If not, I'm not sure what this `cloneScope` is all about.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:34
(Diff revision 1)
>      assertEq("(bar)", _("bar"), "Simple message fallback in default locale.");
>  
> -    assertEq("??", _("some-unknown-locale-string"), "Unknown locale string.");
> +    assertEq("", _("some-unknown-locale-string"), "Unknown locale string.");
>  
> -    assertEq("??", _("@@unknown_builtin_string"), "Unknown built-in string.");
> -    assertEq("??", _("@@bidi_unknown_builtin_string"), "Unknown built-in bidi string.");
> +    assertEq("", _("@@unknown_builtin_string"), "Unknown built-in string.");
> +    assertEq("", _("@@bidi_unknown_builtin_string"), "Unknown built-in bidi string.");

If we are changing the behaviour to set `lastError`, should we add a check for that in the test?

	Kris Maglione [:kmag] Assignee
	Comment 16 • 8 years ago

https://reviewboard.mozilla.org/r/44093/#review40657

> Why do we need this `if`? In what cases do we explicitly *not* want this to happen?

This only applies when this is coming from an extension API call. When we're being called from internal code, there's no extension scope that we can use.

> Is this how `lastError` is set? If not, I'm not sure what this `cloneScope` is all about.

No, `lastError` doesn't get set. This message is just being reported to the console. `cloneScope` is the extension global that the call is coming from, and we create an Error in that scope so the call site and stack trace reflect that.

> If we are changing the behaviour to set `lastError`, should we add a check for that in the test?

We're not changing the behavior of `lastError`. We are changing the error reporting behavior so missing keys are only reported once. We *should* test that, but it's easier said than done, and I don't think it's worth the effort for such a trivial feature at this point.

	Andrew Swan [:aswan]
	Comment 17 • 8 years ago

Comment on attachment 8737689 [details]
MozReview Request: Bug 1258199: [webext] Return an empty string for missing keys in getMessage. r?bsilverberg

https://reviewboard.mozilla.org/r/44093/#review40765

::: toolkit/components/extensions/ExtensionUtils.jsm:320
(Diff revision 1)
>  
>  function LocaleData(data) {
>    this.defaultLocale = data.defaultLocale;
>    this.selectedLocale = data.selectedLocale;
>    this.locales = data.locales || new Map();
> +  this.missingKeys = new Set();

nitpicky, but i would consider calling this something like warnedMissingKeys to indicate that its just used to decide whether to warn, nothing else.

	Kris Maglione [:kmag] Assignee
	Comment 18 • 8 years ago

https://reviewboard.mozilla.org/r/44093/#review40765

> nitpicky, but i would consider calling this something like warnedMissingKeys to indicate that its just used to decide whether to warn, nothing else.

Yeah, that makes sense.

	Kris Maglione [:kmag] Assignee
	Comment 19 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/c67309c62021bf173fc981634de54bc8e9c992ee
Bug 1258199: [webext] Return an empty string for missing keys in getMessage. r=aswan

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 8 years ago

I had to back this out for xpcshell test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8416552&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/b8eaba537717

	Kris Maglione [:kmag] Assignee
	Comment 21 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/0da6b852e66189ae34e5d4204924ae042994f75e
Bug 1258199: [webext] Return an empty string for missing keys in getMessage. r=aswan

	Carsten Book [:Tomcat]
	Comment 22 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0da6b852e661

	YF (Yang)
	Comment 23 • 7 years ago

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/i18n/getMessage

https://github.com/mdn/browser-compat-data/pull/313