Remove the SVG property tear off objects for SVGAnimatedBoolean

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Like what we did for SVGAnimatedInteger in <https://trac.webkit.org/changeset/243036>, we are going to do similar transform for SVGAnimatedBoolean. Instead of saving the raw boolean data in the SVGElement and use the tear off object to provide the animation and the DOM objects, the SVGElement will hold a Ref<SVGAnimatedBoolean>. This will eliminate the need to create any other objects.

[Said Abou-Hallawa]

Created attachment 364965 [details]
Patch

[Said Abou-Hallawa]

Created attachment 364966 [details]
Patch for review

[Simon Fraser (smfr)]

Comment on attachment 364966 [details]
Patch for review

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

> Source/WebCore/svg/SVGAnimatorFactory.h:52
> -            return std::make_unique<SVGAnimatedBooleanAnimator>(animationElement, contextElement);
> +            return nullptr;

This is legacy code that will be removed?

> Source/WebCore/svg/SVGExternalResourcesRequired.h:77
> +    Ref<SVGAnimatedBoolean> m_externalResourcesRequired;

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/externalResourcesRequired says that "externalResourcesRequired" is not animatable (I could not find it in the spec). So why do we make a animated boolean for it?

[Said Abou-Hallawa]

Comment on attachment 364966 [details]
Patch for review

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

>> Source/WebCore/svg/SVGAnimatorFactory.h:52
>> +            return nullptr;
> 
> This is legacy code that will be removed?

Yes.

>> Source/WebCore/svg/SVGExternalResourcesRequired.h:77
>> +    Ref<SVGAnimatedBoolean> m_externalResourcesRequired;
> 
> https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/externalResourcesRequired says that "externalResourcesRequired" is not animatable (I could not find it in the spec). So why do we make a animated boolean for it?

This comment at the top of Source/WebCore/svg/SVGExternalResourcesRequired.h might explain why it was defined as SVGAnimatedBoolean. But I did not understand what is the difference between the SVG DOM and the SVG language definition especially there is no links. 

// Notes on a SVG 1.1 spec discrepancy:
// The SVG DOM defines the attribute externalResourcesRequired as being of type SVGAnimatedBoolean, whereas the 
// SVG language definition says that externalResourcesRequired is not animated. Because the SVG language definition
// states that externalResourcesRequired cannot be animated, the animVal will always be the same as the baseVal.
// FIXME: When implementing animVal support, make sure that animVal==baseVal for externalResourcesRequired

I am not sure what we should do here. According to https://www.w3.org/TR/SVG/changes.html, SVGExternalResourcesRequired was removed from SVG2. Should we remove it? If we remove it, what is the backward compatibility story here?

We still need the SVGAnimatedBoolean because it is used by SVGFEConvolveMatrixElement according to https://drafts.fxtf.org/filter-effects/#element-attrdef-feconvolvematrix-preservealpha.

[Simon Fraser (smfr)]

Let's just keep existing behavior for now.

[Said Abou-Hallawa]

Created attachment 365092 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365092 [details]
Patch

Clearing flags on attachment: 365092

Committed r243121: <https://trac.webkit.org/changeset/243121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49001748>