Complete the implementation of chrome.i18n.getAcceptLanguages

RESOLVED FIXED in Firefox 47

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: andy+bugzilla, Assigned: bsilverberg, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla47
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [i18n]triaged)

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 years ago
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)
Reporter

Updated

4 years ago
Blocks: webext
Reporter

Updated

4 years ago
Flags: blocking-webextensions+
Reporter

Updated

3 years ago
Whiteboard: [i18n] → [i18n]triaged
Reporter

Updated

3 years ago
Assignee: nobody → bob.silverberg
Mentor: kmaglione+bmo
Priority: -- → P3
Assignee

Updated

3 years ago
Blocks: 1246748
Assignee

Updated

3 years ago
No longer blocks: 1246748
Assignee

Updated

3 years ago
Blocks: 1246749
Assignee

Comment 2

3 years ago
Updated to only refer to getAcceptLanguages. Bug 1246749 has been opened to track all of the i18n methods needed.
Summary: Complete the implementation of chrome.i18n → Complete the implementation of chrome.i18n.getAcceptLanguages
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.
Attachment #8717118 - Flags: review?(kmaglione+bmo)
Assignee

Comment 4

3 years ago
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/
Attachment #8717146 - Flags: review?(kmaglione+bmo)
Assignee

Comment 5

3 years ago
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 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?
Attachment #8717146 - Flags: review?(kmaglione+bmo)
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.
Assignee

Comment 8

3 years ago
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.
Assignee

Comment 9

3 years ago
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/
Attachment #8717118 - Flags: review?(kmaglione+bmo)
Assignee

Updated

3 years ago
Attachment #8717146 - Attachment is obsolete: true
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.
Assignee

Comment 11

3 years ago
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?
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.
Assignee

Comment 13

3 years ago
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.
Assignee

Comment 14

3 years ago
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/
Assignee

Comment 15

3 years ago
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.
Attachment #8717118 - Flags: review?(kmaglione+bmo)
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.
Assignee

Comment 17

3 years ago
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/
Attachment #8717118 - Flags: review?(kmaglione+bmo)
Assignee

Comment 18

3 years ago
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.
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 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.
Attachment #8717118 - Flags: review?(kmaglione+bmo)
Assignee

Comment 21

3 years ago
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/
Attachment #8717118 - Flags: review?(kmaglione+bmo)
Assignee

Comment 22

3 years ago
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.
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.
Assignee

Comment 24

3 years ago
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.
(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.
Assignee

Comment 26

3 years ago
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/
Assignee

Comment 27

3 years ago
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 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.
Attachment #8717118 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 29

3 years ago
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/
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Updated

3 years ago
Iteration: --- → 47.3 - Mar 7
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.
Actually, every opt run and Linux64 debug, which doesn't fit any sort of sane pattern.
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.
Assignee

Comment 36

3 years ago
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/
Assignee

Comment 37

3 years ago
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/
Attachment #8722814 - Flags: review?(kmaglione+bmo)
Assignee

Comment 39

3 years ago
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.
https://reviewboard.mozilla.org/r/34049/#review32811

Can you squash these into a single commit?
Assignee

Comment 41

3 years ago
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/
Assignee

Updated

3 years ago
Attachment #8722814 - Attachment is obsolete: true
Attachment #8722814 - Flags: review?(kmaglione+bmo)
Assignee

Updated

3 years ago
Keywords: checkin-needed
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
Flags: needinfo?(bob.silverberg)
Assignee

Comment 44

3 years ago
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/
Assignee

Comment 45

3 years ago
I believe the conflicts have been resolved.
Flags: needinfo?(bob.silverberg) → needinfo?(cbook)
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.
Assignee

Comment 48

3 years ago
I am going to ask Kris to comment on that. I know we looked at that code when implementing this.
Flags: needinfo?(kmaglione+bmo)
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"]
Assignee

Comment 50

3 years ago
Hmm, I wonder if that is a bug in the API docs or in Chrome's code? What is the "correct" behaviour?
Flags: needinfo?(cbook)
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 '-'.
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.
Flags: needinfo?(kmaglione+bmo)
Assignee

Comment 53

3 years ago
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?
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.
Blocks: 1251289

Comment 55

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b34dbc48da45
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.