[webkit-reviews] review denied: [Bug 70634] Mark GraphicsLayers as opaque when possible : [Attachment 112233] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 70634: Mark GraphicsLayers as opaque when possible
https://bugs.webkit.org/show_bug.cgi?id=70634

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112233&action=review


> Source/WebCore/rendering/RenderLayerBacking.cpp:285
> +    if (updateOpaque())
> +	   layerConfigChanged = true;

updateGraphicsLayerConfiguration() is about changes to the layer tree
structure, not just about setting flags on GraphicsLayers.

> Source/WebCore/rendering/RenderLayerBacking.cpp:614
> +bool RenderLayerBacking::updateOpaqueRenderLayer(RenderLayer* layer,
RenderLayer* ancestor, const IntPoint& ancestorDelta)

The method name needs improvement. It's not updating the render layer to be
opaque, it's computing whether the layer should be considered opaque.

> Source/WebCore/rendering/RenderLayerBacking.cpp:619
> +    bool seeThru =
(style->visitedDependentColor(CSSPropertyBackgroundColor).hasAlpha()

I don't like seeThru.

> Source/WebCore/rendering/RenderLayerBacking.cpp:620
> +		       && (!renderObject->isImage() ||
!toRenderImage(renderObject)->backgroundIsObscured()))

It would be clearer to separate 'contents are opaque' from 'backgrounds and
borders are opaque". It would also make sense to push questions about
RenderObject opaqueness down to RenderObject and subclasses. This could share a
lot of logic with the code that would be added for bug 49135.

> Source/WebCore/rendering/RenderLayerBacking.cpp:626
> +		      || style->borderLeftIsTransparent()
> +		      || style->borderRightIsTransparent()
> +		      || style->borderTopIsTransparent()
> +		      || style->borderBottomIsTransparent();

You should check to see if there is a border at all.

> Source/WebCore/rendering/RenderLayerBacking.cpp:647
> +    RenderLayer* child = layer->firstChild();
> +    while (child) {
> +	   if (updateOpaqueRenderLayer(child, layer, delta))
> +	       return true;
> +	   child = child->nextSibling();
> +    }

Sucks to do another layer tree walk here (and probably expensive too). If
needed, we could compute this same info during one of the existing tree walks.

> Source/WebCore/rendering/RenderLayerBacking.cpp:651
> +bool RenderLayerBacking::updateOpaque()

Needs better name.