[webkit-reviews] review denied: [Bug 27398] Handle opacity and opacity animations on transform layers in Leopard : [Attachment 33089] Replacement patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 27398: Handle opacity and opacity animations on transform layers in Leopard
https://bugs.webkit.org/show_bug.cgi?id=27398

Attachment 33089: Replacement patch
https://bugs.webkit.org/attachment.cgi?id=33089&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>

> +float GraphicsLayer::accumulatedOpacity() const
> +{
> +    if (!preserves3D())
> +	   return 1;
> +	   
> +    return m_opacity * (parent() ? parent()->accumulatedOpacity() : 1);
> +}
> +
> +void GraphicsLayer::distributeOpacity(float accumulatedOpacity)
> +{
> +    // If this is a transform layer we need to distribute our opacity to all
our children
> +    
> +    // Incoming accumulatedOpacity is the contribution from our parent(s).
We mutiply this by our own
> +    // opacity to get the total contribution
> +    accumulatedOpacity *= m_opacity;
> +    
> +    if (preserves3D()) {
> +	   size_t numChildren = children().size();
> +	   for (size_t i = 0; i < numChildren; ++i)
> +	       children()[i]->distributeOpacity(accumulatedOpacity);
> +    }
> +}



> +void GraphicsLayerCA::distributeOpacity(float accumulatedOpacity)
> +{
> +    GraphicsLayer::distributeOpacity(accumulatedOpacity);
> +    
> +    // If this is a transform layer we need set the opacity on our m_layer.
Otherwise 
> +    // we just set our own opacity.
> +    accumulatedOpacity *= m_opacity;
> +    
> +    if (preserves3D()) {
> +	   if (m_layer.get())
> +	       [m_layer.get() setOpacity: accumulatedOpacity];
> +    } else
> +	   [primaryLayer() setOpacity: accumulatedOpacity];
> +}

I don't like this duplication of logic between the base class and the derived
class. Logic should live in just one place.

You still have spaces after colons.

I'm also a little concerned about the fact that you're walking the
GraphicsLayer tree, rather than the RenderLayer tree to distribute opacity.
This only happens to work because we have other logic that makes compositing
layers on ancestors of layers with opacity < 1 (in
computeCompositingRequirements()). That logic may changes, so this code needs
to work when a composited layer has a software-rendered parent with opacity <
1.