Last Comment Bug 819639 - Move EventSource to Paris bindings
: Move EventSource to Paris bindings
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla20
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-12-08 04:13 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2015-10-04 01:32 PDT (History)
3 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Rename nsEventSource (41.00 KB, patch)
2012-12-09 10:08 PST, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Part b: Paris bindings (30.21 KB, patch)
2012-12-09 10:09 PST, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Interdiff (9.80 KB, patch)
2012-12-13 01:23 PST, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-12-08 04:13:13 PST

    
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-12-09 10:08:44 PST
Created attachment 690203 [details] [diff] [review]
Part a: Rename nsEventSource
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-12-09 10:09:06 PST
Created attachment 690204 [details] [diff] [review]
Part b: Paris bindings
Comment 3 Masatoshi Kimura [:emk] 2012-12-09 18:11:57 PST
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?
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-12-10 00:57:36 PST
(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 5 Boris Zbarsky [:bz] (still a bit busy) 2012-12-10 07:21:14 PST
Comment on attachment 690203 [details] [diff] [review]
Part a: Rename nsEventSource

r=me
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-12-10 07:23:35 PST
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?
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-12-10 08:29:29 PST
I'm confused myself why the init() was on the interface in the first place. Removing is fine with me.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-12-10 09:01:50 PST
> 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?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-12-10 20:50:05 PST
Olli, how much of this interface's usability from C++ is it OK to gut?
Comment 10 Olli Pettay [:smaug] (reviewing overload) 2012-12-11 03:50:45 PST
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 11 Boris Zbarsky [:bz] (still a bit busy) 2012-12-12 18:53:33 PST
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.
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-12-13 01:23:49 PST
Created attachment 691733 [details] [diff] [review]
Interdiff

In case you wanted to have another look
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-12-13 08:29:29 PST
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 14 Boris Zbarsky [:bz] (still a bit busy) 2012-12-14 18:35:14 PST
Comment on attachment 691733 [details] [diff] [review]
Interdiff

See comment 13.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-12-15 02:10:35 PST
(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.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-12-15 08:29:50 PST
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 17 Boris Zbarsky [:bz] (still a bit busy) 2012-12-19 11:31:58 PST
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
Comment 19 Jean-Yves Perrier [:teoli] 2015-10-04 01:32:56 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.