Closed Bug 616193 Opened 9 years ago Closed 9 years ago

Trademark attribution for Charlton Company should only appear in en-US and en-GB

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: beltzner, Assigned: Gavin)

References

Details

(Whiteboard: [softblocker])

Attachments

(3 files, 2 obsolete files)

As per a recent update to our agreement with the Charlston Company, trademarkinfo.part2 only needs to appear in the en-US and en-GB builds. I suggest the easiest way to accomplish this would be for the other locales to leave the entity as an empty string.
blocking2.0: --- → final+
If we don't want it localized, and only want it in specific locales, the most reliable solution is to not make it a localizable string, and insert it at run time based on a current-locale check.
Summary: Trademark attribution for Charlston Company should only appear in en-US and en-GB → Trademark attribution for Charlton Company should only appear in en-US and en-GB
To clarify, this does not affect en-ZA, and wouldn't affect en-CA if we had one?

Also, an en-US build with a language pack is not affected?

And yes, leaving it empty based on comments or bugs or somesuch is gonna be brittle.
Assignee: nobody → gavin.sharp
I don't think the "theoretical-future en-* locales" or "en-US with a language pack" corner cases are particularly important, the main goal is to cover the en-* builds that we currently release.

I believe the patch that I'm going to attach will show the attribution text for our en-US and en-GB builds in their default state, or any other build with a en-GB or en-US language pack installed, which I think is sufficient.
Attached patch patch (obsolete) — Splinter Review
Attachment #494759 - Flags: review?(dolske)
Attachment #494759 - Flags: feedback?(l10n)
Comment on attachment 494759 [details] [diff] [review]
patch

Yeah, sounds like the right concept. Should we landle license vs licence (en-GB)?
Attachment #494759 - Flags: feedback?(l10n) → feedback+
Don't think that's worth the effort.
Comment on attachment 494759 [details] [diff] [review]
patch

>+++ b/browser/base/content/aboutDialog.js
...
>+#ifdef MOZ_OFFICIAL_BRANDING
>+  // Add Charlton trademark attribution, but only for en-US and en-GB

Johnath's original email on the subject suggested adding a "DO NOT REMOVE" comment, worth adding?

>+  let chromeRegistry = Cc["@mozilla.org/chrome/chrome-registry;1"].
>+                       getService(Ci.nsIXULChromeRegistry);
>+  let currentLocale = chromeRegistry.getSelectedLocale("global");
>+  if (currentLocale == "en-US" || currentLocale == "en-GB") {
>+    document.getElementById("extra-trademark").value = "Some of the trademarks used under license from The Charlton Company.";
>+  }

How about making this more idiot-proof by placing the text into the XUL, and setting .hidden = true here (when it's not en-US / en-GB)? Pehaps I'm just being overly paranoid. :)


>+++ b/other-licenses/branding/firefox/locales/en-US/brand.dtd
>@@ -1,5 +1,4 @@
> <!ENTITY  trademarkInfo.part1   "Firefox and the Firefox logos are trademarks of the Mozilla Foundation.">
>-<!ENTITY  trademarkInfo.part2   "Some of the trademarks used under license from The Charlton Company.">

Change the entity name for .part1? Could be a locale that swapped the order of the two parts, thinking they're a single-but-split unit, and so this would result in dropping the MoFo trademark notice?

Or is it assumed that when .part2 is removed, the l10n tools will alert on that and localizers will fix any .part1 issue?
Attachment #494759 - Flags: review?(dolske) → review+
(In reply to comment #7)
> Change the entity name for .part1? Could be a locale that swapped the order of
> the two parts, thinking they're a single-but-split unit, and so this would
> result in dropping the MoFo trademark notice?

I think the odds of a locale having switched the order is pretty low, and I fear that making this a more complicated change than just a string removal will actually increase the possibility of something being handled wrong (more involved change for localizers -> more error-prone). Would be interested in Axel's opinion on this I guess.
I've looked at mxr, and there are only 4 locales where it's not apparent that part2 talks about Charlton.

That's ar, pa-IN, ml, te, let's just get bugs on file to verify for those.
Attached patch updated patchSplinter Review
Attachment #494759 - Attachment is obsolete: true
Whiteboard: [softblocker]
https://hg.mozilla.org/mozilla-central/rev/be63daa6e0f8
Hardware: x86 → All
Target Milestone: --- → Firefox 4.0b10
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Depends on: 625597
Depends on: 625598
Depends on: 625599
Depends on: 625601
(In reply to comment #9)
> That's ar, pa-IN, ml, te, let's just get bugs on file to verify for those.

Filed those (see dependencies).
Building with official branding doesn't remove the Charlton trademark attribution for sv-SE. Error console reports that "Cc is not defined" in aboutDialog.js.

Changing Cc to Components.classes, and Ci to Components.interfaces, removes the extra-trademark label.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Add Cc, Ci (obsolete) — Splinter Review
Was surprised to hear this, since it pulls in Services.jsm, but then I realized that it doesn't export Cc, Ci.
Attachment #504492 - Flags: review?(gavin.sharp)
Comment on attachment 504492 [details] [diff] [review]
Add Cc, Ci

(In reply to comment #7)

> How about making this more idiot-proof by placing the text into the XUL, and
> setting .hidden = true here (when it's not en-US / en-GB)? Pehaps I'm just
> being overly paranoid. :)

Apparently I am not overly paranoid. ;)

Tested patch, confirmed it fixes the problem.
Attachment #504492 - Flags: review?(gavin.sharp) → review+
The reason I didn't catch this is that I tested on Mac, where this works as-is (presumably because of the menu overlay). Oddly enough, that patch doesn't seem to break things on Mac, though, which I don't quite understand (I would expect a redeclaration error). To play it safe, I think we're better off just not relying on the shorthands.
Attachment #504492 - Attachment is obsolete: true
Pushed fix:

http://hg.mozilla.org/mozilla-central/rev/6cee9f946cbe
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.