Closed Bug 974461 Opened 7 years ago Closed 7 years ago

Display translation icon at the same time as the translation infobar

Categories

(Firefox :: Translation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: sevaan, Assigned: florian)

References

Details

(Whiteboard: [translation] p=5 s=it-31c-30a-29b.2 [qa!])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [translation] p=0
I'm actually working on this as part of bug 974460.
Assignee: nobody → florian
Attached patch Patch (obsolete) — Splinter Review
Before applying this patch, you need to apply first the patches from bug 974460 and from bug 991202.
Attachment #8400780 - Flags: review?(felipc)
Depends on: 991202
Status: NEW → ASSIGNED
Whiteboard: [translation] p=0 → [translation] p=5 s=it-31c-30a-29b.2
QA Contact: bogdan.maris
Whiteboard: [translation] p=5 s=it-31c-30a-29b.2 → [translation] p=5 s=it-31c-30a-29b.2 [qa+]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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
Attachment #8400780 - Flags: review?(felipc) → feedback+
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
Attached patch Patch v2 (comments addressed) (obsolete) — Splinter Review
I'm attaching this one to make the interdiff easy to look at.
Attachment #8400780 - Attachment is obsolete: true
As agreed during our discussion, this patch removes the WIP code that would likely be better suited for bug 971048.
Attachment #8401395 - Attachment is obsolete: true
Attachment #8401402 - Flags: review?(felipc)
Attachment #8401402 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/ad5a607bed97
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Hi Bogdan, will verification of this bug be able to be completed before the end of our iteration on Monday April 14?
Flags: needinfo?(bogdan.maris)
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bogdan.maris)
Whiteboard: [translation] p=5 s=it-31c-30a-29b.2 [qa+] → [translation] p=5 s=it-31c-30a-29b.2 [qa!]
Mass move of translation bugs to the new Translation component.
Component: General → Translation
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.