Bug 217429 – Add support for non-accelerated animation of individual transform properties

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

Add support for non-accelerated animation of individual transform properties

[Radar WebKit Bug Importer]

<rdar://problem/70046645>

[Antoine Quint]

Created attachment 410749 [details]
Patch

[Antoine Quint]

<rdar://problem/70046645>

[Darin Adler]

Comment on attachment 410749 [details]
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?

[Antoine Quint]

(In reply to Darin Adler from comment #4)
> Comment on attachment 410749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410749&action=review
> 
> > Source/WebCore/animation/CSSPropertyAnimation.cpp:848
> > +        : RefCountedPropertyWrapper<ScaleTransformOperation>(CSSPropertyScale, &RenderStyle::scale, &RenderStyle::setScale)
> 
> I think we can omit the template argument here.

I applied all the suggested changes for the commit except this one, removing did not work for me.

I will attempt templating as a separate patch.

Thanks for the review!

[Antoine Quint]

Committed r268173: <https://trac.webkit.org/changeset/268173>