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

Description Said Abou-Hallawa 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.
Comment 1 Said Abou-Hallawa 2019-03-19 09:52:37 PDT
Created attachment 365174 [details]
Patch
Comment 2 EWS Watchlist 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
Comment 3 EWS Watchlist 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
Comment 4 EWS Watchlist 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
Comment 5 EWS Watchlist 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
Comment 6 EWS Watchlist 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
Comment 7 EWS Watchlist 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
Comment 8 Said Abou-Hallawa 2019-03-19 11:56:05 PDT
Created attachment 365199 [details]
Patch
Comment 9 Said Abou-Hallawa 2019-03-19 12:38:00 PDT
Created attachment 365207 [details]
Patch for review
Comment 10 Simon Fraser (smfr) 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.
Comment 11 Said Abou-Hallawa 2019-03-19 16:53:31 PDT
Created attachment 365263 [details]
Patch
Comment 12 Said Abou-Hallawa 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.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2019-03-19 17:59:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-03-19 18:00:25 PDT
<rdar://problem/49044304>