build about:healthreport to display submission info and embed the report itself

RESOLVED FIXED in Firefox 20

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mreid, Assigned: mconnor)

Tracking

unspecified
Firefox 21
Points:
---
Dependency tree / graph
Bug Flags:
sec-review ?

Firefox Tracking Flags

(firefox20 fixed)

Details

(Whiteboard: [sec-assigned:dveditz])

Attachments

(2 attachments, 14 obsolete attachments)

18.03 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
1.16 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
Created attachment 589893 [details] [diff] [review]
Basic about:metrics functionality

Create an about:metrics page with detailed information about the Metrics Data Ping.  

It should include the following:

An explanation of the benefit of data collection, to improve the product.

A detailed description of each data point, as well as displaying the actual data submitted to the server, and display of the cumulative data stored on Mozilla servers.

Language-specific strings externalized for translation

Future enhancements: visualizations, client comparisons showing where their data fits in with various aggregate data slices.
(Reporter)

Updated

6 years ago
Assignee: nobody → mreid
(Reporter)

Updated

6 years ago
Blocks: 718066
(Reporter)

Comment 1

6 years ago
Comment on attachment 589893 [details] [diff] [review]
Basic about:metrics functionality

Axel, please focus on the L10n aspect of the patch - the UI / content are subject to change, but the functionality and approach to L10n need review.  Thanks.
Attachment #589893 - Flags: review?(l10n)

Comment 2

6 years ago
Can you attach a screen shot of how this would look? I think I like this, but it's easier to see pitfalls when seeing the result.

Thanks.
Created attachment 590322 [details]
Top half of about:metrics screenshot

Axel, two screenshots (it is currently a very long page) and a link to where we are prototyping the restructure of the page.
Created attachment 590326 [details]
Bottom half of about:metrics screenshot

You can see the prototype in Trevor Norris's sandbox here:
http://people.mozilla.com/~tnorris/about-metrics/

Comment 5

6 years ago
Comment on attachment 589893 [details] [diff] [review]
Basic about:metrics functionality

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

This looks good. I'd suggest to get the feedback from Matej on the language on the buttons etc.

::: browser/locales/en-US/chrome/browser/aboutMetrics.dtd
@@ +1,1 @@
> +<!-- metrics.locale-direction instead of the usual local.dir entity, so RTL can skip translating page. -->

A localization note here that's a bit more extensive that localizers might want to skip this would be great.
Attachment #589893 - Flags: review?(l10n) → feedback+
For my sanity, here' a basic roadmap (versions are arbitrary):

v0.1
- Explanation of metrics being collected
- Table with list of metrics and descriptions
- "Opt Out" button on the page (explanation paragraph will then show that the user has opted out)
- If opt out, give user ability to opt back in
- Externalize all strings for localization

v0.2
- Populate metrics table with latest data submission (if it exists)
- Add buttons to display latest and aggregate metric data in well formatted JSON

vTBD
- Resolve addon id's to their names (but what to do if the addon has been removed?)
- Time Machine (feature): a user can step back to any date and see where their stats (e.g. how many/which addon were installed)
- Aggregate Graphs (feature): Display graphs of cumulative data from user's local machine.
  - enhancements: dual date range slider, arbitrary data overlays (e.g. crashes over addons installed)
- Cooperative Graphs (feature, dependent on Aggregate Graphs): Display statistics from other users
  - enhancements: choose region or demographic of data to display


Technology stack includes:
- CCC (for graphing)
- (? for AJAX calls, DOM manipulation, etc.?)

This is just a list of what I had going in my head, and would like feedback on what we want/when we want to accomplish.

Thanks

Comment 7

