Closed
Bug 1246748
Opened 9 years ago
Closed 9 years ago
Complete the implementation of chrome.i18n.getUILanguage
Categories
(WebExtensions :: Untriaged, defect, P3)
WebExtensions
Untriaged
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: bsilverberg, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [i18n]triaged)
Attachments
(1 file)
The Chrome implementation is documented at:
https://developer.chrome.com/extensions/i18n#method-getUILanguage
The signature is:
getUILanguage − string chrome.i18n.getUILanguage()
Assignee | ||
Comment 1•9 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
Comment 2•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
Flags: blocking-webextensions+
Comment 4•9 years ago
|
||
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•9 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•9 years ago
|
Attachment #8717401 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 6•9 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•9 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•9 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 9•9 years ago
|
||
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•9 years ago
|
Attachment #8717401 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 10•9 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•9 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.
Comment 12•9 years ago
|
||
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•9 years ago
|
Attachment #8717401 -
Flags: review?(kmaglione+bmo)
Comment 13•9 years ago
|
||
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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
Attachment #8717401 -
Flags: review?(kmaglione+bmo) → review+
Comment 23•9 years ago
|
||
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•9 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•9 years ago
|
||
Assignee | ||
Comment 26•9 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?
Comment 27•9 years ago
|
||
Looks like a mix of intermittent failures and infra issues. You should probably just push again.
Assignee | ||
Comment 28•9 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.
Comment 29•9 years ago
|
||
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•9 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?
Comment 31•9 years ago
|
||
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•9 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
Comment 33•9 years ago
|
||
(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•9 years ago
|
||
Keywords: checkin-needed
Comment 35•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•