All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: freddyb, Assigned: francois)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 29
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
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)

Updated

5 years ago
Assignee: nobody → francois
(Assignee)

Comment 1

5 years ago
Created attachment 8359047 [details] [diff] [review]
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?
Attachment #8359047 - Flags: review?(fbraun)
(Assignee)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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 :)
(Reporter)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 8360178 [details] [diff] [review]
bug948881.patch

Same patch but adding the reviewer to the commit message.
Attachment #8359047 - Attachment is obsolete: true
Attachment #8360178 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8360178 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #8360178 - Flags: checkin?(fbraun)
(Assignee)

Comment 7

5 years ago
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)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b5fd65e0f0d6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

5 years ago
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.