Bug 29841 - Add WebSocket addEventListener and removeEventListener
Summary: Add WebSocket addEventListener and removeEventListener
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-29 03:04 PDT by Fumitoshi Ukai
Modified: 2009-10-15 09:51 PDT (History)
2 users (show)

See Also:


Attachments
JavaScript bindings of WebSocket addEventListener/removeEventListener (5.62 KB, patch)
2009-09-29 03:56 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
update for r48978 (5.54 KB, patch)
2009-10-04 23:04 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
remove V8ObjectEventListener.h (449 bytes, patch)
2009-10-04 23:23 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
JavaScript bindings of WebSocket addEventListener/removeEventListener (5.54 KB, patch)
2009-10-05 22:28 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
WebCore: JavaScript bindings of WebSocket addEventListener/removeEventListener (12.89 KB, patch)
2009-10-06 00:39 PDT, Fumitoshi Ukai
sam: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2009-09-29 03:04:42 PDT
Add Websocket addEventListener and removeEventListner javascript bindings.
Comment 1 Fumitoshi Ukai 2009-09-29 03:56:46 PDT
Created attachment 40292 [details]
JavaScript bindings of WebSocket addEventListener/removeEventListener
Comment 2 Eric Seidel (no email) 2009-09-29 14:03:58 PDT
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?
Comment 3 Fumitoshi Ukai 2009-09-29 19:10:10 PDT
(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?
Comment 4 Fumitoshi Ukai 2009-09-29 19:23:07 PDT
(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?
Comment 5 Eric Seidel (no email) 2009-10-02 12:39:50 PDT
Might want to ask weinig on #webkit as he's more up-to-date on the bindings generator code than I.
Comment 6 Fumitoshi Ukai 2009-10-04 23:04:57 PDT
Created attachment 40613 [details]
update for r48978
Comment 7 Fumitoshi Ukai 2009-10-04 23:23:46 PDT
Created attachment 40614 [details]
remove V8ObjectEventListener.h
Comment 8 Eric Seidel (no email) 2009-10-05 08:14:17 PDT
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-.
Comment 9 Fumitoshi Ukai 2009-10-05 22:28:41 PDT
Created attachment 40691 [details]
JavaScript bindings of WebSocket addEventListener/removeEventListener
Comment 10 Fumitoshi Ukai 2009-10-05 22:35:43 PDT
(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.
Comment 11 Sam Weinig 2009-10-05 22:37:18 PDT
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-.
Comment 12 Fumitoshi Ukai 2009-10-06 00:39:10 PDT
Created attachment 40700 [details]
WebCore: JavaScript bindings of WebSocket addEventListener/removeEventListener
Comment 13 WebKit Commit Bot 2009-10-12 18:28:27 PDT
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.
Comment 14 Fumitoshi Ukai 2009-10-12 18:45:30 PDT
Manually landed as r49488.
Comment 15 Eric Seidel (no email) 2009-10-12 19:14:56 PDT
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.)
Comment 16 Fumitoshi Ukai 2009-10-12 19:17:19 PDT
(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.
Comment 17 Eric Seidel (no email) 2009-10-15 09:51:09 PDT
Going back through logs from last week: The commit-queue rejected it due to the patch to DumpRenderTree.mm not applying.