6 years ago
Be aware that including 3rd party web libraries is far from trivial in our MPL licensed binaries that we ship, and the GPL and LGPL licensed variants that other people are supposed to be able to ship. Licensing, code quality, review, security, all that.
(In reply to Axel Hecht [:Pike] from comment #7)
Noted. CCC was recommended from Daniel, so that's why it's been included in my tech stack. If it is a problem to bundle then I'm cool working either with something else, or building from scratch.
I agree with Axel on the complexity. My intent is actually for us to move the contents of the about:metrics page to be loaded from an external source similar to how the major update offer is done.  That should make it much more reasonable for us to deliver a rich page. Not sure how far that will get for the initial code drop which is why we are doing a code review on the internal version.
(Reporter)

Updated

6 years ago
Attachment #589893 - Flags: review?(Mnovak)
Could someone pull out just the copy that you'd like me to review? Thanks!
(Reporter)

Comment 11

6 years ago
(In reply to Matej Novak [:matej] from comment #10)
> Could someone pull out just the copy that you'd like me to review? Thanks!

Page Title: 
About Metrics Data Collection

Intro (opted in):
With your permission, Nightly collects some data about your computer and usage. We care about your privacy.  More information about your rights when using Nightly can be found here*. Just so we're all clear, here is a list of what is collected.

Intro (opted out):
Since we do not have yout permission, Nightly will not collect data about your computer and usage. You can opt back in and help us make Nightly better by setting the 'toolkit.metrics.ping.enabled' preference to 'true'.  We care about your privacy.  More information about your rights when using Nightly can be found here*. If data collection was enabled, here is a list of what would be collected.

Table Headings:
Data Point      Description

Buttons:
Show Previously Submitted Data
Show Cumulative Data
I'm not cool with this, forget my server data

*link to about:rights
(Reporter)

Comment 12

6 years ago
> Since we do not have yout permission
s/yout/your/
Thanks so much. I made a few really small edits to the below, but otherwise it looks really good.


Page Title: 
About Metrics Data Collection

Intro (opted in):
With your permission, Nightly collects some data about your computer and usage. We care about your privacy and you can find more information about your rights when using Nightly here*. Just so we're clear, here is a list of what is collected.

Intro (opted out):
Since we do not have your permission, Nightly will not collect data about your computer and usage. You can opt back in and help us make Nightly better by setting the "toolkit.metrics.ping.enabled" preference to "true."  We care about your privacy and you can find more information about your rights when using Nightly here*. If data collection were enabled, here is a list of what would be collected.

Table Headings:
Data Point      Description

Buttons:
Show previously submitted data
Show cumulative data
I'm not cool with this, forget my server data

*link to about:rights
Keywords: sec-review-needed
(Reporter)

Comment 14

6 years ago
Created attachment 595401 [details] [diff] [review]
Externalized about:metrics page

Content of the about:metrics page moved to an external website (URL subject to change).
Attachment #589893 - Attachment is obsolete: true
Attachment #589893 - Flags: review?(Mnovak)
(Reporter)

Comment 15

6 years ago
Created attachment 595480 [details] [diff] [review]
Externalized about:metrics page

Tiny correction
Attachment #595401 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #595480 - Flags: review?(curtisk)
(Reporter)

Comment 16

6 years ago
We would also like to add functionality on the page to
1. Opt out of data collection (in addition to the preferences pane)
2. Delete server-side data

Would it be best to extend the communications with the iframe to do that, or to extract that code to run from the client-side chrome?
(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #9)
> My intent is actually for us to move the contents of the about:metrics
> page to be loaded from an external source similar to how the major
> update offer is done.  That should make it much more reasonable for
> us to deliver a rich page.

The Major Update offer loads some data from our servers, it does not load code. The web content is displayed in a non-chrome-privileged frame that--just to be safe--has scripting completely disabled. This is probably not the model you want :-)

This page's functionality appears to require chrome privilege (it grabs the user's stored data) and loading remote chrome-privileged code is not going to pass a security review. AMO won't allow an addon that did that, either.

If it's absolutely necessary to host code on a server you'll have to display it in a "type=content" <browser> or <iframe> within your privileged about: page. If you need to get data from the client to the remotely hosted page (for graphing?) one way to do that would be through postMessage(). Another way might be to include the most-recently-sent documentID as a URL parameter so the page can get the data off the server and doesn't need to communicate with privileged code at all. That option means anyone can see any data if they happen to guess a documentID so there may be privacy implications with that route, although your current plan makes those anonymous and short-lived so maybe that's OK. If neither of those options are sufficient the security team will be happy to help you brainstorm other approaches to solving your problem.

Building up the page you display using innerHTML is dangerous. The local data is twitchy enough--it means we have to search pretty far back in the life of the data to ensure there are no escaping problems that won't show up until innerHTML brings it to life. Doing so with remotely loaded data (in the server-data case) is another way to run remote code with chrome privilege and isn't going to be allowed.
I was looking at the first patch based on an old copy of the page (yay sessionrestore?), sorry. Looks like some of my feedback got back to you guys before I got a chance to add them here. Will try to answer the questions in comment 16 when I get a chance later today.
Whiteboard: [secr:dveditz]
(In reply to Daniel Veditz from comment #17)
> Building up the page you display using innerHTML is dangerous. The local
> data is twitchy enough--it means we have to search pretty far back in the
> life of the data to ensure there are no escaping problems that won't show up
> until innerHTML brings it to life. Doing so with remotely loaded data (in
> the server-data case) is another way to run remote code with chrome
> privilege and isn't going to be allowed.

Any use of innerHTML is only temporary while I figure out the page layout. It will not be in use by security review.
Unfortunately, Trevor and Mark both don't have extensive experience in the sticky case of chrome/content communication, so we can definitely use guidance here.

This was brought up as a specific area that needed further attention in the security review with Curtis.  We told them that when we had a patch that at least demonstrated what we were trying to do, we would notify them so they could help us work the proper approach out and possibly get some additional guidance from experts.

After talking with Mark this morning, there is at least one more tweak (which will make things cleaner and simpler).  When that is posted, we'll reach out to Curtis as well as you or anyone else you feel would be suitable to make sure it is done right.
Attachment #595480 - Flags: review?(curtisk) → review?(dveditz)
Comment on attachment 595480 [details] [diff] [review]
Externalized about:metrics page

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

Wasn't sure if I was waiting for another patch per comment 20 but I'll add a couple of comments now.

Structurally this looks good. The website content is loaded in a type=content iframe and your communication is passing JSON blobs into the page -- good. The _way_ you do it, however, is unsafe. The various JS wrappers were added to provide security checks and by directly manipulating the wrappedJSObject you bypass all that goodness. Using SSL to load the page is good and /should/ prevent easy hack-injection, but if we don't have to rely on the server not being compromised let's not.

The good news is judging by its use your injectJSON() page function looks a lot like the postMessage() API which passes JSON blobs between windows and frames. Switch to that and the outer privileged script won't have to touch page content directly at all.

::: browser/base/content/aboutMetrics.xhtml
@@ +18,5 @@
> +  <script type="text/javascript;version=1.8" src="chrome://browser/content/aboutMetrics.js"/>
> +</head>
> +
> +<body id="metrics-data" onload="populateFrame();">
> +<iframe type="content" src="https://metrics.mozilla.com/about_metrics_test/" id="about-frame" />

You will eventually want the site URL to live in your aboutMetrics.dtd so it can be localized. Unless the page UI strings are bound up in your JSON blobs I'm assuming there's some sort of text on the page, and URLs like https://metrics.mozilla.com/en-US/about_metrics_test/ will let us localize pages as we have them and redirect to the English version for missing localizations.
Attachment #595480 - Flags: review?(dveditz) → review-

Comment 22

6 years ago
Actually, please don't put urls into l10n. Localizing those should work server-side, we're quite good at making that happen these days. Either by accept-lang, or in some parts we put the locale into the url via the url formatter. I prefer accept-lang, though.
Thanks, Axel, I'll keep that in mind for future reviews.
Whiteboard: [secr:dveditz] → [sec-assigned:dveditz]
Flags: sec-review?(dveditz)
Version: 12 Branch → unspecified
Has this had any input from UX?
Flags: needinfo?(mreid)
(Reporter)

Comment 25

6 years ago
> Has this had any input from UX?

Aside from the feedback on the text from Matej in Comment 13, we haven't had UX feedback on this.
Flags: needinfo?(mreid)

Updated

6 years ago
Component: General → General
Product: Toolkit → Firefox

Updated

6 years ago
Duplicate of this bug: 809089
I believe mconnor is tracking down UX resources.
Flags: needinfo?(mconnor)

Comment 28

6 years ago
I think there also needs to be a link to about:telemetry included with either a brief description of it, or an explanation of the differences between the two.
Created attachment 680868 [details] [diff] [review]
about:healthreport, v2

I refactored things to work with the current code base. A lot of the JS code went away because it has been incorporated into the main health reporter service API. I also renamed things to about:healthreport because the product name is "Firefox Health Report."

There are comments from the previous review that have not yet been addressed.
Attachment #595480 - Attachment is obsolete: true
Comment on attachment 680868 [details] [diff] [review]
about:healthreport, v2

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

::: browser/base/content/abouthealthreport/main.css
@@ +23,5 @@
> +
> +#user-options {
> +  width: 100%;
> +}
> +

Meant to give Mark the following snippet to add below #user-options for a slightly cleaner look:


#user-options td {
  width: 33%;
}

