For a event listener like onsubmit="return validate(this)" the onsubmit.toString will return this: function onsubmit(evt) { with (this.ownerDocument ? this.ownerDocument : {}) { with (this.form ? this.form : {}) { with (this) { return (function(evt){return validate(this)}).call(this, evt); } } } } whereas something like this should be returned: function onsubmit(event) { return validate(this); }
Created attachment 29988 [details] Patch to fix the issue.
Comment on attachment 29988 [details] Patch to fix the issue. In general if you need a review, you need to mark things with r=? otherwise they won't appear in the queue. There are several typos in the changelog. This needs a test added to LayoutTests (probably in LayoutTests/fast/js) String::append is generally a slow way to build strings, StringBuilder or Vector<UChar> is faster. But in this case it may not matter. r- for lack of test.
A layout test for this shouldn't live in fast/js since it is specific to event listeners, which are not a JavaScript concept. Tests in fast/js are intended to require only JavaScript to run.
so perhaps fast/events would be more appropriate?
Created attachment 30036 [details] Added test to patch
Created attachment 30037 [details] Fixed name and email in ChangeLog
Comment on attachment 30036 [details] Added test to patch This patch is obsoleted by the later patch, marking it so.
Comment on attachment 29988 [details] Patch to fix the issue. This patch is also now obsolete.
Comment on attachment 30037 [details] Fixed name and email in ChangeLog Thank you for providing a test case. This does show the failure, but it does not fully test the change. I'll upload an example of what I mean.
Created attachment 30040 [details] a test which tests the full change (not quite enough regexp goodness to work in all browsers) Even better would be a js-style test, but those are a little harder to get used to. This test doesn't have the regexps like yours does (which it would need in order to function cross-browser. But it does show the evt vs. event printing which your patch has code in it to do.
Comment on attachment 30037 [details] Fixed name and email in ChangeLog I'll land this later today. Why the extra spaces before: + toStringResult.append( "\n}"); ?
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/events/event-function-toString-expected.txt A LayoutTests/fast/events/event-function-toString.html A LayoutTests/fast/events/resources/event-function-toString.js M WebCore/ChangeLog M WebCore/bindings/v8/ScriptEventListener.cpp M WebCore/bindings/v8/V8LazyEventListener.cpp M WebCore/bindings/v8/V8LazyEventListener.h Committed r43289
If you're curious what modifications I made to your test case when landing, see: http://trac.webkit.org/changeset/43289
(In reply to comment #13) > If you're curious what modifications I made to your test case when landing, > see: > http://trac.webkit.org/changeset/43289 > Thanks for landing this - is the modified test what you refer to as a js-style test?
Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.