680392 - Non localized trademark notice string in about:firefox

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Francesco Lodolo [:flod]]

Attachment: screenshot of the about window in current beta

I've just realized that releases and betas have a non localized (localizable?) string in the about window (logoTrademark, after link to license information).

This string is available in en-US locale under /branding, but compare-locales doesn't check for its presence, so I don't know if adding it to a locale repository would be enough.

Comment 1

[Axel Hecht [:Pike]]

We intentionally don't add the non-release brandings to l10n. For mobile, even the official branding isn't exposed to l10n.

Raises two alternative questions:

- should we expose that? The downside of that might be that people edit the official branding to be nightly or aurora, instead of the nightly or aurora branding. Also, all apps have their slightly different names for the apps on the channels.

- should the trademark string be in branding?

I'd like the mobile guys to weigh in on this, I'm naturally on the side of "sure, let's get that localized". At least for beta and release.

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #1)
> - should we expose that? The downside of that might be that people edit the
> official branding to be nightly or aurora, instead of the nightly or aurora
> branding. Also, all apps have their slightly different names for the apps on
> the channels.

In current nightlies that string is not displayed, so I assume it's used only in releases and betas (for Aurora I checked only the desktop version, so I can't be sure), which means the product name should be always the same.

Comment 3

[Francesco Lodolo [:flod]]

This is still a valid bug in the current Firefox for Android.

Do we want to wontfix it? The alternative is to move that string outside of branding.
http://hg.mozilla.org/mozilla-central/file/616e6924cb0b/mobile/android/branding/beta/locales/en-US/brand.dtd

Comment 4

[:Margaret Leibovic]

We include brand.dtd in about.xhtml:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/about.xhtml#4

Is the issue that brand.dtd is not being localized? We could move this string into about.dtd instead.

Comment 5

[Francesco Lodolo [:flod]]

Yes, brand.dtd is not localized/localizable at the moment.

I don't see a point in having the entire file localizable, while we should definitely move this string to a localizable file (about.dtd sounds good). I believe it's also fine to just use "Firefox" instead of entities, considering that's also what desktop does.

http://hg.mozilla.org/releases/mozilla-beta/file/default/browser/branding/official/locales/en-US/brand.dtd

Comment 6

[aditya]

I would like to work on this bug. As I am a newbie could some one mentor me in fixing the issue.

Comment 7

[:Margaret Leibovic]

