[webkit-reviews] review granted: [Bug 195722] Remove the SVG property tear off objects for SVGAnimatedInteger : [Attachment 364870] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 195722: Remove the SVG property tear off objects for SVGAnimatedInteger
https://bugs.webkit.org/show_bug.cgi?id=195722

Attachment 364870: Patch

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




--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 364870
  --> https://bugs.webkit.org/attachment.cgi?id=364870
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.