[webkit-reviews] review granted: [Bug 217429] Add support for non-accelerated animation of individual transform properties : [Attachment 410749] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 217429: Add support for non-accelerated animation of individual transform
properties
https://bugs.webkit.org/show_bug.cgi?id=217429

Attachment 410749: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 410749
  --> https://bugs.webkit.org/attachment.cgi?id=410749
Patch

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

r=me assuming EWS shows everything passing

> Source/WebCore/animation/CSSPropertyAnimation.cpp:138
> +static inline RefPtr<ScaleTransformOperation> blendFunc(const
CSSPropertyBlendingClient* client, ScaleTransformOperation* from,
ScaleTransformOperation* to, double progress)

Seems unnecessary to write inline here.

Seems to me that this function could be a built as a template so we don’t have
to repeat everything 3 times, but that might be a tricky project.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:166
> +    Ref<TransformOperation> blendedOperation = to->blend(from, progress);

auto

> Source/WebCore/animation/CSSPropertyAnimation.cpp:168
> +	   ScaleTransformOperation& scale =
downcast<ScaleTransformOperation>(blendedOperation.get());

auto&

> Source/WebCore/animation/CSSPropertyAnimation.cpp:202
> +    Ref<TransformOperation> blendedOperation = to->blend(from, progress);

auto

> Source/WebCore/animation/CSSPropertyAnimation.cpp:204
> +	   RotateTransformOperation& rotate =
downcast<RotateTransformOperation>(blendedOperation.get());

auto&

> Source/WebCore/animation/CSSPropertyAnimation.cpp:238
> +    Ref<TransformOperation> blendedOperation = to->blend(from, progress);

auto

> Source/WebCore/animation/CSSPropertyAnimation.cpp:240
> +	   TranslateTransformOperation& translate =
downcast<TranslateTransformOperation>(blendedOperation.get());

auto&

> Source/WebCore/animation/CSSPropertyAnimation.cpp:844
> +class PropertyWrapperScale : public
RefCountedPropertyWrapper<ScaleTransformOperation> {

final

Can this be a template so we don’t have to write it 3 times? Also, my comments
here apply to all 3.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:848
> +	   :
RefCountedPropertyWrapper<ScaleTransformOperation>(CSSPropertyScale,
&RenderStyle::scale, &RenderStyle::setScale)

I think we can omit the template argument here.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:852
> +    bool equals(const RenderStyle* a, const RenderStyle* b) const override

private and final

> Source/WebCore/animation/CSSPropertyAnimation.cpp:861
> +	   ScaleTransformOperation* scaleA = (a->*m_getter)();

auto

> Source/WebCore/animation/CSSPropertyAnimation.cpp:862
> +	   ScaleTransformOperation* scaleB = (b->*m_getter)();

auto

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1837
> +	   new PropertyWrapperScale(),

Parentheses not needed here.

> Source/WebCore/animation/KeyframeEffect.cpp:300
> +    CSSParserContext
cssParserContext(downcast<Document>(*scriptExecutionContext));

Given this function is about CSS parsing, I suggest naming this local just
“parserContext” or even just “context” because the script execution context is
not used except to set this up.

> Source/WebCore/animation/KeyframeEffect.cpp:303
> +    forEachInIterable(lexicalGlobalObject, keyframesInput.get(), method,
[&parsedKeyframes, cssParserContext](VM& vm, JSGlobalObject&
lexicalGlobalObject, JSValue nextValue) -> ExceptionOr<void> {

This copies the CSSParserContext. Maybe use & to pass it by reference?

> Source/WebCore/animation/KeyframeEffect.cpp:368
> +    CSSParserContext
cssParserContext(downcast<Document>(*scriptExecutionContext));

Same comment about the name.

> Source/WebCore/rendering/style/RenderStyle.cpp:695
> +    if (!arePointingToEqualData(first.scale, second.scale)

Other places in this patch we wrote out the arePointingToEqualData rule; maybe
use this function there too?