Bug 196025 – Remove the SVG tear off objects for SVGColorAnimator

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

[Said Abou-Hallawa]

SVG attributes like "fill" and "stroke" do not have reflecting properties in SVGElement but they are animatable by SMIL. Since there is no animated properties for these attributes, a temporary variable must be created and initialized with the attribute value. This The value of this variable is progressed during the animation interval. The new progressed value has to be pushed to SVGElementRareData during the animation. When animation stops, the attribute and its animated value is removed from SVGElementRareData.

Because the attribute can be animated by multiple animators, all the multiple animators must reference the same temporary variable. A new animator named SVGColorAnimator will be created. The creator SVGPropertyAnimatorCreator will keep a RefCounted object for the temporary variable in a HashMap which is indexed by the attribute name. A new entry is created and appended to this HashMap when the first animator is created. An existing entry will be removed when the last animator is removed.

[Said Abou-Hallawa]

Created attachment 365394 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 365394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365394&action=review

> Source/WebCore/ChangeLog:44
> +        -- SVGPropertyAnimatorCreator checks its HashMap it has an entry for the

checks whether its HashMap has an entry

> Source/WebCore/ChangeLog:53
> +           SVGElement about that which will notify its SVGPropertyAnimatorCreator.

Remove "about that"

> Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.cpp:38
> +    static NeverDestroyed<const AtomicString> currentColor("currentColor", AtomicString::ConstructFromLiteral);
> +
> +    if (string != currentColor.get())

Not sure why this uses AtomicString; this code does an explicit string compare, so doesn't atomic-ness.

> Source/WebCore/svg/properties/SVGPropertyAnimatorCreator.h:33
> +class SVGPropertyAnimatorCreator {

Classes that make things are usually called "factories". So SVGPropertyAnimatorFactory

> Source/WebCore/svg/properties/SVGPropertyAnimatorCreator.h:63
> +        // If refCount = 1 (in the animator) + 1 (in m_attributeProperty) = 2, the entry can be deleted.
> +        if (iterator->value->refCount() == 2)
> +            m_attributeProperty.remove(iterator);

Removing based on looking at refCounts feels a bit icky.

[Said Abou-Hallawa]

Comment on attachment 365394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365394&action=review

>> Source/WebCore/ChangeLog:44
>> +        -- SVGPropertyAnimatorCreator checks its HashMap it has an entry for the
> 
> checks whether its HashMap has an entry

Fixed.

>> Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.cpp:38
>> +    if (string != currentColor.get())
> 
> Not sure why this uses AtomicString; this code does an explicit string compare, so doesn't atomic-ness.

static variable is converted to NeverDestroyed<const String>.

>> Source/WebCore/svg/properties/SVGPropertyAnimatorCreator.h:33
>> +class SVGPropertyAnimatorCreator {
> 
> Classes that make things are usually called "factories". So SVGPropertyAnimatorFactory

Done.

>> Source/WebCore/svg/properties/SVGPropertyAnimatorCreator.h:63
>> +            m_attributeProperty.remove(iterator);
> 
> Removing based on looking at refCounts feels a bit icky.

I will think about it.

[Radar WebKit Bug Importer]

<rdar://problem/49085121>

[Said Abou-Hallawa]

Committed r243259: <https://trac.webkit.org/changeset/243259>