[webkit-reviews] review denied: [Bug 27398] Handle opacity and opacity animations on transform layers in Leopard : [Attachment 32990] 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 32990: Patch
https://bugs.webkit.org/attachment.cgi?id=32990&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================

> +	   Second, I turn off all opacity animation.

Not always, I hope!

> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================

>  private:
> +    void setOpacityOnLayer();

I think it should be called "update" if it takes no argument, or "set" if it
takes one.

> Index: WebCore/platform/graphics/mac/GraphicsLayerCA.mm
> ===================================================================

> +float GraphicsLayerCA::accumulatedOpacity() const
> +{
> +    if (!preserves3D())
> +	   return 1;
> +	   
> +    return m_opacity * (parent() ? parent()->accumulatedOpacity() : 1);
> +}
> +
> +void GraphicsLayerCA::distributeOpacity(float opacity)

Argument could be "accumulatedOpacity" for clarity.

> +{
> +    // If this is a transform layer we need to distribute our opacity to all
our children
> +    // and set the opacity on our m_layer. Otherwise we just set our own
opacity.
> +    
> +    // Incoming opacity is the contribution from our parent(s). We mutiply
this by our own
> +    // opacity to get the value that is either passed down to the children
or set on our
> +    // own layer.
> +    opacity *= m_opacity;
> +    
> +    if (preserves3D()) {
> +	   if (m_layer.get())
> +	       [m_layer.get() setOpacity: opacity];
> +
> +	   size_t numChildren = children().size();
> +	   for (size_t i = 0; i < numChildren; ++i)
> +	       children()[i]->distributeOpacity(opacity);
> +    } else
> +	   [primaryLayer() setOpacity: opacity];
> +}

I think put the platform-neutral logic into base class methods, and then add a
small virtual function 
to wrap the [layer setOpacity:] call. BTW, webcore style is no space after
colons.