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

[bugzilla-daemon at webkit.org]

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

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

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


This needs a changelog and some tests, but it's a good start.

> Source/WebCore/rendering/RenderBox.cpp:1160
> +    Color backgroundColor =
style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!backgroundColor.isValid() || backgroundColor.hasAlpha())
> +	   return false;

You could also check the background images here.

> Source/WebCore/rendering/RenderBox.h:598
> +    virtual bool backgroundIsOpaqueInRect(const LayoutPoint&, const
IntRect&) const OVERRIDE;

It's not clear why the first argument is needed here. What is it an offset
relative to? The rect should just be passed in some agreed-upon coordinate
system (e.g. relative to the border box).

> Source/WebCore/rendering/RenderBoxModelObject.h:89
> +    virtual bool isPaintedOpaqueInRect(const LayoutPoint& offset, const
IntRect& rect) const OVERRIDE { return foregroundIsOpaqueInRect(offset, rect)
|| backgroundIsOpaqueInRect(offset, rect); }

I don't like "isPaintedOpaque".

> Source/WebCore/rendering/RenderLayer.cpp:5108
> +static bool isListPaintedOpaqueInRect(const Vector<RenderLayer*>* list,
PaintBehavior paintBehavior, const RenderLayer* rootLayer, const IntRect& rect)


Very clumsy function name.

> Source/WebCore/rendering/RenderLayer.cpp:5142
> +    return renderer()->isPaintedOpaqueInRect(offset, rect) ||
> +	      isListPaintedOpaqueInRect(m_posZOrderList.get(), paintBehavior,
rootLayer, rect) ||
> +	      isListPaintedOpaqueInRect(m_negZOrderList.get(), paintBehavior,
rootLayer, rect) ||
> +	      isListPaintedOpaqueInRect(m_normalFlowList.get(), paintBehavior,
rootLayer, rect);

This is testing just the layer's immediate renderer, which is a reasonable
start but you should add a FIXME that it will miss many cases.

> Source/WebCore/rendering/RenderLayer.h:721
> +    // Input rect is given in rootLayer's coordinate space.

Why not just local to this layer?