1246748 - Complete the implementation of chrome.i18n.getUILanguage

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

The Chrome implementation is documented at:

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

The signature is:

getUILanguage − string chrome.i18n.getUILanguage()

Comment 1

[Bob Silverberg [:bsilverberg]]

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

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: 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/

Comment 4

[Kris Maglione [:kmag]]

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()`

Comment 5

[Bob Silverberg [:bsilverberg]]

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.

Comment 6

[Bob Silverberg [:bsilverberg]]

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/

Comment 7

[Bob Silverberg [:bsilverberg]]

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/

Comment 8

[Bob Silverberg [:bsilverberg]]

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 9

[Kris Maglione [:kmag]]

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.

Comment 10

[Bob Silverberg [:bsilverberg]]

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/

Comment 11

[Bob Silverberg [:bsilverberg]]

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.

Comment 12

[Kris Maglione [:kmag]]

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");

Comment 13

[Kris Maglione [:kmag]]

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.

Comment 14

[Bob Silverberg [:bsilverberg]]

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/

Comment 15

[Bob Silverberg [:bsilverberg]]

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.

Comment 16

[Bob Silverberg [:bsilverberg]]

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.

Comment 17

[Bob Silverberg [:bsilverberg]]

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/

Comment 18

[Bob Silverberg [:bsilverberg]]

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/

Comment 19

[Bob Silverberg [:bsilverberg]]

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.

Comment 20

[Bob Silverberg [:bsilverberg]]

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/

Comment 21

[Bob Silverberg [:bsilverberg]]

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.

Comment 22

[Bob Silverberg [:bsilverberg]]

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?

Comment 23

[Kris Maglione [:kmag]]

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.

Comment 24

[Bob Silverberg [:bsilverberg]]

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/

Comment 25

[Bob Silverberg [:bsilverberg]]

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

Comment 26

[Bob Silverberg [:bsilverberg]]

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?

Comment 27

[Kris Maglione [:kmag]]

Looks like a mix of intermittent failures and infra issues. You should probably just push again.

Comment 28

[Bob Silverberg [:bsilverberg]]

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.

Comment 29

[Kris Maglione [:kmag]]

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.

Comment 30

[Bob Silverberg [:bsilverberg]]

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?

Comment 31

[Kris Maglione [:kmag]]

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.

Comment 32

[Bob Silverberg [:bsilverberg]]

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!

Comment 33

[Kris Maglione [:kmag]]

(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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aefcca55ff

Comment 35

[Carsten Book [:Tomcat]]

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