Comment on attachment 690204 [details] [diff] [review] Part b: Paris bindings > [scriptable, uuid(778e5ae3-c72c-4d4b-9dc7-4a6477651957)] Bump an iid. And is "scriptable" still needed?

(In reply to Masatoshi Kimura [:emk] from comment #3) > Comment on attachment 690204 [details] [diff] [review] > Part b: Paris bindings > > > [scriptable, uuid(778e5ae3-c72c-4d4b-9dc7-4a6477651957)] > Bump an iid. Yes, will do before landing. > And is "scriptable" still needed? Shouldn't be. I'll remove it.

Comment on attachment 690203 [details] [diff] [review] Part a: Rename nsEventSource r=me

Comment on attachment 690204 [details] [diff] [review] Part b: Paris bindings Why bother leaving the C++ init method if you're removing all the other C++ API? Is this interface expected to be usable from C++, or not?

I'm confused myself why the init() was on the interface in the first place. Removing is fine with me.

> I'm confused myself why the init() was on the interface in the first place. Presumably for use of this interface from C++ and JavaScript components, no?

Olli, how much of this interface's usability from C++ is it OK to gut?

Since we killed nsIWebSocket, I would imagine we could kill nsIEventSource too. Both are reasonable new things, and I believe event source is very rarely used feature. Just my gut feeling. (However it does feel a bit odd to make it almost impossible to use these things from C++)

Comment on attachment 690204 [details] [diff] [review] Part b: Paris bindings >+++ b/content/base/public/nsIEventSource.idl So I think you might as well go ahead and remove the initialization method here, since it's pretty useless, and possibly the interface altogether if you can get away with that. Should GetOwner perhaps return the nsPIDOMWindow instead of nsISupports? r=me with that.

In case you wanted to have another look

Hmm. We used to support aOwner not being a window (and in particular being null), right? Do we want to drop that? As in, would someone want to use an EventSource in a sandbox or whatnot?

Comment on attachment 691733 [details] [diff] [review] Interdiff See comment 13.

(In reply to Boris Zbarsky (:bz) from comment #13) > Hmm. We used to support aOwner not being a window (and in particular being > null), right? Do we want to drop that? > > As in, would someone want to use an EventSource in a sandbox or whatnot? Did we really support that? It seems like not calling init() will cause us to dereference a null mPrincipal in some places, at least, and init() is noscript.

Oh, hmm. I guess from script we always went through Initialize(), which bailed if aOwner wasn't a window... But it also didn't get the global as aOwner, it got the thing stashed in the constructor, no? So what happens on trunk right now if you have a sandbox with a window proto but not a window scriptholder and try to new EventSource in it? What happens in the same situation with your patch?

Comment on attachment 691733 [details] [diff] [review] Interdiff Ms2ger stepped through this with me on irc. Looks like Xrays for constructors will enter the compartment of the constructor before calling it. So this should all work out. Given that, r=me

https://hg.mozilla.org/mozilla-central/rev/23817988285b https://hg.mozilla.org/mozilla-central/rev/da12f650d014 https://hg.mozilla.org/mozilla-central/rev/dbb76b67ed73

I've updated https://developer.mozilla.org/en-US/docs/Web/API/EventSource to list only the webidl methods, and removed the C++ ones (like init).