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

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[Bogdan Maris, Desktop QA]

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

Comment 1

[Florian Quèze [:florian]]

Attachment: Fix

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.

Comment 2

[Florian Quèze [:florian]]

Marco, I took this simple follow-up to a bug I handled during this iteration.

Comment 3

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 4

[Florian Quèze [:florian]]

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

Comment 5

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

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

Comment 6

[Andrei Vaida [:avaida]]

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.

Comment 7

[Florian Quèze [:florian]]

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.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d9ac8792c337

Comment 9

[Andrei Vaida [:avaida]]

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?

Comment 10

[Florian Quèze [:florian]]

Attachment: Follow-up

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).

Comment 11

[Florian Quèze [: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.

Comment 12

[Mike de Boer [:mikedeboer]]

(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 13

[Mike de Boer [:mikedeboer]]

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.

Comment 14

[Florian Quèze [:florian]]

(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 15

[Florian Quèze [:florian]]

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.

Comment 16

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

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

Comment 17

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

(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.

Comment 18

[Andrei Vaida [:avaida]]

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.

Comment 19

[Andrei Vaida [:avaida]]

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.

Comment 20

[Andrei Vaida [:avaida]]

Reiterating my request from Comment 18 and Comment 19.

Comment 21

[Florian Quèze [:florian]]

(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.

Comment 22

[Florian Quèze [: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.

Comment 23

[Andrei Vaida [:avaida]]

(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.

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8451505 [details] [diff] [review]
Follow-up

https://hg.mozilla.org/releases/mozilla-aurora/rev/95a4c3c44080

Comment 25

[Andrei Vaida [:avaida]]

This is now verified fixed on Aurora 33.0a2 (2014-08-20) as well.