974461 - Display translation icon at the same time as the translation infobar

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[Sevaan Franks [:sevaan]]

Whenever Firefox detects a page has been written in a language that is not the user's default language, then we must display the Translation icon as well as the Translation Infobar.

Comment 1

[Florian Quèze [:florian]]

I'm actually working on this as part of bug 974460.

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch

Before applying this patch, you need to apply first the patches from bug 974460 and from bug 991202.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8400780 [details] [diff] [review]
Patch

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

::: browser/components/translation/Translation.jsm
@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ["Translation"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource:///modules/translation/LanguageDetector.jsm");

Lazy getter

@@ +29,5 @@
> +  _defaultTargetLanguage: "",
> +  get defaultTargetLanguage() {
> +    if (!this._defaultTargetLanguage) {
> +      this._defaultTargetLanguage =
> +        Services.prefs.getCharPref("general.useragent.locale").split("-")[0];

I think the right way to do this is to use nsIXULChromeRegistry.getSelectedLocale("global")

@@ +110,5 @@
> +    let doc = this.doc;
> +    let self = this;
> +    doc.addEventListener("DOMContentLoaded", function dcl() {
> +      doc.removeEventListener("DOMContentLoaded", dcl);
> +      self.checkLanguage();

do this off a timeout to put it out of the load event bubbling process

@@ +128,5 @@
> +        // Reset all values before showing a new translation infobar.
> +        this.state = 0;
> +        this.translatedFrom = "";
> +        this.translatedTo = "";
> +        this.originalShown = true;

move this part to a separate function to reduce the test stub as we talked about

::: browser/components/translation/test/browser_translation_infobar.js
@@ +190,5 @@
>  
> +  info("Check that clicking the url bar icon reopens the info bar");
> +  checkURLBarIcon();
> +  PopupNotifications.getNotification("translate").anchorElement.click();
> +  waitForCondition(() => !!notificationBox.getNotificationWithValue("translation"), () => {

add a comment about .click() doing its job asynchronously

::: browser/components/translation/translation-infobar.xml
@@ +106,5 @@
>              - showOriginalContent, method showing the original page content.
>              - showTranslatedContent, method showing the translation for an
>                already translated page whose original content is shown.
> +            - originalShown, boolean indicating if the original or translated
> +              version of the page is shown.

move this documentation to translation.jsm

@@ +125,5 @@
>                detectedLanguage.appendItem(name, code);
>                fromLanguage.appendItem(name, code);
>              }
> +            detectedLanguage.value = this.translation.detectedLanguage;
> +            if (aTranslation.translatedFrom)

more spaces before and after if conditions

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8400780 [details] [diff] [review]
Patch

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

::: browser/components/translation/test/browser_translation_infobar.js
@@ +72,5 @@
> +  tab.linkedBrowser.addEventListener("load", function onload() {
> +    tab.linkedBrowser.removeEventListener("load", onload, true);
> +    TranslationStub.browser = gBrowser.selectedBrowser;
> +    run_tests(() => {
> +      gBrowser.removeTab(tab);

use a registerCleanupFunction to removeTab instead of being on the aFinishCallback

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2 (comments addressed)

I'm attaching this one to make the interdiff easy to look at.

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v2 (without WIP code)

As agreed during our discussion, this patch removes the WIP code that would likely be better suited for bug 971048.

Comment 7

[Florian Quèze [:florian]]

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

There's a try server build for QA purposes at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e487810349d2/

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Marco Mucci [:MarcoM]]

Hi Bogdan, will verification of this bug be able to be completed before the end of our iteration on Monday April 14?

Comment 10

[Bogdan Maris, Desktop Test Engineering]

(In reply to Marco Mucci [:MarcoM] from comment #9)
> Hi Bogdan, will verification of this bug be able to be completed before the
> end of our iteration on Monday April 14?
Sure Marco, I tested this fix using try build from comment 7 on Windows XP 32bit, Windows 7 64bit, Windows 8.1 64bit, Windows 8.1 64bit on Surface Pro, Ubuntu 13.10 32bit and Mac OS X 10.9.2.

Comment 11

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.