Closed Bug 1032139 Opened 6 years ago Closed 6 years ago

Make the 'Translations by' string localizable

Categories

(Firefox :: Translation, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
34.1
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 + verified
firefox33 --- verified

People

(Reporter: florian, Assigned: florian)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

In bug 1022856 we hardcoded the string to its German localization.

For the trial on beta, we will need the string in Vietnamese, Polish and Turkish.

What needs to be done here:
- on central, make the string localizable. We need to have a string on both sides of the logo, as some locales may not be able to construct a correct sentence with only a string before the logo.
- on aurora (32), we also need a string on both sides, but we should hardcode in the JavaScript file the values for English, German, Vietnamese, Polish and Turkish.
Flags: firefox-backlog+
Points: --- → 3
Marco, I think this bug should be handled during this iteration, and I'm willing to take it.
Flags: needinfo?(mmucci)
Added to Iteration 33.2
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 33.2
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
I hesitated between duplicating the strings in browser/preferences/content.dtd and browser/translation.dtd, or including browser/translation.dtd from content.xul, but the latter really didn't seem good, and I also wonder if it couldn't be useful for some locales to tweak the string based on how the "Translate web content" string displayed on the same line in preference windows is translated.
Attachment #8450955 - Flags: review?(felipc)
Attached patch Patch for mozilla-aurora (obsolete) — Splinter Review
Unfortunately, writing the localized string directly in UTF8 in Translation.jsm causes it to be double-unicode-encoded, so I had to escape the characters in the JS string.
Attachment #8450976 - Flags: review?(felipc)
I filed bug 1034623, bug 1034625 and bug 1034626 to get the translated strings we need to include in the aurora patch.
Depends on: 1034623, 1034625, 1034626
No longer depends on: 1034626
Depends on: 1034626
I simplified the aurora patch as none of the locales we want to include uses the second string after the logo.
Attachment #8450976 - Attachment is obsolete: true
Attachment #8450976 - Flags: review?(felipc)
Attachment #8451524 - Flags: review?(felipc)
Attachment #8450955 - Flags: review?(felipc) → review+
Comment on attachment 8451524 [details] [diff] [review]
Patch for mozilla-aurora v2 (strings included)

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

::: browser/components/preferences/in-content/content.js
@@ -187,5 @@
>    },
>  
>    openTranslationProviderAttribution: function ()
>    {
> -    Components.utils.import("resource:///modules/translation/Translation.jsm");

I wonder if we should remove this line, or fix both imports to use a tmp object instead of polluting `this`
Attachment #8451524 - Flags: review?(felipc) → review+
Hi Florian, can you mark this bug as [qa+] or [qa-].
Iteration: 33.2 → 33.3
Flags: needinfo?(florian)
(In reply to Marco Mucci [:MarcoM] from comment #8)
> Hi Florian, can you mark this bug as [qa+] or [qa-].

Sure, [qa+], even though I'm not sure we have nightly or aurora builds in the relevant locales, so QA people may need to look at the first localized betas after the next uplift instead.
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(florian)
(In reply to :Felipe Gomes from comment #7)

Thanks for the reviews!

> Comment on attachment 8451524 [details] [diff] [review]
> Patch for mozilla-aurora v2 (strings included)

> >    openTranslationProviderAttribution: function ()
> >    {
> > -    Components.utils.import("resource:///modules/translation/Translation.jsm");
> 
> I wonder if we should remove this line, or fix both imports to use a tmp
> object instead of polluting `this`

I also thought about using a temporary scope to avoid polluting the global namespace, but importing the 2 "Translation", and "TranslationProvider" symbols doesn't seem like it could cause any problem, so I don't think it's worth thinking much about it, especially for aurora.
Comment on attachment 8451524 [details] [diff] [review]
Patch for mozilla-aurora v2 (strings included)

Approval Request Comment
[Feature/regressing bug #]: bug 1022856, but more generally this is part of the translation experiment we intend to ship on Firefox 32 once it reaches beta.
[User impact if declined]: The string will be shown in German for all users.
[Describe test coverage new/current, TBPL]: only tested locally.
[Risks and why]: acceptable.
[String/UUID change made/needed]: New hardcoded localizations of a string that was already hardcoded in bug 1022856.
Attachment #8451524 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5d6701730555
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: bogdan.maris
As discussed with :flod on IRC, in order for me to test what has been fixed for central I would need a locale that has 'string' 'logo' 'string' and for now there is none that has been localized. Leaving [qa+] until there such locale is available for testing.
Given the small number of locales working on central, I'd expect this to be verifiable only when it moves to aurora.
flod - I am expecting that this hard coded approach for the beta trial is ok for l10n but would like to confirm before approving. Can you please confirm.
Flags: needinfo?(francesco.lodolo)
(In reply to Lawrence Mandel [:lmandel] from comment #17)
> flod - I am expecting that this hard coded approach for the beta trial is ok
> for l10n but would like to confirm before approving. Can you please confirm.

This approach was discussed with and approved by Pike.
(In reply to Lawrence Mandel [:lmandel] from comment #17)
> flod - I am expecting that this hard coded approach for the beta trial is ok
> for l10n but would like to confirm before approving. Can you please confirm.

Yes. It's basically an experiment for 3/4 locales, hard-coded strings are there to avoid breaking string freeze for all others. Strings are correctly localizable on m-c
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8451524 [details] [diff] [review]
Patch for mozilla-aurora v2 (strings included)

Thanks flod. Aurora+
Attachment #8451524 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified that builds English, German, Vietnamese, Polish and Turkish have the 'Translations by' string localized according to the dependent bugs. I`ve tested on Windows 7 64bit, Mac OS X 10.9.4 and Ubuntu 14.04 32bit using latest Aurora. Leaving qa+ until the patch arrives in Nightly as well.
Iteration: 33.3 → 34.1
Using Aurora 33 I can`t see 'Translations by' string localized in any of the locales vi, pl, tr, de. Any news on this?
Flags: needinfo?(francesco.lodolo)
Definitely not the right person to NI ;-)

Not all builds have that feature enabled, not sure how to test. Turning NI to Florian.
Flags: needinfo?(francesco.lodolo) → needinfo?(florian)
You were indeed the right person to NI, but the question wasn't clear: do you know if the translation.options.attribution.beforeLogo string is currently localized on aurora in the locale repositories for any of the vi, pl, tr, de locales?
Flags: needinfo?(florian) → needinfo?(francesco.lodolo)
Thanks! So Bogdan, you can either wait and check flod's links periodically, or verify the bug using one of these locales: br, cs, es-AR, es-CL, es-ES, fr, it, lt, lv, nb-NO, pa-IN, ru, sq, sr, zh-TW
I went on and verified all the locales from link provided in comment 26 using latest Aurora 33.0a2, only 'cs' locale does not have the 'Translation by' string translated and only on Windows builds. 

Also build zh-TW has two strings around 'Translation by' (https://db.tt/YQZL0UED) and it should be 翻譯服務由 only (based on http://transvision.mozfr.org/string/?entity=browser/chrome/browser/translation.dtd:translation.options.attribution.beforeLogo&repo=aurora)
cs landed most of the strings on Jul 29, it could it missed that build.

We have "before" and "after" logo, zh-TW is one of the locales with a string also after the logo
http://transvision.mozfr.org/?recherche=translation.options.attribution.&repo=aurora&sourcelocale=en-US&locale=zh-TW&search_type=entities

翻譯服務由 (logo) 提供
Great then, thanks for the info. I will go ahead and mark this issue verified as fixed then since I got to test a locale that has 'string' 'logo' 'string' structure.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Depends on: 1067011
You need to log in before you can comment on or make changes to this bug.