The SVG code in Node::addEventListener/removeEventListener does not seem to have to be in Node.cpp
Benefits of this approach: - code in Node::addEventListener/removeEventListener is easier to read - SVG parts can be hosted in svg/, much more intuitive - no SVG includes in Node.cpp - SVGElement() checks not needed anymore, since it is called from SVGElement. Drawback is losing the static on tryAddEventListener/tryRemoveEventListener, not sure that would lead to performance loss though.
Created attachment 141842 [details] Patch
Comment on attachment 141842 [details] Patch Attachment 141842 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12701297
Comment on attachment 141842 [details] Patch Attachment 141842 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12679764
Comment on attachment 141842 [details] Patch Attachment 141842 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12684662
Comment on attachment 141842 [details] Patch Attachment 141842 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12680757
Comment on attachment 141842 [details] Patch Attachment 141842 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12705297
Comment on attachment 141842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141842&action=review Great patch, long time nobody looked at this piece of code! You need to look into the build issues, reported by EWS, and I have some further suggestions: > Source/WebCore/svg/SVGElement.cpp:351 > +static inline HashSet<SVGElementInstance*> instancesForSVGElement(Node* node) While you're at it: stop copying the HashSet, instead pass it as reference. Also use SVGElement* as parameter, instead of Node*. static inline void collectInstancesForSVGElement(SVGElement* element, HashSet<SVGElementInstance*>& instances) > Source/WebCore/svg/SVGElement.cpp:369 > +extern inline bool tryAddEventListener(Node* targetNode, const AtomicString& eventType, PassRefPtr<EventListener>, bool useCapture); I'd rather expose tryAddEventListener as protected method in Node.h. But make sure its still inlined, so there's no performance regression for Node::addEventLIstener. > Source/WebCore/svg/SVGElement.cpp:398 > +extern inline bool tryRemoveEventListener(Node* targetNode, const AtomicString& eventType, EventListener*, bool useCapture); Ditto. > Source/WebCore/svg/SVGElement.h:117 > + virtual bool addEventListener(const AtomicString& eventType, PassRefPtr<EventListener>, bool useCapture); > + virtual bool removeEventListener(const AtomicString& eventType, EventListener*, bool useCapture); I think you should mark them as OVERRIDE, that seems to be new idiom when overriding parent classes virtual functions.
Created attachment 141938 [details] Patch
Hi Niko, (In reply to comment #8) > (From update of attachment 141842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141842&action=review > > Great patch, long time nobody looked at this piece of code! You need to look into the build issues, reported by EWS, and I have some further suggestions: > > > Source/WebCore/svg/SVGElement.cpp:351 > > +static inline HashSet<SVGElementInstance*> instancesForSVGElement(Node* node) > > While you're at it: stop copying the HashSet, instead pass it as reference. Also use SVGElement* as parameter, instead of Node*. > static inline void collectInstancesForSVGElement(SVGElement* element, HashSet<SVGElementInstance*>& instances) > > > Source/WebCore/svg/SVGElement.cpp:369 > > +extern inline bool tryAddEventListener(Node* targetNode, const AtomicString& eventType, PassRefPtr<EventListener>, bool useCapture); > > I'd rather expose tryAddEventListener as protected method in Node.h. But make sure its still inlined, so there's no performance regression for Node::addEventLIstener. > > > Source/WebCore/svg/SVGElement.cpp:398 > > +extern inline bool tryRemoveEventListener(Node* targetNode, const AtomicString& eventType, EventListener*, bool useCapture); > > Ditto. > > > Source/WebCore/svg/SVGElement.h:117 > > + virtual bool addEventListener(const AtomicString& eventType, PassRefPtr<EventListener>, bool useCapture); > > + virtual bool removeEventListener(const AtomicString& eventType, EventListener*, bool useCapture); > > I think you should mark them as OVERRIDE, that seems to be new idiom when overriding parent classes virtual functions. Good points! I think all of the above is fixed now with my latest patch.
Comment on attachment 141938 [details] Patch Attachment 141938 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12679933
Comment on attachment 141938 [details] Patch Attachment 141938 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12704407
Comment on attachment 141938 [details] Patch Attachment 141938 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12701485
Comment on attachment 141938 [details] Patch Attachment 141938 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12709079
Comment on attachment 141938 [details] Patch Attachment 141938 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12680885
Created attachment 141985 [details] Patch
Comment on attachment 141985 [details] Patch r=me!
Committed r117086: <http://trac.webkit.org/changeset/117086>