Summary: | Refactor SVG parts of Node::addEventListener/removeEventListener | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rwlbuis> | ||||||||
Component: | SVG | Assignee: | 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
Rob Buis
2012-05-14 18:59:32 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. 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> |