currently specialpowers is enabled for chrome and browser-chrome. This is turning into a special case hackathon taking care of different window types. We originall did this in bug 666636 because we share utilities (EventUtils.js, SimpleTest.js, etc...) and we didn't want to special case every privileged call for specialpowers or chrome. We should refactor specialpowers and specialpowersobserver so that the code we add to handle privileged calls can be used as a script in the chrome and browser-chrome harnesses. Then we can remove the specialpowers extension from chrome and browser-chrome harnesses.

the one thing we will need to resolve is how to get redirect.html to work for chrome/a11y tests? We need something that will be a hook for loadURI. Maybe we put this in harness-overlay.xul?

refactor the specialpowers code into: specialpowers.js -> - specialpowers.js (binding glue and message manager api) - ChromePowers.js (fake backend to the api) - specialpowersAPI.js (api the tests use) SpecialPowersObserver.js -> - SpecialPowersObserver.js (binding glue and message manage backend) - ChromePowers.js (fake backend of the observer api) - SpecialPowersObserverAPI.js (api the message manager uses) this patch also resolves the redirect.js/loadURI problem. All green on try, OF=0.00

Comment on attachment 550639 [details] [diff] [review] allow specialpowers to work from chrome harness (1.0) Review of attachment 550639 [details] [diff] [review]: ----------------------------------------------------------------- Overall this approach seems sane, although some of the details make me unhappy. I'm not going to r- this, I'd like to hear if you have any suggestions. ::: testing/mochitest/browser-test-overlay.xul @@ +59,5 @@ > + var listener = 'data:,addEventListener("contentEvent", function (e) { var data=e.getData("data");sendAsyncMessage("chromeEvent", {"data":data}); }, false, true);'; > + messageManager.loadFrameScript(listener, true); > + messageManager.addMessageListener("chromeEvent", messageHandler); > + } > + setupIPC(); Why is this stuff in browser-test-overlay? This overlay is used for mochitest-browser-chrome, isn't it? ::: testing/mochitest/redirect.js @@ +43,5 @@ > + var element = document.createEvent("datacontainerevent"); > + element.initEvent("contentEvent", true, false); > + element.setData("data", aURL + location.search); > + element.setData("type", "loadURI"); > + document.dispatchEvent(element); It would be good to document here where the event gets handled. ::: testing/mochitest/runtests.py @@ +513,5 @@ > + # We only need special powers in non-chrome harnesses (except for chrome redirect.html...ugh!) > + if (not options.browserChrome and > + not options.chrome and > + not options.a11y): > + self.installSpecialPowersExtension(options) Is that comment still correct? Also, feels like maybe we should have a variable somewhere that contains the mochitest type so you don't have to write horrible conditionals like this. ::: testing/mochitest/specialpowers/components/SpecialPowersObserver.js @@ +151,5 @@ > +spo_api = new SpecialPowersObserverAPI(); > +if (spo_api != null) { > + for (obj in spo_api) { > + SpecialPowersObserver.prototype[obj] = spo_api[obj]; > + } This code feels weird. Could we do some kind of inheritance instead of copying the properties over like this? ::: testing/mochitest/specialpowers/content/specialpowers.js @@ +98,2 @@ > > +var sp_api = new SpecialPowersAPI(); It feels a little weird to rely on pulling variables out of the other content script. There's not any better way to do this? @@ +99,5 @@ > +var sp_api = new SpecialPowersAPI(); > +if (sp_api != null) { > + for (obj in sp_api) { > + SpecialPowers.prototype[obj] = sp_api[obj]; > + } Same thing here, this feels ugly. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +24,5 @@ > } > + > + //This is the case where we have ChromePowers in harness.xul and we need it in the iframe > + if (window.SpecialPowers == undefined && parent.SpecialPowers != "undefined") { > + window["SpecialPowers"] = parent.SpecialPowers; you can just say window.SpecialPowers = ...

new patches uses prototype inheritance as well as cleaned up a few pieces as per comments on original patch. In addition, I have removed the redirect.js and put the little bit of code in redirect.html. patch passes local tests, up on try.

Comment on attachment 552374 [details] [diff] [review] allow specialpowers to work from chrome harness (2.0) Review of attachment 552374 [details] [diff] [review]: ----------------------------------------------------------------- Can you update the MDN docs about SpecialPowers to point to the new source locations of things after you land this? https://developer.mozilla.org/en/SpecialPowers ::: testing/mochitest/browser-test.js @@ +30,5 @@ > > ww.openWindow(window, "chrome://mochikit/content/browser-harness.xul", "browserTest", > "chrome,centerscreen,dialog=no,resizable,titlebar,toolbar=no,width=800,height=600", sstring); > + } else { > + //this code allows us to redirct without requiring specialpowers for chrome and a11y tests. Super nitpicky, but can you capitalize the start of your sentences, and put a space after the // ? Sorry, that drives me nuts. ::: testing/mochitest/redirect.html @@ +4,5 @@ > > + <script type="text/javascript"> > + function redirect(aURL) > + { > + var element = document.createEvent("datacontainerevent"); I think I commented last time, but could you put a comment here mentioning what code is going to handle this event? ::: testing/mochitest/specialpowers/components/SpecialPowersObserver.js @@ +70,2 @@ > > +SpecialPowersObserver.prototype = new SpecialPowersObserverAPI(); Thanks, this makes me feel better! @@ +73,5 @@ > + SpecialPowersObserver.prototype.classDescription = "Special powers Observer for use in testing."; > + SpecialPowersObserver.prototype.classID = Components.ID("{59a52458-13e0-4d93-9d85-a637344f29a1}"); > + SpecialPowersObserver.prototype.contractID = "@mozilla.org/special-powers-observer;1"; > + SpecialPowersObserver.prototype.QueryInterface = XPCOMUtils.generateQI([Components.interfaces.nsIObserver]); > + SpecialPowersObserver.prototype._xpcom_categories = [{category: "profile-after-change", service: true }]; I wish we had .extend() or something cool like all those JS libs provide.

updated a lot of stuff to work around leaks, resulting in some good cleanup and adding functionality. Enough has changed that we need a review

Comment on attachment 555409 [details] [diff] [review] allow specialpowers to work from chrome harness (3.0) Review of attachment 555409 [details] [diff] [review]: ----------------------------------------------------------------- My eyes are totally glazing over after reading this patch for the 4th time. Just a few comments. ::: testing/mochitest/redirect.html @@ +8,5 @@ > + var element = document.createEvent("datacontainerevent"); > + element.initEvent("contentEvent", true, false); > + element.setData("data", aURL + location.search); > + element.setData("type", "loadURI"); > + document.dispatchEvent(element); I think I've commented on this like 3 times now. Can you please add a comment saying where this gets handled? ::: testing/mochitest/specialpowers/components/SpecialPowersObserver.js @@ +56,2 @@ > > +//glue to add in the observer API to this object. This allows us to share code with chrome tests Put a space after // How many times do I have to nitpick you? :) @@ +70,4 @@ > > +SpecialPowersObserver.prototype = new SpecialPowersObserverAPI(); > + > + SpecialPowersObserver.prototype.classDescription = "Special powers Observer for use in testing."; I wish JavaScript had a .extend method built in. :-/ This still feels better than the old code, though. @@ +95,1 @@ > this._messageManager.loadFrameScript(CHILD_SCRIPT, true); So! This code might get broken by the patch in bug 673569, which is going to make frame scripts not share the same global anymore. (Which means you won't be able to load a script in one frame script, and have another frame script see its variables.) ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +22,5 @@ > if (!parentRunner && parent.wrappedJSObject) { > parentRunner = parent.wrappedJSObject.TestRunner; > } > + > + //This is the case where we have ChromePowers in harness.xul and we need it in the iframe super-nit space after //

Backed out on inbound because this appeared to cause a new frequent mochitest-browser-chrome leak on Linux debug. https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc1e45afdd7 Example: http://tbpl.allizom.org/php/getParsedLog.php?id=6145965

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/54cb80b5d5a1

Does this landing mean that what this comment suggests ... SimpleTest.executeSoon = function(aFunc) { // Once SpecialPowers is available in chrome mochitests, we can replace the // body of this function with a call to SpecialPowers.executeSoon(). ... is now possible? Because every call to executeSoon() is now spamming the JS console with a notification that enablePrivilege is deprecated.

there are some issues with SpecialPowers.executeSoon(). I have a patch that has 6 failures, I should have more time next week to work on those failures.

Is there a bug that I can make bug 663291 depend upon?