[webkit-reviews] review denied: [Bug 74719] TextureMapper: Simplify transform manipulations. : [Attachment 122255] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Jocelyn Turcotte
<jocelyn.turcotte at nokia.com>'s request for review:
Bug 74719: TextureMapper: Simplify transform manipulations.
https://bugs.webkit.org/show_bug.cgi?id=74719

Attachment 122255: Patch
https://bugs.webkit.org/attachment.cgi?id=122255&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122255&action=review


Good direction, needs some improvements.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:69
> +void TextureMapperNode::computeTransformsSelf(const TransformationMatrix&
parentTransform)

I don't see why we need to functions. We should just have one function, and use
(parent || effectTarget).

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:76
> +    m_transforms.target = parentTransform;
> +    m_transforms.target
>	   .translate3d(originX + m_state.pos.x(), originY + m_state.pos.y(),
m_state.anchorPoint.z() )
> -	   .multiply(m_transforms.base)
> -	   .translate3d(-originX, -originY, -m_state.anchorPoint.z());
> +	   .multiply(m_transforms.base);

You can do
m_transforms.target =
TransformationMatrix(parentTransform).multiply(m_transforms.base);
<nitpick/>

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:78
> +    // In case a parent had preserves3D and this layer has not, flatten our
children.

We should early-return if we don't have children, after we've translated the
target transform back.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:95
> +    // The replica layer isn't parented and needs its transform to be
stacked atop ours.
> +    if (m_state.replicaLayer)
> +	   m_state.replicaLayer->computeTransformsSelf(m_transforms.target);

Instead, you can call effectTarget from within the replica layer if there's no
parent.