Summary: | Refactor EventTargetNode & JSEventTargetNode for an upcoming SVG patch | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, mjs, oliver, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Nikolas Zimmermann
2007-12-14 17:45:17 PST
Created attachment 17900 [details]
Initial patch
Okay, so far this looks sane, although i'd like maciej and possibly hyatt to look at it as well. My only real concern atm is that a number of the new files use large quantities of code from other files, so the appropriate copyrights should be moved across as well. (In reply to comment #2) > Okay, so far this looks sane, although i'd like maciej and possibly hyatt to > look at it as well. > > My only real concern atm is that a number of the new files use large quantities > of code from other files, so the appropriate copyrights should be moved across > as well. > I started of with fresh header templates and forgot about them - readded all existing copyrights in the approriate files (all files in bindings/js for instance). Niko Created attachment 17901 [details]
Updated patch
New patch after discussion with Maciej - remove the two callbacks and the JSEventTarget* owner pointer from JSEventTargetBase to save size. We can now store JSEventTargetBase by value in JSEventTargetNode (& JSEventTargetSVGElementInstance).
Fixed copyright as noticed by Oliver.
When reading the spec, I thought that a more natural approach would be to tweak event dispatching, not the elements themselves. It seems unfortunate that listener lists have to be kept in sync - as a general rule, everything that needs to be kept in sync will get out of sync sooner or later. I.e. when dispatching an event to SVGElementInstance, dispatchGenericEvent() would take listener list from the original, and so would addEventListener/removeEventListener. Does this make any sense? Or did I just misunderstand the desired behavior? (In reply to comment #5) > When reading the spec, I thought that a more natural approach would be to tweak > event dispatching, not the elements themselves. It seems unfortunate that > listener lists have to be kept in sync - as a general rule, everything that > needs to be kept in sync will get out of sync sooner or later. > > I.e. when dispatching an event to SVGElementInstance, dispatchGenericEvent() > would take listener list from the original, and so would > addEventListener/removeEventListener. > > Does this make any sense? Or did I just misunderstand the desired behavior? Hi Alexey, thanks for taking time to check this approach. It will be easier to discuss once this patch is in, and I actually uploaded the follow-up SVG patch. The idea is that EventTargetSVGElementInstance, addEventListener/removeEventListener/dispatchEvent etc.. call EventTarget::addEventListener(correspondingElement(), ...) which means that any events dispatched to an element, which gets directly or indirectly referenced by <use> will be intercepted in dispatchGenericEvent() and their 'target' & 'currentTarget' property will be modified, to point to the SVGElementInstance objects. So what you are describing above, seems perfectly fine. Greetings, Niko Rereading my patch, I have some fears regarding s_prototypeName initialization. Anyone has a suggestion for a nicer way not involving a static const char*? Greetings, Niko Created attachment 17915 [details]
Updated patch v2
Incorporated Erics suggestions:
- create helper method to convert from JSEventTargetProperties tokens to AtomicString to simplify getListener/setListener implementation
- Better way to handle s_prototypeName (better, as in actually working reliable!)
No regressions.
Comment on attachment 17915 [details]
Updated patch v2
I think this needs at least one more round for code clarity:
WildFox: your indenting is kinda strange in JSEventTargetPrototypeInformation
WildFox: IMO the big + if (eventType == DOMSubtreeModifiedEvent) if-chain should be abstracted into a static inline funtion
type = listenerTypeForEventType(eventType)
WildFox: actually, better would probably be shouldEnableSpecialListenerTypeOnDocument(eventType, type)
then you can combine it with the next if
something like that
WildFox: you also seem to have done a search replace of /listen//
WildFox: + // We do want capturing event ers to be invoked here, even though
WildFox: you should probably search for " ers"
WildFox: if you're moving the code anyway, you shoudl clean up the "why don't we recalc the node chain here" comment
which is about 15 lines long and only needs to be 2 :)
"We don't recalc the node chain here because she spec says so:"
[spec quote]
WildFox: all of your:
+#if ENABLE(SVG)
+ evt->setCurrentTarget(applySVGEventTargetRules(eventTargetNode, evt.get()));
blocks shoudl just change to a single static iline
setCurrentTargetRespectingSVGTargetRules(evt, eventTargetNode)
WildFox: and then you can have the special logic all in one plac,e instead of lots of copy/paste #if ENABLE code
Created attachment 17917 [details]
Updated patch v3
Updated, fixing Erics comments.
Comment on attachment 17917 [details]
Updated patch v3
Looks good.
+ // Remove existing identical er set with identical arguments.
Still need to do a search for " er" and fix those to " listener". :)
dispatchGenericEvent is horribly long, we'll eventually need to break it up into more static inlines.
Looks good.
Landed in r28756 with Erics last comment fixed. |