Bug 195722 – Remove the SVG property tear off objects for SVGAnimatedInteger

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The SVG properties tear-off objects have been troublesome and difficult area to maintain in WebKit. The patches in https://bugs.webkit.org/show_bug.cgi?id=191237 tried to remove all them and replace them in one giant patch. To make reviewing this work easier, I am going to do it incrementally. This patch will remove the SVG property tear off objects for SVGAnimatedInteger only. Once this patch lands, doing the same thing for the other types will be straightforward and reviewing the patches should be mechanical.

[Said Abou-Hallawa]

Created attachment 364603 [details]
Patch

[EWS Watchlist]

Attachment 364603 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGAnimationElement.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 364603 [details]
Patch

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

Looks good, but my suggestions:
1. Do the enum class change first.
2. Do a file rename to put "legacy" in existing files/classnames that will go away when you're done.
3. The rest of this patch.

> Source/WebCore/ChangeLog:11
> +        integer as Ref<SVGAnimatedInteger> in SVGElement. This will make the 
> +        representation of the property in IDL file matches the C++ header file.

...will make the representation ... match (not matches)

> Source/WebCore/ChangeLog:13
> +        When the DOM requesting the SVGAnimatedInteger, we get return a reference

requests ... we return

> Source/WebCore/ChangeLog:16
> +        baseVal() which will depend on whether the property is animating or not.

which will depend on -> depending on

> Source/WebCore/ChangeLog:50
> +        -- SVGNewProperty.h will replace SVGProperty.h 
> +
> +        -- SVGNewAnimatedProperty.h will replace SVGAnimatedProperty.h

I think the way to do this is to rename the old ones to SVGLegacyFoo first, so we never have files with "new" in their names in the tree.

> Source/WebCore/svg/SVGAnimateElementBase.cpp:31
> +#include "SVGNewAnimationController.h"

We don't have SVGAnimationController.h so why does this new "new" I the name?

> Source/WebCore/svg/SVGAnimateElementBase.cpp:46
> +SVGAnimationControllerBase& SVGAnimateElementBase::ensureController()

