Bobby Holley (:bholley) Assignee
	Description • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

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.

	Bobby Holley (:bholley) Assignee
	Comment 4 • 12 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

> 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.

	Bobby Holley (:bholley) Assignee
	Comment 6 • 12 years ago

(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.

	Bobby Holley (:bholley) Assignee
	Comment 7 • 12 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 8 • 12 years ago

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

	Bobby Holley (:bholley) Assignee
	Comment 9 • 12 years ago

annnd, the last patch was an empty push. Today is not my day with HG :-(

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0523f50100

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 12 years ago

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