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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 364603: Patch

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




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