Bug 25544 - toString (function source) for event listeners in V8 bindings shows wrapper code
Summary: toString (function source) for event listeners in V8 bindings shows wrapper code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-04 04:40 PDT by Søren Gjesse
Modified: 2014-04-24 16:45 PDT (History)
2 users (show)

See Also:


Attachments
Patch to fix the issue. (7.26 KB, patch)
2009-05-04 04:42 PDT, Søren Gjesse
eric: review-
Details | Formatted Diff | Diff
Added test to patch (9.23 KB, patch)
2009-05-05 15:31 PDT, Søren Gjesse
no flags Details | Formatted Diff | Diff
Fixed name and email in ChangeLog (9.21 KB, patch)
2009-05-05 15:37 PDT, Søren Gjesse
eric: review+
Details | Formatted Diff | Diff
a test which tests the full change (not quite enough regexp goodness to work in all browsers) (966 bytes, text/html)
2009-05-05 16:09 PDT, Eric Seidel (no email)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Søren Gjesse 2009-05-04 04:40:15 PDT
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);
  }
Comment 1 Søren Gjesse 2009-05-04 04:42:00 PDT
Created attachment 29988 [details]
Patch to fix the issue.
Comment 2 Eric Seidel (no email) 2009-05-04 05:35:56 PDT
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.
Comment 3 Mark Rowe (bdash) 2009-05-04 08:52:00 PDT
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.
Comment 4 Darin Fisher (:fishd, Google) 2009-05-04 09:42:49 PDT
so perhaps fast/events would be more appropriate?
Comment 5 Søren Gjesse 2009-05-05 15:31:25 PDT
Created attachment 30036 [details]
Added test to patch
Comment 6 Søren Gjesse 2009-05-05 15:37:45 PDT
Created attachment 30037 [details]
Fixed name and email in ChangeLog
Comment 7 Eric Seidel (no email) 2009-05-05 16:06:01 PDT
Comment on attachment 30036 [details]
Added test to patch

This patch is obsoleted by the later patch, marking it so.
Comment 8 Eric Seidel (no email) 2009-05-05 16:06:20 PDT
Comment on attachment 29988 [details]
Patch to fix the issue.

This patch is also now obsolete.
Comment 9 Eric Seidel (no email) 2009-05-05 16:06:58 PDT
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.
Comment 10 Eric Seidel (no email) 2009-05-05 16:09:45 PDT
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 11 Eric Seidel (no email) 2009-05-05 16:49:54 PDT
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}");
?
Comment 12 Eric Seidel (no email) 2009-05-06 01:25:39 PDT
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
Comment 13 Eric Seidel (no email) 2009-05-06 01:26:11 PDT
If you're curious what modifications I made to your test case when landing, see:
http://trac.webkit.org/changeset/43289
Comment 14 Søren Gjesse 2009-05-06 01:49:17 PDT
(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?
Comment 15 Darin Adler 2014-04-24 16:45:01 PDT
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.