Never translate 'language' disappears when close/open translation infobar using a expired key

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bogdan_maris, Assigned: florian)

Tracking

33 Branch
Firefox 33
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox32 fixed, firefox33 verified, firefox34 verified)

Details

(Whiteboard: [translation] p=1 s=33.1 )

Attachments

(2 attachments)

STR:
1. Start Firefox
2. Use a expired key (client_id : FxTrTest)
3. Visit https://www.google.fr/
4. Hit Translate
5. If 'Translation is not available at the moment. Please try again later' does not appear chose another language to trigger another translation.
6. Close the infobar and reopen it.
7. Open Options menu from infobar.

Expected result: All the options from Options menu are available.

Actual result: Never translate 'Language' is not displayed anymore.

Notes:
1. This issue reproduces on all OS`s.
2. Browser console:
'NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]  translation-infobar.xml:257'
3. Screenshot showing the issue: 
Windows and Linux: https://db.tt/4IYG90o4
Mac OS X: https://db.tt/EeAdi1Kh
Posted patch FixSplinter Review
We can't return early in the init method of the translation infobar because the Options menu depends on the language lists being initialized correctly.

            if (aTranslation.state)
              this.state = aTranslation.state;
at the end of the init method does the right thing in the STATE_UNAVAILABLE case, so no additional code is required.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8442062 - Flags: review?(felipc)
Blocks: 1012535
Flags: firefox-backlog+
Whiteboard: [translation] p=1 [qa+]
Marco, I took this simple follow-up to a bug I handled during this iteration.
Flags: needinfo?(mmucci)
Added to Iteration 33.1
Flags: needinfo?(mmucci)
Whiteboard: [translation] p=1 [qa+] → [translation] p=1 s=33.1 [qa+]
Attachment #8442062 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/c16d14ddb71b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Verified fixed on Nightly 33 (2014-06-22) using: Windows 7 64-bit, Windows 8.1 64-bit, Mac OS X 10.9.2 and Ubuntu 14.04 LTS 32-bit.

Please flip back the [qa+] keyword once this fix lands on Aurora 32.
Status: RESOLVED → VERIFIED
Whiteboard: [translation] p=1 s=33.1 [qa+] → [translation] p=1 s=33.1 [qa!]
Comment on attachment 8442062 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Part of the Translation trial.
User impact if declined: Little user impact (the bug is a corner case), I'm requesting approval to uplift mostly to avoid pain when uplifting future Translation patches / remembering what was fixed in version N/N+1.
Testing completed (on m-c, etc.): Verified on m-c by QA.
Risk to taking this patch (and alternatives if risky): very low.
String or IDL/UUID changes made by this patch: none.
Attachment #8442062 - Flags: approval-mozilla-aurora?
Attachment #8442062 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This fix still needs verification on Aurora 32 but unfortunately, the expired key we previously used to test it is no longer working (it behaves as a valid key).

Florian, could you please provide us with a new expired key?
Flags: needinfo?(florian)
Whiteboard: [translation] p=1 s=33.1 [qa!] → [translation] p=1 s=33.1 [qa+]
Posted patch Follow-upSplinter Review
I currently don't have an expired key, so I poked around to find new steps to reproduce instead.

STR:
1. Start Firefox
2. Open the browser console and execute in it:
Components.utils.import("resource:///modules/translation/Translation.jsm").Translation.serviceUnavailable = true;
3. Visit https://www.google.fr/
4. Expect the infobar to not be displayed, but a gray translation icon near the URL bar.
5. Click that gray icon, the 'Translation is not available at the moment. Please try again later' message should appear.
6. Open Options menu from infobar.

Unfortunately, with these STRs, it seems the bug wasn't really fixed by attachment 8442062 [details] [diff] [review] (it looked too simple to be true at the time; I guess it was).
Attachment #8451505 - Flags: review?(mdeboer)
Flags: needinfo?(florian)
Comment on attachment 8451505 [details] [diff] [review]
Follow-up

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

::: browser/components/translation/Translation.jsm
@@ +70,5 @@
>      let trUI = aBrowser.translationUI;
>  
>      // Set all values before showing a new translation infobar.
> +    trUI._state = Translation.serviceUnavailable ? Translation.STATE_UNAVAILABLE
> +                                                 : aData.state;

If we wanted, instead of this change I think we could revert attachment 8442062 [details] [diff] [review].

@@ -233,5 @@
> -  showTranslationUI: function(aDetectedLanguage) {
> -    this.detectedLanguage = aDetectedLanguage;
> -
> -    // Reset all values before showing a new translation infobar.
> -    this.state = 0;

I think this is dead code I forgot to remove in bug 1015527. I've removed it here because it confused me while debugging that initializing this.state here to STATE_UNAVAILABLE did nothing; but we could totally do this dead code removal in a separate bug.
(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> If we wanted, instead of this change I think we could revert attachment

Personally I prefer having UI state handling code in the binding and all other state handling in JS. As close to the metal as possible.

> I think this is dead code I forgot to remove in bug 1015527. I've removed it
> here because it confused me while debugging that initializing this.state
> here to STATE_UNAVAILABLE did nothing; but we could totally do this dead
> code removal in a separate bug.

There no such thing as bad timing to remove dead code ;)
Comment on attachment 8451505 [details] [diff] [review]
Follow-up

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

Thanks for doing this! I apologize for the late review, I was caught up in PTO and other personal things.

I hope you don't mind.

::: browser/components/translation/translation-infobar.xml
@@ +266,5 @@
>                lang = this._getAnonElt("fromLanguage").value;
> +
> +              // If we have never attempted to translate the page before the
> +              // service became unavailable, "fromLanguage" isn't set.
> +              if (!lang && this.state == Translation.STATE_UNAVAILABLE)

I'm not sure if the last check against the translation module state is necessary. We always need the `lang` var to be set to something, whatever the state may be...

But I'll leave that up to you.
Attachment #8451505 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #13)

> ::: browser/components/translation/translation-infobar.xml
> @@ +266,5 @@
> >                lang = this._getAnonElt("fromLanguage").value;
> > +
> > +              // If we have never attempted to translate the page before the
> > +              // service became unavailable, "fromLanguage" isn't set.
> > +              if (!lang && this.state == Translation.STATE_UNAVAILABLE)
> 
> I'm not sure if the last check against the translation module state is
> necessary. We always need the `lang` var to be set to something, whatever
> the state may be...

It's not strictly necessary, I added it only to show that STATE_UNAVAILABLE is the only case where lang may not be set without our code having a bug.
Comment on attachment 8451505 [details] [diff] [review]
Follow-up

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

I think at the time I wrote this patch I wanted to request aurora approval on it to have it in Fx 32 for our larger trial... but given how late in the cycle it is now, I'm not sure if I want to request aurora approval, beta approval, or just let the patch ride the trains.
(In reply to Wes Kocher (:KWierso) from comment #16)
> https://hg.mozilla.org/mozilla-central/rev/b9c60f0356c2

This didn't make the merge into m-c until after 33 got tagged and merged down to Aurora, so the followup is only landed for Firefox 34 at the moment.
Flags: needinfo?(florian)
My attempt of verifying this fix on Nightly 34.0a1 (2014-07-21), Windows 7 64-bit, was still unsuccessful. 

Florian, I followed the steps provided in Comment 10, but the "Translation is not available at the moment. Please try again later" error message is *not* displayed despite running the specified code in the Browser Console - the pages are successfully translated.

Please advise.
FWIW, I did manage to verify the fix on Nightly 34.0a1 (2014-07-21), using the initial STR implying an expired key, as Bogdan Maris was able to provide one.
Reiterating my request from Comment 18 and Comment 19.
(In reply to Andrei Vaida, QA [:avaida] from comment #20)
> Reiterating my request from Comment 18 and Comment 19.

Sorry for the delay.

I just tried the steps from comment 10 again, and could follow them.

One thing that was probably missing is that to be able to execute any code from the browser console, you need first to go in about:config and set devtools.chrome.enabled to true.

Then you can open the browser console from the Tools -> Web Developer -> Browser Console menuitem.
Flags: needinfo?(florian)
Comment on attachment 8451505 [details] [diff] [review]
Follow-up

(In reply to Wes Kocher (:KWierso) from comment #17)
> (In reply to Wes Kocher (:KWierso) from comment #16)
> > https://hg.mozilla.org/mozilla-central/rev/b9c60f0356c2
> 
> This didn't make the merge into m-c until after 33 got tagged and merged
> down to Aurora, so the followup is only landed for Firefox 34 at the moment.

I think we should land it on aurora for 33, sorry for the delay.


Approval Request Comment
[Feature/regressing bug #]: Part of the Translation trial. Follow-up to address a regression from the patch landed in comment 4.
[User impact if declined]: Minor. Some of the translation UI may appear in a broken state if we ever run out of quota before the end of the experiment.
[Describe test coverage new/current, TBPL]: On m-c for weeks.
[Risks and why]: Low.
[String/UUID change made/needed]: none.
Attachment #8451505 - Flags: approval-mozilla-aurora?
Attachment #8451505 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> (In reply to Andrei Vaida, QA [:avaida] from comment #20)
> > Reiterating my request from Comment 18 and Comment 19.
> 
> Sorry for the delay.
> 
> I just tried the steps from comment 10 again, and could follow them.
> 
> One thing that was probably missing is that to be able to execute any code
> from the browser console, you need first to go in about:config and set
> devtools.chrome.enabled to true.
> 
> Then you can open the browser console from the Tools -> Web Developer ->
> Browser Console menuitem.
No worries, thank you for following up on this. 

I was able to confirm the fix for this bug using the steps provided in Comment 10 on Nightly 34.0a1 (2014-08-18) with Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.4.

Leaving the [qa+] keyword in place until this patch lands on Aurora.
QA Whiteboard: [qa+]
Whiteboard: [translation] p=1 s=33.1 [qa+] → [translation] p=1 s=33.1
This is now verified fixed on Aurora 33.0a2 (2014-08-20) as well.
QA Whiteboard: [qa+]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.