Complete the implementation of chrome.i18n.detectLanguage

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-detectLanguage

The signature is:

detectLanguage − chrome.i18n.detectLanguage(string text, function callback)
(Assignee)

Updated

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

Updated

2 years ago
Summary: Complete the implementation of chrome.i18n.getUILanguage.detectLanguage → Complete the implementation of chrome.i18n.detectLanguage
(Assignee)

Comment 1

2 years ago
I found `LanguageDetector.detectLanguage`() [1] and I've been experimenting with it, but I have a concern.

The Chrome docs [2] suggest that detectLanguage() should return an object that includes a boolean indicating whether the detection is reliable, plus an array of languages, each with an integer representing its percentage within the string. `LanguageDetector.detectLanguage` returns an object with the boolean `confident` which does correspond to the `isReliable` in the Chrome docs but that is accompanied by a simple `language` string that contains a single detected language. This does not conform to the API as described in [2].

How do we want to approach this? Do we have a facility in our codebase to deliver the results as requested by the API?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/translation/LanguageDetector.jsm#46
[2] https://developer.chrome.com/extensions/i18n#method-detectLanguage
LanguageDetector.jsm actually uses the same language detection code as Chromium, so this should be doable.

We currently use the simplest API that the library provides:

https://github.com/CLD2Owners/cld2/blob/master/public/compact_lang_det.h#L177

What we probably need is:

https://github.com/CLD2Owners/cld2/blob/master/public/compact_lang_det.h#L183

Which means we need to add a new binding stub here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/translation/cld2/cldapp.cc

And add a new function to LanguageDetector.jsm to access it externally.

I can handle that part, if you want.
(Assignee)

Comment 3

2 years ago
Thanks Kris. I would definitely appreciate you taking care of the binding stub. The jsm looks like it might also be beyond my current skills, but I'd be willing to try it with some advice/assistance. I'll leave that up to you.

I am working on the code for the API method now, using LanguageDetector.detectLanguage, and I think I'll be able to reuse most of that for the new method, so I will keep on with that.
Sounds good. I'll try to get to it today. If not, sometime this week.
(Assignee)

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Depends on: 1248501
(Assignee)

Updated

2 years ago
Iteration: --- → 47.3 - Mar 7
(Assignee)

Comment 5

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

Review commit: https://reviewboard.mozilla.org/r/35869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35869/
Attachment #8722126 - Flags: review?(kmaglione+bmo)
Comment on attachment 8722126 [details]
MozReview Request: Bug 1246754 - Complete the implementation of chrome.i18n.detectLanguage, r?kmag

https://reviewboard.mozilla.org/r/35869/#review32511

