Implement Yandex translation engine

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Felipe, Assigned: yarik.sheptykin)

Tracking

unspecified
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Posted patch YandexTranslator.jsm (obsolete) — Splinter Review
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
Attachment #8558641 - Flags: review?(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?
Attachment #8558641 - Flags: review?(florian) → feedback+
Assignee

Comment 3

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

Comment 4

4 years ago
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.
Attachment #8588296 - Flags: feedback?(felipc)
Assignee

Comment 5

4 years ago
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.
Attachment #8588296 - Attachment is obsolete: true
Attachment #8588296 - Flags: feedback?(felipc)
Attachment #8607725 - Flags: feedback?(felipc)
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.
Attachment #8607725 - Flags: feedback?(felipc) → feedback+
Assignee

Comment 7

4 years ago
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.
Attachment #8607725 - Attachment is obsolete: true
Attachment #8614556 - Flags: review?(felipc)
Comment on attachment 8614556 [details] [diff] [review]
Implement Yandex translation engine

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

Yep, send it to tryserver!
Attachment #8614556 - Flags: review?(felipc) → review+
Assignee: felipc → yarik.sheptykin
Assignee

Comment 9

4 years ago
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
Attachment #8558641 - Attachment is obsolete: true
Attachment #8614556 - Attachment is obsolete: true
Flags: needinfo?(felipc)
Attachment #8615553 - Flags: review+
Yep! Landed
Flags: needinfo?(felipc)
Assignee

Comment 12

4 years ago
Wow, thanks!
https://hg.mozilla.org/mozilla-central/rev/1473a588274a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

4 years ago
Depends on: 1174468
You need to log in before you can comment on or make changes to this bug.