[webkit-reviews] review denied: [Bug 16445] Refactor EventTargetNode & JSEventTargetNode for an upcoming SVG patch : [Attachment 17915] Updated patch v2

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 16445: Refactor EventTargetNode & JSEventTargetNode for an upcoming SVG
patch
http://bugs.webkit.org/show_bug.cgi?id=16445

Attachment 17915: Updated patch v2
http://bugs.webkit.org/attachment.cgi?id=17915&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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