Complete the implementation of chrome.i18n.getUILanguage

RESOLVED FIXED in Firefox 47

Status

()

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

People

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

(Assignee)

Description

2 years ago
The Chrome implementation is documented at:

https://developer.chrome.com/extensions/i18n#method-getUILanguage

The signature is:

getUILanguage − string chrome.i18n.getUILanguage()
(Assignee)

Updated

2 years ago
Assignee: nobody → bob.silverberg
No longer depends on: 1213450
(Assignee)

Updated

2 years ago
Blocks: 1246749
(Assignee)

Updated

2 years ago
Blocks: 1246754
(Assignee)

Updated

2 years ago
No longer blocks: 1246754
(Assignee)

Comment 1

2 years ago
It looks like this should be implemented by simply returning the first language returned by `browser.i18n.getAcceptLanguages`, as per [1].

The Chrome doc [2] states:
"Gets the browser UI language of the browser. This is different from i18n.getAcceptLanguages which returns the preferred user languages."

Also, MDN mentions in a footnote [3]:
That Chrome "Returns the browser UI language, not the value of the Accept-Language HTTP header."

[2] and [3] seem to suggest that if we follow [1] we may not be implementing the same thing as Chrome, but perhaps that is still what we want to do.

Does anyone have any comments about the above, and what our implementation should be?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#494
[2] https://developer.chrome.com/extensions/i18n#method-getUILanguage
[3] https://developer.mozilla.org/en-US/docs/Web/API/NavigatorLanguage/language
navigator.language intentionally lies about the UI language. Extensions don't have the same privacy restrictions as web pages.

We need to return the same value as the @@ui_locale translation key here. It probably wouldn't hurt to move that logic to a method of the LocaleData class.
(Assignee)

Comment 3

2 years ago
Created attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Implement chrome.i18n.getUILanguage including a test

Review commit: https://reviewboard.mozilla.org/r/34165/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34165/
Attachment #8717401 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Flags: blocking-webextensions+
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

https://reviewboard.mozilla.org/r/34165/#review30895

Looks pretty good, but it's missing a few pieces, and I think we should probably generalize it a bit more.

::: toolkit/components/extensions/ext-i18n.js:10
(Diff revision 1)
> +        return extension.localizeMessage("@@ui_locale");

This works, but let's add a new getter to the `LocaleData` class, and use that for both the locale string and this method. From here, it would be accessible as `extension.localeData.uiLocale`.

Also, we need to add this to the content script API as well, in ExtensionContent.jsm (and add tests for that too).

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:167
(Diff revision 1)
> +    let expectedLang = "en_US";

