[webkit-reviews] review granted: [Bug 232086] [Web Animations] add support for animation-composition CSS property : [Attachment 449755] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 232086: [Web Animations] add support for animation-composition CSS property
https://bugs.webkit.org/show_bug.cgi?id=232086

Attachment 449755: Patch

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




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

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

Looks great, a few comments about improving the coding style a bit

> Source/WebCore/animation/CompositeOperation.h:28
> +#include "CSSPrimitiveValue.h"

Seems unfortunate to have to add so many headers to this header. Is it super
important that the function be inline?

> Source/WebCore/animation/CompositeOperation.h:36
> +inline std::optional<CompositeOperation>
compositeOperationFromCSSValue(const CSSValue& value)

I would omit the words “FromCSSValue” from this function name. We only need to
mention the argument types in the function name if it’s particularly surprising
or there is some kind of overloading ambiguity we are trying to work around.
Otherwise, it just makes our code unnecessarily wordy.

> Source/WebCore/animation/KeyframeEffect.cpp:611
> +    auto supportsCompositeOperation = is<Document>(scriptExecutionContext)
&&
downcast<Document>(*scriptExecutionContext).settings().webAnimationsCompositeOp
erationsEnabled();

Should we add a way to get to settings from any script execution context? It
seems unnecessary here that we are down casting to document.

Or maybe we should change the arguments that are passed to this by DOM
bindings? There is a CallWith=Document available.

> Source/WebCore/animation/KeyframeEffect.cpp:710
> +			   computedKeyframe.composite =
CompositeOperationOrAuto::Replace;

Seems like we need a function that converts CompositeOperation into
CompositeOperationOrAuto. No way we want to write this out as a switch
statement here. Even better there may be a way to make that an implicit
conversion but a named function would still be better than a switch statement
here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1381
> +static Ref<CSSPrimitiveValue>
valueForAnimationComposition(CompositeOperation compositeOperation)

Same comment about the function name including the words
“ForAnimationComposition”. A shorter name for this function would be better.
Also I suggest naming the argument a single word operation. In the context of
this function there aren’t different types of operation that we need to
disambiguate.