701182 - Additional descriptive text for Help > About $PRODUCTNAME in Aurora and Nightly

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tom Lowenthal]

The following text should be added to the About boxes in both Aurora and Nightly, in preparation for the upcoming Telemetry-by-default in these testing releases.

$PRODUCTNAME is experimental software and things break from time to time. $PRODUCTNAME automatically sends useful test information back to Mozilla so that we can make Firefox better.

Comment 1

[Tom Lowenthal]

Just a comment on the "good first bug" whiteboard: this needs to land before the next merge.

Comment 2

[:Margaret Leibovic]

Okay, I can be on the hook for doing it if no one else does - I just figured it would be an easy way for a new person to get involved.

Comment 3

[Théo Chevalier]

Hi everybody ! I'm just a beginner, and I'm looking to solve a first bug in order to progress and learn. I can, if you like, try to provide a patch tonight.

Feel free to tell me if I do something clumsy, I have not yet become accustomed.

Also, I probably would ask you questions right here.

Comment 4

[Théo Chevalier]

I need some help.

I identified the files where I'm going to apply my patches:
mozilla-central\browser\base\content\aboutDialog.js
mozilla-central\browser\base\content\aboutDialog.xul

But I haven't found where I should place the text itself. The texts are they defined in a location directly in mozilla-central?

Comment 5

[Théo Chevalier]

Ok, I've found it ! :)

mozilla-central\browser\locales\en-US\chrome\browser\aboutDialog.dtd

I'm working on the patch.

Comment 6

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window

First patch, I have not tested yet. I run a compilation.

I am sure that everything is not perfect, I await your comments!

Comment 7

[Théo Chevalier]

Looks good, but may be I should remove the space between the two sentences?

Then it will update mozilla-central\browser\branding\nightly\content\about-background.png

Comment 8

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V2

I've improve the first patch:

I corrected the "else" who has incorrect syntax, and the comment who was illogic.

PATCH V1:
-  // Include the build ID if this is an "a#" (nightly or aurora) build
+  // Include the build ID and warning if this is an "a#" (nightly or aurora) build
   let version = Services.appinfo.version;
   if (/a\d+$/.test(version)) {
     let buildID = Services.appinfo.appBuildID;
     let buildDate = buildID.slice(0,4) + "-" + buildID.slice(4,6) + "-" + buildID.slice(6,8);
     document.getElementById("version").textContent += " (" + buildDate + ")";
   }
+  else
+  {
+  	document.getElementById("warningDescExperimental").hidden = true;
+	document.getElementById("warningDescDatas").hidden = true;
+  }

PATCH V2:
-  // Include the build ID if this is an "a#" (nightly or aurora) build
+  // Include the build ID if this is an "a#" (nightly or aurora) build and hide warning if it isn't
   let version = Services.appinfo.version;
   if (/a\d+$/.test(version)) {
     let buildID = Services.appinfo.appBuildID;
     let buildDate = buildID.slice(0,4) + "-" + buildID.slice(4,6) + "-" + buildID.slice(6,8);
     document.getElementById("version").textContent += " (" + buildDate + ")";
   }
+  else {
+  	document.getElementById("warningDesc").hidden = true;
+  }


Also, I've deleted the carriage return between the two sentences, in order to have a single paragraph.

I will provide a screenshot of the result.

Comment 9

[Théo Chevalier]

Attachment: Result of the patch V2

Here is the screenshot of the result.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 574191 [details] [diff] [review]
Patch to add warnings in Nightly and Aurora in About window V2

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

Congratulations on your first patch. I've looked it over and provided some general feedback on your patch so far. When you are happy with the state of the patch, request feedback from one of the browser peers: https://wiki.mozilla.org/Modules/Firefox

::: browser/base/content/aboutDialog.js
@@ +73,5 @@
>      document.getElementById("version").textContent += " (" + buildDate + ")";
>    }
> +  else {
> +  	document.getElementById("warningDesc").hidden = true;
> +  }

