1013861 - Implement runtime switch to disable translation UI

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[Mike de Boer [:mikedeboer]]

As mention in bug 101337, comment 3:

- a useful setup for testing would be one where we would have language detection running during talos, but with the UI disabled, because that would be testing the real work done during pageload for every page

- we will need this setup anyway (lang. detect enabled, UI disabled) because during the trial run we want to have a control group for our data analysis, and in that group we want to measure the language of websites visited without the presence of a translation feature

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add pref to control showing the translation UI

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: disable translation UI for all tests

not setting flags for this one yet, it requires a longer story.

Comment 3

[Mike de Boer [:mikedeboer]]

Marco, can you add this to the current iteration?

Comment 4

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8426164 [details] [diff] [review]
Patch v1: add pref to control showing the translation UI

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

Thanks Mike! Summarizing our conversation:

- this pref will control the UI being displayed (the infobar, the urlbar icon, and it also controls the removal of hidden="true" in the entry in the preferences dialog and in-content prefs: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/content.xul#137)

- browser.translation.detectLanguage continues to work as is, and is controlled by the checkbox in the Preferences

- For the trial, the control group can get detectLanguage=true, and showUi=false
- Test group gets both prefs to true

- And we also want a boolean flag switch somewhere that would temporarily disable the icon/infobar, without having to toggle the pref (to be used by bug 1012535)

- For talos, we would have detectLanguage=true, showUi=false

::: browser/app/profile/firefox.js
@@ +1563,5 @@
>  pref("browser.cache.frecency_experiment", 0);
>  
>  pref("browser.translation.detectLanguage", true);
>  pref("browser.translation.neverForLanguages", "");
> +pref("browser.translation.ui.show", true);

at the moment this pref should default to false

::: browser/components/translation/Translation.jsm
@@ +11,5 @@
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +const kUIShowPref = "browser.translation.ui.show";

nitpicky, but I tend to prefer CONSTS_WRITTEN_AS_CAPS_LIKE_THIS, e.g. TRANSLATION_SHOW_UI_PREF

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: add pref to control showing the translation UI

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: add pref to control showing the translation UI

decided not to rely on other work.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8427047 [details] [diff] [review]
Patch 1.2: add pref to control showing the translation UI

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

Thanks Mike! I was expecting that two other things from comment 5 (Displaying the checkbox in the Preferences dialog, and the boolean flag in Translation.jsm) would also be covered in this bug. But if you prefer to leave that to a separate bug that's fine, and I can file it. Let me know!

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to :Felipe Gomes from comment #8)
> Thanks Mike! I was expecting that two other things from comment 5

Ah! I misunderstood. I'll add these to in-content prefs _and_ old style prefs windows. New patch coming up soon.

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: Show translation prefs in (in-content) preferences

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: Show translation prefs in (in-content) preferences

Comment 13

[Mike de Boer [:mikedeboer]]

Pushed to fx-team with a comment explaining the pref.

https://hg.mozilla.org/integration/fx-team/rev/33b6c149a3d0
https://hg.mozilla.org/integration/fx-team/rev/9e1f1aba0cdd

Comment 14

[Mike de Boer [:mikedeboer]]

(these are parts 1 & 2. Part 3 still needs to be reviewed and landed - which can happen in another bug.)

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33b6c149a3d0
https://hg.mozilla.org/mozilla-central/rev/9e1f1aba0cdd

Comment 16

[Florian Quèze [:florian]]

I'm wondering if the changes landed here aren't breaking the UI tests we had; shouldn't browser.translation.ui.show be set to true during these tests?

Comment 17

[:Felipe Gomes (needinfo for replies!)]

I bet it did.. sorry that I didn't think of that during review

Comment 18

[Asaf Romano (gone)]

I'll take care of this in bug 1015933.

Comment 19

[Mike de Boer [:mikedeboer]]

Clearing n-i per comment 18. Thanks Mano!