Bug 5874 - WebCore+SVG needs support for "evt" instead of event
Summary: WebCore+SVG needs support for "evt" instead of event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 6026
  Show dependency treegraph
 
Reported: 2005-11-29 03:48 PST by Eric Seidel (no email)
Modified: 2006-01-22 16:52 PST (History)
0 users

See Also:


Attachments
simple patch adding "evt" support (1.04 KB, patch)
2005-11-29 03:48 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
addressed Darin's concerns (18.50 KB, patch)
2006-01-22 04:40 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
addressed darin's further concerns (22.28 KB, patch)
2006-01-22 15:18 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-11-29 03:48:31 PST
For whatever reason the SVG working group chose to specify "evt" as the name of the parameter passed to 
inline event handlers.

This simple patch adds support for this to WebCore.
Comment 1 Eric Seidel (no email) 2005-11-29 03:48:49 PST
Created attachment 4844 [details]
simple patch adding "evt" support
Comment 2 Eric Seidel (no email) 2005-11-29 03:49:08 PST
Comment on attachment 4844 [details]
simple patch adding "evt" support

mjs, darin or ggaren could easily review this.
Comment 3 Darin Adler 2005-11-29 07:29:19 PST
Comment on attachment 4844 [details]
simple patch adding "evt" support

I think a cleaner and clearer way to do this is to have a createSVGEventHandler
alternative to the createHTMLEventHandler function that follows the SVG rules
instead.

To control the parameter name used when constructing the event listener, I
suggest either adding a parameter to the JSLazyEventListener constructor or
creating derived classes specific to HTML and SVG.
Comment 4 Maciej Stachowiak 2005-11-29 15:38:47 PST
Agreed, there should be a different code path for SVG rather than testing the element like this.
Comment 5 Dave Hyatt 2006-01-15 00:21:17 PST
I believe you should support both evt and event.  I think that's something I recall was decided (either by 
Mozilla or CDF or someone).
Comment 6 Eric Seidel (no email) 2006-01-22 04:40:26 PST
Created attachment 5836 [details]
addressed Darin's concerns

This new patch adds an JSSVGLazyEventListener subclass for SVG to use.  This patch also simplifies the way that event listeners are created by removing two unecessary layers of abstraction.  This should address both darin and maciej's concerns.  We do not currently support both "evt" and "event" for SVG event handlers like hyatt (and others) suggested.  That dual functionality can be tracked by a separate bug.
Comment 7 Darin Adler 2006-01-22 10:46:20 PST
Comment on attachment 5836 [details]
addressed Darin's concerns

I don't like the underscore-prefixed name for the local variable. We should steer away from underscore prefixes in general since they are reserved for the library and platform.

Patch does not include JSSVGLazyEventListener.h and .cpp so I couldn't review them. Because of that, review-.
Comment 8 Eric Seidel (no email) 2006-01-22 15:18:07 PST
Created attachment 5851 [details]
addressed darin's further concerns
Comment 9 Darin Adler 2006-01-22 15:23:03 PST
Comment on attachment 5851 [details]
addressed darin's further concerns

I made a couple of comments on IRC. r=me (with or without the refinements from IRC).