Closed Bug 819639 Opened 11 years ago Closed 11 years ago

Move EventSource to Paris bindings

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(3 files)

      No description provided.
Attachment #690203 - Flags: review?(bzbarsky)
Attachment #690204 - Flags: review?(bzbarsky)
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
Attachment #690203 - Flags: review?(bzbarsky) → review+
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.
Attachment #690204 - Flags: review?(bzbarsky) → review+
Attached patch InterdiffSplinter Review
In case you wanted to have another look
Attachment #691733 - Flags: review?(bzbarsky)
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.
Attachment #691733 - Flags: review?(bzbarsky) → review-
(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
Attachment #691733 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/23817988285b
https://hg.mozilla.org/mozilla-central/rev/da12f650d014
https://hg.mozilla.org/mozilla-central/rev/dbb76b67ed73
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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).
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.