This should just be called animationController() (see the guidelines at https://webkit.org/code-style-guidelines/, search for "ifexists")

> Source/WebCore/svg/SVGAnimateElementBase.cpp:55
> +            m_controller = std::make_unique<SVGNewAnimationController>(*this, *targetElement());
> +        else
> +            m_controller = std::make_unique<SVGAnimationController>(*this, *targetElement());

Presumably everything will use SVGNewAnimationController eventually? More reasons to rename SVGAnimationController to SVGLegacyAnimationController first.

> Source/WebCore/svg/SVGAnimateElementBase.cpp:74
> +    return SVGAnimationControllerBase::determineAnimatedPropertyType(*this, targetElement, attributeName());

Is there a reason this calls determineAnimatedPropertyType on SVGAnimationControllerBase explicitly, rather than just SVGAnimationController?

> Source/WebCore/svg/SVGAnimateElementBase.cpp:135
> +    if (animationMode() == AnimationMode::By || animationMode() == AnimationMode::FromBy) {

You could do these num class changes in a different patch.

> Source/WebCore/svg/SVGAnimateMotionElement.h:40
> +    bool hasValidAttributeType() const override;
> +    bool hasValidAttributeName() const override;

These could be a separate patch, no?

> Source/WebCore/svg/SVGAnimationController.cpp:46
> +SVGAnimatedTypeAnimator* SVGAnimationController::ensureAnimator()

animatedTypeAnimator() or maybe just animator(). It should return a reference.

> Source/WebCore/svg/SVGAnimationController.cpp:161
> +        m_animatedProperties = animator->findAnimatedPropertiesForAttributeName(m_targetElement, attributeName);
> +        if (m_animatedProperties.isEmpty())
> +            return;
> +
> +        ASSERT(propertyTypesAreConsistent(m_animatedPropertyType, m_animatedProperties));
> +        if (!m_animatedType)
> +            m_animatedType = animator->startAnimValAnimation(m_animatedProperties);
> +        else {
> +            animator->resetAnimValToBaseVal(m_animatedProperties, *m_animatedType);
> +            animator->animValDidChange(m_animatedProperties);
> +        }

If you moved this into a separate function you wouldn't need the comment

> Source/WebCore/svg/SVGAnimationController.cpp:177
> +    ASSERT(m_animatedProperties.isEmpty());
> +    String baseValue;
> +
> +    if (shouldApply == SVGAnimationElement::ApplyCSSAnimation) {
> +        ASSERT(SVGAnimationElement::isTargetAttributeCSSProperty(&m_targetElement, attributeName));
> +        m_animationElement.computeCSSPropertyValue(&m_targetElement, cssPropertyID(attributeName.localName()), baseValue);
> +    }
> +
> +    if (!m_animatedType)
> +        m_animatedType = animator->constructFromString(baseValue);
> +    else
> +        m_animatedType->setValueAsString(attributeName, baseValue);

Ditto

> Source/WebCore/svg/SVGAnimationController.cpp:185
> +    SVGAnimationElement::ShouldApplyAnimation shouldApply =  m_animationElement.shouldApplyAnimation(&m_targetElement, attributeName);

two spaces after ==

> Source/WebCore/svg/SVGAnimationController.cpp:225
> +    // Be sure to detach list wrappers before we modfiy their underlying value. If we'd do
> +    // if after calculateAnimatedValue() ran the cached pointers in the list propery tear
> +    // offs would point nowhere, and we couldn't create copies of those values anymore,
> +    // while detaching. This is covered by assertions, moving this down would fire them.

Comment is hard to parse.

> Source/WebCore/svg/SVGAnimationController.cpp:234
> +static inline void applyCSSPropertyToTarget(SVGElement& targetElement, CSSPropertyID id, const String& value)

id -> propertyID

> Source/WebCore/svg/SVGAnimationController.cpp:244
> +static inline void removeCSSPropertyFromTarget(SVGElement& targetElement, CSSPropertyID id)

ditto. avoiding "id" in general is good because it's a reserved word in Objective-C

> Source/WebCore/svg/SVGAnimationController.cpp:257
> +    CSSPropertyID id = cssPropertyID(attributeName.localName());

here too

> Source/WebCore/svg/SVGAnimationController.cpp:259
> +    SVGElement::InstanceUpdateBlocker blocker(targetElement);

At some point we should rename InstanceUpdateBlocker to SomethingScope

> Source/WebCore/svg/SVGAnimationController.cpp:273
> +    CSSPropertyID id = cssPropertyID(attributeName.localName());

id

> Source/WebCore/svg/SVGAnimationController.cpp:321
> +    // We do update the style and the animation property independent of each other.

remove "do"

> Source/WebCore/svg/SVGAnimationController.h:38
> +class SVGAnimationController : public SVGAnimationControllerBase {

I think this might need a better name. We already have CSSAnimationController which handles ALL CSS animations for a document. This is handling animations for a single property on a single SVG element I think, so maybe it's SVGElementPropertyAnimationController or SVGElementPropertyAnimation or SVGElementPropertyAnimator?

> Source/WebCore/svg/SVGAnimationController.h:60
> +    AnimatedPropertyType m_animatedPropertyType;

can this be const?

> Source/WebCore/svg/SVGAnimationControllerBase.h:34
> +class SVGAnimationControllerBase {

Same comment about naming.

> Source/WebCore/svg/SVGAnimationElement.h:137
> +    enum class AttributeType { CSS, XML, Auto };

: uint8_t

> Source/WebCore/svg/SVGElement.cpp:731
> +void SVGElement::commitPropertyChange(SVGNewAnimatedProperty* animatedProperty)

Can animatedProperty be a reference?

> Source/WebCore/svg/SVGElement.h:221
> +    PropertyRegistry m_propertyRegistry { *this };

This pattern is a bit odd. I would expect to see these initialized in the constructor.

> Source/WebCore/svg/SVGFEConvolveMatrixElement.h:84
> +    Ref<SVGAnimatedInteger>& orderXAnimated() { return m_orderX; }

Why not just return SVGAnimatedInteger& ?

> Source/WebCore/svg/SVGNewAnimationController.cpp:42
> +RefPtr<SVGAnimator> SVGNewAnimationController::createAnimator() const

Should be called animator()

> Source/WebCore/svg/SVGNewAnimationController.cpp:45
> +        m_animator = m_targetElement.createAnimator(m_animationElement.attributeName(), m_animationElement.animationMode(), m_animationElement.calcMode(), m_animationElement.isAccumulated(), m_animationElement.isAdditive());

The fact that you ask m_animationElement for all the arguments suggests that m_animationElement should create this thing.

> Source/WebCore/svg/SVGNewAnimationController.cpp:47
> +    return m_animator;

Can m_targetElement.createAnimator() ever return null?

> Source/WebCore/svg/SVGNewAnimationController.cpp:70
> +    if (!createAnimator())
> +        return false;
> +
> +    m_animator->setFromAndToValues(&m_targetElement, from, to);

if (auto* animator = createAnimator()) { animator->set...; return true; } return false;

> Source/WebCore/svg/SVGNewAnimationController.cpp:77
> +    if (!createAnimator())
> +        return false;

I don't like this pattern where you rely on the side-effect of createAnimator(). Reads better if you use the return value from createAnimator().

> Source/WebCore/svg/SVGNewAnimationController.cpp:137
> +    if (!m_animator)
> +        return;
> +    m_animator->stop(targetElement);

This would be an animatorIfExists()

> Source/WebCore/svg/SVGNewAnimationController.h:41
> +    RefPtr<SVGAnimator> createAnimator() const;

Why not return SVGAnimator*?. But I really wonder if you should just call this makeAnimator() and have it return void, since all the use cases are internal to the class.

> Source/WebCore/svg/properties/SVGAccessor.h:37
> +class SVGAccessor {

This name is a bit generic. What is being accessed?

> Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h:137
> +    mutable Optional<PropertyType> m_animVal;

Can we use Marked here instead?

> Source/WebCore/svg/properties/SVGAnimator.h:40
> +class SVGAnimator : public RefCounted<SVGAnimator> {

Name is a bit generic. Is it a property animator?

> Source/WebCore/svg/properties/SVGNewAnimatedProperty.cpp:35
> +    // Casting from SVGElement to SVGPropertyOwner requires SVGElement.h.

Seem like that comment explains the #include above? Maybe remove it.

> Source/WebCore/svg/properties/SVGPropertyOwnerRegistry.h:56
> +    // Enumrate all the SVGAccessors recursively. The functor will be called and will

"Enumrate"

[Said Abou-Hallawa]

Comment on attachment 364603 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        representation of the property in IDL file matches the C++ header file.
> 
> ...will make the representation ... match (not matches)

Fixed

>> Source/WebCore/ChangeLog:13
>> +        When the DOM requesting the SVGAnimatedInteger, we get return a reference
> 
> requests ... we return

Fixed

>> Source/WebCore/ChangeLog:16
>> +        baseVal() which will depend on whether the property is animating or not.
> 
> which will depend on -> depending on

Fixed

>> Source/WebCore/ChangeLog:50
>> +        -- SVGNewAnimatedProperty.h will replace SVGAnimatedProperty.h
> 
> I think the way to do this is to rename the old ones to SVGLegacyFoo first, so we never have files with "new" in their names in the tree.

Done in <https://trac.webkit.org/changeset/242978>.

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:31
>> +#include "SVGNewAnimationController.h"
> 
> We don't have SVGAnimationController.h so why does this new "new" I the name?

Three classes will be added:
    SVGAnimationControllerBase
    SVGLegacyAnimationController
    SVGAnimationController

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:46
>> +SVGAnimationControllerBase& SVGAnimateElementBase::ensureController()
> 
> This should just be called animationController() (see the guidelines at https://webkit.org/code-style-guidelines/, search for "ifexists")

Fixed.

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:55
>> +            m_controller = std::make_unique<SVGAnimationController>(*this, *targetElement());
> 
> Presumably everything will use SVGNewAnimationController eventually? More reasons to rename SVGAnimationController to SVGLegacyAnimationController first.

Fixed. And I think eventually there will no need for any SVGAnimationController. All the code in SVGAnimationController will be moved to SVGAnimateElementBase. The functions in SVGAnimationController are just wrappers around the function in the SVGAnimator.

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:74
>> +    return SVGAnimationControllerBase::determineAnimatedPropertyType(*this, targetElement, attributeName());
> 
> Is there a reason this calls determineAnimatedPropertyType on SVGAnimationControllerBase explicitly, rather than just SVGAnimationController?

It is just convenient. The static method SVGAnimationControllerBase::determineAnimatedPropertyType() takes more argument than the one here. Also everything about the AnimatedPropertyType will be removed eventually.

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:135
>> +    if (animationMode() == AnimationMode::By || animationMode() == AnimationMode::FromBy) {
> 
> You could do these num class changes in a different patch.

Done in <https://trac.webkit.org/changeset/242970>.

>> Source/WebCore/svg/SVGAnimationController.cpp:46
>> +SVGAnimatedTypeAnimator* SVGAnimationController::ensureAnimator()
> 
> animatedTypeAnimator() or maybe just animator(). It should return a reference.

Fixed.

>> Source/WebCore/svg/SVGAnimationController.cpp:161
>> +        }
> 
> If you moved this into a separate function you wouldn't need the comment

I just moved this code from SVGAnimateElementBase::resetAnimatedType(). I do not want to change it since it is going to be removed eventually.

>> Source/WebCore/svg/SVGAnimationController.cpp:185
>> +    SVGAnimationElement::ShouldApplyAnimation shouldApply =  m_animationElement.shouldApplyAnimation(&m_targetElement, attributeName);
> 
> two spaces after ==

Fixed.

>> Source/WebCore/svg/SVGAnimationController.cpp:225
>> +    // while detaching. This is covered by assertions, moving this down would fire them.
> 
> Comment is hard to parse.

This is the legacy tear off object code. It will be removed eventually.

>> Source/WebCore/svg/SVGAnimationController.cpp:234
>> +static inline void applyCSSPropertyToTarget(SVGElement& targetElement, CSSPropertyID id, const String& value)
> 
> id -> propertyID

Ditto.

>> Source/WebCore/svg/SVGAnimationController.h:38
>> +class SVGAnimationController : public SVGAnimationControllerBase {
> 
> I think this might need a better name. We already have CSSAnimationController which handles ALL CSS animations for a document. This is handling animations for a single property on a single SVG element I think, so maybe it's SVGElementPropertyAnimationController or SVGElementPropertyAnimation or SVGElementPropertyAnimator?

I renamed it SVGAttributeAnimationController. I do not want to use SVGPropertyAnimationController because the SVGAnimateElementBase can animate animated property like SVGAnimatedLength 'x' of the <rect> element or none animated property like float 'opacity' of the <rect> element. Eventually these controllers will be deleted.

>> Source/WebCore/svg/SVGAnimationController.h:60
>> +    AnimatedPropertyType m_animatedPropertyType;
> 
> can this be const?

Done.

>> Source/WebCore/svg/SVGAnimationElement.h:137
>> +    enum class AttributeType { CSS, XML, Auto };
> 
> : uint8_t

Fixed.

>> Source/WebCore/svg/SVGElement.cpp:731
>> +void SVGElement::commitPropertyChange(SVGNewAnimatedProperty* animatedProperty)
> 
> Can animatedProperty be a reference?

Done.

>> Source/WebCore/svg/SVGElement.h:221
>> +    PropertyRegistry m_propertyRegistry { *this };
> 
> This pattern is a bit odd. I would expect to see these initialized in the constructor.

I think this pattern is okay if the initializer list includes *this" or constant values. The initialization of all the properties are done the same way. For example, the SVGFETurbulenceElement.h have the following definition. 

Ref<SVGAnimatedInteger> m_numOctaves { SVGAnimatedInteger::create(this, 1) };

This makes the constructor look simpler. If we add the initialization of all the members to the constructor we will have to add six lines SVGFETurbulenceElement.cpp in the just for the initialization.

>> Source/WebCore/svg/SVGFEConvolveMatrixElement.h:84
>> +    Ref<SVGAnimatedInteger>& orderXAnimated() { return m_orderX; }
> 
> Why not just return SVGAnimatedInteger& ?

Done.

>> Source/WebCore/svg/SVGNewAnimationController.cpp:42
>> +RefPtr<SVGAnimator> SVGNewAnimationController::createAnimator() const
> 
> Should be called animator()

Fixed.

>> Source/WebCore/svg/SVGNewAnimationController.cpp:45
>> +        m_animator = m_targetElement.createAnimator(m_animationElement.attributeName(), m_animationElement.animationMode(), m_animationElement.calcMode(), m_animationElement.isAccumulated(), m_animationElement.isAdditive());
> 
> The fact that you ask m_animationElement for all the arguments suggests that m_animationElement should create this thing.

The plan is to move all the code of this class to the SVGAnimateElementBase which is derived form SVGAnimationElement. So this function eventually will look like this:

RefPtr<SVGAnimator> SVGAnimateElementBase::createAnimator() const
{
    if (!m_animator)
        m_animator = targetElement()->createAnimator(attributeName(), animationMode(), calcMode(), isAccumulated(), isAdditive());
}

>> Source/WebCore/svg/SVGNewAnimationController.cpp:47
>> +    return m_animator;
> 
> Can m_targetElement.createAnimator() ever return null?

Yes if the attribute is not animatable.

>> Source/WebCore/svg/SVGNewAnimationController.cpp:70
>> +    m_animator->setFromAndToValues(&m_targetElement, from, to);
> 
> if (auto* animator = createAnimator()) { animator->set...; return true; } return false;

Fixed.

>> Source/WebCore/svg/SVGNewAnimationController.cpp:77
>> +        return false;
> 
> I don't like this pattern where you rely on the side-effect of createAnimator(). Reads better if you use the return value from createAnimator().

Fixed.

>> Source/WebCore/svg/SVGNewAnimationController.cpp:137
>> +    m_animator->stop(targetElement);
> 
> This would be an animatorIfExists()

Fixed.

>> Source/WebCore/svg/SVGNewAnimationController.h:41
>> +    RefPtr<SVGAnimator> createAnimator() const;
> 
> Why not return SVGAnimator*?. But I really wonder if you should just call this makeAnimator() and have it return void, since all the use cases are internal to the class.

I changed the type of m_animator to be std::unique_ptr instead of RefPtr. And I changed this function to return SVGAnimator*.

>> Source/WebCore/svg/properties/SVGAccessor.h:37
>> +class SVGAccessor {
> 
> This name is a bit generic. What is being accessed?

I named it SVGMemberAccessor. The plan is to have derived classes from it: SVGAnimatedPropertyAccessor and SVGPropertyAccessor. The first one will manage the animated properties the SVGAnimatedLength 'x' of the <rect> element. The second one will manage the non animated properties like SVGStringList 'requiredFeatures' of the SVGTests class.

>> Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h:137
>> +    mutable Optional<PropertyType> m_animVal;
> 
> Can we use Marked here instead?

I could not relate PropertyType to MarkableTraits. Maybe I am confused about the use of Markable.

>> Source/WebCore/svg/properties/SVGAnimator.h:40
>> +class SVGAnimator : public RefCounted<SVGAnimator> {
> 
> Name is a bit generic. Is it a property animator?

I remained it SVGAttributeAnimator. The plan is to have two types of animators SVGAnimatedPropertyAnimator and SVGPropertyAnimator. The first will animate properties like SVGAnimatedLength 'x' of the <rect> element. The second will animate properties like the float 'opacity' of the <rect> element.

>> Source/WebCore/svg/properties/SVGNewAnimatedProperty.cpp:35
>> +    // Casting from SVGElement to SVGPropertyOwner requires SVGElement.h.
> 
> Seem like that comment explains the #include above? Maybe remove it.

Fixed. I was explaining why I had to put this function in the source file instead of the header file. SVGElement.h can't be added to the header file because will add cyclic header file inclusion.

>> Source/WebCore/svg/properties/SVGPropertyOwnerRegistry.h:56
>> +    // Enumrate all the SVGAccessors recursively. The functor will be called and will
> 
> "Enumrate"

Fixed.

[Said Abou-Hallawa]

Created attachment 364870 [details]
Patch

[EWS Watchlist]

Attachment 364870 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGAnimationElement.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 364870 [details]
Patch

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

> Source/WebCore/svg/SVGAnimateElementBase.cpp:114
> +    if (!targetElement || !m_controller)

This would be a good use of animationControllerIfExists()

> Source/WebCore/svg/SVGAnimateElementBase.h:38
> +    SVGAttributeAnimationControllerBase& animationController();

Would prefer it be called attributeAnimationController

> Source/WebCore/svg/SVGFEConvolveMatrixElement.h:122
> +    Ref<SVGAnimatedInteger> m_orderX { SVGAnimatedInteger::create(this) };
> +    Ref<SVGAnimatedInteger> m_orderY { SVGAnimatedInteger::create(this) };

Feedback on webkit-dev was to only use this kind of initialization if the input data is known at compile time.

> Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h:115
> +    void stopAnimation() override

Add blank line above.

> Source/WebCore/svg/properties/SVGAttributeAnimator.h:38
> +enum class AnimationMode : uint8_t { None, FromTo, FromBy, To, By, Values, Path };
> +enum class CalcMode : uint8_t { Discrete, Linear, Paced, Spline };

I prefer enums like this to be one value per line.

> Source/WebCore/svg/properties/SVGMemberAccessorPtr.h:33
> +class SVGMemberAccessorPtr : public SVGMemberAccessor<OwnerType> {

Is this a member accessor for pointer types, or a pointer to a member accessor? If the former, maybe SVGPointerMemberAccessor, or SVGMemberPointerAccessor?

> Source/WebCore/svg/properties/SVGPropertyOwnerRegistry.h:122
> +    // This is a virtual function because SVGElement will access it through the base class.

Odd to see this comment here and not no the base class where it's declared virtual.

> Source/WebCore/svg/properties/SVGPropertyOwnerRegistry.h:135
> +    // Creates an SVGAttributeAnimator for a given attributeName.

Comment doesn't add anything.

[Said Abou-Hallawa]

Comment on attachment 364870 [details]
Patch

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

>> Source/WebCore/svg/SVGAnimateElementBase.cpp:114
>> +    if (!targetElement || !m_controller)
> 
> This would be a good use of animationControllerIfExists()

Fixed.

>> Source/WebCore/svg/SVGAnimateElementBase.h:38
>> +    SVGAttributeAnimationControllerBase& animationController();
> 
> Would prefer it be called attributeAnimationController

Done.

>> Source/WebCore/svg/SVGFEConvolveMatrixElement.h:122
>> +    Ref<SVGAnimatedInteger> m_orderY { SVGAnimatedInteger::create(this) };
> 
> Feedback on webkit-dev was to only use this kind of initialization if the input data is known at compile time.

*this* for SVGElements are almost known at compile time since all of them are created with the same state. Can we address this issue once the tear off objects are removed completely, please?

>> Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h:115
>> +    void stopAnimation() override
> 
> Add blank line above.

Fixed.

>> Source/WebCore/svg/properties/SVGAttributeAnimator.h:38
>> +enum class CalcMode : uint8_t { Discrete, Linear, Paced, Spline };
> 
> I prefer enums like this to be one value per line.

Fixed.

>> Source/WebCore/svg/properties/SVGMemberAccessorPtr.h:33
>> +class SVGMemberAccessorPtr : public SVGMemberAccessor<OwnerType> {
> 
> Is this a member accessor for pointer types, or a pointer to a member accessor? If the former, maybe SVGPointerMemberAccessor, or SVGMemberPointerAccessor?

Class is renamed to SVGPointerMemberAccessor.

>> Source/WebCore/svg/properties/SVGPropertyOwnerRegistry.h:122
>> +    // This is a virtual function because SVGElement will access it through the base class.
> 
> Odd to see this comment here and not no the base class where it's declared virtual.

Comment was removed.

>> Source/WebCore/svg/properties/SVGPropertyOwnerRegistry.h:135
>> +    // Creates an SVGAttributeAnimator for a given attributeName.
> 
> Comment doesn't add anything.

Comment was removed.

[Said Abou-Hallawa]

Created attachment 364897 [details]
Patch

[EWS Watchlist]

Attachment 364897 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGAnimationElement.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

> >> Source/WebCore/svg/SVGFEConvolveMatrixElement.h:122
> >> +    Ref<SVGAnimatedInteger> m_orderY { SVGAnimatedInteger::create(this) };
> > 
> > Feedback on webkit-dev was to only use this kind of initialization if the input data is known at compile time.
> 
> *this* for SVGElements are almost known at compile time since all of them
> are created with the same state. Can we address this issue once the tear off
> objects are removed completely, please?

Sure

[WebKit Commit Bot]

Comment on attachment 364897 [details]
Patch

Clearing flags on attachment: 364897

Committed r243036: <https://trac.webkit.org/changeset/243036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48950939>