Closed
Bug 680392
Opened 13 years ago
Closed 10 years ago
Non localized trademark notice string in about:firefox
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: flod, Assigned: ju.sanchez9, Mentored)
Details
(Keywords: l12y, productization, Whiteboard: [good first bug][lang=html])
Attachments
(2 files, 2 obsolete files)
198.11 KB,
image/png
|
Details | |
5.97 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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.
Reporter | ||
Comment 2•13 years ago
|
||
(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.
Reporter | ||
Comment 3•11 years ago
|
||
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
Updated•11 years ago
|
Keywords: l12y,
productization
Product: Fennec Graveyard → Firefox for Android
Summary: Non localized string in about Window → Non localized trademark notice string in about Window
Comment 4•11 years ago
|
||
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.
Flags: needinfo?(francesco.lodolo)
Reporter | ||
Comment 5•11 years ago
|
||
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
Flags: needinfo?(francesco.lodolo)
Updated•11 years ago
|
Mentor: margaret.leibovic
Whiteboard: [good first bug][lang=html]
I would like to work on this bug. As I am a newbie could some one mentor me in fixing the issue.
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
(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!
Summary: Non localized trademark notice string in about Window → Non localized trademark notice string in about:firefox
Updated•10 years ago
|
Assignee: nobody → ju.sanchez9
Comment 10•10 years ago
|
||
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.
Attachment #8481778 -
Flags: review?(margaret.leibovic)
Comment 11•10 years ago
|
||
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.
Attachment #8481778 -
Flags: review?(margaret.leibovic) → feedback+
Assignee | ||
Comment 12•10 years ago
|
||
Thanks for the review. I've made changes for the build flag, and for the brand name on locales.
Attachment #8481778 -
Attachment is obsolete: true
Attachment #8483680 -
Flags: review?(margaret.leibovic)
Comment 13•10 years ago
|
||
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
Attachment #8483680 -
Flags: review?(margaret.leibovic) → feedback+
Assignee | ||
Comment 14•10 years ago
|
||
My bad, i read it to fast.
I made the comment on the locales file.
Attachment #8483680 -
Attachment is obsolete: true
Attachment #8484477 -
Flags: review?(margaret.leibovic)
Comment 15•10 years ago
|
||
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
Attachment #8484477 -
Flags: review?(margaret.leibovic) → review+
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•