1213450 - Complete the implementation of chrome.i18n.getAcceptLanguages

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Andy McKay]

The methods on the API are:

https://developer.chrome.com/extensions/i18n

Currently implemented:

getMessage

To be implemented:


Types

LanguageCode

Methods

getAcceptLanguages − chrome.i18n.getAcceptLanguages(function callback)
getUILanguage − string chrome.i18n.getUILanguage()
detectLanguage − chrome.i18n.detectLanguage(string text, function callback)

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

Implement browser.i18n.getAcceptLanguages() including tests

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

Comment 2

[Bob Silverberg [:bsilverberg]]

Updated to only refer to getAcceptLanguages. Bug 1246749 has been opened to track all of the i18n methods needed.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

https://reviewboard.mozilla.org/r/34049/#review30713

Looks pretty good. Just a few minor issues to fix.

Thanks

::: toolkit/components/extensions/ext-i18n.js:5
(Diff revision 1)
> +                                   "nsIPrefService");

This is already available as `Services.prefs`. However, these days we use Preferences.jsm[1] in new code.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.jsm

::: toolkit/components/extensions/ext-i18n.js:24
(Diff revision 1)
> +        runSafe(context, callback, result);

We don't deal with callbacks directly anymore. Just return a Promise that resolves to the result:

  return Promise.resolve(result);

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:168
(Diff revision 1)
> +  SpecialPowers.setCharPref("intl.accept_languages", "en-US, en, fr-CA, fr");

Please check against the default value before making any changes.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:177
(Diff revision 1)
> +        });

We need to check that the lengths of the arrays are the same too.

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

Address review comments
- Use Preferences.jsm
- return a Promise instead of calling runSafe()
- Check lengths of arrays in tests

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

Comment 5

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review30713

> Please check against the default value before making any changes.

I'm not sure what you mean. What exactly should I check? That the value I am setting is not exactly the same as the default?

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8717146 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

https://reviewboard.mozilla.org/r/34063/#review30745

Can you squash these into a single commit?

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34049/#review30741

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:178
(Diff revision 1)
> +        SpecialPowers.clearUserPref("intl.accept_languages");

Oh, and you can't call this from a background script. SpecialPowers isn't defined in that scope.

Comment 8

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review30741

> Oh, and you can't call this from a background script. SpecialPowers isn't defined in that scope.

It seems to work. It doesn't throw an exception, and if I remove that line the next test fails because it sees the pref as set in this test. I would expect neither of those to be true if the call wasn't working.

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 10

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34049/#review30713

> I'm not sure what you mean. What exactly should I check? That the value I am setting is not exactly the same as the default?

Your task that checks the default value comes after the one that checks a custom value. The tests just need to come in the opposite order.

Comment 11

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review30713

> Your task that checks the default value comes after the one that checks a custom value. The tests just need to come in the opposite order.

I actually did that intentionally, to prove that the resetting of the pref works. If I change the order then we don't really know whether the pref reset or not, because the 2nd test passes with the custom values. Also, by doing the tests in this order we prove that we are getting the default value in `test_get_accept_languages_default` because that test would fail if we were not, as we would be getting the four languages that were set in the previous test. So this makes sense to me. How would it be better to run the default test first?

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34049/#review30713

> I actually did that intentionally, to prove that the resetting of the pref works. If I change the order then we don't really know whether the pref reset or not, because the 2nd test passes with the custom values. Also, by doing the tests in this order we prove that we are getting the default value in `test_get_accept_languages_default` because that test would fail if we were not, as we would be getting the four languages that were set in the previous test. So this makes sense to me. How would it be better to run the default test first?

I guess you could make an argument either way. In general, I'd rather test before we make any kind of changes to the pref. But I guess it's possible that another test could have changed and failed to restore the value before our test runs.

Comment 13

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review30713

> I guess you could make an argument either way. In general, I'd rather test before we make any kind of changes to the pref. But I guess it's possible that another test could have changed and failed to restore the value before our test runs.

That was my concern, kmag. I feel safer with the tests in the current order as we know that if the pref fails to restore that our tests will fail. I'm therefore going to drop this issue, but I'm perfectly happy to change the order if you'd rather keep the consistency of checking defaults first. Just re-open if that is the case.

Comment 14

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 15

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34047/#review31049

