toString (function source) for event listeners in V8 bindings shows wrapper code

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Søren Gjesse]

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);
  }

[Søren Gjesse]

Created attachment 29988 [details]
Patch to fix the issue.

[Eric Seidel (no email)]

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.

[Mark Rowe (bdash)]

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.

[Darin Fisher (:fishd, Google)]

so perhaps fast/events would be more appropriate?

[Søren Gjesse]

Created attachment 30036 [details]
Added test to patch

[Søren Gjesse]

Created attachment 30037 [details]
Fixed name and email in ChangeLog

[Eric Seidel (no email)]

Comment on attachment 30036 [details]
Added test to patch

This patch is obsoleted by the later patch, marking it so.

[Eric Seidel (no email)]

Comment on attachment 29988 [details]
Patch to fix the issue.

This patch is also now obsolete.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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}");
?

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

If you're curious what modifications I made to your test case when landing, see:
http://trac.webkit.org/changeset/43289

[Søren Gjesse]

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

[Darin Adler]

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.