[webkit-reviews] review denied: [Bug 83856] SMIL animation causes leak of the related Document (and many elements) : [Attachment 138490] patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 83856: SMIL animation causes leak of the related Document (and many
elements)
https://bugs.webkit.org/show_bug.cgi?id=83856

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138490&action=review


Thanks Tim for taking this! The changes to the storage in SVGPropertyTearOff
and SVGAnimatedProperty are fine, I'm just worried that both properties and
instanceProperties are passed onto the SVGAnimatedTypeAnimators. That should be
avoided:

> Source/WebCore/svg/SVGAnimatedTypeAnimator.h:80
> +    SVGElementAnimatedPropertyListMap
findAnimatedPropertiesFromInstancesForAttributeName(SVGElement* targetElement,
const QualifiedName& attributeName)

A seperated code-path for collecting the instances, returning a HashMap<> seems
hacky, I'd rather unfiy this code with findAnimatedPropertiesForAttributeName:
void collectAnimatedPropertiesForAttributeName(SVGElement* targetElement, const
QualifiedName& attributeName,  HashMap<SVGElement*, RefPtr<SVGAnimatedProperty>
>& map)

The map could be filled as follows:
- key: targetElement | value: properties returned by
targetElement->localAttributeToPropertyMap().animatedPropertiesForAttribute(...
)
- key: shadowTreeElement1 | value: properties returned by
shadowTreeElement->localAttributeToPropertyMap()...
- key: shadowTreeElement2 | value: ditto.
...

This way we can continue to use a single call to collect all properties from
the target element and its instances.
The classes deriving from SVGAnimatedTypeAnimator don't need to know about the
origin of the SVGAnimatedProperty (if they belong to an instance in the shadow
tree element or a regular element), thus exposing both "properties" and
"instanceProperties" should be avoided.

What do you think?