::: toolkit/components/extensions/ext-i18n.js:17
(Diff revision 1)
> +      detectLanguage: function(text) {

This needs to be implemented in ExtensionContent.jsm too.

::: toolkit/components/extensions/ext-i18n.js:18
(Diff revision 1)
> +        return new Promise((resolve, reject) => {

No need for `new Promise` when you already have a promise chain. Just:

`return LanguageDetector.detectLanguage(text).then(result => ({ isReliable: result.confident, languages: result.languages }))`

(make sure you're returning the results in the right format. LanguageDetector's format isn't directly compatible with this API.)

::: toolkit/components/extensions/test/mochitest/test_ext_i18n.html:259
(Diff revision 1)
> +      browser.test.assertEq(expected.confident, result.confident, "result.confident is true");

Should be `isReliable`, not `confident`.
Attachment #8722126 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

2 years ago
https://reviewboard.mozilla.org/r/35869/#review32511

> This needs to be implemented in ExtensionContent.jsm too.

Oh, so this needs to be available to content scripts too, just like the other i18n methods? I guess I could have guessed that. :(
https://reviewboard.mozilla.org/r/35869/#review32511

> Oh, so this needs to be available to content scripts too, just like the other i18n methods? I guess I could have guessed that. :(

Yup, the entire i18n API is available to content scripts.
(Assignee)

Comment 9

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35869/diff/1-2/
Attachment #8722126 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 10

2 years ago
https://reviewboard.mozilla.org/r/35869/#review32511

> Yup, the entire i18n API is available to content scripts.

I have done this, and I think it is all working, but I cannot get the test to work. The problem is line 229 in the test file, where I am trying to call `browser.i18n.detectLanguage(msg, respond);`. The code is identical to the code in the `getAcceptLanguages` patch, except that the `detectLanguage` method accepts an argument, while `getAcceptLanguages` does not. My various attempts to call `detectLanguage` in the content script listener have all resulted in either the promise never resolving (which is the current case), or errors being thrown. If you can advise me as to how to get this call to work that would be great.

Comment 11

2 years ago
(In reply to Bob Silverberg [:bsilverberg] from comment #10)
> https://reviewboard.mozilla.org/r/35869/#review32511
> 
> > Yup, the entire i18n API is available to content scripts.
> 
> I have done this, and I think it is all working, but I cannot get the test
> to work. The problem is line 229 in the test file, where I am trying to call
> `browser.i18n.detectLanguage(msg, respond);`. The code is identical to the
> code in the `getAcceptLanguages` patch, except that the `detectLanguage`
> method accepts an argument, while `getAcceptLanguages` does not. My various
> attempts to call `detectLanguage` in the content script listener have all
> resulted in either the promise never resolving (which is the current case),
> or errors being thrown. If you can advise me as to how to get this call to
> work that would be great.


I think the reason of your issues with detectLanguage is related to the current differences between "implementing a WebExtension API exposed in content-script contexts" vs. "implementing a API exposed to the other extension pages".

Currently the schema wrapping that we've recently introduced and which provides the information needed to:

- auto-validate methods parameters
- implement the Promise-based behavior (and being able to just return a promise and resolve/reject it to automatically pass the results or the error to the callback)

is not currently used in the ExtensionContent.jsm, and the API methods implemented for it cannot yet use the conventions we (actually we should all thank Kris for this awesome refactoring) introduced in the API implementation which is used by the other kind of extension pages (which run in the main process and doesn't use the ExtensionContent.jsm immplementation).

So, if I'm not wrong (and Kris can correct me if I'm), you have to be more explicit (and manually check the parameters), e.g. something like:

       detectLanguage: function(text, callback) {
         context.extension.localeData.detectLanguage(text).then((res) => {
           if (typeof callback == "function") {
             runSafe(context, callback, res);
           }
         });
       },
(Assignee)

Comment 12

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

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

Comment 13

2 years ago
https://reviewboard.mozilla.org/r/35869/#review32511

> I have done this, and I think it is all working, but I cannot get the test to work. The problem is line 229 in the test file, where I am trying to call `browser.i18n.detectLanguage(msg, respond);`. The code is identical to the code in the `getAcceptLanguages` patch, except that the `detectLanguage` method accepts an argument, while `getAcceptLanguages` does not. My various attempts to call `detectLanguage` in the content script listener have all resulted in either the promise never resolving (which is the current case), or errors being thrown. If you can advise me as to how to get this call to work that would be great.

Luca pointed out my folly. It now works. :)
Comment on attachment 8722126 [details]
MozReview Request: Bug 1246754 - Complete the implementation of chrome.i18n.detectLanguage, r?kmag

https://reviewboard.mozilla.org/r/35869/#review32803

::: toolkit/components/extensions/ExtensionUtils.jsm:438
(Diff revision 3)
> +  detectLanguage(text) {

I don't think this really belongs in LocaleData. Maybe just add a standalone helper function?

::: toolkit/components/extensions/ExtensionUtils.jsm:440
(Diff revision 3)
> +      isReliable: result.confident, languages: result.languages.map(lang => {

Please put each property on its own line. Same below.
Attachment #8722126 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 15

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

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

Comment 16

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

Comment 17

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

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

Comment 18

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

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/35b8118cf841
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/35b8118cf841
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.