We should test with non-default locales, too. And we should probably check that it matches `getLanguage("@@ui_locale")`. See [1] for an example of how to do this (note that we don't need to change the locale direction for this test).

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_i18n_css.html#96

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:169
(Diff revision 1)
> +    browser.test.assertEq(expectedLang,

Please either allign the following lines with the opening `(` or move the first argument to the next line.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:176
(Diff revision 1)
> +    background: "(" + backgroundScript.toString() + ")()",

No need for `.toString()`
Attachment #8717401 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/34165/#review30895

> This works, but let's add a new getter to the `LocaleData` class, and use that for both the locale string and this method. From here, it would be accessible as `extension.localeData.uiLocale`.
> 
> Also, we need to add this to the content script API as well, in ExtensionContent.jsm (and add tests for that too).

I am working on this, and I started by looking at exposing `localeData` in `extension` so that I would end up with `extension.localeData.uiLocale` as you suggest, but I'm a bit confused by that. `localeData` is not currently exposed in `extension`, and I'm not sure why we want to do that. When I tried to simply pass `this.localeData` out of `extension` I got errors about "Permission denied to pass object to exported function". I decided to take a different route, which I can certainly change once I understand exactly what we want, which is to simply expose `extension.uiLocale` as a property. I will add my changes thus far to the review. Note that I have not addressed the content script part yet.
(Assignee)

Updated

2 years ago
Attachment #8717401 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 6

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/1-2/
(Assignee)

Comment 7

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/2-3/
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/34165/#review30895

> I am working on this, and I started by looking at exposing `localeData` in `extension` so that I would end up with `extension.localeData.uiLocale` as you suggest, but I'm a bit confused by that. `localeData` is not currently exposed in `extension`, and I'm not sure why we want to do that. When I tried to simply pass `this.localeData` out of `extension` I got errors about "Permission denied to pass object to exported function". I decided to take a different route, which I can certainly change once I understand exactly what we want, which is to simply expose `extension.uiLocale` as a property. I will add my changes thus far to the review. Note that I have not addressed the content script part yet.

I have now added this to the content script API as well, and included a test. It seemed like one test was sufficient, as all we are testing is the use of this method from content scripts. The functionality of the method itself is tested via the tests for the API as implemented in `ext-i18n.js` (or at least I think so).
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

https://reviewboard.mozilla.org/r/34165/#review31103

This is very close. I think it just needs some minor test refactoring to make sure everything's consistent across locales (especially in content scripts).

::: toolkit/components/extensions/Extension.jsm:647
(Diff revision 3)
> +    return this.localeData.uiLocale;

I'm not sure we need this getter. I'd rather just access it through `localeData`.

::: toolkit/components/extensions/ExtensionContent.jsm:120
(Diff revision 3)
>        },

Please add a blank line here.

::: toolkit/components/extensions/ext-i18n.js:9
(Diff revision 3)
> +      getUILanguage: function() {

Please add a blank line here too.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:170
(Diff revision 3)
> +  function backgroundScript() {

I believe ESLint will complain about the unnecessary blank line at the start of a block.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:176
(Diff revision 3)
> +    browser.test.assertEq(browser.i18n.getMessage("@@ui_locale"),

Let's test this against `expectedLang` too. `assertEq` reports the first argument as the expected value, so if this test winds up failing, the results will be confusing.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:225
(Diff revision 3)
> +add_task(function* test_get_ui_language_content_script() {

I think it would be good to test the locale from the content script in the same test as we check the background script.

And, ideally, we should move extract the common code from the 'en-US' and 'he' tests to a helper function so we get the same tests for both.
Attachment #8717401 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8717401 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 10

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/3-4/
(Assignee)

Comment 11

2 years ago
https://reviewboard.mozilla.org/r/34165/#review31103

> I'm not sure we need this getter. I'd rather just access it through `localeData`.

I also removed the getter in `ExtensionContent.jsm`.

> Let's test this against `expectedLang` too. `assertEq` reports the first argument as the expected value, so if this test winds up failing, the results will be confusing.

I'm not sure what you mean by "Let's test this against expectedLang too." Do you want a third assert like `assertEq(expectedLang, browser.i18n.getMessage("@@ui_locale"))`, inserted at line 176?

I added this assert based on an earlier comment of yours because I thought you wanted me to check the value of `getUILanguage()` against that of `@@ui_locale`. I realize it's a bit odd, but in this test `browser.i18n.getMessage("@@ui_locale")` _is_ the expected value. Maybe there's a better way for me to get the value of `@@ui_locale`?

> I think it would be good to test the locale from the content script in the same test as we check the background script.
> 
> And, ideally, we should move extract the common code from the 'en-US' and 'he' tests to a helper function so we get the same tests for both.

I will work on these next.
https://reviewboard.mozilla.org/r/34165/#review31103

> I'm not sure what you mean by "Let's test this against expectedLang too." Do you want a third assert like `assertEq(expectedLang, browser.i18n.getMessage("@@ui_locale"))`, inserted at line 176?
> 
> I added this assert based on an earlier comment of yours because I thought you wanted me to check the value of `getUILanguage()` against that of `@@ui_locale`. I realize it's a bit odd, but in this test `browser.i18n.getMessage("@@ui_locale")` _is_ the expected value. Maybe there's a better way for me to get the value of `@@ui_locale`?

browser.test.assertEq(expectedLang,
                          browser.i18n.getUILanguage(),
                          "returned language should be the expected default");
    browser.test.assertEq(expectedLang,
                          browser.i18n.getMessage("@@ui_locale"),
                          "returned language should be the expected default");

Updated

2 years ago
Attachment #8717401 - Flags: review?(kmaglione+bmo)
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

https://reviewboard.mozilla.org/r/34165/#review31129

::: toolkit/components/extensions/Extension.jsm:648
(Diff revision 4)
> +  //},

Please remove rather than commenting out.

::: toolkit/components/extensions/ExtensionContent.jsm:126
(Diff revision 4)
> +

No blank line here, please.

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

Remove blank line.

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:238
(Diff revision 4)
> +    browser.runtime.onMessage.addListener(function() {});

This line isn't necessary.
(Assignee)

Comment 14

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/4-5/
Attachment #8717401 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 15

2 years ago
https://reviewboard.mozilla.org/r/34165/#review31103

> browser.test.assertEq(expectedLang,
>                           browser.i18n.getUILanguage(),
>                           "returned language should be the expected default");
>     browser.test.assertEq(expectedLang,
>                           browser.i18n.getMessage("@@ui_locale"),
>                           "returned language should be the expected default");

Thanks for clarifying, Kris.
(Assignee)

Comment 16

2 years ago
https://reviewboard.mozilla.org/r/34165/#review31129

> Please remove rather than commenting out.

Sorry, I did that suring development and forgot to go back and remove it entirely.
(Assignee)

Comment 17

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/5-6/
(Assignee)

Comment 18

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/6-7/
(Assignee)

Comment 19

2 years ago
https://reviewboard.mozilla.org/r/34165/#review31103

> I will work on these next.

I combined the tests for content script and background script and did some refactoring to remove duplicate code/logic. I ended up doing this by combining all the tests into one task - I'm not sure if that's what you intended. I wasn't sure how to create a common function at a higher level and then have it injected into background scripts in different tasks.
(Assignee)

Comment 20

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/7-8/
(Assignee)

Comment 21

2 years ago
https://reviewboard.mozilla.org/r/34165/#review31677

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:169
(Diff revisions 7 - 8)
> -
> +      getUILanguage: browser.i18n.getUILanguage(),

Kris, I updated the test to use the newer format you suggested in the review for the `getAcceptLanguages` bug. I do not think there are any outstanding issues on this one now.
(Assignee)

Comment 22

2 years ago
I think this is nearly ready to land. Should I push it to try (which I just learned how to do this morning)? Which tests should I request be run to cover Web Extension stuff?

Updated

2 years ago
Attachment #8717401 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

https://reviewboard.mozilla.org/r/34165/#review32021

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:177
(Diff revision 8)
> +          results.getUILanguage,

Please either line all arguments up with the opening `(`, or move the first argument to the next line and indent by 2 spaces.
(Assignee)

Comment 24

2 years ago
Comment on attachment 8717401 [details]
MozReview Request: Bug 1246748 - Complete the implementation of chrome.i18n.getUILanguage, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34165/diff/8-9/
(Assignee)

Comment 25

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2fe41b5c378
(Assignee)

Comment 26

2 years ago
I submitted a try push for this, but it looks like there were tons of failures/issues. I cannot believe that most (if any) of these were the result of my patch, and I think my push command was accurate. Any ideas as to what went wrong?
Looks like a mix of intermittent failures and infra issues. You should probably just push again.
(Assignee)

Comment 28

2 years ago
I pushed to try again this afternoon, but it never updated the bug. Here's the link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5dd8625dbc5. It looks odd to me. I don't even see my commit in there.
That's normal. Try only shows commits it didn't know about before the push. Since you'd already pushed that commit before, it only shows the temporary commit with the try command.
(Assignee)

Comment 30

2 years ago
Thanks Kris. I think the try looks good. There is a bunch of orange stuff in there, but maybe that's ok? Is this good to land now? If so, do I just add `checkin-needed` to it?
Yeah, it's all imtermittents. The gl failures have been happening to everyone for days.

I think you can just push the autoland button in reviewboard? If not, yeah, just add checkin-needed.
(Assignee)

Comment 32

2 years ago
Do you mean the menu item `Automation -> Land Commits`? If so, that option is not available to me and presents me with the message `You must have scm_level_3 access to land`. Perhaps I should have different access? For now I'll just add checkin-needed.

Thanks for all of the reviews and help with this patch!
Keywords: checkin-needed
(In reply to Bob Silverberg [:bsilverberg] from comment #32)
> Do you mean the menu item `Automation -> Land Commits`? If so, that option
> is not available to me and presents me with the message `You must have
> scm_level_3 access to land`.

Oh, I thought they'd fixed that. Well, I can push it for you, anyway.

> Perhaps I should have different access?

Eventually, yes. For now you should be fine with level 1, though.

Comment 34

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aefcca55ff
Keywords: checkin-needed

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7aefcca55ff
Status: NEW → RESOLVED
Last Resolved: 2 years 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.