[webkit-reviews] review denied: [Bug 12511] <use> has event dispatching issues : [Attachment 12833] Initial patch

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 12511: <use> has event dispatching issues
http://bugs.webkit.org/show_bug.cgi?id=12511

Attachment 12833: Initial patch
http://bugs.webkit.org/attachment.cgi?id=12833&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This has too much SVG_SUPPORT code sprinkled in the core. We need to aim to
avoid doing that. I don't buy the "trying not to affect the non-SVG version"
argument; most mainstream versions of WebKit are including SVG, so lets not try
to optimize too much for a less-common case.

+    SVGElementInstance* instance = target->toSVGElementInstance();
+    if (instance)
+	 return toJS(exec, instance);

We usually declare inside the if statements for lines like this one.

+#ifdef SVG_SUPPORT
+    class SVGElementInstance; 
+#endif

No need to put the declaration inside an #ifdef.

-    RegisteredEventListenerList::Iterator end = listenersCopy.end();	 
-    
+    RegisteredEventListenerList::Iterator end = listenersCopy.end();
+

???

+#ifdef SVG_SUPPORT
+    for (n = this; n; n = n->isShadowNode() ? n->shadowParentNode() :
n->parentNode()) {
+#else
     for (n = this; n; n = n->parentNode()) {
+#endif

I think it's a bad idea to put this in an ifdef. We have no need to slightly
optimize the no-SVG case like this.

I'm not cormfortable with all this special-case code in
EventTargetNode::dispatchEvent -- please make this non-SVG-specific.

 Node* Node::shadowAncestorNode()
 {
+#ifdef SVG_SUPPORT
+    if (isSVGElement())
+	 return this;
+#endif

Is there some way we can avoid this.