Closed Bug 948880 Opened 6 years ago Closed 6 years ago

Move inline scripts and styles into separate file for toolkit/content/aboutAbout.xhtml (URL=about:about)

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: freddy, Assigned: desiradaniel2007)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 7 obsolete files)

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.
I am ready to take this bug. Another question? Should the JS be taken out in a single file, including that in inline XUL event listeners.
Yes. See bug 948879 and bug 948894 for examples.
Attached patch Moving inline JS (obsolete) — Splinter Review
Here is my manually created diff. Will check hg diff soon, looking at what MDN says.
Flags: needinfo?(nobody)
Comment on attachment 8349988 [details] [diff] [review]
Moving inline JS

The patch in itself looks good. 
But it's not in the right format: HG omits the changes to the xhtml file somehow. Probably a syntax problem with the diff.
Attachment #8349988 - Flags: feedback+
Flags: needinfo?(nobody)
Attached patch aboutAbout.patch (obsolete) — Splinter Review
Here's the patch made by hg diff.
Attachment #8349988 - Attachment is obsolete: true
Comment on attachment 8351461 [details] [diff] [review]
aboutAbout.patch

Review of attachment 8351461 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good but did you check that about:about in your custom build is still working? "aboutAbout.js" needs to be declared in toolkit/content/jar.mn otherwise it won't be included in the build.
Attachment #8351461 - Flags: feedback+
Daniel, it seems your patch needs only a quick update. Let's get this ready to be checked in! :)
Attached patch aboutAboutJs.patch (obsolete) — Splinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

I haven't really looked into the build system yet, but given this is a simple bug I expect my latest patch to be OK. Would it be a problem if you try an incremental build on your machine?

I will delve deeper in Mozilla builds when I have some time. Sorry, for taking long with this too, but had some things to hurry up with :)
Attachment #8351461 - Attachment is obsolete: true
Attachment #8361844 - Flags: feedback+
Attachment #8361844 - Flags: approval-mozilla-aurora?
Attached patch Patch ready for merging (obsolete) — Splinter Review
Attachment #8362252 - Flags: checkin+
Comment on attachment 8361844 [details] [diff] [review]
aboutAboutJs.patch

This bug is not critical for aurora and should land first in central.

Besides that, your patch contains the hg help (and does not contain the information about the author, description of the bug, etc).
You should have a look to:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8361844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Exactly "hg qnew aboutAboutJs.patch" has messed my patch to that degree. I do not find the MDN docs to explain patches clearly. I am running Mercurial 2.2.2 and cannot upgrade since I am using Mint :( Switching to Ubuntu soon.

Going through this tutorial since I am completely new to mq http://mercurial.selenic.com/wiki/MqTutorial Perhaps it should get a reference on MDN.

I am sorry for these delays and would appreciate any further guidance on this.
Assignee: nobody → desiradaniel2007
Status: NEW → ASSIGNED
Attached patch a.patch (obsolete) — Splinter Review
Attached patch aboutAboutJs.patch (obsolete) — Splinter Review
Attached patch aboutAboutInlineJs.patch (obsolete) — Splinter Review
Thanks for all who have helped me especially #introduction people on IRC.

I have everything in place for this patch! (At least, I think.)
Attachment #8361844 - Attachment is obsolete: true
Attachment #8362252 - Attachment is obsolete: true
Attachment #8373070 - Attachment is obsolete: true
Attachment #8373071 - Attachment is obsolete: true
Attachment #8373332 - Flags: checkin?
Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

This doesn't have review AFAICT. Also, please use the checkin-needed keyword when this meets the requirements for landing.
Attachment #8373332 - Flags: checkin?
Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

This looks good. Thank you Daniel.
Let's get a review from a module peer, before this is checked in.
Attachment #8373332 - Flags: review?(netzen)
Attachment #8373332 - Flags: feedback+
Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

Gavin, or someone he will delegate to, is probably a better reviewer for this.  Forwarding to Gavin for now.
Attachment #8373332 - Flags: review?(netzen) → review?(gavin.sharp)
Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

Looks like aboutAbout.js needs an MPL license header (from https://www.mozilla.org/MPL/headers/), and a newline at the end

r=me with those added.
Attachment #8373332 - Flags: review?(gavin.sharp) → review+
Thanks again!
Attachment #8373332 - Attachment is obsolete: true
Attachment #8375661 - Flags: checkin?
Comment on attachment 8375661 [details] [diff] [review]
aboutAboutInlineJs.patch

Please just use checkin-needed in the future.
Attachment #8375661 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/3adf0dffdf19
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3adf0dffdf19
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 30
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.