Bug 14111 – Autogenerate Event JS binding

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sam Weinig]

Patch in a minute.

[Sam Weinig]

Created attachment 14991 [details]
patch

[Darin Adler]

Comment on attachment 14991 [details]
patch

Is there no way to avoid special-casing clipboardData and dataTransfer? Can't we just move those to the appropriate event classes?

+    // FIXME: Is this lock necessary?
+    KJS::JSLock lock;

Seems clear that it is.

+            if (WebCore::JSUnprotectedEventListener* listener = static_cast<WebCore::JSUnprotectedEventListener*>(m_impl->onReadyStateChangeListener()))
Why is the WebCore prefix needed?

-        [OldStyleObjC] void initEvent(in DOMString eventTypeArg, 
+        [OldStyleObjC] void initEvent(in AtomicString eventTypeArg, 

This seems wrong. The API should be DOMString even though the implementation is AtomicString.

[Sam Weinig]

Created attachment 15042 [details]
Update patch

>Is there no way to avoid special-casing clipboardData and dataTransfer? Can't
>we just move those to the appropriate event classes?

I have moved dataTransfer into MouseEvent (though perhaps it should be in a DragEvent class that we don't have) but have to special-case clipboardData for now as we have no JSClipboardEvent event class to push it into yet.  If you think it is appropriate, I can create one and put the method there.

>+    // FIXME: Is this lock necessary?
>+    KJS::JSLock lock;
>
> Seems clear that it is.

Oops, didn't mean to leave that in,  Removed.

>+            if (WebCore::JSUnprotectedEventListener* listener =
>static_cast<WebCore::JSUnprotectedEventListener*>(m_impl->onReadyStateChangeListener()))
>Why is the WebCore prefix needed?

Fixed.

>-        [OldStyleObjC] void initEvent(in DOMString eventTypeArg, 
>+        [OldStyleObjC] void initEvent(in AtomicString eventTypeArg, 
>
>This seems wrong. The API should be DOMString even though the implementation is
>AtomicString.

Fixed.

[Maciej Stachowiak]

Comment on attachment 15042 [details]
Update patch

I think it would be good to split the actual bindings change from the various style cleanups in other nearby code (I think those are worth doing too though).

Please split.

[Sam Weinig]

Created attachment 15250 [details]
just the event autogeneration

First part of the split.  This patch is just the autogeneration of JSEvent and the associated changes needed to get it to work.

[Geoffrey Garen]

Comment on attachment 15250 [details]
just the event autogeneration

Looks good to me, unless other reviewers object.

+    if (!event)
+        return KJS::jsNull();
+
+    KJS::ScriptInterpreter* interp = static_cast<KJS::ScriptInterpreter*>(exec->dynamicInterpreter());
+
+    KJS::JSLock lock;

The lock here needs to go at the top of the function. jsNull() used to perform a GC allocation. There's no guarantee it won't again at some time in the future.

[Darin Adler]

(In reply to comment #6)
> The lock here needs to go at the top of the function. jsNull() used to perform
> a GC allocation. There's no guarantee it won't again at some time in the
> future.

Geoff, that sounds like a reasonable rule. But if we want clients to consistently follow that rule, then I think the jsNull() function's implementation should ASSERT(JSLock::currentThreadIsHoldingLock()) with a comment like your remark above.

[Sam Weinig]

Landed in r23795.

[Sam Weinig]

Reopening bug to deal with second half of the split.

[Sam Weinig]

Created attachment 15261 [details]
cleanup portion of the patch

[Maciej Stachowiak]

Comment on attachment 15261 [details]
cleanup portion of the patch

r=me with some trepidation, since we shouldn't really be making cleanup-only changes right now. But I did tell you to split it.

[Sam Weinig]

Landed 2nd patch in r23841.