[Webkit-unassigned] [Bug 70634] Mark GraphicsLayers as opaque when possible

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70634


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114960|review?                     |review-
               Flag|                            |




--- Comment #48 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-11-15 12:12:34 PST ---
(From update of attachment 114960)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.