+++ This bug was initially created as a clone of Bug #956482 +++ 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. If you want to work on this, check our project page on the wiki, that will help resolving some initial questions: https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles P.S.: I wonder if there's a way to convince Firefox and Toolkit Module peer that they will encourage upcoming internal pages not to contain things inline.

I would like to work on this one!

Proposed solution for Bug 980911. Please provide feedback.

Comment on attachment 8388938 [details] [diff] [review] Bug980911.patch Review of attachment 8388938 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! Please address these comments in your next revision and flag me again for feedback. ::: browser/base/content/aboutaccounts/aboutaccounts.js @@ +283,5 @@ > +var oldsync=document.getElementById("oldsync"); > +oldsync.onclick = function(){handleOldSync()}; > + > +var openPrefs=document.getElementById("openPrefs"); > +openPrefs.onclick = function(){openPrefs()}; About this block: 1) Please move it into the |init| function, at around line 251. 2) Please use addEventListener instead of onclick, see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener 3) You don't need to wrap the function calls into another function. You can just reference the functions itself, like ...addEventListener("click"; handleOldSync); 4) Your variable "openPrefs" clashes with the existing function name. Choose a different name for the element, like "var openPrefsElement". 5) Use consistent naming for your variables (e.g. capitalization). @@ +285,5 @@ > + > +var openPrefs=document.getElementById("openPrefs"); > +openPrefs.onclick = function(){openPrefs()}; > + > +document.getElementByID This line appears to be a leftover. Please remove it. ::: browser/base/content/aboutaccounts/aboutaccounts.xhtml @@ +46,5 @@ > <section> > <div class="graphic graphic-sync-intro"> </div> > > <div class="button-row"> > + <a class="button" id="openPrefs" href="#" >&aboutAccountsConfig.manage.label;</a> There's a leftover whitespace before the closing '>'. It's in your other edits too.

My try to fix the bug.

Comment on attachment 8413193 [details] [diff] [review] bug980911.patch Review of attachment 8413193 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK, please fix this minor thing: ::: browser/base/content/aboutaccounts/aboutaccounts.xhtml @@ +46,4 @@ > <div class="graphic graphic-sync-intro"> </div> > > <div class="button-row"> > + <a id="buttonOpenPrefs"class="button" href="#">&aboutAccountsConfig.manage.label;</a> Missing space here :)

added missing space

Comment on attachment 8413208 [details] [diff] [review] bug980911.patch Review of attachment 8413208 [details] [diff] [review]: ----------------------------------------------------------------- Works! Nice. Tim, can you give this a review?

I guess Zach is also interested...CCing him :)

Comment on attachment 8413208 [details] [diff] [review] bug980911.patch Review of attachment 8413208 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, great work! Looks like this patch is ready for checkin-needed [1]. [1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

This seems to have skipped Bernd's attention. Proposing this for checkin.

Hint to the nice person that pushes this: The commit message doesn't mention the reviewers currently. That's what I asked Bernd to do originally but we shouldn't nag him only about that :)