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

[bugzilla-daemon at webkit.org]

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





--- Comment #49 from Dana Jansens <danakj at chromium.org>  2011-11-15 15:11:58 PST ---
(In reply to comment #48)
> (From update of attachment 114960 [details])
> 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.

It was meant to check only the content box. Thanks for noticing this discrepancy. I think this will be much more consistent and clear in the next patch by checking instead in isOpaqueInRect().

I'm moving the method to RenderBox (this is where your methods lived as well) so that it can determine the content box properly. And moving the top level virtual declarations down to RenderBoxModelObject since concepts like borderBoundingBox() don't exist above that. boxDecorationAreaIsOpaque only really makes sense on a RBMO, for example.

> Why not check for alpha in background images?

Judging from maskClipRect, this looks like it will be complicated, requiring at least a walk through the style's FillLayers, determining which are opaque, and then if their regions cover the query rect. I think this should come in another CL? Added a FIXME.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2682
> > +    const bool fullyContainQueryRect = bounds.contains(rect);
> 
> Why not early 'return false' here?

The function used to be a set of false early-outs, but I removed them to make false the default return instead. I can early out here again.

> > 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.

ok.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2694
> > +bool RenderBoxModelObject::boxDecorationsAreOpaqueInRect(const IntRect& rect) const
> 
> I think a better blame would be boxDecorationAreaIsOpaqueInRect().

ok.

> > 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.

ok.

> > 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.

RenderBox does, which is a better place for these methods anyhow.  RenderInline should consider each line independently, and can do so using Regions once that code lands.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2712
> > +    const bool contentsFullyContainQueryRect = contentsBounds.contains(rect);
> 
> Another opportunity for an early return?

sure.

> > 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).

ok.

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

ok.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2745
> > +
> > +
> 
> Two blank lines.

fixed.

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

ok.

> > 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.

Great, thanks for this pointer. Added this check.

> > 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.

Sounds good. Bug #72412.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:195
> > +    Vector<IntRect> opaqueRegions;
> 
> This is where you could use a Region, right?

Yes. Bug #72298 will do this.

> > 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.

ok.

> > LayoutTests/compositing/opaque/borders.html:1
> > +<!DOCTYPE html>
> 
> I think you need to test a lot more combinations of border, padding, image content etc.

Will add more, coming soon. Patch incoming with the other above changes in the meantime.

Thanks.

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