RESOLVED FIXED 14111
Autogenerate Event JS binding
https://bugs.webkit.org/show_bug.cgi?id=14111
Summary Autogenerate Event JS binding
Sam Weinig
Reported 2007-06-12 22:50:02 PDT
Patch in a minute.
Attachments
patch (47.35 KB, patch)
2007-06-12 23:15 PDT, Sam Weinig
darin: review-
Update patch (48.20 KB, patch)
2007-06-14 23:38 PDT, Sam Weinig
mjs: review-
just the event autogeneration (25.70 KB, patch)
2007-06-26 10:28 PDT, Sam Weinig
no flags
cleanup portion of the patch (24.76 KB, patch)
2007-06-26 16:38 PDT, Sam Weinig
mjs: review+
Sam Weinig
Comment 1 2007-06-12 23:15:42 PDT
Darin Adler
Comment 2 2007-06-14 20:53:09 PDT
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
Comment 3 2007-06-14 23:38:31 PDT
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 4 2007-06-26 00:33:45 PDT
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
Comment 5 2007-06-26 10:28:14 PDT
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 6 2007-06-26 12:32:50 PDT
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
Comment 7 2007-06-26 12:43:35 PDT
(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
Comment 8 2007-06-26 12:56:13 PDT
Landed in r23795.
Sam Weinig
Comment 9 2007-06-26 12:56:54 PDT
Reopening bug to deal with second half of the split.
Sam Weinig
Comment 10 2007-06-26 16:38:42 PDT
Created attachment 15261 [details] cleanup portion of the patch
Maciej Stachowiak
Comment 11 2007-06-26 21:36:33 PDT
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
Comment 12 2007-06-27 20:35:56 PDT
Landed 2nd patch in r23841.
Note You need to log in before you can comment on or make changes to this bug.