Complete the implementation of chrome.i18n.getAcceptLanguages

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: andym, Assigned: bsilverberg, Mentored)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [i18n]triaged)

MozReview Requests

()

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 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

2 years ago
Blocks: 1214433
(Reporter)

Updated

2 years ago
Flags: blocking-webextensions+
(Reporter)

Updated

a year ago
Whiteboard: [i18n] → [i18n]triaged
(Reporter)

Updated

a year ago
Assignee: nobody → bob.silverberg
Mentor: kmaglione+bmo@mozilla.com
Priority: -- → P3
(Assignee)

Comment 1

a year ago
Created attachment 8717118 [details]
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/
Attachment #8717118 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

a year ago
Blocks: 1246748
(Assignee)

Updated

a year ago
No longer blocks: 1246748
(Assignee)

Updated

a year ago
Blocks: 1246749
(Assignee)

Comment 2

a year 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

a year ago
Created attachment 8717146 [details]
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/
Attachment #8717146 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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)

Comment 30

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d06bbdee624
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 31

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdf757c8a164
(Assignee)

Updated

a year ago
Iteration: --- → 47.3 - Mar 7

Comment 32

a year ago
bugherderlanding
https://hg.mozilla.org/integration/fx-team/rev/9466f6513f50
Keywords: checkin-needed
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

a year 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

a year ago
Created attachment 8722814 [details]
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/
Attachment #8722814 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 38

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=389634ff35bf
(Assignee)

Comment 39

a year 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

a year 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

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

Comment 42

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e67231f2bfa6
(Assignee)

Updated

a year 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

a year 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

a year ago
I believe the conflicts have been resolved.
Flags: needinfo?(bob.silverberg) → needinfo?(cbook)

Comment 46

a year ago
https://hg.mozilla.org/integration/fx-team/rev/b34dbc48da45
Keywords: checkin-needed
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

a year 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

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b34dbc48da45
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.