Bug 29841 – Add WebSocket addEventListener and removeEventListener

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Fumitoshi Ukai]

Add Websocket addEventListener and removeEventListner javascript bindings.

[Fumitoshi Ukai]

Created attachment 40292 [details]
JavaScript bindings of WebSocket addEventListener/removeEventListener

[Eric Seidel (no email)]

Comment on attachment 40292 [details]
JavaScript bindings of WebSocket addEventListener/removeEventListener

This seems like pretty generic code.  I'm surprised we don't have a way to share this with the other addEventListener implementations.  Why does this one need customs bindings?  Should we be extending the bindings generators instead?

[Fumitoshi Ukai]

(In reply to comment #2)
> (From update of attachment 40292 [details])
> This seems like pretty generic code.  I'm surprised we don't have a way to
> share this with the other addEventListener implementations.  Why does this one
> need customs bindings?  Should we be extending the bindings generators instead?

Sounds good. May I introduce new extended attribute, like AddEventListener and RemoveEventListener, for addEventListener and removeEventListener respectively?
Or should we generate the bindings code by method names?

[Fumitoshi Ukai]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 40292 [details] [details])
> > This seems like pretty generic code.  I'm surprised we don't have a way to
> > share this with the other addEventListener implementations.  Why does this one
> > need customs bindings?  Should we be extending the bindings generators instead?
> 
> Sounds good. May I introduce new extended attribute, like AddEventListener and
> RemoveEventListener, for addEventListener and removeEventListener respectively?
> Or should we generate the bindings code by method names?

Or introduce new extended attribute, GenerateEventTarget, for interface?

[Eric Seidel (no email)]

Might want to ask weinig on #webkit as he's more up-to-date on the bindings generator code than I.

[Fumitoshi Ukai]

Created attachment 40613 [details]
update for r48978

[Fumitoshi Ukai]

Created attachment 40614 [details]
remove V8ObjectEventListener.h

[Eric Seidel (no email)]

Comment on attachment 40614 [details]
remove V8ObjectEventListener.h

I'm confused by the purpose of this last patch.  All changes require ChangeLog entries, so r-.

[Fumitoshi Ukai]

Created attachment 40691 [details]
JavaScript bindings of WebSocket addEventListener/removeEventListener

[Fumitoshi Ukai]

(In reply to comment #5)
> Might want to ask weinig on #webkit as he's more up-to-date on the bindings
> generator code than I.

I catched weinig on @webkit and he said just copy the xhr code for now and he will clean that up in the future.

[Sam Weinig]

Comment on attachment 40691 [details]
JavaScript bindings of WebSocket addEventListener/removeEventListener

> -        // [Custom] void addEventListener(in DOMString type,
> -        //   in EventListener listener,
> -        //   in boolean useCapture);
> -        // [Custom] void removeEventListener(in DOMString type,
> -        //   in EventListener listener,
> -        //   in boolean useCapture);
> -        // boolean dispatchEvent(in Event evt)
> -        //   raises(EventException);
> +        [Custom] void addEventListener(in DOMString type,
> +           in EventListener listener,
> +           in boolean useCapture);
> +        [Custom] void removeEventListener(in DOMString type,
> +           in EventListener listener,
> +           in boolean useCapture);
> +         boolean dispatchEvent(in Event evt)
> +           raises(EventException);

The indentation here is quite odd.

This needs tests, so r-.

[Fumitoshi Ukai]

Created attachment 40700 [details]
WebCore: JavaScript bindings of WebSocket addEventListener/removeEventListener

[WebKit Commit Bot]

Comment on attachment 40700 [details]
WebCore: JavaScript bindings of WebSocket addEventListener/removeEventListener

Rejecting patch 40700 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=40700 from bug 29841 failed to download and apply.

[Fumitoshi Ukai]

Manually landed as r49488.

[Eric Seidel (no email)]

There were two problems with this landing:

1.  It broke all the bots.
2.  The commit message was missing the reviewer.

bugzilla-tool land-diff
can help with both, by building and running all the tests, and updating the reviewer correctly.
(There are obviously ways to do the landing manually as well.)

[Fumitoshi Ukai]

(In reply to comment #15)
> There were two problems with this landing:
> 
> 1.  It broke all the bots.
> 2.  The commit message was missing the reviewer.
> 
> bugzilla-tool land-diff
> can help with both, by building and running all the tests, and updating the
> reviewer correctly.
> (There are obviously ways to do the landing manually as well.)

Sorry for that.
I'll fix broken layout tests soon.
I will use bugzilla-tool land-diff to commit the change in future.  Thanks for the info.

[Eric Seidel (no email)]

Going back through logs from last week: The commit-queue rejected it due to the patch to DumpRenderTree.mm not applying.