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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dana Jansens
<danakj 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 114960: Patch
https://bugs.webkit.org/attachment.cgi?id=114960&action=review

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


> Source/WebCore/rendering/RenderBoxModelObject.cpp:2673
> +bool RenderBoxModelObject::backgroundContentsAreOpaqueInRect(const IntRect&
rect) const

'contents' in the name makes me wonder if this method checks the contentBox
only, which it does not. I think backgroundIsOpaqueInRect is better.

Why not check for alpha in background images?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2682
> +    const bool fullyContainQueryRect = bounds.contains(rect);

Why not early 'return false' here?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2689
> +    const Color bgColor =
style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    const bool bgColorIsOpaque = bgColor.isValid() && !bgColor.hasAlpha();

We don't normally use const on local variables.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2694
> +bool RenderBoxModelObject::boxDecorationsAreOpaqueInRect(const IntRect&
rect) const

I think a better blame would be boxDecorationAreaIsOpaqueInRect().

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2699
> +    // If the query rect leaves the render object, then we can't vouch that
the entire rect is opaque.
> +    const bool fullyContainQueryRect = bounds.contains(rect);

You should early return here to avoid all the extra work later in the method.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2711
> +    const LayoutRect contentsBounds(bounds.x() + borderLeft() +
paddingLeft(),
> +				       bounds.y() + borderTop() + paddingTop(),

> +				       bounds.width() - borderLeft() -
borderRight() - paddingLeft() - paddingRight(),
> +				       bounds.height() - borderTop() -
borderBottom() - paddingTop() - paddingBottom());

I'm surprised we don't have method that does this already.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2712
> +    const bool contentsFullyContainQueryRect =
contentsBounds.contains(rect);

Another opportunity for an early return?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2736
> +	   // If the above for loop completed, then all borders obscure the
background.
> +	   if (i > BSLeft)
> +	       bordersOpaque = true;

It would be clearer just to set a flag from within the loop (then you can make
i a variable with loop scope).

A future optimization could only look at the borders that intersect 'rect',
also. Add a FIXME comment?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2745
> +
> +

Two blank lines.

> Source/WebCore/rendering/RenderImage.cpp:418
> +    const bool fullyContainQueryRect = bounds.contains(rect);

Early return.

> Source/WebCore/rendering/RenderImage.cpp:436
> +    return fullyContainQueryRect && imageIsOpaque;

This method will do the wrong thing in cases where the image doesn't fill the
entire contentsBox. That can happen for SVG images with intrinsic aspect ratio,
and will happen in future when we implement object-fit. I think you should
compute the image destination rect like RenderImage::paintReplaced() does, and
check that against 'rect', and add a comment about object-fit.

> Source/WebCore/rendering/RenderImage.cpp:452
> +    return foregroundContentsAreOpaqueInRect(borderBoundingBox());

I think you should change the caller of backgroundIsObscured() to use your new
methods, maybe in a followup patch.

> Source/WebCore/rendering/RenderLayerBacking.cpp:195
> +    Vector<IntRect> opaqueRegions;

This is where you could use a Region, right?

> Source/WebCore/rendering/RenderObject.h:741
> +    // Returns true if the foreground within the query rect will be filled
completely opaque by the RenderObject.

"filled completely opaque"  is not correct English. "filled opaquely" would be
better.

> LayoutTests/compositing/opaque/borders.html:1
> +<!DOCTYPE html>

I think you need to test a lot more combinations of border, padding, image
content etc.