Bug 86426

Summary: Refactor SVG parts of Node::addEventListener/removeEventListener
Product: WebKit Reporter: Rob Buis <rwlbuis>
Component: SVGAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch zimmermann: review+

Description Rob Buis 2012-05-14 18:59:32 PDT
The SVG code in  Node::addEventListener/removeEventListener does not seem to have to be in Node.cpp
Comment 1 Rob Buis 2012-05-14 19:02:43 PDT
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.
Comment 2 Rob Buis 2012-05-14 19:11:17 PDT
Created attachment 141842 [details]
Patch
Comment 3 Early Warning System Bot 2012-05-14 19:26:24 PDT
Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12701297
Comment 4 Early Warning System Bot 2012-05-14 19:33:49 PDT
Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12679764
Comment 5 Gyuyoung Kim 2012-05-14 20:25:36 PDT
Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12684662
Comment 6 Build Bot 2012-05-14 20:35:50 PDT
Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12680757
Comment 7 WebKit Review Bot 2012-05-14 22:37:03 PDT
Comment on attachment 141842 [details]
Patch

Attachment 141842 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12705297
Comment 8 Nikolas Zimmermann 2012-05-15 00:11:09 PDT
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.
Comment 9 Rob Buis 2012-05-15 05:54:35 PDT
Created attachment 141938 [details]
Patch
Comment 10 Rob Buis 2012-05-15 06:42:38 PDT
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 11 Early Warning System Bot 2012-05-15 06:46:42 PDT
Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12679933
Comment 12 Build Bot 2012-05-15 06:52:07 PDT
Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12704407
Comment 13 Early Warning System Bot 2012-05-15 06:57:29 PDT
Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12701485
Comment 14 WebKit Review Bot 2012-05-15 07:38:28 PDT
Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12709079
Comment 15 Gyuyoung Kim 2012-05-15 07:48:04 PDT
Comment on attachment 141938 [details]
Patch

Attachment 141938 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12680885
Comment 16 Rob Buis 2012-05-15 09:07:14 PDT
Created attachment 141985 [details]
Patch
Comment 17 Nikolas Zimmermann 2012-05-15 09:47:38 PDT
Comment on attachment 141985 [details]
Patch

r=me!
Comment 18 Rob Buis 2012-05-15 10:04:30 PDT
Committed r117086: <http://trac.webkit.org/changeset/117086>