Closed
Bug 719484
Opened 12 years ago
Closed 11 years ago
build about:healthreport to display submission info and embed the report itself
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 21
Tracking | Status | |
---|---|---|
firefox20 | --- | fixed |
People
(Reporter: mreid, Assigned: mconnor)
References
Details
(Whiteboard: [sec-assigned:dveditz])
Attachments
(2 files, 14 obsolete files)
18.03 KB,
patch
|
Dolske
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.16 KB,
patch
|
rnewman
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → mreid
Reporter | ||
Comment 1•12 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•12 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.
Comment 3•12 years ago
|
||
Axel, two screenshots (it is currently a very long page) and a link to where we are prototyping the restructure of the page.
Comment 4•12 years ago
|
||
You can see the prototype in Trevor Norris's sandbox here: http://people.mozilla.com/~tnorris/about-metrics/
Comment 5•12 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+
Comment 6•12 years ago
|
||
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•12 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.
Comment 8•12 years ago
|
||
(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.
Comment 9•12 years ago
|
||
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•12 years ago
|
Attachment #589893 -
Flags: review?(Mnovak)
Comment 10•12 years ago
|
||
Could someone pull out just the copy that you'd like me to review? Thanks!
Reporter | ||
Comment 11•12 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•12 years ago
|
||
> Since we do not have yout permission
s/yout/your/
Comment 13•12 years ago
|
||
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
Updated•12 years ago
|
Keywords: sec-review-needed
Reporter | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
Tiny correction
Attachment #595401 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Attachment #595480 -
Flags: review?(curtisk)
Reporter | ||
Comment 16•12 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?
Comment 17•12 years ago
|
||
(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.
Comment 18•12 years ago
|
||
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]
Comment 19•12 years ago
|
||
(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.
Comment 20•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #595480 -
Flags: review?(curtisk) → review?(dveditz)
Comment 21•12 years ago
|
||
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•12 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.
Comment 23•12 years ago
|
||
Thanks, Axel, I'll keep that in mind for future reviews.
Updated•12 years ago
|
Whiteboard: [secr:dveditz] → [sec-assigned:dveditz]
Updated•12 years ago
|
Flags: sec-review?(dveditz)
Updated•12 years ago
|
Version: 12 Branch → unspecified
Reporter | ||
Comment 25•12 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•12 years ago
|
Product: Toolkit → Firefox
Comment 28•12 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.
Comment 29•12 years ago
|
||
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 30•12 years ago
|
||
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?
Comment 31•12 years ago
|
||
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
Comment 32•12 years ago
|
||
Attachment #590322 -
Attachment is obsolete: true
Comment 33•12 years ago
|
||
Attachment #590326 -
Attachment is obsolete: true
Comment 34•12 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?
Comment 35•12 years ago
|
||
(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.
Comment 36•12 years ago
|
||
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.
Comment 37•12 years ago
|
||
(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•12 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•12 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.
Comment 40•12 years ago
|
||
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
Comment 41•12 years ago
|
||
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)
Comment 42•12 years ago
|
||
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).
Comment 43•12 years ago
|
||
I can be the UX contact-point here.
Comment 44•11 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•11 years ago
|
||
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•11 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•11 years ago
|
||
The URLs will be live later today.
Attachment #701285 -
Flags: review?(dolske)
Comment 47•11 years ago
|
||
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 48•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 years ago
|
||
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 52•11 years ago
|
||
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 53•11 years ago
|
||
Comment on attachment 701359 [details] [diff] [review] v1.0.2 [Approval Request Comment] See bug 804745.
Attachment #701359 -
Flags: approval-mozilla-aurora?
Comment 54•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0a6d75dd0d6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment 55•11 years ago
|
||
You forgot the licence text in the aboutHealthReport.dtd file ;)
Comment 56•11 years ago
|
||
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 57•11 years ago
|
||
License addition: https://hg.mozilla.org/services/services-central/rev/b78592e34853
Comment 59•11 years ago
|
||
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•11 years ago
|
Attachment #701540 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 60•11 years ago
|
||
Pushed to Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/be55bf9fff6f
status-firefox20:
--- → fixed
Comment 61•11 years ago
|
||
and part a: http://hg.mozilla.org/releases/mozilla-aurora/rev/38f76c38d670
Updated•6 years ago
|
Flags: sec-review?(dveditz)
You need to log in
before you can comment on or make changes to this bug.
Description
•