Bug 14111 - Autogenerate Event JS binding
Summary: Autogenerate Event JS binding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 13779
  Show dependency treegraph
 
Reported: 2007-06-12 22:50 PDT by Sam Weinig
Modified: 2007-06-27 20:35 PDT (History)
1 user (show)

See Also:


Attachments
patch (47.35 KB, patch)
2007-06-12 23:15 PDT, Sam Weinig
darin: review-
Details | Formatted Diff | Diff
Update patch (48.20 KB, patch)
2007-06-14 23:38 PDT, Sam Weinig
mjs: review-
Details | Formatted Diff | Diff
just the event autogeneration (25.70 KB, patch)
2007-06-26 10:28 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
cleanup portion of the patch (24.76 KB, patch)
2007-06-26 16:38 PDT, Sam Weinig
mjs: 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 2007-06-12 22:50:02 PDT
Patch in a minute.
Comment 1 Sam Weinig 2007-06-12 23:15:42 PDT
Created attachment 14991 [details]
patch
Comment 2 Darin Adler 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.
Comment 3 Sam Weinig 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.
Comment 4 Maciej Stachowiak 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.
Comment 5 Sam Weinig 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.
Comment 6 Geoffrey Garen 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.
Comment 7 Darin Adler 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.
Comment 8 Sam Weinig 2007-06-26 12:56:13 PDT
Landed in r23795.
Comment 9 Sam Weinig 2007-06-26 12:56:54 PDT
Reopening bug to deal with second half of the split.
Comment 10 Sam Weinig 2007-06-26 16:38:42 PDT
Created attachment 15261 [details]
cleanup portion of the patch
Comment 11 Maciej Stachowiak 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.
Comment 12 Sam Weinig 2007-06-27 20:35:56 PDT
Landed 2nd patch in r23841.