Switching the warning description to be hidden by default will require adjusting this code to only show the warning description when running nightly or aurora.

::: browser/base/content/aboutDialog.xul
@@ +121,5 @@
>            </description>
>  #endif
> +		  <description class="text-blurb" id="warningDesc">
> +            &warning.desc;
> +          </description>

I think we should make this |hidden="true"| by default since it will be hidden for the majority of our users.

Comment 11

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V3

Thanks a lot for your feedback, Jared ! :) It's not much, but I'm really happy to finally make my contribution to the Mozilla project.

I have considered your comments, so I asked to me as you have suggested, a review for this patch V3.

Comment 12

[:Margaret Leibovic]

Comment on attachment 574322 [details] [diff] [review]
Patch to add warnings in Nightly and Aurora in About window V3

Thanks for the patch, indeed! I just have two quick drive-by comments:

1) It looks like you used tabs for indentation, but we generally use spaces, so you'll want to update that for consistency (I've changed the setting in my text editor to use spaces instead of tab characters).

2) This additional text increases the height of the about dialog. Because of the dark black overlay at the bottom of box it's hard to tell that the background image is cut off, but it is. This doesn't strike me as a huge deal, since this is just for Nightly and Aurora, but maybe we can file a follow-up bug to have a taller background image.

Comment 13

[Théo Chevalier]

Oops, indeed, the tabs have crept into my code, thank you for the tip of the editor, it's not stupid! I'll post the new patch.

I have already planned to file a bug to the bottom of the box, as soon as my patch is validated by a reviewer (the final height of the box depends on my patch).

Maybe you want me to file it now?

Comment 14

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V4

I removed tab characters

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 574371 [details] [diff] [review]
Patch to add warnings in Nightly and Aurora in About window V4

Thanks for the patch Théo! A few comments:

- you can set hidden="true" on the warningDesc <description>, to avoid the need to hide it programmatically in init()
- there's a leftover "Firefox" in the warning.desc string that should be &brandShortName;
- the "Mozilla" in warning.desc should probably be changed to &vendorShortName;
- this whole warning about telemetry should probably be put behind MOZ_TELEMETRY_REPORTING ifdef, to match the actual telemetry code. This makes the patch slightly more complicated since you need to split the description in two somehow, but it shouldn't be too difficult.

r- since we need to sort out these issues. Do feel free to find me on IRC (I'm "gavin" in #fx-team) or ask questions here if you need any clarification.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #15)
> - the "Mozilla" in warning.desc should probably be changed to
> &vendorShortName;

Hmm, or maybe it should use toolkit.telemetry.server_owner.

Comment 17

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V5

Thank you for the improvements Gavin!

I tried to use &toolkit.telemetry.server_owner; but it doesn't work after the compilation (I don't know why), so I used &vendorShortName; who works.

An other thing, on my build, with telemetry activated, MOZ_TELEMETRY_REPORTING ifdef isn't recognized. I guess it's because MOZ_TELEMETRY_REPORTING is defined ONLY on Mozilla official build, right?

Apart from this, everything seems to work.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 575270 [details] [diff] [review]
Patch to add warnings in Nightly and Aurora in About window V5

Could we get a little wordsmithing from UX? This makes the about dialog feel like it's got an awful lot of text in it, and I'd like to think that our cunning wordsmiths can pare that down.

Also, what's the reasoning for wanting to put this in the about dialog, instead of something like about:rights or about:license?

Comment 19

[Alex Limi (:limi) — Firefox UX Team]

I agree that this is starting to be a bit excessive. CCing our writer to see if there's a way to improve the situation.

Comment 20

[Matej Novak [:matej]]

Here's the bare bones of what I think this could say:

$PRODUCTNAME is experimental and unstable. It automatically sends test information back to Mozilla to help make Firefox better.

