Bug 86426 – Refactor SVG parts of Node::addEventListener/removeEventListener

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Rob Buis]

The SVG code in  Node::addEventListener/removeEventListener does not seem to have to be in Node.cpp

[Rob Buis]

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.

[Rob Buis]

Created attachment 141842 [details]
Patch

[Early Warning System Bot]

Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12701297

[Early Warning System Bot]

Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12679764

[Gyuyoung Kim]

Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12684662

[Build Bot]

Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12680757

[WebKit Review Bot]

Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12705297

[Nikolas Zimmermann]

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.

[Rob Buis]

Created attachment 141938 [details]
Patch

[Rob Buis]

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.

[Early Warning System Bot]

Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12679933

[Build Bot]

Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12704407

[Early Warning System Bot]

Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12701485

[WebKit Review Bot]

Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12709079

[Gyuyoung Kim]

Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12680885

[Rob Buis]

Created attachment 141985 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 141985 [details]
Patch

r=me!

[Rob Buis]

Committed r117086: <http://trac.webkit.org/changeset/117086>