[webkit-reviews] review denied: [Bug 236116] Accumulation support for transform properties : [Attachment 450858] wip

[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: Accumulation support for transform properties
https://bugs.webkit.org/show_bug.cgi?id=236116

Attachment 450858: wip

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




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

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

> Source/WebCore/ChangeLog:3
> +	   Accumulation support for transform properties

Needs the word "Animation" here somewhere.

> Source/WebCore/ChangeLog:13
> +	   A couple of bugs are also addressed. For a property with a different
transform operation
> +	   than the keyframes given, transformFunctionListsMatch() would return
true when it shouldn't. 

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

> Source/WebCore/platform/graphics/transforms/MatrixTransformOperation.cpp:42
> +	   to.blend(from, context.progress, context.compositeOperation);       


trailing whitespace

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:26
> +#include "CSSPropertyBlendingClient.h"

This is a layering violation. We're in platform code here, so we can't include
files outside of "platform"

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:29
> +#include "RenderBox.h"

Ditto

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1643
>      if (compositeOperation == CompositeOperation::Replace)
>	   from = from + (to - from) * progress;
> +    else if (compositeOperation == CompositeOperation::Accumulate)
> +	   from += from + (to - from - 1) * progress;

Should this be a switch()?

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

Not sure if I'm being dense, but how is 'compositeOperation ==
CompositeOperation::Accumulate ? CompositeOperation::Accumulate :
compositeOperation" different from "compositeOperation"?