The default bug view has changed. See this FAQ.

Move EventSource to Paris bindings

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete})

Trunk
mozilla20
addon-compat, dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 690203 [details] [diff] [review]
Part a: Rename nsEventSource
Attachment #690203 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
Created attachment 690204 [details] [diff] [review]
Part b: Paris bindings
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?
(Assignee)

Comment 4

4 years ago
(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?
(Assignee)

Comment 7

4 years ago
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+
(Assignee)

Comment 12

4 years ago
Created attachment 691733 [details] [diff] [review]
Interdiff

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-
(Assignee)

Comment 15

4 years ago
(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+
(Assignee)

Updated

4 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Comment 18

4 years ago
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
Last Resolved: 4 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).
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.