948881 - Move inline scripts and styles into separate file for browser/base/content/abouthealthreport/abouthealth.xhtml (URL=about:healthreport)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Frederik Braun [:freddy]]

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.

Comment 1

[François Marier [:francois]]

Attachment: bug948881.patch

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?

Comment 2

[François Marier [:francois]]

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.

Comment 3

[Frederik Braun [:freddy]]

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

Comment 4

[François Marier [:francois]]

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 5

[Frederik Braun [:freddy]]

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.

Comment 6

[François Marier [:francois]]

Attachment: bug948881.patch

Same patch but adding the reviewer to the commit message.

Comment 7

[François Marier [:francois]]

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?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fd65e0f0d6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5fd65e0f0d6