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.

Here is my manually created diff. Will check hg diff soon, looking at what MDN says.

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.

Here's the patch made by hg diff.

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.

Daniel, it seems your patch needs only a quick update. Let's get this ready to be checked in! :)

[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 :)

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

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.

Thanks for all who have helped me especially #introduction people on IRC. I have everything in place for this patch! (At least, I think.)

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.

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.

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.

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.

Thanks again!

Comment on attachment 8375661 [details] [diff] [review] aboutAboutInlineJs.patch Please just use checkin-needed in the future.