#user-options td:nth-child(2) {
  text-align: center;
}

#user-options td:nth-child(3) {
  text-align: right;
}

::: browser/base/content/abouthealthreport/main.js
@@ +32,5 @@
> +    let url = prefs.get("url").replace("%LOCALE%", getLocale());
> +
> +    let iframe = document.getElementById("about-frame");
> +    iframe.src = url;
> +    // populateFrame() is called from the iframe's onload handler.

As long as this javascript is loaded after the DOM, is a try/catch really necessary? If this exists because of the possibility of not being able to load the LOCALE, then I think the catch should load an error message that the page can't finish loading.

@@ +41,5 @@
> +
> +function populateFrame() {
> +  let iframe = document.getElementById("about-frame");
> +  if (!iframe.src || iframe.src.length == 0) {
> +     // Don't do anything until we have set the iframe src.

Since populateFrame() loads on the event handler, is this if() case even possible?
Created attachment 681770 [details] [diff] [review]
about:healthreport, v3

I have refactored the page so the tabs are local. When we need an iframe with remote-loaded content, we can load it inside one of the tabs.

Screenshots will follow.
Attachment #680868 - Attachment is obsolete: true
Created attachment 681772 [details]
Default state screenshot
Attachment #590322 - Attachment is obsolete: true
Created attachment 681773 [details]
"My Data" tab selected
Attachment #590326 - Attachment is obsolete: true

Comment 34

6 years ago
Comment on attachment 681772 [details]
Default state screenshot

Thanks for the screenshots.

Looking at this one, I'd add my ad-hoc reading of "I'm not cool with this, forget my server data". I understand that Matej review this in comment 13, but still.

I read "my server data" much more as "my data about the server" than "the data the server has about me".
From a more german perspective, "forget" doesn't sound intentional enough. That might be a localization aspect we can catch with comments. Anyway, to me, "forget" sounds like something that may or may not happen, or may only happen partially. "delete" is a hard word, but it conveys a message of getting rid of the data on our side in full, and now.
The "I'm not cool with this" part is likely hard to localize right.

Not sure if this question was asked before, what happens if the "submit periodically" box is unchecked, and you back to that page? Does it check if we have data on the server still or not? Might be frustrating for a user to go back to the page after deleting, and then seeing the button again. Also, if someone pressed the "forget my data" button, do we uncheck the "submit data" pref/checkbox?
(In reply to Axel Hecht [:Pike] from comment #34)
> Comment on attachment 681772 [details]
> Default state screenshot
> 
> Thanks for the screenshots.
> 
> Looking at this one, I'd add my ad-hoc reading of "I'm not cool with this,
> forget my server data". I understand that Matej review this in comment 13,
> but still.

There's no way that will be the final string. I haven't incorporated copy feedback from comments because I've been focused on other aspects.

> Not sure if this question was asked before, what happens if the "submit
> periodically" box is unchecked, and you back to that page? Does it check if
> we have data on the server still or not? Might be frustrating for a user to
> go back to the page after deleting, and then seeing the button again. Also,
> if someone pressed the "forget my data" button, do we uncheck the "submit
> data" pref/checkbox?

I'm still waiting on someone from UX to give concrete feedback on what this all will look like. So far we've mostly been iterating on the general layout. My latest feedback from this (not captured in bug, sorry) is that the tabs are unnecessary at this juncture and we should just go with one column with the raw data shown inline if a link or button is clicked. The tabs may be necessary in the future. But, we'll cross that bridge when we get to it.

I'm generally new to development of browser UX, especially all the l10n subtleties. So, any pointers you give will be appreciated.
Axel, I would love to get some guidance from you on this. Here are a couple of thoughts.


If the user opts out, whether via the preferences dialog, or here, the client will attempt to send a request to delete the previous document on the server if there is one. If the request fails due to server error or lack of connectivity, it will continue attempting the delete periodically. I would be happy with a single control that made it easy to opt out and delete rather than two. It doesn't really make sense to opt out but leave your data, nor to delete your data but not opt out. I don't know what the best choice of control MIT be nor how much explanatory text is sufficient.


Besides this control, it should also be possible for the user to view the current set of collected raw data. This should be available to the user even if they are offline.  It could be hidden behind some control like a tab or a button to display it.


When the user is online, the most prominent part of the final page should be the visualization(s). There is one to start, with a few others to follow once we have enough data available to support them.
(In reply to Axel Hecht [:Pike] from comment #34)
> Comment on attachment 681772 [details]
> Default state screenshot
> 
> Thanks for the screenshots.
> 
> Looking at this one, I'd add my ad-hoc reading of "I'm not cool with this,
> forget my server data". I understand that Matej review this in comment 13,
> but still.
> 
> I read "my server data" much more as "my data about the server" than "the
> data the server has about me".
> From a more german perspective, "forget" doesn't sound intentional enough.
> That might be a localization aspect we can catch with comments. Anyway, to
> me, "forget" sounds like something that may or may not happen, or may only
> happen partially. "delete" is a hard word, but it conveys a message of
> getting rid of the data on our side in full, and now.
> The "I'm not cool with this" part is likely hard to localize right.

I agree on both counts. Maybe we should make this really clear and straightforward. Something like "Please delete my server data"

Would that work?

Comment 38

6 years ago
In my opinion, "Please" should be ommited. It's a command that will be executed without fail by an automated system, not a request to some organization or private entity that may or may not do it.

Comment 39

6 years ago
Sounds like "disable and delete previously uploaded data" would be one way to phrase it. We may want to store that there's no data on the server anymore, and show "no submitted data" and "enable", or if we still try to delete the data, "retry to delete uploaded data at DATE".

But yeah, this sounds like UX should look at this, instead of me doing my amateur thing.
Created attachment 682232 [details]
about:healthreport screenshot (default state)

Per UX feedback, I refactored things into a single column. We still need some style love. But, this should give people a feel for things.

I combined the "submit data?" checkbox and "delete my data" button into a single button element. When clicked, you get a drop-down prompt from the top of the tab. If you click "OK" in that, data submission is disabled and server data is deleted.

If you are opted out of data submission, the "Stop submitting data" button is replaced with a similar "Submit data periodically" button. Clicking it immediately enables data submission (which will probably occur sometime later).

I'm not sure if I need separate elements for opt in and opt out or if it is OK for them to share the same button.

I'm not sure how display of the raw data should be toggled. Should we have an inline text link? A button to toggle display? A radio box? A checkbox? Something else?
Attachment #681772 - Attachment is obsolete: true
Attachment #681773 - Attachment is obsolete: true
Created attachment 682234 [details] [diff] [review]
about:healthreport, v4

I think Frank volunteered to provide some CSS love to things.

Ping me on IRC if you need help getting this incorporated into a build (hint: it won't just work with mozilla-central).
Flags: needinfo?(fryn)
Created attachment 682234 [details] [diff] [review]
about:healthreport, v4

I think Frank volunteered to provide some CSS love to things.

Ping me on IRC if you need help getting this incorporated into a build (hint: it won't just work with mozilla-central).
I can be the UX contact-point here.

Updated

6 years ago
Depends on: 815278

Comment 44

5 years ago
Some things have landed in the last few weeks, and I'm not sure what I need patches I need to get about:healthreport working again (lots of Makefile and aboutRedirector bitrot).

I'll let Madhava take it from here, but I'm still happy to cobble together some CSS if needed.
Flags: needinfo?(fyan)
(Assignee)

Comment 45

5 years ago
Created attachment 701177 [details] [diff] [review]
take 2, v0.8

Needs:

* better end URL for glossary
* mockup URL for coming soon
* postMessage API for actual report to get data and process

Followups:

* correct/final URL for the report page
* much visual polish
Assignee: mreid → mconnor
Attachment #681770 - Attachment is obsolete: true
Attachment #682232 - Attachment is obsolete: true
Attachment #682234 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mconnor)
(Assignee)

Updated

5 years ago
Summary: Information page for Metrics Data Ping to explain and display what is being collected → build about:healthreport to display submission info and embed the report itself
(Assignee)

Comment 46

5 years ago
Created attachment 701285 [details] [diff] [review]
v1

The URLs will be live later today.
Attachment #701285 - Flags: review?(dolske)
Comment on attachment 701285 [details] [diff] [review]
v1

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

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +7,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://services-common/preferences.js");
> +
> +const reporter = Cc["@mozilla.org/healthreport/service;1"]

@mozilla.org/datareporting/service;1

@@ +12,5 @@
> +                   .getService(Ci.nsISupports)
> +                   .wrappedJSObject
> +                   .reporter;
> +
> +const prefs = new Preferences("healthreport.about.");

datareport.healthreport.about.

@@ +76,5 @@
> +  reporter.getLastPayload().then(refreshDataView);
> +}
> +
> +function onOptInClick() {
> +  reporter.recordPolicyAcceptance("Clicked opt in button on about page.");

You now have to get the .policy from the datareporting;service and .healthReportDataUpload = true;
Comment on attachment 701285 [details] [diff] [review]
v1

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

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +12,5 @@
> +                   .getService(Ci.nsISupports)
> +                   .wrappedJSObject
> +                   .reporter;
> +
> +const prefs = new Preferences("healthreport.about.");

I meant datareporting.healthreport.about.

Comment 49

5 years ago
Do we have brand guidance on localizing "Health Report"? I think it's probably good to localize it, but it may make sense to call it out in a localization note or two that this is in CamelCase because it's a proper noun in en-US.
(Assignee)

Comment 50

5 years ago
Created attachment 701353 [details] [diff] [review]
v1.0.1

rebased on Greg's latest patches, with some visual tweaks from Larissa. (woo! \o/ )
Attachment #701177 - Attachment is obsolete: true
Attachment #701285 - Attachment is obsolete: true
Attachment #701285 - Flags: review?(dolske)
Attachment #701353 - Flags: review?(dolske)
(Assignee)

Comment 51

5 years ago
Created attachment 701359 [details] [diff] [review]
v1.0.2

interdiff showed me a merge failure (reverting a string change, nothing major)
Attachment #701353 - Attachment is obsolete: true
Attachment #701353 - Flags: review?(dolske)
Attachment #701359 - Flags: review?(dolske)
Comment on attachment 701359 [details] [diff] [review]
v1.0.2

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

r+ with comments addressed.

::: browser/base/content/abouthealthreport/abouthealth.css
@@ +9,5 @@
> +
> +body {
> +  background-color: window;
> +  color: windowtext;
> +  font-family: "Trebuchet MS", "Helvetica";

Do we want the OS X system-default font here too?

@@ +30,5 @@
> +  align-items: center;
> +}
> +
> +#content-line {
> +	display: flex;

fix indentation -- here and elsewhere. (are these -- gasp -- tabs?)

@@ +135,5 @@
> +	border: 0;
> +}
> +
> +#raw-data {
> +}

Uhm.

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +25,5 @@
> +  refreshState();
> +  document.getElementById("details-link").href = prefs.get("glossaryUrl");
> +}
> +
> +function refreshState() {

Roll this into init()? Seems it's the only caller, and cuts down on the amount of indirection.

@@ +41,5 @@
> +  document.getElementById("intro-enabled").style.display = enabled ? "" : "none";
> +  document.getElementById("intro-disabled").style.display = enabled ? "none" : "";
> +
> +  document.getElementById("btn-optin").style.display = enabled ? "none" : "";
> +  document.getElementById("btn-optout").style.display = enabled ? "" : "none";

It would probably be a bit clearer to control the display in CSS via an attribute, perhaps on a parent element or the body. (IE, either 2 rules  -- .foo[enabled] / .bar:not([enabled]) -- or a default state plus attribute to flip to not-default.) Maybe the same attr as the updateView() stuff is using?

@@ +82,5 @@
> +function onOptOutClick() {
> +  let prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +                  .getService(Ci.nsIPromptService);
> +
> +  let messages = document.getElementById("optout-slideout");

The "slideout" references look like they're cruft from an older version of the patch/UI... cleanup?

@@ +118,5 @@
> +}
> +
> +function onHideReportClick() {
> +  updateView("default");
> +  document.getElementById("remote-report").src = "";

Also CSS controlled?

::: browser/base/content/abouthealthreport/abouthealth.xhtml
@@ +36,5 @@
> +          <p id="intro-disabled">&abouthealth.intro-disabled;</p>
> +          <div id="control-container">
> +            <button id="btn-optin" onclick="onOptInClick();">&abouthealth.optin;</button>
> +            <button id="btn-optout" onclick="onOptOutClick();">&abouthealth.optout;</button>
> +      

Greg's whitespace here is appalling.

::: browser/base/content/baseMenuOverlay.xul
@@ +55,5 @@
> +        <menuitem id="healthReport"
> +                  label="&healthReport.label;"
> +                  accesskey="&healthReport.accesskey;"
> +                  oncommand="openHealthReport()"
> +                  onclick="checkForMiddleClick(this,event);"/>

nit: space after comma

::: browser/base/content/browser-appmenu.inc
@@ +380,5 @@
>                        onclick="checkForMiddleClick(this, event);"/>
> +            <menuitem id="appmenu_healthReport"
> +                      label="&healthReport.label;"
> +                      oncommand="openHealthReport()"
> +                      onclick="checkForMiddleClick(this,event);"/>

nit: space after comma
Attachment #701359 - Flags: review?(dolske) → review+
Comment on attachment 701359 [details] [diff] [review]
v1.0.2

[Approval Request Comment]
See bug 804745.
Attachment #701359 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a0a6d75dd0d6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You forgot the licence text in the aboutHealthReport.dtd file ;)
Created attachment 701540 [details] [diff] [review]
Add license.

Trivial license addition. Would be nice to hit Aurora alongside the other patch in this bug.
Attachment #701540 - Flags: review+
Attachment #701540 - Flags: approval-mozilla-aurora?
Comment on attachment 701359 [details] [diff] [review]
v1.0.2

https://bugzilla.mozilla.org/show_bug.cgi?id=804745#c37
Attachment #701359 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #701540 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Depends on: 830418

Comment 60

5 years ago
Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/be55bf9fff6f
status-firefox20: --- → fixed
You need to log in before you can comment on or make changes to this bug.