(In reply to aditya from comment #6)
> I would like to work on this bug. As I am a newbie could some one mentor me
> in fixing the issue.

Hi aditya! First of all, do you have a build of Fennec set up? If not, you'll want to follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

To solve this bug, you'll want to look at these files:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/about.xhtml
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/about.dtd

http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/aurora/locales/en-US/brand.dtd
http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/beta/locales/en-US/brand.dtd
http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/nightly/locales/en-US/brand.dtd
http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/official/locales/en-US/brand.dtd
http://mxr.mozilla.org/mozilla-central/source/mobile/android/branding/unofficial/locales/en-US/brand.dtd

As flod mentioned in comment 5, we can move the logoTrademark out of brand.dtd and into about.dtd. Currently that string is empty for non-official version of Firefox (ones that don't use the Firefox logo), so we could put the element that displays that text behind a build flag to make it only appear on official builds:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/about.xhtml#69

But, as a first step, let's just move over the string, then we can worry about hiding it in pre-release builds.

Comment 8

[Julien Sanchez]

Attachment: patch-680392-1.patch

Hi,

I moved the locales and target official build with the MOZILLA_OFFICIAL flag.


I also find others references to logoTrademark in files :

- mobile/xul/branding/official/locales/en-US/brand.dtd
- b2g/branding/official/locales/en-US/brand.dtd

Should we also move and internationalize them ?

Comment 9

[:Margaret Leibovic]

(In reply to Julien Sanchez from comment #8)
> Created attachment 8481778 [details] [diff] [review]
> patch-680392-1.patch
> 
> Hi,
> 
> I moved the locales and target official build with the MOZILLA_OFFICIAL flag.
> 
> 
> I also find others references to logoTrademark in files :
> 
> - mobile/xul/branding/official/locales/en-US/brand.dtd

This is not used at all anymore... in fact, this /xul directory was removed from the tree. Where did you find this file?

> - b2g/branding/official/locales/en-US/brand.dtd

We can file a separate bug for b2g. I'm not sure if they're using that brand file, since the UI for Firefox OS is created with gaia (a separate project/repo), so maybe that bug would turn into a clean-up bug.

> Should we also move and internationalize them ?

Let's just keep this bug about Firefox for Android. But good job investigating the rest of the tree!

Comment 10

[:Margaret Leibovic]

Comment on attachment 8481778 [details] [diff] [review]
patch-680392-1.patch

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

When you upload a patch, you should flag a peer for review. I can review this patch for you, so I'll set the flag to myself :) I'll take a closer look at this some time today.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8481778 [details] [diff] [review]
patch-680392-1.patch

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

Looking good! There are just two small tweaks you should make, and then you should upload a new version of your patch for me to review.

::: mobile/android/chrome/content/about.xhtml
@@ +64,5 @@
>        <li><a href="about:license">&aboutPage.license.label;</a></li>
>        <div class="bottom-border"></div>
>      </ul>
>  
> +#ifdef MOZILLA_OFFICIAL

You should update this to be RELEASE_BUILD. See this wiki page for some useful details:
https://wiki.mozilla.org/Platform/Channel-specific_build_defines

MOZILLA_OFFICIAL is true for any version built in Mozilla automation, so it's true for Nightly/Aurora as well, which isn't what we want.

::: mobile/android/locales/en-US/chrome/about.dtd
@@ +18,5 @@
>  <!ENTITY aboutPage.rights.label                 "Know Your Rights">
>  <!ENTITY aboutPage.relNotes.label               "Release Notes">
>  <!ENTITY aboutPage.credits.label                "Credits">
>  <!ENTITY aboutPage.license.label                "Licensing Information">
> +<!ENTITY aboutPage.logoTrademark                "Firefox and the Firefox logos are trademarks of the Mozilla Foundation.">

You should add a localization comment above this string to explain that it is only used in official versions of Firefox. Usually we use &brandShortName; instead of "Firefox", but in this case, the message is explicitly about the word Firefox being trademarked, so I think it's fine to keep it in the string like this.

Comment 12

[Julien Sanchez]

Attachment: patch-680392-2.patch

Thanks for the review. I've made changes for the build flag, and for the brand name on locales.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8483680 [details] [diff] [review]
patch-680392-2.patch

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

Looking good, I just wasn't clear in my last set of review comments. Almost there!

::: mobile/android/branding/aurora/locales/en-US/brand.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY  brandShortName  "Aurora">
>  <!ENTITY  brandFullName   "Mozilla Aurora">
> +<!ENTITY  vendorShortName "Mozilla">
\ No newline at end of file

Nit: Please keep newline characters at the end of the files.

::: mobile/android/locales/en-US/chrome/about.dtd
@@ +18,5 @@
>  <!ENTITY aboutPage.rights.label                 "Know Your Rights">
>  <!ENTITY aboutPage.relNotes.label               "Release Notes">
>  <!ENTITY aboutPage.credits.label                "Credits">
>  <!ENTITY aboutPage.license.label                "Licensing Information">
> +<!ENTITY aboutPage.logoTrademark                "&brandShortName; and the &brandShortName; logos are trademarks of the Mozilla Foundation.">

I'm sorry, my last comment wasn't clear. I think you should keep the word Firefox in here, since this sentence is explicitly about the word "Firefox".

I was just suggesting that you add a localization comment so that localizers aren't confused about why this isn't &brandShortName;.

Here are some examples of how we format localization notes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/aboutFeedback.dtd#17

Comment 14

[Julien Sanchez]

Attachment: patch-680392-3.patch

My bad, i read it to fast.

I made the comment on the locales file.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8484477 [details] [diff] [review]
patch-680392-3.patch

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

Excellent, thank you! I can land this for you

Comment 16

[:Margaret Leibovic]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ace87ff1c55