Bug 14111

Created attachment 14991 [details] patch

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.

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.

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.

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.

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.

(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.

Landed in r23795.

Reopening bug to deal with second half of the split.

Created attachment 15261 [details] cleanup portion of the patch

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.

Landed 2nd patch in r23841.