[webkit-reviews] review granted: [Bug 21122] Autogenerate JS event listeners : [Attachment 23960] patch 2 - standardize EventListeners

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 21122: Autogenerate JS event listeners
https://bugs.webkit.org/show_bug.cgi?id=21122

Attachment 23960: patch 2 - standardize EventListeners
https://bugs.webkit.org/attachment.cgi?id=23960&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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!