[webkit-reviews] review denied: [Bug 236116] Web animations- Composite operation accumulation support for transform properties : [Attachment 451508] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 236116: Web animations- Composite operation accumulation support for
transform properties
https://bugs.webkit.org/show_bug.cgi?id=236116

Attachment 451508: Patch

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




--- Comment #18 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 451508
  --> https://bugs.webkit.org/attachment.cgi?id=451508
Patch

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

> Source/WebCore/ChangeLog:8
> +	   First thing being addressed is handling accumulation of
TransformationMatrices.

Try to avoid using abbreviated language; it gets in the way of communicating
the information.

If you need to, break the text into paragraphs.

Currently, this changelog reads like a laundry list of intermingled bug fixes
and feature work, and it's unclear to me if they should be, or can be split
out.

> Source/WebCore/ChangeLog:12
> +	   A couple of bugs are also addressed. For a property with a different
transform operation

Can these bugs be fixed independently of the composition change? If so, you
should do that.

> Source/WebCore/ChangeLog:16
> +	   Another bug is that we need to pass the composite operation to
TransformationMatrix blend().
> +	   Another bug is that we should to use Replace behavior if either of
the transform matrices are

Ditto.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:173
> +    auto size = is<RenderBox>(context.client->renderer()) ?
downcast<RenderBox>(*context.client->renderer()).borderBoxRect().size() :
LayoutSize();

size -> boxSize

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:77
> +TransformOperations TransformOperations::blendByMatchingOperations(const
TransformOperations& from, const BlendingContext& context, const LayoutSize&
size) const

size -> boxSize

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:83
> +    unsigned maxSize = std::max(fromSize, toSize);

This illustrates why renaming "size" to "boxSize" is necessary.

This code would be clearer if "fromSize" etc referred to "operationCount".

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:92
> +	   if (fromOperation && toOperation &&
!fromOperation->sharedPrimitiveType(toOperation.get()))
> +	       return blendByUsingMatrixInterpolation(from, context, size);

Doesn't this need to do something different based on Martin's changes?

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:130
> +	   return blendByMatchingOperations(from, context, size);

rename size to boxSize

> Source/WebCore/platform/graphics/transforms/TransformOperations.h:35
> +class CSSPropertyBlendingClient;

Unused

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1685
> +    blendFloat(fromDecomp.m12, toDecomp.m12, progress, compositeOperation ==
CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);
> +    blendFloat(fromDecomp.m21, toDecomp.m21, progress, compositeOperation ==
CompositeOperation::Accumulate ? CompositeOperation::Add : compositeOperation);

I'm still looking for a local variable with a name that tells me why m12, m21
etc get special treatment.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1718
> +    blendFloat(fromDecomp.skewXY, toDecomp.skewXY, progress,
compositeOperation == CompositeOperation::Accumulate ? CompositeOperation::Add
: compositeOperation);

Ditto