Closed Bug 948881 Opened 8 years ago Closed 7 years ago
Move inline scripts and styles into separate file for browser/base/content/abouthealthreport/abouthealth
Summary: Move inline scripts and styles into separate file for browser/base/content/abouthealthreport (URL=about:healthreport) → Move inline scripts and styles into separate file for browser/base/content/abouthealthreport/abouthealth.xhtml (URL=about:healthreport)
I tested this change manually by making sure that init() is called when loading the page and then uninit() is called when browsing to a different URL from the same tab. Is there another way (automated?) to test these kinds of changes?
Also, don't let the @mozilla.com email address fool you, this is my first patch to mozilla-central, so any feedback is more than welcome.
The patch looks OK, but I wonder if you need to wrap the event handlers in an anonymous functions. Just referencing to init and uninit might work as well. I don't think there are automated tests for these kinds of changes :( If this is your first patch, I can recommend giving it a local spin. Once you have a working local build of firefox, consequent builds that change only HTML/JS are quite fast. It's also a nice exercise. ;) See https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
I tried without an anonymous function: window.addEventListener("load", healthReportWrapper.init); but the "this" variable in "this._getReportURI();" wasn't being set to the right thing and the init function was failing. The uninit() function doesn't seem to use "this" so it would probably work, but I figured it might be better to have both of them be the same. And yes, I did try this in my own build of Firefox. I was pleasantly surprised to see that I didn't even have to recompile (or even restart Firefox) for the changes to HTML and JS files to take effect :)
Comment on attachment 8359047 [details] [diff] [review] bug948881.patch Review of attachment 8359047 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a Firefox peer, so all I can give you is feedback+ ;) Looping in Tim for a review.
Attachment #8359047 - Flags: review?(ttaubert) → review+
Same patch but adding the reviewer to the commit message.
Sorry for clearing the review flag on that patch, I thought I should fix the commit message before the patch went in. What's the best way to do this? Leave the commit message as-is so that the r+ is visible on the patch? Or do like I did and ask for another review?
(In reply to François Marier [:francois] from comment #7) > What's the best way to do this? Leave the commit message as-is so that the > r+ is visible on the patch? Or do like I did and ask for another review? You can carry it forward and just set the r+ yourself, the bug comments state that there was a proper review. You could also just leave it out and name the patch like "patch for checkin". Before pushing your patch people will check that it has been reviewed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [email@example.com][lang=html][good first bug][lang=js] → [firstname.lastname@example.org][lang=html][good first bug][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.