Bug 195949

Summary: Remove the SVG property tear off objects of SVGAnimatedRect
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, rniwa, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 195863    
Bug Blocks: 191237, 195960    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch
none
Patch for review
none
Patch none

Said Abou-Hallawa
Reported 2019-03-19 09:45:48 PDT
SVGRect is an SVG type, so we need to introduce two classes to represent it and its animated property SVGAnimatedRect. -- SVGValueProperty: It is derived from SVGProperty and it will be the base class of all the SVG type based properties. It is basically a RefCounted object with m_value member. For SVGRect, m_value will be of type FloatRect. -- SVGAnimatedValueProperty: It is derived from SVGAnimatedProperty and it will be the base class of all the animated SVG type based properties. It is RefCounted object with two RefCounted members. These two members are of type SVGValueProperty. For SVGAnimatedRect, baseVal and animVal will be of type SVGRect. SVGAnimatedValueProperty will be responsible of: 1) Provide access to its baseVal and animVal. Note the same interface is used internally and used by the DOM. 2) Managing the animation of the property by starting and stopping it. 3) Coordinating the changes in its baseVal and animVal with the owner element.
Attachments
Patch (50.34 KB, patch)
2019-03-19 09:52 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews102 for mac-highsierra (2.62 MB, application/zip)
2019-03-19 10:52 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.12 MB, application/zip)
2019-03-19 11:52 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews115 for mac-highsierra (2.29 MB, application/zip)
2019-03-19 11:54 PDT, EWS Watchlist 		no flags
Details
Patch (52.28 KB, patch)
2019-03-19 11:56 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch for review (70.21 KB, patch)
2019-03-19 12:38 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (54.42 KB, patch)
2019-03-19 16:53 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2019-03-19 09:52:37 PDT
Created attachment 365174 [details] Patch
EWS Watchlist
Comment 2 2019-03-19 10:52:47 PDT
Comment on attachment 365174 [details] Patch Attachment 365174 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11567502 New failing tests: svg/stroke/animated-non-scaling-stroke.html imported/mozilla/svg/as-image/background-resize-4.html
EWS Watchlist
Comment 3 2019-03-19 10:52:49 PDT
Created attachment 365185 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 4 2019-03-19 11:52:25 PDT
Comment on attachment 365174 [details] Patch Attachment 365174 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11568205 New failing tests: svg/stroke/animated-non-scaling-stroke.html imported/mozilla/svg/as-image/background-resize-4.html
EWS Watchlist
Comment 5 2019-03-19 11:52:27 PDT
Created attachment 365195 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 6 2019-03-19 11:54:02 PDT
Comment on attachment 365174 [details] Patch Attachment 365174 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11567948 New failing tests: svg/stroke/animated-non-scaling-stroke.html imported/mozilla/svg/as-image/background-resize-4.html
EWS Watchlist
Comment 7 2019-03-19 11:54:04 PDT
Created attachment 365197 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Said Abou-Hallawa
Comment 8 2019-03-19 11:56:05 PDT
Created attachment 365199 [details] Patch
Said Abou-Hallawa
Comment 9 2019-03-19 12:38:00 PDT
Created attachment 365207 [details] Patch for review
Simon Fraser (smfr)
Comment 10 2019-03-19 15:49:36 PDT
Comment on attachment 365199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365199&action=review > Source/WebCore/ChangeLog:22 > + SVGAnimatedValueProperty will be responsible of: responsible for > Source/WebCore/svg/properties/SVGAnimatedPropertyAnimatorImpl.h:84 > + return std::unique_ptr<SVGAnimatedRectAnimator>(new SVGAnimatedRectAnimator(attributeName, animated, animationMode, calcMode, isAccumulated, isAdditive)); std::make_unique. Seeing a 'new' in the code should ring alarm bells.
Said Abou-Hallawa
Comment 11 2019-03-19 16:53:31 PDT
Created attachment 365263 [details] Patch
Said Abou-Hallawa
Comment 12 2019-03-19 16:55:18 PDT
Comment on attachment 365199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365199&action=review >> Source/WebCore/ChangeLog:22 >> + SVGAnimatedValueProperty will be responsible of: > > responsible for Fixed. >> Source/WebCore/svg/properties/SVGAnimatedPropertyAnimatorImpl.h:84 >> + return std::unique_ptr<SVGAnimatedRectAnimator>(new SVGAnimatedRectAnimator(attributeName, animated, animationMode, calcMode, isAccumulated, isAdditive)); > > std::make_unique. Seeing a 'new' in the code should ring alarm bells. std::make_unique is now used. But I had to make the constructor of this class and its base class be public.
WebKit Commit Bot
Comment 13 2019-03-19 17:59:56 PDT
Comment on attachment 365263 [details] Patch Clearing flags on attachment: 365263 Committed r243183: <https://trac.webkit.org/changeset/243183>
WebKit Commit Bot
Comment 14 2019-03-19 17:59:58 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2019-03-19 18:00:25 PDT
<rdar://problem/49044304>
Note You need to log in before you can comment on or make changes to this bug.