Closed Bug 836451 Opened 7 years ago Closed 7 years ago

Add distribution info to about:firefox

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: Margaret, Assigned: capella)

References

Details

Attachments

(7 files, 6 obsolete files)

Desktop Firefox includes distribution information in the about dialog. I think it makes sense for us to do something similar in our about page.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.xul#46
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.js#16
I see that information being collected, but not displayed or used by desktop (I'm not really a JS expert ... maybe I missed something).

In either case, would you still like this for mobile?
Are you using a distribution build? This information is only there in that case.

E.g. if you're using Firefox with Twitter [1], you'll see "Mozilla Firefox with Twitter Search / twitter - 1.0" in the about dialog: http://cl.ly/image/3n2w2o1I3c1x

[1] https://twitter.com/download/firefox/
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
Is this something like you're looking for? 

Mobile seems to have an id and version available, while desktop has an additional complex var |about| ... 

I added an aboutDistribution div and show either that or aboutTelemetry based on whether the distribution.json file exists and dummied up one of those for testing ...
Attachment #709416 - Flags: feedback?(margaret.leibovic)
Attached image Sample Screenshot
Comment on attachment 709416 [details] [diff] [review]
Patch (v1)

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

Nice! This is definitely the right idea. I'd like to get some UX feedback to see if there's anything about the styling that could be improved.

::: mobile/android/chrome/content/about.xhtml
@@ +94,5 @@
> +          var distroVersion = Services.prefs.getCharPref("distribution.version");
> +          var distroIdField = document.getElementById("distributionId");
> +          distroIdField.innerHTML = distroId + " - " + distroVersion;
> +          document.getElementById("aboutDistribution").hidden = false;
> +          document.getElementById("aboutTelemetry").hidden = true;

Why hide the aboutTelemetry div?

@@ +97,5 @@
> +          document.getElementById("aboutDistribution").hidden = false;
> +          document.getElementById("aboutTelemetry").hidden = true;
> +        }
> +      }
> +      catch (e) {

Nit: Put the catch on the same line as the close brace.

::: mobile/android/themes/core/aboutPage.css
@@ +82,5 @@
> +  margin: 40px auto 0 auto;
> +  padding: 10px 0;
> +  font-weight: bold;
> +  font-size: 15px;
> +  text-align: center;

Do you need this as well as the margin-left/margin-right auto values? Or could you just do width 100% and not worry about the margins?

(It looks like you modeled this after the #aboutTelemetry styles, so this isn't really your fault. I'm just curious for the reasoning behind it.)
Attachment #709416 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 709417 [details]
Sample Screenshot

Ian, what do you think about this for distribution info? Thinking about it a bit, I feel like it should be closer to the version number, like it is on desktop, since the distribution information is about which Firefox you're running.
Attachment #709417 - Flags: feedback?(ibarlow)
Attached patch Patch (v2) (obsolete) — Splinter Review
Second version, will display Telemetry and / or Distribution info ... nit corrected ... 

I'm not what you're asking re: the width / |auto| question ... maybe the attached is cleaner and answers your questions ... but setting width to 70% puts the elements in smaller box than full screen with, setting |margin: 0 auto;| centers that element, and setting |text-align: center| further centers text inside the element ...

Let me know :)

Screenshots follow ...
Attachment #709416 - Attachment is obsolete: true
Attached image New screenshot
I might flip it so the dist. info is above the disclaimer, and them make it Regular and not Bold.
Attached patch Patch (v3) (obsolete) — Splinter Review
Elements reversed, and bold removed ....
Attachment #709823 - Attachment is obsolete: true
Attachment #710479 - Flags: review?(margaret.leibovic)
Comment on attachment 710479 [details] [diff] [review]
Patch (v3)

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

This looks pretty good to be, but unfortunately the #logo element overlaps the text when there's no update button. I suspect that if we can just make sure the text appears on top of the logo it will look good. It's an edge case that the updater wouldn't be included with a build, but it's still possible, so we should see if we can fix this problem.

::: mobile/android/chrome/content/about.xhtml
@@ +38,5 @@
>      </div>
>  #endif
>  
> +    <div id="messages">
> +      <div id="aboutDistribution" hidden="true">

Is there a reason why you need these divs to wrap these p elements? Seems to me like you could just have the p elements on their own (although you'd need to update the CSS selectors to look for their ids).
Attachment #710479 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Patch (v4) (obsolete) — Splinter Review
I had book-ended the <p> element with <div>'s to be consistent with the existing style. It's not really needed for the aboutTelemetry nor the new aboutDistribution elements, so I removed both in this patch.

re: disappearing text over the semi-transparent .png image .... >OOOPS< ... I "lost" 
the |position: absolute| parm while shifting things around, and this was the result.

Note in this version I also removed the |min-height| from the banner, to better style the smaller-sized edge case. I don't see why it was required, but I'm not a CSS expert ... graphics / layouts are a little more into UI design than I usually get, so small exercises like this are good learning experiences to me.
Attachment #710479 - Attachment is obsolete: true
Attachment #711155 - Flags: review?(margaret.leibovic)
Comment on attachment 711155 [details] [diff] [review]
Patch (v4)

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

Definitely much better, but there are some things I didn't notice until doing a more thorough review right now. Sorry for the extra cycles! With these issues addressed, I think this patch will be good to land.

::: mobile/android/chrome/content/about.xhtml
@@ +90,5 @@
> +      try {
> +        var distroId = Services.prefs.getCharPref("distribution.id");
> +        if (distroId) {
> +          var distroVersion = Services.prefs.getCharPref("distribution.version");
> +          var distroIdField = document.getElementById("aboutDistribution");

Please use let instead of var.

@@ +92,5 @@
> +        if (distroId) {
> +          var distroVersion = Services.prefs.getCharPref("distribution.version");
> +          var distroIdField = document.getElementById("aboutDistribution");
> +          distroIdField.innerHTML = distroId + " - " + distroVersion;
> +          distroIdField.hidden = false;

I also just realized you're missing a place to put the distibution.about string. See the desktop code for an example of how we do it there:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.js#16

The about pref is required, so you can expect it to always be there:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#8191

::: mobile/android/themes/core/aboutPage.css
@@ -27,5 @@
>  }
>  
>  #banner {
>    margin: 0 -10px;
> -  min-height: 150px;

I think we should keep the min-height, if only because it's out of scope for this bug. We can file a follow-up for that if we want to make more style tweaks.

@@ +83,1 @@
>    text-align: center;

I think we should just keep the original #aboutTelemetry style rules and just apply them to #messages instead, since you're basically just renaming #aboutTelemetry and sticking another <p> inside it (the default p styles will take care of margins between the two messages).
Attachment #711155 - Flags: review?(margaret.leibovic) → review-
Attachment #709417 - Flags: feedback?(ibarlow)
Attached patch Patch (v5) (obsolete) — Splinter Review
I had wondered where that |about| field was supposed to come from, but apparently didn't dig far enough to realize there were two files involved:

   /data/data/org.mozilla.fennec/distribution.json
   /data/data/org.mozilla.fennec/distribution/preferences.json

My understanding here might not yet be complete, re: timing / origination of the "prefservice:after-app-defaults" message. I didn't see it being triggered in my testing (nor data being retrieved from the preferences.json file).

In the first screenshot I hard-coded a value in place of the expected data. The second screenshot is the patch as attached, where the Try / Catch fails and no data is displayed for that line.
Attachment #711155 - Attachment is obsolete: true
Attached patch Patch (v5.1) (obsolete) — Splinter Review
Forgot to QREF
(In reply to Mark Capella [:capella] from comment #16)

> I had wondered where that |about| field was supposed to come from, but
> apparently didn't dig far enough to realize there were two files involved:
> 
>    /data/data/org.mozilla.fennec/distribution.json

You have to blame mfinkle for this one. This file is just used for tracking marketing campaigns from the play store, and we (ab)use the distribution prefs to keep track of this.

> My understanding here might not yet be complete, re: timing / origination of
> the "prefservice:after-app-defaults" message. I didn't see it being
> triggered in my testing (nor data being retrieved from the preferences.json
> file).

How are you trying to test distribution data? Did you create a /distribution/preferences.json in your objdir before packaging? The way the code works is we look for distribution files in the APK, then copy them over to the /data directory for Gecko to use. Bug 834681 should have some more details about this, but if you're just putting a file directly in your /data directory, we won't be looking for them to use.
(In reply to :Margaret Leibovic from comment #19)

> Bug 834681 should have some more
> details about this, but if you're just putting a file directly in your /data
> directory, we won't be looking for them to use.

Actually, this change came from bug 836838. Maybe that caused some confusion.
Ah yes, I built the distribution.json in my /data folder. It did occur to me I was missing something involving installation process ... thanks for explaining it. 

Let's see if I can get a better end-to-end test including that for my comfort level, then I'll flag for review?
Comment on attachment 711586 [details] [diff] [review]
Patch (v5.1)

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

Looking good! Just two small things, but this is ready to land with these comments addressed.

::: mobile/android/chrome/content/about.xhtml
@@ +40,5 @@
>  
> +    <div id="messages">
> +      <p id="aboutDistribution" hidden="true"/>
> +      <p id="aboutDistributionID" hidden="true"/>
> +      <p id="aboutTelemetry" hidden="true">

Nit: Can we call these distributionAbout/distributionId/telemetry? The "about" in front of all of them seems redundant :)

@@ +91,5 @@
> +        let distroId = Services.prefs.getCharPref("distribution.id");
> +        if (distroId) {
> +          let distroVersion = Services.prefs.getCharPref("distribution.version");
> +          let distroIdField = document.getElementById("aboutDistributionID");
> +          distroIdField.innerHTML = distroId + " - " + distroVersion;

To be on the safe side, I think we should use .textContent here instead of .innerHTML.

@@ +96,5 @@
> +          distroIdField.hidden = false;
> +
> +          let distroAbout = Services.prefs.getComplexValue("distribution.about", Ci.nsISupportsString);
> +          let distroField = document.getElementById("aboutDistribution");
> +          distroField.innerHTML = distroAbout;

Same here.
Attachment #711586 - Flags: review+
(In reply to Mark Capella [:capella] from comment #21)
> Ah yes, I built the distribution.json in my /data folder. It did occur to me
> I was missing something involving installation process ... thanks for
> explaining it. 
> 
> Let's see if I can get a better end-to-end test including that for my
> comfort level, then I'll flag for review?

Oops, I already reviewed it :) But feel free to flag me again when you post a new version.

Today or tomorrow I intend to flesh out the distribution documentation to make it more obvious how to set all this up :)
https://wiki.mozilla.org/Mobile/Distribution_Files
Attached patch Patch (v6)Splinter Review
This *so* works :D

I created a folder |distribution| under mozilla-central/mobile/android/app with the file preferences.json containing:

{ "Global" : { "id" : "Preferences ID value", "version" : "Preferences VERSION value", "about" : "Preferences ABOUT value" } }

And it built / packaged / installed and displayed quite nicely !
Attachment #711577 - Attachment is obsolete: true
Attachment #711586 - Attachment is obsolete: true
Attachment #711631 - Flags: review?(margaret.leibovic)
Attached image End to end test
Comment on attachment 711631 [details] [diff] [review]
Patch (v6)

Nice work! This is great.
Attachment #711631 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/a74d4bc64a70
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.