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.xhtml (URL=about:healthreport)

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: freddy, Assigned: francois)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][qa-])

Attachments

(1 file, 1 obsolete file)

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.
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)
Assignee: nobody → francois
Attached patch bug948881.patch (obsolete) — Splinter Review
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?
Attachment #8359047 - Flags: review?(fbraun)
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)
Attachment #8359047 - Flags: review?(fbraun)
Attachment #8359047 - Flags: feedback+
Attachment #8359047 - Flags: review?(ttaubert) → review+
Attached patch bug948881.patchSplinter Review
Same patch but adding the reviewer to the commit message.
Attachment #8359047 - Attachment is obsolete: true
Attachment #8360178 - Flags: review+
Attachment #8360178 - Flags: review+
Attachment #8360178 - Flags: checkin?(fbraun)
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.
Attachment #8360178 - Flags: checkin?(fbraun)
https://hg.mozilla.org/mozilla-central/rev/b5fd65e0f0d6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.