i18n getMessage with missing key

VERIFIED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: glen.little, Assigned: kmag)

Tracking

({addon-compat, dev-doc-complete})

47 Branch
mozilla48
addon-compat, dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 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.
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.
Assignee: nobody → kmaglione+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 2

2 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!
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?
(Reporter)

Comment 4

2 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!
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.
(Reporter)

Comment 6

2 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!
(Reporter)

Comment 7

2 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!

:)
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?
(Reporter)

Comment 9

2 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!
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.
(Reporter)

Comment 11

2 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!
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.
(Reporter)

Comment 13

2 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!
Created attachment 8737689 [details]
MozReview Request: Bug 1258199: [webext] Return an empty string for missing keys in getMessage. r?bsilverberg

Review commit: https://reviewboard.mozilla.org/r/44093/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44093/
Attachment #8737689 - Flags: review?(bob.silverberg)
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?
Attachment #8737689 - Flags: review?(bob.silverberg)
Iteration: --- → 48.3 - Apr 18
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.
Attachment #8737689 - Flags: review?(aswan)
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.
Attachment #8737689 - Flags: review?(aswan) → review+
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.
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
Flags: needinfo?(kmaglione+bmo)

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0da6b852e661
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(kmaglione+bmo)

Comment 23

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

https://github.com/mdn/browser-compat-data/pull/313
Status: RESOLVED → VERIFIED
Keywords: addon-compat, dev-doc-complete
You need to log in before you can comment on or make changes to this bug.