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

[bugzilla-daemon at webkit.org]

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


Dana Jansens <danakj at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #112705|1                           |0
        is obsolete|                            |




--- Comment #24 from Dana Jansens <danakj at chromium.org>  2011-10-27 13:51:01 PST ---
(From update of attachment 112705)
View in context: https://bugs.webkit.org/attachment.cgi?id=112705&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:2379
>> +inline bool RenderBlock::shouldPaintColumnRules() const
> 
> Why doesn't this method check the rule color?

This is testing if the rule is visible at all (i.e. alpha > 0).  It uses style()->columnRuleIsTransparent() to that effect, which checks the color's alpha value in BorderValue::isTransparent().

>> Source/WebCore/rendering/RenderBlock.cpp:2381
>>      bool ruleTransparent = style()->columnRuleIsTransparent();
> 
> You could early return here.

I turned the function into a set of "if (fail) { return }" to early return for each test.

>> Source/WebCore/rendering/RenderBlock.cpp:2385
>>      bool renderRule = ruleStyle > BHIDDEN && !ruleTransparent && ruleWidth <= colGap;
> 
> You could eliminate the local variable.

Done, all local variables removed even.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:2562
>> +        return false;
> 
> We can if the rect covers an opaque outline or shadow.

You're right, the comment says more than it should.  I moved the comment directly below up here instead, which says basically: Outside the border bounds is hard and we're not doing that right now.  I can add a FIXME if you would like, but I think it's something to be done in a future CL if it really is a common use case.

Also changed similar comment in boxDecorationsAreOpaque().

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:2570
>> +    if (!boxDecorationsAreOpaqueInRect(rect))
> 
> Why test this here when RenderObject:: isOpaqueInRect() calls both backgroundIsOpaqueInRect() and boxDecorationsAreOpaqueInRect()?

My thought was to make backgroundIsOpaqueInRect() function on its own, but it is called twice now.

I have removed the call here, and added a comment to the effect on backgroundIsOpaqueInRect(), similar to the foreground version.

isOpaqueInRect() becomes true if (box decor && (foreground || background)).

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:2594
>> +        && 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.

Fixed to use a rect.

Border-image seems complex, and ignoring it completely will possibly make for false negatives, but not positives, so I'd like to just leave it out for this CL.  Have put in a comment here and in backgroundIsOpaqueInRect() to mention its existence.

Consider if the background color is not opaque, however the borders are opaque, there is no padding, and the foreground contents area is opaque.  Then the render object is opaque.

For this reason, I split the code into these three functions, so that (foreground && box decor) || (background && box decor) would be possible.  Otherwise the foreground check's usefulness is degraded (You'd need 4+ queries on the background pieces around it as well). I will make this more clear in the comment here and on the function in RenderObject.h.  Am I understanding this correctly?

>> Source/WebCore/rendering/RenderLayerBacking.cpp:237
>> +    //        in a vector for that reason.
> 
> We don't normally indent comments like this.

Done.

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

Right angle rotations are an important use case here.  For example, with screen rotations, and losing opaqueness optimizations because of drawing on a 90 degree turn is a big loss.  While it looks complex at a glance, the code is checking 1 bool, and at most 12 ints for non-zero state.  isIdentityOrTransform() is checking 13.  I could write isAxisAligned() in a different manner if that would help, with more if statements and early returns.

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