There doesn't appear to be any substantive difference between the two, and there are only two consumers.

Hmm. Once we do this, will that mean we can probably drop the mContext member of nsIJSEventListener? That'd be pretty nice... I've had it sorta on my todo list for a bit.

Comment on attachment 704832 [details] [diff] [review] Remove nsJSContext::CompileEventHandler and move consumers to nsJSUtils::CompileFunction. v2 >+++ b/content/events/src/nsEventListenerManager.cpp >+ JSAutoCompartment ac(cx, context->GetNativeGlobal()); We didn't use to do that. Why do it now? >+ .setUserBit(true); // Flag us as XBL No, not at all. Please write a test that would have caught this? r=me with those two issues fixed.

(In reply to Boris Zbarsky (:bz) from comment #3) > >+++ b/content/events/src/nsEventListenerManager.cpp > >+ JSAutoCompartment ac(cx, context->GetNativeGlobal()); > > We didn't use to do that. Why do it now? Because nsJSUtils::CompileFunction requires that its caller enter a compartment, as we decided in bug 824864 comment 24. > >+ .setUserBit(true); // Flag us as XBL > > No, not at all. Please write a test that would have caught this? Whoops, yeah I was too quick with my copy-paste. I'm not really wild about writing a test for this, though, because it basically involves testing that SOWs function properly for event handlers, but we actually don't have tests for SOWs at all. So if I were to do that, it would only make sense to spend some hours writing comprehensive tests for SOWs, but those tests will be obsolete as soon as bug 821850 lands, which is hopefully Real Soon Now. In other words, setUserBit is going away in a matter of weeks, so I'd prefer to to spend time writing SOW tests, though it's ultimately your call. Let me know either way, and my answer for point #1 is satisfactory?

> Because nsJSUtils::CompileFunction requires that its caller enter a compartment Ah, yeah. And we didn't use to be calling it here, ok. I'm not exactly happy about all the boilerplate we're ending up with here, but ah, well. > In other words, setUserBit is going away in a matter of weeks Great. Given that, no need for tests.

(In reply to Boris Zbarsky (:bz) from comment #5) > > Because nsJSUtils::CompileFunction requires that its caller enter a compartment > > Ah, yeah. And we didn't use to be calling it here, ok. I'm not exactly > happy about all the boilerplate we're ending up with here, but ah, well. FWIW I'm not either. But the nice thing is that much of this boilerplate is going to go away. In particular: * The JSAutoRequest will go away when we kill requests. * The JSAutoCompartment will go away, or at least be abstracted somehow, when we only have once cx, because once we no longer go through nsIScriptContext we should probably already be in the correct compartment in one of the callers (though the exact convention we should establish there is debatable). * The rooting stuff will go away when all our objects are rooted by default.

This got an implicit try run in the recent pushes for bug 821850. Pushing to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4bd18abd869

Oh Jeeze, somehow the pushed patch didn't address review comments. I'm really sorry about that :-( https://hg.mozilla.org/integration/mozilla-inbound/rev/a27ed59008c3

annnd, the last patch was an empty push. Today is not my day with HG :-( https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0523f50100

https://hg.mozilla.org/mozilla-central/rev/f4bd18abd869 https://hg.mozilla.org/mozilla-central/rev/a27ed59008c3 https://hg.mozilla.org/mozilla-central/rev/0e0523f50100