[webkit-reviews] review denied: [Bug 70634] Mark GraphicsLayers as opaque when possible : [Attachment 112705] moved opaque rect detection into the compositing area computation - saves a RenderLayer tree walk and cleans up the code significantly

[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 112705: moved opaque rect detection into the compositing area
computation - saves a RenderLayer tree walk and cleans up the code
significantly
https://bugs.webkit.org/attachment.cgi?id=112705&action=review

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


Getting closer, but I'd like to see the 'isOpaque' code mirror the painting
code more closely.

> Source/WebCore/rendering/RenderBlock.cpp:2379
> +inline bool RenderBlock::shouldPaintColumnRules() const

Why doesn't this method check the rule color?

> Source/WebCore/rendering/RenderBlock.cpp:2381
>      bool ruleTransparent = style()->columnRuleIsTransparent();

You could early return here.

> Source/WebCore/rendering/RenderBlock.cpp:2385
>      bool renderRule = ruleStyle > BHIDDEN && !ruleTransparent && ruleWidth
<= colGap;

You could eliminate the local variable.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2562
> +    // If the query rect leaves the render object, then we can't vouch that
the entire rect is opaque.
> +    if (!bounds.contains(rect))
> +	   return false;

We can if the rect covers an opaque outline or shadow.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2570
> +    if (!boxDecorationsAreOpaqueInRect(rect))

Why test this here when RenderObject:: isOpaqueInRect() calls both
backgroundIsOpaqueInRect() and boxDecorationsAreOpaqueInRect()?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2594
> +    if (rect.x() >= bounds.x() + borderLeft() + paddingLeft()
> +	   && rect.maxX() <= bounds.maxX() - borderRight() - paddingRight()
> +	   && rect.y() >= bounds.y() + borderTop() + paddingTop()
> +	   && rect.maxY() <= bounds.maxY() - borderBottom() - paddingBottom())

Can't you construct the contentsRect and just use rect methods to test for
inclusion?

Also, border-image with 'fill' can cause a box decoration to paint inside the
contents box.

I'm confused why this method would return 'true' if the rect is entirely inside
the contents box. Perhaps it's confusing to split out box decorations into
their own method, because they almost always paint with a hole in the middle.
Maybe just merge this and the background-testing code.

In general, the 'is opaque' methods should mirror the painting methods as
closely as possible, otherwise it's going to be hard to keep them in sync.

> Source/WebCore/rendering/RenderLayerBacking.cpp:237
> +    // FIXME: Consider the union of all the rects instead of just a single
one at a time to prevent many false negatives.
> +    //	 In order to do this computation we will need to walk the
opaque regions at least once after determining the
> +    //	 full compositing bounds, and we will want to sort them to
compute this efficiently, so we are collecting them
> +    //	 in a vector for that reason.

We don't normally indent comments like this.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:529
> +	   axisAligned =
affineTrans->isAxisAligned(layer->renderer()->style()->hasPerspective());

This seems like overkill, and isAxisAligned() is nontrivial code. Why not just
punt on opaqueness testing if there's any transform?