[webkit-reviews] review denied: [Bug 86426] Refactor SVG parts of Node::addEventListener/removeEventListener : [Attachment 141842] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 86426: Refactor SVG parts of Node::addEventListener/removeEventListener
https://bugs.webkit.org/show_bug.cgi?id=86426

Attachment 141842: Patch
https://bugs.webkit.org/attachment.cgi?id=141842&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.