1129056 - Implement Yandex translation engine

Status: RESOLVED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Yandex now have a Translation service! This bug is to implement a translation engine using their service.

http://api.yandex.com/translate/doc/dg/reference/translate.xml

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: YandexTranslator.jsm

Copied from the previous engine, most things stood up on their own. The API returns JSON, not XML, so there was some clean-up win.

They also return clear error codes for errors, but the HTTP response code is not a reflection of the error code. So that's why I removed the [400, 401] and always check for what was reported in the json structure.

Two things not included in this patch:
 - remove attribution stuff from previous engine
 - adjust API limits, which I'm still analyzing what they are

I intend to do these two as separate bugs or at least separate patches in this bug.

Two other separate bugs will be: to improve the handling of the supported languages list, and to add the build flags to include the API key

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8558641 [details] [diff] [review]
YandexTranslator.jsm

Review of attachment 8558641 [details] [diff] [review]:
-----------------------------------------------------------------

The code here looks good :-). f+ for now, which can be changed to r+ once the 2 following questions are answered:
- it seems this hardcodes Yandex instead of Bing in TranslationContentHandler.jsm. If we are making Bing completely unusable, should we hg remove BingTranslator.jsm, or at least stop packaging it (ie remove it from EXTRA_JS_MODULES.translation)?
- should we copy/adapt the browser_translation_bing.js test?

::: browser/components/translation/TranslationContentHandler.jsm
@@ +132,1 @@
>                                                  msg.data.from,

nit: fix the indent here.

::: browser/components/translation/YandexTranslator.jsm
@@ +148,5 @@
> +      try {
> +        json = JSON.parse(body);
> +      } catch (e) {}
> +
> +      if (json.code && YANDEX_PERMANENT_ERRORS.indexOf(json.code) != -1)

Can we use Array.includes() yet?

@@ +193,5 @@
>      let results;
>      try {
> +      let result = JSON.parse(yandexRequest.networkRequest.response.body);
> +      if (result.code != 200) {
> +        Services.console.logStringMessage("Result is " + result.code);

Remove this line, make the message more descriptive (ie. mention at least that it's from the YandexTranslation code), or maybe we need to implement better logging?

Comment 3

[Iaroslav Sheptykin]

Hi everybody! I switching to Yandex still relevant? I would be interested in contributing to this work.

Comment 4

[Iaroslav Sheptykin]

Attachment: Implement Yandex translation engine

Hi guys!

I was not sure if were are still interested in implementing yandex translation engine. I therefore though to apply the patch from Felipe and try it in action. I could not apply patch as it was because the BingTranslator has changed meanwhile. As of Bug 1012532 the translator is now making use of Http.jsm translation requests. So I merged the changes from Felipe with the BingTranslator from master by hand. The translation with new YandexTranslator works fine.

Also, instead of renaming BingTranslator to YandexTranslator I added a new YandexTranslator module. Maybe we could keep both engines and switch between them depending on the usage limits?

If this patch is going into the right direction I would start working on tests for the Yandex engine and working on sub-bugs mentioned by Felipe in the description.

Comment 5

[Iaroslav Sheptykin]

Attachment: Implement Yandex translation engine

Hi Again! I modified my last patch by adding a test case and an option for setting the translation engine of choice. Bing is used by default.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8607725 [details] [diff] [review]
Implement Yandex translation engine

Review of attachment 8607725 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/translation/test/fixtures/result-yandex.json
@@ +1,1 @@
> +{"code":200,"lang":"fr-en","text":["Football world cup 2014","The football world Cup 2014 is the 20th edition of the world Cup, a football competition organized by FIFA and which brings together thirty-two of the best national selections. The final phase takes place in the summer of 2014 in Brazil. With the host country, all the teams world champions since 1930 (Uruguay, Italy, Germany, England, Argentina, France and Spain) have qualified for this competition. It is also the first international competition of Bosnia and Herzegovina."]}

Similar to the result-da39a3ee5e.txt file, please replace the actual translated content with Lorem Ipsum to avoid copyright issues (the original text was retrieved from Wikipedia but translations through the service might not be redistributable)

::: browser/components/translation/test/yandex.sjs
@@ +162,5 @@
> +    res.setStatusLine("1.1", 200, "OK");
> +    res.setHeader("Content-Type", "text/xml");
> +
> +    let inputStream = getInputStream(
> +      "browser/browser/components/translation/test/fixtures/result-yandex.json");

could you do the same hashing as bing.sjs instead of hardcoding result-yandex.json?  I imagine you did this because otherwise it would generate a naming conflict, but you could modify bing.sjs as well to use result-bing-{hash}.txt and result-yandex-{hash}.txt.

Comment 7

[Iaroslav Sheptykin]

Attachment: Implement Yandex translation engine

Hi Felipe! Thanks for your feedback! I updated yandex.sjs as you suggested. Tests pass locally. If the rest looks fine, I could push test it on the try server.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8614556 [details] [diff] [review]
Implement Yandex translation engine

Review of attachment 8614556 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, send it to tryserver!

Comment 9

[Iaroslav Sheptykin]

Attachment: Implement Yandex translation engine

Hi, Felipe! I pushed the patch to the tryserver and it reported failures in browser_translation_fhr.js. The test failed due to a missing preference for the preferred translation engine (browser.translation.engine) in the testing profile [1].

I added the preference and resubmitted the patch to the try server again. None of the translation tests failed. There are some other failures but they are not related to this patch I believe. I updated the patch here and carried over the earlier review +. Should we try to check it in?

1. https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c5835b6aeaa
2. https://treeherder.mozilla.org/#/jobs?repo=try&revision=05aa6bed414b

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1473a588274a

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Yep! Landed

Comment 12

[Iaroslav Sheptykin]

Wow, thanks!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1473a588274a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1473a588274a