Mozilla is a global community working together to keep the Web open, public and accessible to all.


We should probably update this for Beta and GA as well:

$PRODUCTNAME is designed by Mozilla, a global community working together to keep the Web open, public and accessible to all.


This is beyond the scope of this bug, but we should also update $PRODUCTNAME to Firefox Nightly, Firefox Aurora, etc. and use the new and complete wordmarks (http://www.mozilla.org/en-US/firefox/brand/identity/wordmarks/) at the top of the About dialog boxes (including Beta and GA). Is that possible and what's the best way to proceed?

Comment 21

[:Margaret Leibovic]

(In reply to Matej Novak [:matej] from comment #20)
> This is beyond the scope of this bug, but we should also update $PRODUCTNAME
> to Firefox Nightly, Firefox Aurora, etc. and use the new and complete
> wordmarks (http://www.mozilla.org/en-US/firefox/brand/identity/wordmarks/)
> at the top of the About dialog boxes (including Beta and GA). Is that
> possible and what's the best way to proceed?

Bug 656518 was filed about that, but it doesn't look like anyone did any work in there. If you provide more details in there, it should probably be easy for someone to pick up. Maybe Théo would help you out :)

Comment 22

[Théo Chevalier]

Yep, I would help with pleasure, you just need to tell me what should I do, I'm afraid of not knowing what to do according to the latest comment.

To put it in a nutshell, I should update all $PRODUCTNAME, in bug 656518, and what do I do in this bug?

Do we need to wait to decide where put the warnings, or I'll just have to update the current patch with Matej's proposition?

Comment 23

[:Margaret Leibovic]

(In reply to Théo Chevalier from comment #22)
> Yep, I would help with pleasure, you just need to tell me what should I do,
> I'm afraid of not knowing what to do according to the latest comment.

Awesome! We'll definitely help you out :)

> To put it in a nutshell, I should update all $PRODUCTNAME, in bug 656518,
> and what do I do in this bug?

Yeah, updating $PRODUCTNAME and wordmarks should happen in bug 656518 (we should discuss an approach to that over in that bug, just to keep these tasks separate).

In this bug, I believe your current approach is still correct, but we're just increasing the scope a bit to also update the community strings, so that the text of the about dialog matches what Matej specified in comment 20.

> Do we need to wait to decide where put the warnings, or I'll just have to
> update the current patch with Matej's proposition?

I think we'll still be putting the warnings above the community text in Nightly/Aurora like you're doing in your current patch. The one thing that may be complicated is changing the community blurb based on the channel, but we could probably use the same approach to do that.

Comment 24

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V6

Patch updated from Matej proposition https://bugzilla.mozilla.org/show_bug.cgi?id=701182#c20.

So, now, background image height is ok (at least in en-US).

Comment 25

[Théo Chevalier]

Attachment: Result of the patch V6

Result of the patch on Firefox Nightly with telemetry enabled.

(Just focus on the warnings and the community strings, new wordmark and $PRODUCTNAME are relative to bug 656518)

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

I have assigned this to Théo as it appears that he is working on this. 

I'm trying to keep the status of our mentored bugs up to date. Please unassign if this was done in error.

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Limi - Looks like the patch is waiting on your review. Can you please take a look so that we can land this change before the cut over to Aurora?

Comment 28

[:Margaret Leibovic]

Comment on attachment 576592 [details]
Result of the patch V6

I think what we need is ui-review from Limi, not code review, so I'm flagging the screenshot.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 576591 [details] [diff] [review]
Patch to add warnings in Nightly and Aurora in About window V6

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js

>+    document.getElementById("warningDesc").hidden = false;
>+    document.getElementById("communityExperimentalDesc").hidden = false;

Can you put both of these descriptions in a <vbox id="experimental">, and just hide that, to avoid needing to toggle both individually? You could also use a <deck> for this and switch its selectedIndex, but that's probably overkill.

>diff --git a/browser/locales/en-US/chrome/browser/aboutDialog.dtd b/browser/locales/en-US/chrome/browser/aboutDialog.dtd

>+<!-- LOCALIZATION NOTE (community.Exp.MozillaLink): This is a link title that links to http://www.mozilla.org/. -->
>+<!ENTITY community.Exp.MozillaLink  "&vendorShortName;">
>+<!ENTITY community.Exp.Middle2      " is a ">
>+<!-- LOCALIZATION NOTE (community.Exp.CreditsLink): This is a link title that links to about:credits. -->
>+<!ENTITY community.Exp.CreditsLink  "global community">
>+<!ENTITY community.Exp.End2         " working together to keep the Web open, public and accessible to all.">

You should keep the string names lowercase to match the other strings in this file. You don't need the "2" suffix for new strings, those are just there because these strings were changed and they needed to change the string name accordingly.

>-<!ENTITY community.end2             " working together to make the Internet better. We believe that the Internet should be open, public, and accessible to everyone without any restrictions.">
>+<!ENTITY community.end2             " working together to keep the Web open, public and accessible to all.">

This string name needs to change, since the string value is changing. I recommend using "community.end3".

r- for these minor changes, but I'll r+ a patch that addresses them!

Comment 30

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V7

Corrected ! :)

Comment 31

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V8

(Tab char removed.)

Comment 32

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 576592 [details]
Result of the patch V6

This looks fine, although I think "is experimental and unstable" is a bit strong, and something like "may be unstable" would be better — but don't let this block landing the patch. :)

Comment 33

[Théo Chevalier]

No problem Alex, I can change it immediately :)

Comment 34

[Théo Chevalier]

Attachment: Patch to add warnings in Nightly and Aurora in About window V9

Accordingly to Alex comment (comment 32), I've changed
&brandShortName; is experimental and unstable.
into:
&brandShortName; is experimental and may be unstable.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Thanks again for the patch, Théo!

I've made a few little tweaks:
- added some metadata to the diff (see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed for details on how to do that generally)
- added a community.exp.start, even though it's blank for en-US, because other locales might require it.
- fixed a small mistake in addressing my last comment - you needed to change the existing string's name to community.end3, not the new one that you're adding.
- added some additional detail to the localization notes

with those changes, I'm granting r+, and I'll go ahead and push this to inbound for you.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63d94ab75606

This should be merged to inbound within 24 hours - once that happens we'll mark this FIXED and it should be in the next Nightly!

Comment 37

[Théo Chevalier]

Thanks Gavin, great job ! :) It's a good news.
With this patch, background is broke again. I'll open a bug to have a bigger background (I'll calculate how exactly should be the new height. Maybe 2 new lines will be a good margin.)

What about 699806 and 656518 ? Too late for this merge, no?
Or 699806 will follow this patch because it's related?

BTW, your article about metadata is very interesting, I'll take some notes ;) And if it's not already done, it should be on MDN.

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/63d94ab75606

Comment 39

[Francesco Lodolo [:flod]]

Isn't "test information" a bit too cryptic, also considering that telemetry always (AFAIK) refers to "performance date"?

Comment 40

[Théo Chevalier]

(In reply to flod (Francesco Lodolo) from comment #39)
> Isn't "test information" a bit too cryptic, also considering that telemetry
> always (AFAIK) refers to "performance date"?

This is exactly how Telemetry is describded in the user prompt:
Will you help improve %1$S by sending anonymous information about performance, hardware characteristics, feature usage, and browser customizations to %2$S?

So telemetry is wider than performances datas sending, I guess.

Comment 41

[Tom Lowenthal]

(In reply to flod (Francesco Lodolo) from comment #39)
> Isn't "test information" a bit too cryptic, also considering that telemetry
> always (AFAIK) refers to "performance date"?

"Test information" is the language that we've decided to use to encompass all of telemetry's data-gathering.