WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2007-06-12 23:15:42 PDT
Created
attachment 14991
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug