Bug 195862

Summary: Remove the SVG property tear off objects for SVGAnimatedBoolean
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, rniwa, simon.fraser, thorton, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=245844
Bug Depends on: 195859    
Bug Blocks: 191237, 195863    
Attachments:
Description Flags
Patch
none
Patch for review
none
Patch none

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.