::: toolkit/components/extensions/ext-i18n.js:12
(Diff revision 4)
> +      getAcceptLanguages: function() {

I removed the callback argument from the getAcceptLanguages method as I believe it is now unneccesary.

Comment 16

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34049/#review31097

Sorry I didn't notice this before, but this is another function that needs to be available to content scripts as well. We should probably use the same strategy as for getUILanguage, and just add a getter to `LocaleData`, and use that for both APIs.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:179
(Diff revision 3)
> +        SpecialPowers.clearUserPref("intl.accept_languages");

This still needs to be moved outside of the background script.

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 18

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review31097

Ok, I did this in the same way that I dealt with `getUILanguage`.

> This still needs to be moved outside of the background script.

I understand you want me to move the `clearUserPref` out of the background script, but two things:
1. It works.
2. I need it now with the refactor because otherwise the content script test picks up the changed pref.

Of course you may not like my refactor that combines everything into a single task, and if that needs to be changed I think I'll be able to move this statement out of the background script.

Comment 19

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34049/#review31097

> I understand you want me to move the `clearUserPref` out of the background script, but two things:
> 1. It works.
> 2. I need it now with the refactor because otherwise the content script test picks up the changed pref.
> 
> Of course you may not like my refactor that combines everything into a single task, and if that needs to be changed I think I'll be able to move this statement out of the background script.

It works now, yes, but it may not always work. None of our other tests currently use SpecialPowers in extension scripts, and I'd rather not change that unless it makes things *much* simpler.

In the case of this test, there are ways of doing it just as simply without resorting to this, e.g.: https://gist.github.com/kmaglione/f11f655f126e6e675c4f

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

https://reviewboard.mozilla.org/r/34049/#review31481

::: toolkit/components/extensions/Extension.jsm:646
(Diff revision 4)
> +  get acceptLanguages() {

This isn't used.

::: toolkit/components/extensions/ExtensionContent.jsm:121
(Diff revision 4)
> +      getAcceptLanguages: function(callback) {

Please add a blank line.

::: toolkit/components/extensions/ExtensionContent.jsm:618
(Diff revision 4)
> +  get acceptLanguages() {

This isn't used.

Comment 21

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 22

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review31097

> It works now, yes, but it may not always work. None of our other tests currently use SpecialPowers in extension scripts, and I'd rather not change that unless it makes things *much* simpler.
> 
> In the case of this test, there are ways of doing it just as simply without resorting to this, e.g.: https://gist.github.com/kmaglione/f11f655f126e6e675c4f

Thanks Kris. I addressed your other issues. Sorry for missing the removal of those getters. I will attempt to do something similar to your gist for this test. I'm still trying to get my head around your example. Also, the example you gave is for `getUILanguage`, which is fine, I can use it for that, but this one involves a callback so I'm going to have to figure out how to work that into your example.

Comment 23

[Kris Maglione [:kmag]]

Right, it was just an example. The callback shouldn't make it much different. Ideally you'd just use promises, but the sendMessage API is one of the few callback-based APIs that doesn't support promises yet. So it would go something like this, instead:

  function background(getResults) {
    ...

    Promise.all([
      new Promise(resolve => browser.tabs.sendMessage(tabId, "get-results", resolve)),
      browser.i18n.getAcceptLanguages(),
    ]).then(([contentResults, backgroundResults]) => {
        checkResults("contentScript", result, expected);
        checkResults("background", getResults(), expected);

        browser.test.sendMessage("done");
    });
    ...
  }

  function content(getResults) {
    browser.runtime.onMessage.addListener((msg, sender, respond) => {
      browser.i18n.getAcceptLanguages(respond);
      return;
    });
  }

If there's anything in particular you're having trouble understanding, feel free to ask.

I know the sendMessage and response logic is a bit hairy. To send a message to a content script, you have to send it to the tab that the content script lives in, and then all of your content scripts running in that tab receive a copy of the message.

The listeners also get a chance to send a response. Their third argument is a function that they can call if they want to send an explicit response, which is delivered to the callback argument passed to sendMessage in the background script. If they want to send a response after the listener function returns (as in the example above), they need to return `true`. Otherwise, an empty response is sent back immediately.

Comment 24

[Bob Silverberg [:bsilverberg]]

Thanks Kris. I am struggling a bit to figure this out, but I am trying. Here are a couple of specific questions about your suggestion:

In your sample code, 
> 
>   function background(getResults) {
>     ...
> 
>     Promise.all([
>       new Promise(resolve => browser.tabs.sendMessage(tabId, "get-results",
> resolve)),
>       browser.i18n.getAcceptLanguages(),
>     ]).then(([contentResults, backgroundResults]) => {
>         checkResults("contentScript", result, expected);
>         checkResults("background", getResults(), expected);
> 
>         browser.test.sendMessage("done");
>     });
>     ...
>   }

I am getting an error on the `sendMessage` because `tabId` is undefined. I believe this is because this `Promise.all()` code runs before the code that sets the `tabId`:

    browser.tabs.query({currentWindow: true, active: true}, tabs => {
      tabId = tabs[0].id;
      browser.test.sendMessage("ready");
    });

I am guessing this is because the `Promise.all()` is not waiting for the "ready" message. I tried to find a version of `awaitMessage` that I could add to the `Promise.all()` but the only examples I can find use `extension.awaitMessage` and `extension` does not seem to be available in this `background` code block. How else can I ensure that `tabId` is set when this code is called?

My second question is regarding what I would actually put into `getResults` and `checkResults` considering that I am now dealing with a Promise and not a simple value. Would `getResults` be as simple as:

  function getResults() {
    return browser.i18n.getAcceptLanguages();
  }

Would `checkResults` receive a value, or a Promise?

I guess I'm still having trouble getting my head around this Promise stuff - how they are passed around, how they are used, etc.

I think I can continue to experiment with this if I can get past the `tabId` issue.

Comment 25

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #24)
> Thanks Kris. I am struggling a bit to figure this out, but I am trying. Here
> are a couple of specific questions about your suggestion:
> 
> In your sample code, 
> > 
> >   function background(getResults) {
> >     ...
> > 
> >     Promise.all([
> >       new Promise(resolve => browser.tabs.sendMessage(tabId, "get-results",
> > resolve)),
> >       browser.i18n.getAcceptLanguages(),
> >     ]).then(([contentResults, backgroundResults]) => {
> >         checkResults("contentScript", result, expected);
> >         checkResults("background", getResults(), expected);
> > 
> >         browser.test.sendMessage("done");
> >     });
> >     ...
> >   }
> 
> I am getting an error on the `sendMessage` because `tabId` is undefined. I
> believe this is because this `Promise.all()` code runs before the code that
> sets the `tabId`:

Yeah, you'd have to run it from something like the "expect-results" message
listener in the original version.

> I am guessing this is because the `Promise.all()` is not waiting for the
> "ready" message. I tried to find a version of `awaitMessage` that I could
> add to the `Promise.all()`

Unfortunately, we don't have anything like that on the extension side. That
part of the API is based on the Chrome test API, which has no support for
promises. You just have to use `browser.test.onMessage.addListener`.

> My second question is regarding what I would actually put into `getResults`
> and `checkResults` considering that I am now dealing with a Promise and not
> a simple value. Would `getResults` be as simple as:
> 
>   function getResults() {
>     return browser.i18n.getAcceptLanguages();
>   }

Yeah, getResults shouldn't even be necessary. I meant to remove it from the
code above. It should just be something like:

     ]).then(([contentResults, backgroundResults]) => {
         checkResults("contentScript", contentResults, expected);
         checkResults("background", backgroundResults, expected);

> I guess I'm still having trouble getting my head around this Promise stuff -
> how they are passed around, how they are used, etc.

Yeah, it takes some getting used to, but we use them everywhere, so it's worth
the effort.

Comment 26

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 27

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/34049/#review31097

> Thanks Kris. I addressed your other issues. Sorry for missing the removal of those getters. I will attempt to do something similar to your gist for this test. I'm still trying to get my head around your example. Also, the example you gave is for `getUILanguage`, which is fine, I can use it for that, but this one involves a callback so I'm going to have to figure out how to work that into your example.

I do believe I finally figured it out.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

https://reviewboard.mozilla.org/r/34049/#review32023

::: toolkit/components/extensions/ExtensionContent.jsm:618
(Diff revision 6)
> +

Remove empty line.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:173
(Diff revision 6)
> +      results.map((lang, index) => {

Please use `.forEach` or a for-of loop rather than `.map` when you don't need the result array.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:192
(Diff revision 6)
> +          browser.i18n.getAcceptLanguages(),

This is indented too far.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:194
(Diff revision 6)
> +          checkResults("contentScript", contentResults, expected);

Two space indent please.

Comment 29

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 30

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d06bbdee624

Comment 31

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf757c8a164

Comment 32

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

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

Comment 33

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/93a1dd50eb45 - not sure what changed between your try push's parent and landing, but what you got was https://treeherder.mozilla.org/logviewer.html#?job_id=7497517&repo=fx-team and curiously it looks like only on opt.

Comment 34

[Phil Ringnalda (:philor)]

Actually, every opt run and Linux64 debug, which doesn't fit any sort of sane pattern.

Comment 35

[Phil Ringnalda (:philor)]

Apparently I don't understand the concept of time, in particular how debug builds running more slowly means that they will fail *after* faster-running opt builds. No mysterious pattern, it just times out everywhere.

Comment 36

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 37

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n.getAcceptLanguages, r?kmag

Rebase against fx-team, resolving some conflicts
Fix eslint errors/warnings

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

Comment 38

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=389634ff35bf

Comment 39

[Bob Silverberg [:bsilverberg]]

I'm not sure what happened either. I rebased against fx-team and did encounter some conflicts. I also fixed some eslint issues. Pushed to try again.

Comment 40

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/34049/#review32811

Can you squash these into a single commit?

Comment 41

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 42

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e67231f2bfa6

Comment 43

[Carsten Book [:Tomcat]]

has conflicts:

patching file toolkit/components/extensions/ExtensionContent.jsm
Hunk #1 FAILED at 113
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/ExtensionContent.jsm.rej
patching file toolkit/components/extensions/ExtensionUtils.jsm
Hunk #1 FAILED at 10
Hunk #2 FAILED at 422
2 out of 2 hunks FAILED -- saving rejects to file toolkit/components/extensions/ExtensionUtils.jsm.rej
patching file toolkit/components/extensions/ext-i18n.js
Hunk #1 FAILED at 1
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/ext-i18n.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 44

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8717118 [details]
MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag

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

Comment 45

[Bob Silverberg [:bsilverberg]]

I believe the conflicts have been resolved.

Comment 46

[Pulsebot]

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

Comment 47

[Axel Hecht [:Pike]]

Is there a reason why we're not using navigator.languages here?

Its implementation in https://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#445 sanitizes the pref values a lot more.

Comment 48

[Bob Silverberg [:bsilverberg]]

I am going to ask Kris to comment on that. I know we looked at that code when implementing this.

Comment 49

[Axel Hecht [:Pike]]

And it seems that the underscoring in the chrome api examples is wrong:

I played in Canary, and got:

chrome.i18n.getAcceptLanguages(console.log.bind(console))

 ["en-US", "en"]

Comment 50

[Bob Silverberg [:bsilverberg]]

Hmm, I wonder if that is a bug in the API docs or in Chrome's code? What is the "correct" behaviour?

Comment 51

[Axel Hecht [:Pike]]

I guess that code is always right, when it comes to chrome apis? They don't reference anything but code in their api docs, sadly.

The internet uses BCP 47 language codes, and they're using '-'.
glibc uses POSIX, which uses '_' (and '@').
Unicode allows both '-' and '_', but no '@'.

My personal take would be to use '-'.

Comment 52

[Kris Maglione [:kmag]]

Chrome uses _ in most places. I suppose this API is an exception.

As for why we don't use `navigator.languages`, it's mainly for the sake of content scripts, where the only Navigator instance we have available belongs to a different principal than the extension script.

As for the mangling of the pref, Navigator.cpp seems to be the only code that does that. All other code that uses it assumes the pref value is sane, and the comments in Navigator.cpp says it should probably do the same.

Comment 53

[Bob Silverberg [:bsilverberg]]

Thanks Kris. So does this mean that we are going to leave the code for getAcceptLanguages as is, i.e., converting `-` to `_`, or should I change it to match Chrome's behaviour?

Comment 54

[Kris Maglione [:kmag]]

If Chrome uses - here, we should too. It makes more sense, anyway, given that this is supposed to match the values we send in the HTTP header.

Comment 55

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

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