RESOLVED FIXED 195862
Remove the SVG property tear off objects for SVGAnimatedBoolean 
https://bugs.webkit.org/show_bug.cgi?id=195862
Summary Remove the SVG property tear off objects for SVGAnimatedBoolean
Said Abou-Hallawa
Reported 2019-03-17 09:07:42 PDT
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.
Attachments
Patch (156.73 KB, patch)
2019-03-17 09:19 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch for review (46.57 KB, patch)
2019-03-17 09:26 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (38.90 KB, patch)
2019-03-18 16:59 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2019-03-17 09:19:47 PDT
Created attachment 364965 [details] Patch
Said Abou-Hallawa
Comment 2 2019-03-17 09:26:57 PDT
Created attachment 364966 [details] Patch for review
Simon Fraser (smfr)
Comment 3 2019-03-18 13:35:56 PDT
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 4 2019-03-18 16:12:20 PDT
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)
Comment 5 2019-03-18 16:26:23 PDT
Let's just keep existing behavior for now.
Said Abou-Hallawa
Comment 6 2019-03-18 16:59:50 PDT
Created attachment 365092 [details] Patch
WebKit Commit Bot
Comment 7 2019-03-18 17:39:47 PDT
Comment on attachment 365092 [details] Patch Clearing flags on attachment: 365092 Committed r243121: <https://trac.webkit.org/changeset/243121>
WebKit Commit Bot
Comment 8 2019-03-18 17:39:48 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2019-03-18 17:40:22 PDT
<rdar://problem/49001748>
Note You need to log in before you can comment on or make changes to this bug.