1174468 - Firefox shows Microsoft logo in preferences with Yandex set as translation engine

Status: RESOLVED FIXED

(Firefox :: Translations, defect)

Description

[Sören Hentzschel]

Yandex was implemented as alternative translation engine in bug 1129056. After changing the translation engine to Yandex the preferences still show the Microsoft logo.

Comment 1

[Iaroslav Sheptykin]

Hi Sören! Thanks for reporting this issue! I am one of those who added yandex translation engine. I will be taking care of this bug.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

I think we can use here plain-text (like "Powered by Yandex") instead of looking for a logo, and just keep the logo a special case for Bing.

Comment 3

[Iaroslav Sheptykin]

(In reply to :Felipe Gomes from comment #2)
> I think we can use here plain-text (like "Powered by Yandex") instead of
> looking for a logo, and just keep the logo a special case for Bing.

Hi, Felipe! Right, Yandex requires the following attribution in their terms of use:

"2.7. When using the Service the User shall include reference to the Yandex technology in the description of the software application, in the respective help topic, on the official website of the software application, as well as on all pages/screens where the data of the service “Yandex.Translate” is used, strictly over or under the results of translation, in format of the following text: “Powered by Yandex.Translate” with the clickable hyperlink to the page http://translate.yandex.com/. The font size of this reference shall not be less than the size of the main text font, and its color shall not differ from the color of the main text font." (https://legal.yandex.com/translate_api/)

To add this I am planning to start from translation-infobar.xul and wrap the contents of attribution menu item into xul:deck. I will then switch between appropriate attribution texts depending on the selected engine. According to my search (http://mxr.mozilla.org/mozilla-central/search?string=microsoft-translator-attribution.png) we also need to mention Yandex in the Preference window.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

(In reply to Iaroslav Sheptykin from comment #3)
> To add this I am planning to start from translation-infobar.xul and wrap the
> contents of attribution menu item into xul:deck. I will then switch between
> appropriate attribution texts depending on the selected engine.

Sounds good!


> According to my search we also need to mention Yandex in the Preference window.

From that terms of use I think there's no reason to include it in the Preferences window, so I think you can just hide the Microsoft part in that case.

Comment 5

[Iaroslav Sheptykin]

Attachment: Showing Yandex.Translate attribution when Yandex engine is selected

Hi Guys!

With this patch and Yandex.Translate selected as the translation engine Firefox will 
1. hide Microsoft's attribution from the preference page
2. use "Powered by Yandex.Translate" in the Options list of translation infobar.
3. open http://translate.yandex.com/ when clicked on attribution.

The patch also includes a test that ensures the attribution is added. However, does not cover features 1 and 3. Let me know if this should be added.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8624435 [details] [diff] [review]
Showing Yandex.Translate attribution when Yandex engine is selected

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

::: browser/components/preferences/content.js
@@ +19,5 @@
>        let row = document.getElementById("translationBox");
>        row.removeAttribute("hidden");
> +      // Showing attribution only for Bing Translator.
> +      Components.utils.import("resource:///modules/translation/Translation.jsm");
> +      if(Translation.translationEngine == "bing") {

nit: needs a space between if and (

::: browser/components/preferences/in-content/content.js
@@ +24,5 @@
>        let row = document.getElementById("translationBox");
>        row.removeAttribute("hidden");
> +      // Showing attribution only for Bing Translator.
> +      Components.utils.import("resource:///modules/translation/Translation.jsm");
> +      if(Translation.translationEngine == "bing") {

same nit

Comment 7

[Iaroslav Sheptykin]

Attachment: Showing Yandex.Translate attribution when Yandex engine is selected

Hi guys! I updated the patch according to the feedback from Felipe (comment 6). How does it look now?

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8634642 [details] [diff] [review]
Showing Yandex.Translate attribution when Yandex engine is selected

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

Looks good, thanks! (Note: given Felipe's previous r+, my review was more a sanity check than a thorough review.)

::: browser/components/translation/test/browser_translation_yandex.js
@@ +65,5 @@
> +  gBrowser.removeTab(tab);
> +});
> +
> +
> +add_task(function* test_preference_attribution(){

nit: space between () and {

Comment 9

[Iaroslav Sheptykin]

Attachment: Showing Yandex.Translate attribution when Yandex engine is selected

Hi Guys! 
I updated the patch and pushed it to the tryserver a couple of days ago.
Here is what happened: https://treeherder.mozilla.org/#/jobs?repo=try&revision=013f8256d199
I believe those failures are unrelated. What do you think Florian?

Comment 10

[Florian Quèze [:florian]]

(In reply to Iaroslav Sheptykin from comment #9)

> I updated the patch and pushed it to the tryserver a couple of days ago.
> Here is what happened:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=013f8256d199
> I believe those failures are unrelated. What do you think Florian?

I also think it's unrelated.

Comment 11

[Iaroslav Sheptykin]

Are we ready to check this patch in? Should I add [checkin-needed]?

Comment 12

[Florian Quèze [:florian]]

Yes, this is ready to add the checkin-needed keyword (doing it for you now; feel free to do it next time :-)).

Comment 13

[Iaroslav Sheptykin]

(In reply to Florian Quèze [:florian] [:flo] (PTO until August 3rd) from comment #12)
> Yes, this is ready to add the checkin-needed keyword (doing it for you now;
> feel free to do it next time :-)).

Thanks Florian! Next time I'll do it!

Comment 14

[Pulsebot]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfa0f07aeccb