[webkit-reviews] review granted: [Bug 196025] Remove the SVG tear off objects for SVGColorAnimator : [Attachment 365394] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 196025: Remove the SVG tear off objects for SVGColorAnimator
https://bugs.webkit.org/show_bug.cgi?id=196025

Attachment 365394: Patch

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




--- Comment #2 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 365394
  --> https://bugs.webkit.org/attachment.cgi?id=365394
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.