Closed Bug 1213450 Opened 9 years ago Closed 8 years ago

Complete the implementation of chrome.i18n.getAcceptLanguages

Categories

(WebExtensions :: Untriaged, defect, P3)

Product:
WebExtensions
Component:
Untriaged
Type:
defect

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla47
Iteration:
47.3 - Mar 7
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

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

References

(Blocks 1 open bug)

Details

(Whiteboard: [i18n]triaged)

Attachments

(1 file, 2 obsolete files)

MozReview Request: Bug 1213450 - Complete the implementation of chrome.i18n - getAcceptLanguages, r?kmag
58 bytes, text/x-review-board-request
kmag
: review+
Details 
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)
Blocks: webext
Flags: blocking-webextensions+
Whiteboard: [i18n] → [i18n]triaged
Assignee: nobody → bob.silverberg
Mentor: kmaglione+bmo
Priority: -- → P3
Blocks: 1246748
No longer blocks: 1246748
Blocks: 1246749
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)
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)
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.
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 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)
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.
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.
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 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/
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.
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)
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)
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)
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.
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.
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/
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+
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/
Keywords: checkin-needed
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.
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/
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)
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?
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/
Attachment #8722814 - Attachment is obsolete: true
Attachment #8722814 - Flags: review?(kmaglione+bmo)
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)
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/
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.
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"]
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)
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
https://hg.mozilla.org/mozilla-central/rev/b34dbc48da45
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: