1246754 - Complete the implementation of chrome.i18n.detectLanguage

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

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)

Comment 1

[Bob Silverberg [:bsilverberg]]

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

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Bob Silverberg [:bsilverberg]]

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.

Comment 4

[Kris Maglione [:kmag]]

Sounds good. I'll try to get to it today. If not, sometime this week.

Comment 5

[Bob Silverberg [:bsilverberg]]

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

Comment 6

[Kris Maglione [:kmag]]

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`.

Comment 7

[Bob Silverberg [:bsilverberg]]

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. :(

Comment 8

[Kris Maglione [:kmag]]

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.

Comment 9

[Bob Silverberg [:bsilverberg]]

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/

Comment 10

[Bob Silverberg [:bsilverberg]]

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

[Luca Greco [:rpl] [:luca] [:lgreco]]

(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);
           }
         });
       },

Comment 12

[Bob Silverberg [:bsilverberg]]

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/

Comment 13

[Bob Silverberg [:bsilverberg]]

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 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Bob Silverberg [:bsilverberg]]

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/

Comment 16

[Bob Silverberg [:bsilverberg]]

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

Comment 17

[Bob Silverberg [:bsilverberg]]

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/

Comment 18

[Bob Silverberg [:bsilverberg]]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/35b8118cf841

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35b8118cf841