[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 #112705|review?                     |review-
               Flag|                            |




--- Comment #21 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-10-27 11:20:06 PST ---
(From update of attachment 112705)
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?

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