Bug 21122 - Autogenerate JS event listeners
Summary: Autogenerate JS event listeners
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-25 15:29 PDT by Sam Weinig
Modified: 2008-10-01 00:28 PDT (History)
2 users (show)

See Also:


Attachments
patch 1 - generate EventTargetNode (74.42 KB, patch)
2008-09-29 22:36 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
patch 2 - standardize EventListeners (66.33 KB, patch)
2008-09-30 16:22 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch 3 - Generation (65.50 KB, patch)
2008-09-30 23:54 PDT, Sam Weinig
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2008-09-25 15:29:37 PDT
A big chuck of the remain code to be autogenerated is the event listener code (ie. window.onload and the like).  Getting it generated would be a great move in the right direction
Comment 1 Sam Weinig 2008-09-29 22:36:10 PDT
Created attachment 23932 [details]
patch 1 - generate EventTargetNode
Comment 2 Sam Weinig 2008-09-30 16:21:41 PDT
Comment on attachment 23932 [details]
patch 1 - generate EventTargetNode

First patch landed in 37090.
Comment 3 Sam Weinig 2008-09-30 16:22:15 PDT
Created attachment 23960 [details]
patch 2 - standardize EventListeners
Comment 4 Eric Seidel (no email) 2008-09-30 16:53:17 PDT
Comment on attachment 23960 [details]
patch 2 - standardize EventListeners

yay!  yay!  yay!

+    if (JSUnprotectedEventListener* listener = static_cast<JSUnprotectedEventListener*>(m_impl->onchecking()))
+        listener->mark();
Should be a static inline function, IMO
markListener(m_impl->onchecking());
Seems such a static-inline probably should be in some JSBinding.h type header since it could be used all over the place.  Then we could later add a type-check in a single place to handle the obj-c bad static cast check.

If there is a way to set any of these via obj-c, it seems all of this code would crash due to bad static_casts. :(

Can a frame ever not have a window object?  Even say, during destruction?  Cause your code seems to assume that toJSDOMWindow() can never return NULL (which may very well be the case).

It seems also given how common:
toJSDOMWindow(frame)->findOrCreateJSUnprotectedEventListener(exec, value, true)
is we might consider making that a static inline function in some JSBinding.h-like header.
findOrCreateJSUnprotectedEventLisener(frame, exec, value) (I guess that only really saves toJSDOMWindow(frame)-> and , true from typing... 

In terms of solving hte "obj-c listeners cause crash case) one could very easily add a toUnprotectedEventListener() function which did the conversion and was used by all the above proposed functions, as well as any case in this existing code where one needs to do that cast.  

Otherwise looks great!
Comment 5 Darin Adler 2008-09-30 17:13:46 PDT
(In reply to comment #4)
> Seems such a static-inline probably should be in some JSBinding.h type header

As I discussed with Sam the other day. If we're putting an inline in a header it should not be marked static. The static gives it internal linkage which is not what we want for inline functions in a header.
Comment 6 Sam Weinig 2008-09-30 17:39:51 PDT
Comment on attachment 23960 [details]
patch 2 - standardize EventListeners

Second patch landed in r37128.
Comment 7 Sam Weinig 2008-09-30 23:54:40 PDT
Created attachment 23968 [details]
Patch 3 - Generation
Comment 8 Nikolas Zimmermann 2008-10-01 00:10:26 PDT
Comment on attachment 23968 [details]
Patch 3 - Generation 

Yay! Another excellent patch in the right direction, r+ r+ r+!
Comment 9 Alexey Proskuryakov 2008-10-01 00:18:28 PDT
Comment on attachment 23968 [details]
Patch 3 - Generation 

r=me

Please don't auto-generate MessagePort listeners yet - they will need to learn how to work when there is no frame or window soon.

XMLHttpRequest will also need to work in worker threads, but that will happen much later, so I think that it's good to auto-generate it now.
Comment 10 Sam Weinig 2008-10-01 00:28:25 PDT
Last patch landed in r37142.