Bug 70634 – Mark GraphicsLayers as opaque when possible

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dana Jansens]

Set the opacity flag for GraphicsLayer.

[Dana Jansens]

Created attachment 112004 [details]
Patch

[Dana Jansens]

I tested this with a z-transformed div and the layer was indeed marked as opaque().

Not sure if this is the right place for the code, but it was the best place I was able to find.  All comments appreciated.

I do wonder how it would interact with

[Dana Jansens]

Sorry, cut off.

I do wonder how it would interact with https://bugs.webkit.org/show_bug.cgi?id=70564

[Adrienne Walker]

Comment on attachment 112004 [details]
Patch

It's possible for multiple layers to go into the same backing, so I think you'll need some sort of bounds check of the layer vs. the bounds of the backing to make sure that a layer with an opaque background color fills the whole space.

I'm not sure how this can be easily tested.  The best I can think of is exposing an isBackingOpaque(RenderObject*) function to windows.internal and then writing a layout test that with a bunch of different cases.

(Also, this is super exciting to see.  You're wanting this for culling, but I also want this sort of information to know whether we can turn on subpixel antialiasing on composited layers.  Although, I guess for that use case the border style checks can be ignored.)

[Dana Jansens]

(In reply to comment #4)
> (From update of attachment 112004 [details])
> It's possible for multiple layers to go into the same backing, so I think you'll need some sort of bounds check of the layer vs. the bounds of the backing to make sure that a layer with an opaque background color fills the whole space.

Why is this needed? The opaque flag is being set for a single /layer/, and I don't understand why that is incorrect when the layer doesn't fill a backing.  Unless you want to add an opaque flag to the backing? Just for testing?

[Adrienne Walker]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 112004 [details] [details])
> > It's possible for multiple layers to go into the same backing, so I think you'll need some sort of bounds check of the layer vs. the bounds of the backing to make sure that a layer with an opaque background color fills the whole space.
> 
> Why is this needed? The opaque flag is being set for a single /layer/, and I don't understand why that is incorrect when the layer doesn't fill a backing.  Unless you want to add an opaque flag to the backing? Just for testing?

RenderLayer has an n->1 relationship with RenderLayerBacking.  RenderLayerBacking has a 1-1 relationship with GraphicsLayer.

You're checking the style of a RenderObject for the RenderLayer that was composited and setting a flag on the GraphicsLayer for that RenderLayer's RenderLayerBacking.  Because there's an n->1 relationship along that path, I don't think what you're checking is sufficient.

The owning RenderLayer for the RenderLayerBacking could have RenderLayer descendants that go into the same backing.  Those descendants might not be opaque and might not be contained within the bounding box of the owning RenderLayer.  Maybe take a look at RenderLayerCompositor::calculateCompositedBounds to see what I'm trying to get at.

[Dana Jansens]

Created attachment 112233 [details]
Patch

[Dana Jansens]

Simple logic to make sure no false-positive opaque happens from RenderLayers that don't fill the GraphicsLayer.

There's a FIXME to improve this logic, but the test "do a set of rects fill this rect" is something we can also leverage to decide whether to paint a tile or not (https://bugs.webkit.org/show_bug.cgi?id=70533) so it is a future step to occur in a more general place.

I'll start to look at what some layout tests would look like.

[Simon Fraser (smfr)]

Comment on attachment 112233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112233&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:285
> +    if (updateOpaque())
> +        layerConfigChanged = true;

updateGraphicsLayerConfiguration() is about changes to the layer tree structure, not just about setting flags on GraphicsLayers.

> Source/WebCore/rendering/RenderLayerBacking.cpp:614
> +bool RenderLayerBacking::updateOpaqueRenderLayer(RenderLayer* layer, RenderLayer* ancestor, const IntPoint& ancestorDelta)

The method name needs improvement. It's not updating the render layer to be opaque, it's computing whether the layer should be considered opaque.

> Source/WebCore/rendering/RenderLayerBacking.cpp:619
> +    bool seeThru = (style->visitedDependentColor(CSSPropertyBackgroundColor).hasAlpha()

I don't like seeThru.

> Source/WebCore/rendering/RenderLayerBacking.cpp:620
> +                    && (!renderObject->isImage() || !toRenderImage(renderObject)->backgroundIsObscured()))

It would be clearer to separate 'contents are opaque' from 'backgrounds and borders are opaque". It would also make sense to push questions about RenderObject opaqueness down to RenderObject and subclasses. This could share a lot of logic with the code that would be added for bug 49135.

> Source/WebCore/rendering/RenderLayerBacking.cpp:626
> +                   || style->borderLeftIsTransparent()
> +                   || style->borderRightIsTransparent()
> +                   || style->borderTopIsTransparent()
> +                   || style->borderBottomIsTransparent();

You should check to see if there is a border at all.

> Source/WebCore/rendering/RenderLayerBacking.cpp:647
> +    RenderLayer* child = layer->firstChild();
> +    while (child) {
> +        if (updateOpaqueRenderLayer(child, layer, delta))
> +            return true;
> +        child = child->nextSibling();
> +    }

Sucks to do another layer tree walk here (and probably expensive too). If needed, we could compute this same info during one of the existing tree walks.

> Source/WebCore/rendering/RenderLayerBacking.cpp:651
> +bool RenderLayerBacking::updateOpaque()

Needs better name.

[Dana Jansens]

Created attachment 112350 [details]
Patch

[Dana Jansens]

Created attachment 112351 [details]
Patch

[Dana Jansens]

Updated patch to address feedback, except for the extra tree walk.  I hope I understood your critiques correctly, comments below.

(In reply to comment #9)
> (From update of attachment 112233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112233&action=review
> updateGraphicsLayerConfiguration() is about changes to the layer tree structure, not just about setting flags on GraphicsLayers.

I believe you meant to not set the layerConfigChanged flag, since the hierarchy didn't change.  If you meant to update the opaque flag in a different location entirely, can you suggest where?

> The method name needs improvement. It's not updating the render layer to be opaque, it's computing whether the layer should be considered opaque.

Renamed to findOpaqueRenderLayer. This will go away once I find another tree walk to merge it into.

> It would be clearer to separate 'contents are opaque' from 'backgrounds and borders are opaque". It would also make sense to push questions about RenderObject opaqueness down to RenderObject and subclasses. This could share a lot of logic with the code that would be added for bug 49135.

From a comment enne@ made, and bug 49135, I split this logic into three pieces:
1. contents foreground (image data etc)
2. contents background (background color)
3. contents exterior (borders, padding, shadow, etc.)

Currently:
- RenderBoxModelObject overrides background and exterior.
- RenderBlock extends exterior for column rule.
- RenderImage overrides foreground.

> You should check to see if there is a border at all.

See RenderBoxModelObject::opaqueContentsExterior, many improvements here.

> Sucks to do another layer tree walk here (and probably expensive too). If needed, we could compute this same info during one of the existing tree walks.

Will work on this piece next, but would appreciate feedback on the rest of the code as it is.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:651
> > +bool RenderLayerBacking::updateOpaque()
> 
> Needs better name.

Renamed to updateContentsOpaque(), since it sets the "contentsOpaque" flag on the graphics layer.  If you're thinking of something else, can you suggest what you're looking for?

[Simon Fraser (smfr)]

Comment on attachment 112351 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112351&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2569
> +static bool opaqueBorder(EBorderStyle bstyle)
> +{
> +    switch (bstyle) {
> +    case BNONE:
> +    case BHIDDEN:
> +    case INSET:
> +    case GROOVE:
> +    case OUTSET:
> +    case RIDGE:
> +    case SOLID:
> +        return true;
> +    case DOTTED:
> +    case DASHED:
> +    case DOUBLE:
> +        return false;
> +    }
> +    ASSERT_NOT_REACHED();
> +}

Seems like this would work as a method on BorderEdge nicely.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2589
> +    if (style()->hasBorder()
> +        && ((style()->borderLeftWidth() && !opaqueBorder(style()->borderLeftStyle()))
> +         || (style()->borderTopWidth() && !opaqueBorder(style()->borderTopStyle()))
> +         || (style()->borderRightWidth() && !opaqueBorder(style()->borderRightStyle()))
> +         || (style()->borderBottomWidth() && !opaqueBorder(style()->borderBottomStyle())))) {
> +        if (bgClip == BorderFillBox)
> +            bgVisible = true; // Can see the background through the border
> +        else
> +            return false; // Can see through the border, and it isn't filled by the background
> +    }

Would also be better to fit into the BorderEdge logic.

> Source/WebCore/rendering/RenderBoxModelObject.h:123
> +    virtual bool opaqueContentsBackground() const
> +    {
> +        const Color color = style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +        return color.isValid() && !color.hasAlpha();
> +    }

I think you should check for border-radius here.

> Source/WebCore/rendering/RenderObject.h:739
> +    virtual bool opaqueContentsForeground() const { return false; }
> +
> +    // Returns true if the background within the object's contents bounds is known to be completely opaque.
> +    virtual bool opaqueContentsBackground() const { return false; }

We try to use method names that read well; these do not. If you're adding these methods, I'd prefer you pass a rect as required for bug 49135, so they become:

virtual bool foregroundIsOpaqueInRect(const LayoutRect&) const
virtual bool backgroundIsOpaqueInRect(const LayoutRect&) const

> Source/WebCore/rendering/RenderObject.h:743
> +    virtual bool opaqueContentsExterior() const { return false; }

We use 'box decorations' to refer to border, outline, shadow etc. The method name doesn't really say what it does.

[Dana Jansens]

Created attachment 112424 [details]
Patch

[WebKit Review Bot]

Attachment 112424 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:358:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:359:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:360:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:361:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:366:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:367:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:368:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:369:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 9 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Created attachment 112427 [details]
Patch

[WebKit Review Bot]

Attachment 112427 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:358:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:359:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:360:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:361:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:366:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:367:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:368:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:369:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dana Jansens]

Created attachment 112588 [details]
added rects to opaque functions

[Dana Jansens]

Created attachment 112705 [details]
moved opaque rect detection into the compositing area computation - saves a RenderLayer tree walk and cleans up the code significantly

[Dana Jansens]

I believe I've addressed all review comments up to now in this version, so I'd appreciate another look at the code. Thanks!

[Simon Fraser (smfr)]

Comment on attachment 112705 [details]
moved opaque rect detection into the compositing area computation - saves a RenderLayer tree walk and cleans up the code significantly

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?

[Simon Fraser (smfr)]

Also, this needs a bucketload of tests.

[Dana Jansens]

Created attachment 112743 [details]
addressed feedback, and some discussion. tests will be incoming next.

[Dana Jansens]

Comment on attachment 112705 [details]
moved opaque rect detection into the compositing area computation - saves a RenderLayer tree walk and cleans up the code significantly

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.

[Dana Jansens]

Regarding testing:

I think the right way to test this is to add contentsOpaque to the output of GraphicsLayer::dumpProperties().  However this change alone which break and require rebaselining a *lot* of layout tests.

There are many ways this could go and I'd like to know what you all think is the best.

First I'll say my own thoughts:  Currently LayoutTestController.layoutTreeAsText(bool) takes an argument to show debug info.  If we extend this to allow setting various flags for layoutTreeAsText(), we can enable "contentsOpaque" in the DRT output only when we want it, and expose this to tests.

To do this:
- Remove boolean argument to layoutTreeAsText().
- Add setLayerTreeAsTextFlag(string, bool) to LayoutTestController.
  - Eventually this string is parsed to a value from the enum LayerTreeAsTextBehaviourFlags in GraphicsLayer.h

The big win here is that we can use DRT output to test this property nicely, without having to rebaseline any other tests.  If/when future properties are flipped on in GraphicsLayer or equivalent, this could pay off there also.

I will attach a patch to give an idea what this would look like.

[Dana Jansens]

Created attachment 112783 [details]
proposed method for testing contentsOpaque flag via DRT.

[Simon Fraser (smfr)]

(In reply to comment #25)
> Regarding testing:
> 
> I think the right way to test this is to add contentsOpaque to the output of GraphicsLayer::dumpProperties().  However this change alone which break and require rebaselining a *lot* of layout tests.

No, not if you only print it out when it has the non-default value.

> First I'll say my own thoughts:  Currently LayoutTestController.layoutTreeAsText(bool) takes an argument to show debug info.  If we extend this to allow setting various flags for layoutTreeAsText(), we can enable "contentsOpaque" in the DRT output only when we want it, and expose this to tests.
> 
> To do this:
> - Remove boolean argument to layoutTreeAsText().
> - Add setLayerTreeAsTextFlag(string, bool) to LayoutTestController.
>   - Eventually this string is parsed to a value from the enum LayerTreeAsTextBehaviourFlags in GraphicsLayer.h
> 
> The big win here is that we can use DRT output to test this property nicely, without having to rebaseline any other tests.  If/when future properties are flipped on in GraphicsLayer or equivalent, this could pay off there also.
> 
> I will attach a patch to give an idea what this would look like.

No need for any of this. Just print out contentsOpaque when it's true.

[Dana Jansens]

(In reply to comment #27)
> (In reply to comment #25)
> > Regarding testing:
> > 
> > I think the right way to test this is to add contentsOpaque to the output of GraphicsLayer::dumpProperties().  However this change alone which break and require rebaselining a *lot* of layout tests.
> 
> No, not if you only print it out when it has the non-default value.
...
> 
> No need for any of this. Just print out contentsOpaque when it's true.

The default value is non-opaque, since we want to assume things blend if we're not sure. But I imagine most tests actually actually use opaque layers, and the root layer should always be opaque. How come this won't make all these tests fail?

[Simon Fraser (smfr)]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > Regarding testing:
> > > 
> > > I think the right way to test this is to add contentsOpaque to the output of GraphicsLayer::dumpProperties().  However this change alone which break and require rebaselining a *lot* of layout tests.
> > 
> > No, not if you only print it out when it has the non-default value.
> ...
> > 
> > No need for any of this. Just print out contentsOpaque when it's true.
> 
> The default value is non-opaque, since we want to assume things blend if we're not sure. But I imagine most tests actually actually use opaque layers, and the root layer should always be opaque. How come this won't make all these tests fail?

I think we omit the root layer from the layer dump to avoid platform differences. You're right in that some existing tests may have opaque layers, but that's a progression we'd like to know about. You'll just have to land new results.

[Simon Fraser (smfr)]

Comment on attachment 112783 [details]
proposed method for testing contentsOpaque flag via DRT.

Just print out m_contentsOpaque if it has the non-default value.

[Adrienne Walker]

(In reply to comment #25)
> Regarding testing:
> 
> I think the right way to test this is to add contentsOpaque to the output of GraphicsLayer::dumpProperties().  However this change alone which break and require rebaselining a *lot* of layout tests.

Maybe everybody disagrees with my position on this, but I personally don't think this is the right way to go.  One of the big problems with layout tests in my opinion is that they over-test.  Over-testing leads to needless rebaselining and fragile tests, which makes the signal that they provide ultimately less useful.

In some theoretical world, the ideal approach for this bug would be to write a few tests that specifically test the opacity flag.  Modifying the testing structure so that every test starts testing the opacity flag in addition to what it's already testing seems like a backwards design.

Adding a function to specifically test opacity in window.internals is what I suggested earlier.  Adding a filter to layoutTreeAsText could also be an approach that could limit the impact to other tests.

[Dana Jansens]

Created attachment 113198 [details]
consider all RenderObjects in a RenderLayer

[Dana Jansens]

There are a few LayoutTests included now, which showed some bugs/omissions in the previous code.  Wanted to verify that this is still heading in the right direction.  The code now traverses the RenderObjects associated with a RenderLayer to find all the opaque regions, instead of just 1 per RenderLayer.

More tests will be coming, but this is a start.

[Simon Fraser (smfr)]

Comment on attachment 113198 [details]
consider all RenderObjects in a RenderLayer

View in context: https://bugs.webkit.org/attachment.cgi?id=113198&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2678
> +    // We do not consider overflow bounds here, and if we get a query rect on the overflow area for the
> +    // render object (i.e. outside the border bounds), then we just return false. This does not mean the area
> +    // in the rect is neccessarily non-opaque, and may be a false negative.
> +    // NOTE: Because of this, we don't need to look for style()->boxShadow(), since that is always outside of the border
> +    // bounds.
> +    if (!bounds.contains(rect))
> +        return false;
> +
> +    // FIXME: Check border-image. With 'fill' it can be drawn behind the contents area and could be opaque.
> +
> +    const Color color = style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!color.isValid() || color.hasAlpha())
> +        return false;

I think these new methods need to be written conservatively; they should err towards returning 'false' unless known to be opaque. Then the FIXME becomes harmless.

> Source/WebCore/rendering/RenderLayerBacking.cpp:244
> +    bool opaque = false;
> +    for (size_t i = 0; i < opaqueRegions.size(); ++i)
> +        if (opaqueRegions[i].contains(layerBounds)) {
> +            opaque = true;
> +            break;
> +        }
> +    m_graphicsLayer->setContentsOpaque(opaque);

Rather than collect rects like this, why not just have calculateCompositedBounds() have an out param that returns whether the layers are known to be fully opaque in the given bounds? Then you could avoid extra work as soon as you find out that it is not.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:452
> +void recursiveCollectOpaqueRenderObjects(const RenderLayer* layer, const RenderObject* renderer, const RenderBoxModelObject* container, Vector<LayoutRect>& opaqueRegions, LayoutPoint totalRelOffset)

We prefer static methods to using anonymous namespaces.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:455
> +        const RenderBoxModelObject* boxmodel = static_cast<const RenderBoxModelObject*>(renderer);

Don't like boxmodel as a variable name. It's a boxModelObject.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:458
> +        if (boxmodel->layer() && boxmodel->layer() != layer)
> +            return; // We've left the render layer.

This is wrong. RenderBoxModelObject::layer() only returns a RL if the RO has one; it never returns the layer for some ancestor RO.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:468
> +        if (boxmodel->isOpaqueInRect(area)) {
> +            area.move(containerOffset);
> +            opaqueRegions.append(area);
> +        }

The approach taken here is doing an exhaustive walk of all the RO even when one is found to be non-opaque. This could get very expensive. You need to be able to bail early if you find non-opaque content. Ideally we'd do this testing at the same time as some other tree-walk (maybe you cache it during painting).

> Source/WebCore/rendering/RenderLayerCompositor.cpp:472
> +        // Containers are not always parents, so in general it is not okay to just assume we are the container for our
> +        // child because we are its parent. However, if we are not its container (for e.g. it is positioned absolute and has
> +        // a different container), then it will also be composited independently, and have its own RenderLayer/GraphicsLayer.

The comment (and maybe the code) is confused. Not every RL has a GL. A GL can render a subtree of RLs.

> Source/WebCore/rendering/RenderObject.h:749
> +    // Returns true if the foreground within the query rect will be filled completely opaque by the RenderObject.
> +    // This checks only within the contents area of the RenderObject, so if you want to test the entire surface
> +    // area, you need to test boxDecorationsAreOpaqueInRect() as well.
> +    virtual bool foregroundIsOpaqueInRect(const IntRect&) const { return false; }
> +
> +    // Returns true if the background within the query rect will be filled completely opaque by the RenderObject.
> +    // This checks only within the contents area of the RenderObject, so if you want to test the entire surface
> +    // area, you need to test boxDecorationsAreOpaqueInRect() as well.
> +    virtual bool backgroundIsOpaqueInRect(const IntRect&) const { return false; }
> +
> +    // Returns true if the background within the query rect would be filled completely opaque (assuming the content area was opaque).
> +    // This returns true if the query rect is entirely in the contents area of the render object, as the opaque result depends
> +    // only on the foregroundIsOpaqueInRect() or backgroundIsOpaqueInRect() result. A true result from this function should
> +    // *never* be used without also consulting at least one of the previously mentioned functions as well.
> +    virtual bool boxDecorationsAreOpaqueInRect(const IntRect&) const { return false; }

The verbosity of the comments indicates that there's something wrong with the code factoring and/or the method names.

> LayoutTests/compositing/opaque/borders.html:67
> +      layoutTestController.waitUntilDone();
> +    }
> +
> +    function doTest()
> +    {
> +      if (window.layoutTestController) {
> +        document.getElementById('layers').innerText = layoutTestController.layerTreeAsText();
> +        layoutTestController.notifyDone();
> +      }
> +    }
> +    
> +    window.addEventListener('load', doTest, false);

You don't need waitUntilDone if your test finishes inside the load event.

[Dana Jansens]

Comment on attachment 113198 [details]
consider all RenderObjects in a RenderLayer

View in context: https://bugs.webkit.org/attachment.cgi?id=113198&action=review

Thanks for the comments. The process for determining if a GraphicsLayer is opaque is still very simplified in this patch (Does some RenderObject fully occlude the GraphicsLayer?) but this is far from sufficient.  The goal is to determine if the union of opaque ROs fully occlude the GL. More comments inline.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:2678
>> +        return false;
> 
> I think these new methods need to be written conservatively; they should err towards returning 'false' unless known to be opaque. Then the FIXME becomes harmless.

I do agree, and I thought that was what I had done.  Currently this function returns true only if the query rect is entirely over the background color, and the background color is opaque.

It would be possible to do this with early exit returning true instead (though I think there would have to be only a single early exit then, since it is basically an AND of all the conditions to get to the true statement.

if (contains && colorvalid && !colorhasalpha)
    return true;
return false;

Would that be preferable to you?  Or I could include a comment indicating this intent also.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:244
>> +    m_graphicsLayer->setContentsOpaque(opaque);
> 
> Rather than collect rects like this, why not just have calculateCompositedBounds() have an out param that returns whether the layers are known to be fully opaque in the given bounds? Then you could avoid extra work as soon as you find out that it is not.

The reason we collect rects here rather than computing as soon as we find a new rect, is that in order to determine if the union of the rects cover the GraphicsLayer in some reasonable (polynomial) amount of time, we will need to do something like sorting the rects.  For this reason I collect them into a Vector which can be easily sorted and then tested for covering.

It doesn't look needed right now, since the test is simply checking for a single RO that covers the GraphicsLayer, but that's not the final intent, as that misses a lot (A composited div with two divs in it, etc).

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:452
>> +void recursiveCollectOpaqueRenderObjects(const RenderLayer* layer, const RenderObject* renderer, const RenderBoxModelObject* container, Vector<LayoutRect>& opaqueRegions, LayoutPoint totalRelOffset)
> 
> We prefer static methods to using anonymous namespaces.

Fixing.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:455
>> +        const RenderBoxModelObject* boxmodel = static_cast<const RenderBoxModelObject*>(renderer);
> 
> Don't like boxmodel as a variable name. It's a boxModelObject.

Fixing.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:458
>> +            return; // We've left the render layer.
> 
> This is wrong. RenderBoxModelObject::layer() only returns a RL if the RO has one; it never returns the layer for some ancestor RO.

Can it return a decendant's RenderLayer?  The intent here was to prevent looking at any RO that are inside another RL (since we are walking the RLs already in calculateCompositedBounds.  If RL->renderer() points to a subtree of RO that all belong to the single RL, then we can remove this check. Otherwise what should we do instead to  not look at a RO twice.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:468
>> +        }
> 
> The approach taken here is doing an exhaustive walk of all the RO even when one is found to be non-opaque. This could get very expensive. You need to be able to bail early if you find non-opaque content. Ideally we'd do this testing at the same time as some other tree-walk (maybe you cache it during painting).

Yes, because the intent is to determine if the union of opaque ROs cover the entire GraphicsLayer. To do so, we need to find them all since they can be in arbitrary positions within the GraphicsLayer, unless there is some underlying structure here that I can take advantage of and am not right now?  As I understood, a non-opaque RO could still be above/below some (set of) opaque RO belonging to the current or other RenderLayers in the GraphicsLayer.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:472
>> +        // a different container), then it will also be composited independently, and have its own RenderLayer/GraphicsLayer.
> 
> The comment (and maybe the code) is confused. Not every RL has a GL. A GL can render a subtree of RLs.

The comment meant to say that such a RO will be composited and thus be in a different GraphicsLayer, which implies a different RenderLayer. I will fix that.

>> Source/WebCore/rendering/RenderObject.h:749
>> +    virtual bool boxDecorationsAreOpaqueInRect(const IntRect&) const { return false; }
> 
> The verbosity of the comments indicates that there's something wrong with the code factoring and/or the method names.

Will improve for clarity by refering to the content rect in the names, and shorten the comments.

foregroundContentsAreOpaqueInRect()
backgroundContentsAreIsOpaqueInRect()

>> LayoutTests/compositing/opaque/borders.html:67
>> +    window.addEventListener('load', doTest, false);
> 
> You don't need waitUntilDone if your test finishes inside the load event.

Thanks.

[Dana Jansens]

Created attachment 113673 [details]
consider the union of opaque rects when deciding if a GraphicsLayer is opaque + nits fixes

[Dana Jansens]

The last patch addresses the smaller things you've brought up, hopefully sufficiently, as well as considers the full union of opaque rects when deciding if the opaque flag should be set or not.

Using the painting tree walk would be problematic, I believe, since properties change between paints, and we would like to know the opaque state during the painting process to limit painting as well as blitting to the screen.

However, the layout tree walk was suggested as a place for this computation.  I will look into using the RenderObject walk there.

[Dana Jansens]

Created attachment 114066 [details]
move the RenderObject walk

[Dana Jansens]

This version moves the walk of RenderObjects to the RenderObject::layout() code paths.

I'm not sure if this will be an overall win when compared to the previous version, but will leave it to you to judge. This buys us more lines of code (37 more), and spreads the opaque computation out to 3 classes:
- RenderLayerCompositor walks RenderLayers, collects their opaque regions, and determines if they cover the entire backing.
- RenderLayer holds a set of opaque regions given to it by RenderObjects.
- RenderObject computes an opaque region for itself after layout() and gives it to its enclosingLayer().

When a RenderObject saves its opaque rectangle, it needs to save an offset relative to its enclosing RenderLayer.  The old code did this by keeping an aggregate offset and thus required no extra computation to determine the offset. We don't have the same information here in this version, so after a RenderObject completes its layout(), we have to walk up to the enclosing RenderLayer to determine its offset (also to determine its enclosing RenderLayer).

I see code that walks up to container() used a fair bit in the layout code, so the above walk up to enclosingLayer() may not be a big deal in this part of the code (compared to RenderLayerCompositor::calculateCompositedBounds(), but I wanted to point it out.

It would be possible to pass data along to subsequent layout() calls when recursing down the tree, saving some walking (only walk to nearest container() not to enclosingLayer()), but I did not do that since the layout() function is sprinkled around something like 15 classes in the RenderObject hierarchy and it seemed a poor idea to make that sort of change.

[Early Warning System Bot]

Comment on attachment 114066 [details]
move the RenderObject walk

Attachment 114066 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10380037

[Gyuyoung Kim]

Comment on attachment 114066 [details]
move the RenderObject walk

Attachment 114066 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10365106

[WebKit Review Bot]

Comment on attachment 114066 [details]
move the RenderObject walk

Attachment 114066 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10376247

New failing tests:
fast/events/scale-and-scroll-iframe-window.html
platform/gtk/scrollbars/overflow-scrollbar-horizontal-wheel-scroll.html
scrollbars/disabled-scrollbar.html
scrollbars/scrollbar-buttons.html
media/media-blocked-by-willsendrequest.html
scrollbars/listbox-scrollbar-combinations.html
fast/events/scale-and-scroll-window.html
scrollbars/hidden-iframe-scrollbar-crash2.html
fast/frames/iframe-double-scale-contents.html
scrollbars/overflow-custom-scrollbar-crash.html
scrollbars/hidden-iframe-scrollbar-crash.html
fast/events/scale-and-scroll-body.html
fast/events/scale-and-scroll-iframe-body.html
scrollbars/basic-scrollbar.html
scrollbars/overflow-scrollbar-combinations.html

[Simon Fraser (smfr)]

Comment on attachment 114066 [details]
move the RenderObject walk

View in context: https://bugs.webkit.org/attachment.cgi?id=114066&action=review

I don't like that this patch is getting more and more complex. I think you should start by getting the easy cases working first (e.g. the layer's RenderObject is known to be opaque), and then progress to doing RenderObject walks in later patches. The IntRectEdge code is a large amount of new code. It should be tested on all platforms, not just Chromium, but I'd prefer that it just not be included in the initial patch.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2693
> +    const LayoutRect& bounds = borderBoundingBox();
> +
> +    // We do not consider overflow bounds here, and if we get a query rect on the overflow area for the
> +    // render object (i.e. outside the border bounds), then we just return false. This does not mean the area
> +    // in the rect is neccessarily non-opaque, and may be a false negative.
> +    // NOTE: Because of this, we don't need to look for style()->boxShadow(), since that is always outside of the border
> +    // bounds.
> +    if (!bounds.contains(rect))
> +        return false;
> +
> +    // FIXME: Check border-image. With 'fill' it can be drawn behind the contents area and could be opaque.
> +    // Currently we check background color only, but border-image could make the rect opaque even though the
> +    // background color is not.
> +
> +    const Color color = style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!color.isValid() || color.hasAlpha())
> +        return false;
> +
> +    return true;

This is contrary to the direction I expressed a preference for. I think the methods should default to returning false, and return true only in cases they know they can give the correct answer for.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2740
> +    return true;

Ditto.

> Source/WebCore/rendering/RenderImage.cpp:432
> +    return true;

Ditto.

> Source/WebCore/rendering/RenderLayer.h:794
> +    // This is a mapping from RenderObject* to IntRect, which is the opaque rect for each RenderObject
> +    // belonging to this RenderLayer.
> +    HashMap<const RenderObject*, IntRect> m_opaqueRectForRenderObject;
> +
> +    // This list contains bounding box rectangles of opaque RenderObjects in the layer.
> +    OwnPtr<Vector<IntRectEdge> > m_opaqueRectsList;

It seems wrong to be adding these members to RenderLayer, when there's no contract about who updates them. A caller has no way to know if opaqueRects() will be return a stale list. There's just an implicit contract that RenderLayerBacking will update the rects at the right time.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:474
> +    LayoutPoint ancestorRelOffset;
> +    layer->convertToLayerCoords(ancestorLayer, ancestorRelOffset);

Have you tested composited layers that contain 2d-transformed layers? It seems like this convertToLayerCoords() could cross a transform boundary, which is bad.

> Source/WebCore/rendering/RenderObject.cpp:2703
> +void RenderObject::updateOpaqueRect()

I don't know why this has to be on RenderObject, since it's a layer thing.

> Source/WebCore/rendering/RenderObject.h:1004
> +        updateOpaqueRect();

This is going to be way too expensive. This is a hot function (hence the inline).

[Dana Jansens]

(In reply to comment #43)
> (From update of attachment 114066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114066&action=review
> 
> I don't like that this patch is getting more and more complex. I think you should start by getting the easy cases working first (e.g. the layer's RenderObject is known to be opaque), and then progress to doing RenderObject walks in later patches. The IntRectEdge code is a large amount of new code. It should be tested on all platforms, not just Chromium, but I'd prefer that it just not be included in the initial patch.

I was starting to feel the same way, and I'll be happy to do so. I feel I should clarify what the patch will lose though in the process.

The IntRectEdge code was used to determine if the RenderLayers in a backing together would make it opaque.  Removing that will revert the check to this back to "is there a single opaque RenderLayer that fills the backing?"

I think the inconsistency I wanted to pointed here is that the IntRectEdge code has less to do with walking the RenderObject tree, and more to do with considering all the layers in a backing together (eventually all the RenderObjects in a backing).

Regarding testing: The IntRectEdge code will be tested by LayoutTests across all platforms. But I wanted a unit test also for the unionOfRectsCoverARect logic, especially while I was writing it. I would write unit tests for all platforms if such a general mechanism existed, but to my knowledge it does not?

> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2693
> > +    const LayoutRect& bounds = borderBoundingBox();
> > +
> > +    // We do not consider overflow bounds here, and if we get a query rect on the overflow area for the
> > +    // render object (i.e. outside the border bounds), then we just return false. This does not mean the area
> > +    // in the rect is neccessarily non-opaque, and may be a false negative.
> > +    // NOTE: Because of this, we don't need to look for style()->boxShadow(), since that is always outside of the border
> > +    // bounds.
> > +    if (!bounds.contains(rect))
> > +        return false;
> > +
> > +    // FIXME: Check border-image. With 'fill' it can be drawn behind the contents area and could be opaque.
> > +    // Currently we check background color only, but border-image could make the rect opaque even though the
> > +    // background color is not.
> > +
> > +    const Color color = style()->visitedDependentColor(CSSPropertyBackgroundColor);
> > +    if (!color.isValid() || color.hasAlpha())
> > +        return false;
> > +
> > +    return true;
> 
> This is contrary to the direction I expressed a preference for. I think the methods should default to returning false, and return true only in cases they know they can give the correct answer for.

Understood, I will change how these are written.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2740
> > +    return true;
> 
> Ditto.
> 
> > Source/WebCore/rendering/RenderImage.cpp:432
> > +    return true;
> 
> Ditto.
> 
> > Source/WebCore/rendering/RenderLayer.h:794
> > +    // This is a mapping from RenderObject* to IntRect, which is the opaque rect for each RenderObject
> > +    // belonging to this RenderLayer.
> > +    HashMap<const RenderObject*, IntRect> m_opaqueRectForRenderObject;
> > +
> > +    // This list contains bounding box rectangles of opaque RenderObjects in the layer.
> > +    OwnPtr<Vector<IntRectEdge> > m_opaqueRectsList;
> 
> It seems wrong to be adding these members to RenderLayer, when there's no contract about who updates them. A caller has no way to know if opaqueRects() will be return a stale list. There's just an implicit contract that RenderLayerBacking will update the rects at the right time.

- The set of opaque rects is contained in the RenderLayer, since we already walk RenderLayers for any given backing, and it is easier to tell which Layer a RO belongs to, than which backing.
- The /set/ of opaque rects in a RenderLayer is updated by each RenderObject. When the RenderObject makes a change to its opaque rect, the list is marked dirty (by being deleted).
- The list of opaque rects is rebuilt by RenderLayer whenever the list is requested, and the previous list was dirtied. Thus, the backing does not actually update the list explicitly, it just queries on the RenderLayer, who builds the list if required and returns it. And a dirty list is never returned.

I don't particularly like this approach though, either. Is there another RO tree walk that could be considered? The previous patch was a lot cleaner.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:474
> > +    LayoutPoint ancestorRelOffset;
> > +    layer->convertToLayerCoords(ancestorLayer, ancestorRelOffset);
> 
> Have you tested composited layers that contain 2d-transformed layers? It seems like this convertToLayerCoords() could cross a transform boundary, which is bad.

Will look at this, thank you.

> > Source/WebCore/rendering/RenderObject.cpp:2703
> > +void RenderObject::updateOpaqueRect()
> 
> I don't know why this has to be on RenderObject, since it's a layer thing.

Each RO has an opaque Rect (its entire area or an empty rect at the moment).  This function mostly was added to not interact with the RenderLayer class in the .h file as that would need another include.  It's purpose is to determine if "this" is opaque and update its opaque rect in the RenderLayer's set.

> > Source/WebCore/rendering/RenderObject.h:1004
> > +        updateOpaqueRect();
> 
> This is going to be way too expensive. This is a hot function (hence the inline).

I had the impression that the function is especially hot when b==true (marking as needing layout). Is that incorrect? With b==false, the function is called once per layout(). And if the updating is to be done in the layout() tree walk, then it needs to be done exactly this many times.

As above, is there a better tree walk to consider than layout()?


I will split this bug into two, so we can do the next step and discuss it in a new bug. I'll copy this comment over as well, as I have questions not related to the simpler version. Thanks.

[Simon Fraser (smfr)]

(In reply to comment #44)
> (In reply to comment #43)
> > (From update of attachment 114066 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114066&action=review
> > 
> > I don't like that this patch is getting more and more complex. I think you should start by getting the easy cases working first (e.g. the layer's RenderObject is known to be opaque), and then progress to doing RenderObject walks in later patches. The IntRectEdge code is a large amount of new code. It should be tested on all platforms, not just Chromium, but I'd prefer that it just not be included in the initial patch.
> 
> I was starting to feel the same way, and I'll be happy to do so. I feel I should clarify what the patch will lose though in the process.
> 
> The IntRectEdge code was used to determine if the RenderLayers in a backing together would make it opaque.  Removing that will revert the check to this back to "is there a single opaque RenderLayer that fills the backing?"
> 
> I think the inconsistency I wanted to pointed here is that the IntRectEdge code has less to do with walking the RenderObject tree, and more to do with considering all the layers in a backing together (eventually all the RenderObjects in a backing).
> 
> Regarding testing: The IntRectEdge code will be tested by LayoutTests across all platforms. But I wanted a unit test also for the unionOfRectsCoverARect logic, especially while I was writing it. I would write unit tests for all platforms if such a general mechanism existed, but to my knowledge it does not?

Can you use platform/graphics/Region.h instead for this?

> > > Source/WebCore/rendering/RenderLayer.h:794
> > > +    // This is a mapping from RenderObject* to IntRect, which is the opaque rect for each RenderObject
> > > +    // belonging to this RenderLayer.
> > > +    HashMap<const RenderObject*, IntRect> m_opaqueRectForRenderObject;
> > > +
> > > +    // This list contains bounding box rectangles of opaque RenderObjects in the layer.
> > > +    OwnPtr<Vector<IntRectEdge> > m_opaqueRectsList;
> > 
> > It seems wrong to be adding these members to RenderLayer, when there's no contract about who updates them. A caller has no way to know if opaqueRects() will be return a stale list. There's just an implicit contract that RenderLayerBacking will update the rects at the right time.
> 
> - The set of opaque rects is contained in the RenderLayer, since we already walk RenderLayers for any given backing, and it is easier to tell which Layer a RO belongs to, than which backing.
> - The /set/ of opaque rects in a RenderLayer is updated by each RenderObject. When the RenderObject makes a change to its opaque rect, the list is marked dirty (by being deleted).
> - The list of opaque rects is rebuilt by RenderLayer whenever the list is requested, and the previous list was dirtied. Thus, the backing does not actually update the list explicitly, it just queries on the RenderLayer, who builds the list if required and returns it. And a dirty list is never returned.
> 
> I don't particularly like this approach though, either. Is there another RO tree walk that could be considered? The previous patch was a lot cleaner.

Let's do this in the other bug.
> I will split this bug into two, so we can do the next step and discuss it in a new bug. I'll copy this comment over as well, as I have questions not related to the simpler version. Thanks.

Sounds good.

[Dana Jansens]

Created attachment 114960 [details]
Patch

[Dana Jansens]

(In reply to comment #45)
> Can you use platform/graphics/Region.h instead for this?

Yes, that looks like a very appropriate place for such things.


So here is the simpler version of the patch.  Considers a single RenderObject per RenderLayer in a backing.  The backing is considered opaque iff any single such RenderObject fills the entire backing and is opaque.

The *AreOpaqueInRect() functions are rewritten as requested.

[Simon Fraser (smfr)]

Comment on attachment 114960 [details]
Patch

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.

[Dana Jansens]

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

[Dana Jansens]

Created attachment 115261 [details]
Patch

[Dana Jansens]

Created attachment 115808 [details]
Patch

[Dana Jansens]

Added a bunch of tests to borders.html.

- Test combinations of opaque/transparent borders, padding, background-clip, opaque-transparent background colors.
- Tests that it catches javascript changing css properties.
- Tests images with/without alpha, and with borders/padding.

Do these look sufficient for this step?

[WebKit Review Bot]

Comment on attachment 115808 [details]
Patch

Attachment 115808 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10519129

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/layer-creation/overlap-child-layer.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/layer-creation/overlap-transformed-layer.html
compositing/layer-creation/translatez-overlap.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/layer-creation/spanOverlapsCanvas.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/layer-creation/overlap-clipping.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/layer-creation/overflow-scroll-overlap.html

[WebKit Review Bot]

Created attachment 115815 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Created attachment 116092 [details]
Column rules are in contents area background.

[WebKit Review Bot]

Comment on attachment 116092 [details]
Column rules are in contents area background.

Attachment 116092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10378497

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/layer-creation/overlap-child-layer.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/layer-creation/overlap-transformed-layer.html
compositing/layer-creation/translatez-overlap.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/layer-creation/spanOverlapsCanvas.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/layer-creation/overlap-clipping.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/layer-creation/overflow-scroll-overlap.html

[Dana Jansens]

Created attachment 116240 [details]
Remove chromium specific things

[Dana Jansens]

Created attachment 116251 [details]
Remove tests of directly composited images

[Dana Jansens]

With composited <img> tags, the backing uses GraphicsLayer::setContentsToImage() rather than the general code-path.  Opaque-ness needs to be tested in the GraphicsLayer subclass.

In order to test opaque RenderImage objects, we need the image to be a child of a composited object.  This patch doesn't consider child RenderObjects so we can't test them just yet.  I've created tests for them in the next-step edition.

[Dana Jansens]

(In reply to comment #59)
> With composited <img> tags, the backing uses GraphicsLayer::setContentsToImage() rather than the general code-path.  Opaque-ness needs to be tested in the GraphicsLayer subclass.
> 
> In order to test opaque RenderImage objects, we need the image to be a child of a composited object.  This patch doesn't consider child RenderObjects so we can't test them just yet.  I've created tests for them in the next-step edition.

Sorry for the noise. I see that RenderImage objects are used for composited <img> tags when they have a border/padding/etc.  So adding tests in here for images with borders/padding/etc.

As noted by the bots, this will break some existing LayoutTests due to them now have opaque stuff.  I've never dealt with this before and would appreciate some guidance on this process. Should I be resetting results in those tests in this CL? Should I make a follow up CL with the reset?

[Dana Jansens]

Created attachment 116261 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116261 [details]
Patch

Attachment 116261 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10608256

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/layer-creation/overlap-child-layer.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/layer-creation/overlap-transformed-layer.html
compositing/layer-creation/translatez-overlap.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/layer-creation/spanOverlapsCanvas.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/layer-creation/overlap-clipping.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/layer-creation/overflow-scroll-overlap.html

[WebKit Review Bot]

Created attachment 116332 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Rather than creating this Transformation::isAxisAligned business, we can use the transformation on the bounding box, and test if the result is rectilinear. This is a more reliable test and doesn't require new code.

[Dana Jansens]

Created attachment 116403 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116403 [details]
Patch

Attachment 116403 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10633071

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/layer-due-to-layer-children.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/layer-creation/overlap-clipping.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/geometry/layer-due-to-layer-children-deep.html
compositing/layer-creation/overlap-child-layer.html
compositing/geometry/preserve-3d-switching.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/layer-creation/overflow-scroll-overlap.html

[Dana Jansens]

Created attachment 116550 [details]
Patch

[Dana Jansens]

The LayoutTests with images were coming across as flakey because sometimes the composited bounds would be computed before the images were loaded. When the image is loaded/changed, it needs to redecide if it is opaque or not.

Small change to the patch to make RenderLayerBacking::contentChanged recompute the contentsOpaque flag when it won't be via recomputing bounds.

[WebKit Review Bot]

Comment on attachment 116550 [details]
Patch

Attachment 116550 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10669157

New failing tests:
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/layer-due-to-layer-children.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/iframes/connect-compositing-iframe-delayed.html
animations/missing-from-to-transforms.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/geometry/layer-due-to-layer-children-deep.html
compositing/animation/computed-style-during-delay.html
compositing/iframes/connect-compositing-iframe2.html
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-overflow-root.html

[Dana Jansens]

Comment on attachment 116550 [details]
Patch

This patch has some bad effects with WebGL Canvas. In particular, when an image contents change, we need to determine its opaque status for the RenderLayer's opaque region. But in order to create an Image* for a generated image, the contents may change causing a loop where there should not be. This causes a synchronization issue in the HTMLCanvasElement/WebGL interaction, since it does not expect to generate an image in its contents changed listeners.

[Dana Jansens]

Created attachment 116768 [details]
notify backing about content changes in layers

[WebKit Review Bot]

Comment on attachment 116768 [details]
notify backing about content changes in layers

Attachment 116768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10675335

New failing tests:
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/connect-compositing-iframe.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/layer-due-to-layer-children.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/iframes/connect-compositing-iframe-delayed.html
animations/missing-from-to-transforms.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/geometry/layer-due-to-layer-children-deep.html
compositing/animation/computed-style-during-delay.html
compositing/iframes/connect-compositing-iframe2.html
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-overflow-root.html

[Simon Fraser (smfr)]

The number of patches here is getting overwhelming. Can you do more local testing to avoid all the patch spam?

[Dana Jansens]

Sure, I can avoid uploading patches before feedback on the previous one.

[Kenneth Russell]

(In reply to comment #70)
> (From update of attachment 116550 [details])
> This patch has some bad effects with WebGL Canvas. In particular, when an image contents change, we need to determine its opaque status for the RenderLayer's opaque region. But in order to create an Image* for a generated image, the contents may change causing a loop where there should not be. This causes a synchronization issue in the HTMLCanvasElement/WebGL interaction, since it does not expect to generate an image in its contents changed listeners.

I haven't been watching this bug closely, but wanted to make it clear that it isn't feasible to determine whether WebGL content is completely opaque -- aside from seeing whether the WebGL context was created without an alpha channel. See http://www.khronos.org/registry/webgl/specs/latest/#5.2.1 . The assumption in the general case must be that WebGL content is not opaque. It would be fine to assume this all the time initially, and optimize it later. Please make sure that this patch does not introduce a CPU readback during WebGL rendering; this would kill performance.

[Dana Jansens]

(In reply to comment #75)
> (In reply to comment #70)
> > (From update of attachment 116550 [details] [details])
> > This patch has some bad effects with WebGL Canvas. In particular, when an image contents change, we need to determine its opaque status for the RenderLayer's opaque region. But in order to create an Image* for a generated image, the contents may change causing a loop where there should not be. This causes a synchronization issue in the HTMLCanvasElement/WebGL interaction, since it does not expect to generate an image in its contents changed listeners.
> 
> I haven't been watching this bug closely, but wanted to make it clear that it isn't feasible to determine whether WebGL content is completely opaque -- aside from seeing whether the WebGL context was created without an alpha channel. See http://www.khronos.org/registry/webgl/specs/latest/#5.2.1 . The assumption in the general case must be that WebGL content is not opaque. It would be fine to assume this all the time initially, and optimize it later. Please make sure that this patch does not introduce a CPU readback during WebGL rendering; this would kill performance.

Thanks for this. I will require some plumbing through the RenderImageResource to be able to tell if it is a WebGL image at all and avoid this readback.

[Simon Fraser (smfr)]

Why can't you just assume that all canvases are not opaque?

[Kenneth Russell]

(In reply to comment #77)
> Why can't you just assume that all canvases are not opaque?

That's a good assumption. Analyzing the contents of a Canvas rendered via any means is likely to impose a massive slowdown.

[Dana Jansens]

(In reply to comment #77)
> Why can't you just assume that all canvases are not opaque?

What I'm not seeing is a way to tell that a RenderImage is a canvas at all. Am I just missing it somewhere?

What I've seen is that the StyleImage class has an isGeneratedImage() which is true for a canvas. I was thinking to have

RenderImageResource::isGeneratedImage() { return false; }

and then RenderImageResourceStyleImage could override it and give something useful. And the opaque-check could just ignore generated images.

Side-note: it would be easier and cleaner to simply remove any changes to RenderImage.cpp from this bug and stick that in another one, would that be good for you Simon?

[Simon Fraser (smfr)]

Are you talking about -webkit-canvas (i.e. canvas in CSS)? Doesn't currentFrameHasAlpha() just do the right thing for that kind of image?

[Dana Jansens]

(In reply to comment #80)
> Are you talking about -webkit-canvas (i.e. canvas in CSS)? Doesn't currentFrameHasAlpha() just do the right thing for that kind of image?

I think so, the HTMLCanvasElement class.

I presume it would give the right value, but the problem is that currentFrameHasAlpha() is in the Image object.  And grabbing the Image object causes WebGL to paint, which we don't want to do in response to the image contentChanged notification.

Calling RenderImageResource::image() ends up calling:
- RenderImageResourceStyleImage::image()
- StyleGeneratedImage::image()
- CSSImageGeneratorValue::image()
- CSSCanvasValue::image()
- HTMLCanvasElement::copiedImage() which gets the WebGLRenderingContext to paint itself and caches the result.

From what I can see we need a way to see that it is a canvas/generated image without the above, in order to update the opaque region when image contents change.

[Dana Jansens]

Comment on attachment 116768 [details]
notify backing about content changes in layers

View in context: https://bugs.webkit.org/attachment.cgi?id=116768&action=review

Not sure if this would help but I can imagine splitting this into a few smaller patches if you'd like. These are all somewhat independent pieces mashed together. If not at least we can talk about them somewhat independently.
1. RenderLayerCompositor builds opaque region for a backing (uses stub RenderLayer::opaqueRegion()), and backing decides opaque flag
2. RenderLayer::contentChanged() causes its compositing backing to re-decide opaque flag.
3. Add the *AreaIsOpaqueInRect() methods to RenderBox and RBMO.
4. Have RenderLayer::opaqueRegion() test its renderer() with RBMO::isOpaque().
5. Add foregroundContentsAreaIsOpaqueInRect() for RenderImage.

> Source/WebCore/rendering/RenderImage.cpp:437
> +            Image* image = m_imageResource->image(cWidth, cHeight).get();

This is the call we do not make for a canvas/webgl image when its contents are changed, as it causes a CPU readback.

> Source/WebCore/rendering/RenderLayer.h:470
> +    Region opaqueRegion() const { return renderer()->isOpaqueInRect(renderer()->borderBoundingBox()) ? renderer()->borderBoundingBox() : Region(); }

I can stick this behind #ifdef USE(ACCELERATED_COMPOSITING).

[Simon Fraser (smfr)]

*** Bug 40629 has been marked as a duplicate of this bug. ***

[Dana Jansens]

Created attachment 117907 [details]
Patch

@smfr, thanks for the conversation on Friday. I'm submitting a new patch that is more along the lines you described
on IRC. This considers only a single RenderObject for a backing in a simple way. It updates contentsOpaque when bounds
or content changed.

I experimented with just doing this computation during the painting step, but that made using DRT for testing it not
possible, as nothing showed up opaque there.

I've approached this with a better understanding of what "contents" means. This patch will check the background and
box decorations for a backing to see if they are opaque. If the background is not, then it will try the foreground, but
only if it will be painted (ie is part of the "contents"). If the foreground will be composited then it is not tested
here.

There are 3 sets of layout tests. One to test combinations or box decorations and backgrounds. One to test these same
things with composited image foregrounds. And one to test these together with painted image foregrounds.

[WebKit Review Bot]

Comment on attachment 117907 [details]
Patch

Attachment 117907 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10732708

New failing tests:
compositing/geometry/limit-layer-bounds-transformed.html
compositing/layer-creation/overlap-transformed-layer.html
compositing/geometry/clip.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
accessibility/aria-checkbox-text.html
compositing/layer-creation/overlap-clipping.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/iframes/connect-compositing-iframe2.html
compositing/layer-creation/overlap-child-layer.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/layer-creation/overflow-scroll-overlap.html

[WebKit Review Bot]

Created attachment 117909 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

This bug has gone epic.  It's going to be difficult to find reviewers for a 20 patch, 80+ comment bug.  I recommend attempting to break your work into smaller pieces and opening new bugs for those smaller pieces.

[Nat Duca]

Dana, should we mark this wontfix since we found a different way to do this?

[Simon Fraser (smfr)]

You did?

[Dana Jansens]

Yes, though this could do a better job in some cases, we are watching the calls to GraphicsContext and tracking an opaque rect via that.

I don't think this is truly WontFix, as this could be beneficial too, especially to making painting faster. But am not planning on working on it any time soon either.

[Simon Fraser (smfr)]

I presume that was Chromium only, so this bug is still valid for other platforms.

[Dana Jansens]

Yep.

[Alok Priyadarshi]

Reviving this old thread. I would like to mark GraphicsLayers as opaque to enable LCD text. I have created a patch by stealing code from here and referenced bugs. I will upload it shortly and would appreciate early feedback on the overall approach.

[Alok Priyadarshi]

Created attachment 185337 [details]
Patch

[WebKit Review Bot]

Attachment 185337 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/RenderBox.cpp', u'Source/WebCore/rendering/RenderBox.h', u'Source/WebCore/rendering/RenderBoxModelObject.h', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayer.h', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerModelObject.h']" exit_code: 1
Source/WebCore/rendering/RenderLayer.cpp:5139:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayer.cpp:5140:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayer.cpp:5140:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/RenderLayer.cpp:5141:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayer.cpp:5141:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/RenderLayer.cpp:5142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 185337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185337&action=review

This needs a changelog and some tests, but it's a good start.

> Source/WebCore/rendering/RenderBox.cpp:1160
> +    Color backgroundColor = style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!backgroundColor.isValid() || backgroundColor.hasAlpha())
> +        return false;

You could also check the background images here.

> Source/WebCore/rendering/RenderBox.h:598
> +    virtual bool backgroundIsOpaqueInRect(const LayoutPoint&, const IntRect&) const OVERRIDE;

It's not clear why the first argument is needed here. What is it an offset relative to? The rect should just be passed in some agreed-upon coordinate system (e.g. relative to the border box).

> Source/WebCore/rendering/RenderBoxModelObject.h:89
> +    virtual bool isPaintedOpaqueInRect(const LayoutPoint& offset, const IntRect& rect) const OVERRIDE { return foregroundIsOpaqueInRect(offset, rect) || backgroundIsOpaqueInRect(offset, rect); }

I don't like "isPaintedOpaque".

> Source/WebCore/rendering/RenderLayer.cpp:5108
> +static bool isListPaintedOpaqueInRect(const Vector<RenderLayer*>* list, PaintBehavior paintBehavior, const RenderLayer* rootLayer, const IntRect& rect)

Very clumsy function name.

> Source/WebCore/rendering/RenderLayer.cpp:5142
> +    return renderer()->isPaintedOpaqueInRect(offset, rect) ||
> +           isListPaintedOpaqueInRect(m_posZOrderList.get(), paintBehavior, rootLayer, rect) ||
> +           isListPaintedOpaqueInRect(m_negZOrderList.get(), paintBehavior, rootLayer, rect) ||
> +           isListPaintedOpaqueInRect(m_normalFlowList.get(), paintBehavior, rootLayer, rect);

This is testing just the layer's immediate renderer, which is a reasonable start but you should add a FIXME that it will miss many cases.

> Source/WebCore/rendering/RenderLayer.h:721
> +    // Input rect is given in rootLayer's coordinate space.

Why not just local to this layer?

[Alok Priyadarshi]

Comment on attachment 185337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185337&action=review

Simon: Thanks for the quick review. I will add tests and Changelog in the next patch. I wanted to get early feedback on the approach before investing too much time.

>> Source/WebCore/rendering/RenderBox.cpp:1160
>> +        return false;
> 
> You could also check the background images here.

I would prefer to do it in a separate patch. I will add a FIXME here.

>> Source/WebCore/rendering/RenderBox.h:598
>> +    virtual bool backgroundIsOpaqueInRect(const LayoutPoint&, const IntRect&) const OVERRIDE;
> 
> It's not clear why the first argument is needed here. What is it an offset relative to? The rect should just be passed in some agreed-upon coordinate system (e.g. relative to the border box).

I agree and that will in fact be cleaner. I was trying to keep it consistent with the RenderObject::paint() method which takes an offset parameter. May be I misunderstood your suggestion in comment 21 that you wanted the "isOpaque code mirror the painting code more closely".

>> Source/WebCore/rendering/RenderBoxModelObject.h:89
>> +    virtual bool isPaintedOpaqueInRect(const LayoutPoint& offset, const IntRect& rect) const OVERRIDE { return foregroundIsOpaqueInRect(offset, rect) || backgroundIsOpaqueInRect(offset, rect); }
> 
> I don't like "isPaintedOpaque".

What about isOpaqueInRect? Any other suggestions?

>> Source/WebCore/rendering/RenderLayer.cpp:5108
>> +static bool isListPaintedOpaqueInRect(const Vector<RenderLayer*>* list, PaintBehavior paintBehavior, const RenderLayer* rootLayer, const IntRect& rect)
> 
> Very clumsy function name.

How about listContentsOpaqueInRect? Any other suggestions?

> Source/WebCore/rendering/RenderLayer.cpp:5123
> +bool RenderLayer::isPaintedOpaqueInRect(PaintBehavior paintBehavior, const RenderLayer* rootLayer, const IntRect& rect) const

How about the function name contentsOpaqueInRect()?

>>> Source/WebCore/rendering/RenderLayer.cpp:5142
>>> +           isListPaintedOpaqueInRect(m_normalFlowList.get(), paintBehavior, rootLayer, rect);
>> 
>> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> This is testing just the layer's immediate renderer, which is a reasonable start but you should add a FIXME that it will miss many cases.

Will do.

>> Source/WebCore/rendering/RenderLayer.h:721
>> +    // Input rect is given in rootLayer's coordinate space.
> 
> Why not just local to this layer?

Sure. I was trying to keep it consistent with the paint method. Will change if that is no longer desired.

[Alok Priyadarshi]

Created attachment 186713 [details]
Patch

[Alok Priyadarshi]

Added Tests and ChangeLog. Converted query-rect to local coordinates.

[WebKit Review Bot]

Comment on attachment 186713 [details]
Patch

Attachment 186713 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16370923

New failing tests:
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/geometry/preserve-3d-switching.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/columns/composited-in-paginated.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/geometry/limit-layer-bounds-transformed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-perspective.html

[Build Bot]

Comment on attachment 186713 [details]
Patch

Attachment 186713 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16390022

New failing tests:
compositing/iframes/invisible-nested-iframe-hide.html
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/connect-compositing-iframe2.html
compositing/columns/composited-in-paginated.html
compositing/geometry/fixed-position-composited-switch.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/iframes/connect-compositing-iframe3.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/tiled-layers-hidpi.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/iframes/composited-parent-iframe.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html

[Build Bot]

Comment on attachment 186713 [details]
Patch

Attachment 186713 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16388085

[Build Bot]

Comment on attachment 186713 [details]
Patch

Attachment 186713 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16392054

[Build Bot]

Comment on attachment 186713 [details]
Patch

Attachment 186713 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16390167

New failing tests:
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/layer-creation/fixed-position-and-transform.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/columns/composited-in-paginated.html
compositing/layer-creation/fixed-position-change-out-of-view-in-view.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/iframes/connect-compositing-iframe3.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/tiled-layers-hidpi.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/iframes/composited-parent-iframe.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/iframes/connect-compositing-iframe2.html
compositing/backing/no-backing-for-perspective.html
compositing/layer-creation/animation-overlap-with-children.html

[Alok Priyadarshi]

The reason so many tests are failing because tests use opaque background for composited layers. I am going through each failure and making sure that the new baselines are correct.

[Alok Priyadarshi]

I manually examined all failing tests on cr-linux. All of them just need new rebaselines. The layers that are now marked opaque should indeed be marked opaque.

I suspected the results for compositing/geometry/flipped-writing-mode.html, but noticed that renderer background-rect does not cover the whole graphics-layer. So  the results are correct. But I wonder if I still need to call flipForWritingMode  to convert a rect in layer coordinates to that in renderer's local space.

[Simon Fraser (smfr)]

Comment on attachment 186713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186713&action=review

> Source/WebCore/rendering/RenderLayer.cpp:5124
> +        || listContentsOpaqueInRect(m_posZOrderList.get(), localRect)
> +        || listContentsOpaqueInRect(m_negZOrderList.get(), localRect)
> +        || listContentsOpaqueInRect(m_normalFlowList.get(), localRect);

These should use the accessors (posZOrderList() etc), which will assert if the lists are stale.

> Source/WebCore/rendering/RenderLayer.cpp:5139
> +        childLayer->convertToLayerCoords(this, childOffset);

convertToLayerCoords() doesn't handle transforms, so it's a bit weird to always use it to convert the rect, then rely on the fact that contentsOpaqueInRect() bails if there's a transform.

[Alok Priyadarshi]

Comment on attachment 186713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186713&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:5124
>> +        || listContentsOpaqueInRect(m_normalFlowList.get(), localRect);
> 
> These should use the accessors (posZOrderList() etc), which will assert if the lists are stale.

will do.

>> Source/WebCore/rendering/RenderLayer.cpp:5139
>> +        childLayer->convertToLayerCoords(this, childOffset);
> 
> convertToLayerCoords() doesn't handle transforms, so it's a bit weird to always use it to convert the rect, then rely on the fact that contentsOpaqueInRect() bails if there's a transform.

so is this sufficient?:
if (!childLayer->canUseConvertToLayerCoords()) continue;

sorry I am still learning about render-tree.

[Alok Priyadarshi]

Created attachment 187160 [details]
Patch

[Build Bot]

Comment on attachment 187160 [details]
Patch

Attachment 187160 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16408485

New failing tests:
compositing/iframes/invisible-nested-iframe-hide.html
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/connect-compositing-iframe2.html
compositing/columns/composited-in-paginated.html
compositing/geometry/fixed-position-composited-switch.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/iframes/composited-parent-iframe.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html

[Simon Fraser (smfr)]

Comment on attachment 187160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187160&action=review

> Source/WebCore/ChangeLog:8
> +        Mark layers as opaque in a very simple case - any child renderer has an opaque background and covers the entire composited bounds. RenderLayer::contentsOpaqueInRect has been implemented conservatively, i.e. it errs towards returning false negative.

It's not "any child renderer" currently; it's just the layer's main renderer.

[Build Bot]

Comment on attachment 187160 [details]
Patch

Attachment 187160 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16443391

New failing tests:
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/layer-creation/fixed-position-and-transform.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/columns/composited-in-paginated.html
compositing/layer-creation/fixed-position-change-out-of-view-in-view.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/iframes/connect-compositing-iframe3.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/tiled-layers-hidpi.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/iframes/composited-parent-iframe.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/iframes/connect-compositing-iframe2.html
compositing/backing/no-backing-for-perspective.html
compositing/layer-creation/animation-overlap-with-children.html

[Alok Priyadarshi]

Created attachment 187244 [details]
updated baselines

[WebKit Review Bot]

Comment on attachment 187244 [details]
updated baselines

Attachment 187244 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16429511

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
platform/chromium/compositing/rounded-corners.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-fixed.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-change-out-of-view-in-view.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-absolute.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-fixed-overflow-scrolled.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-relative.html

[WebKit Review Bot]

Comment on attachment 187244 [details]
updated baselines

Attachment 187244 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16426573

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
platform/chromium/compositing/rounded-corners.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-fixed.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-change-out-of-view-in-view.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-absolute.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-fixed-overflow-scrolled.html
platform/chromium/virtual/softwarecompositing/rtl/rtl-iframe-relative.html

[Build Bot]

Comment on attachment 187244 [details]
updated baselines

Attachment 187244 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16415665

New failing tests:
compositing/contents-opaque/contents-opaque-background-color.html
compositing/layer-creation/fixed-position-and-transform.html
compositing/iframes/iframe-resize.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/layer-creation/fixed-position-out-of-view.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/columns/composited-in-paginated.html
compositing/geometry/fixed-position-composited-switch.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html
compositing/layer-creation/fixed-position-change-out-of-view-in-view.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/iframes/connect-compositing-iframe2.html
compositing/backing/no-backing-for-perspective.html
compositing/layer-creation/animation-overlap-with-children.html

[Alok Priyadarshi]

Created attachment 187383 [details]
another try to rebaseline

[Build Bot]

Comment on attachment 187244 [details]
updated baselines

Attachment 187244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16427930

New failing tests:
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/iframes/iframe-resize.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/columns/composited-in-paginated.html
compositing/geometry/fixed-position-composited-switch.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/iframes/connect-compositing-iframe3.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/tiled-layers-hidpi.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/iframes/connect-compositing-iframe2.html
compositing/backing/no-backing-for-perspective.html
compositing/layer-creation/animation-overlap-with-children.html

[Build Bot]

Comment on attachment 187383 [details]
another try to rebaseline

Attachment 187383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16466141

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/visible-rect/3d-transform-style.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/2d-transformed.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/connect-compositing-iframe2.html
compositing/visible-rect/3d-transformed.html
compositing/visible-rect/animated-from-none.html
compositing/iframes/overlapped-iframe.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/visible-rect/clipped-by-viewport.html
compositing/plugins/no-backing-store.html
compositing/iframes/connect-compositing-iframe.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/composited-parent-iframe.html
compositing/visible-rect/clipped-visible-rect.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/visible-rect/animated.html
compositing/iframes/resizer.html
compositing/layer-creation/animation-overlap-with-children.html
compositing/visible-rect/iframe-and-layers.html

[Build Bot]

Comment on attachment 187383 [details]
another try to rebaseline

Attachment 187383 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16473064

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/visible-rect/3d-transform-style.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/2d-transformed.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/connect-compositing-iframe2.html
compositing/visible-rect/3d-transformed.html
compositing/visible-rect/animated-from-none.html
compositing/iframes/overlapped-iframe.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/visible-rect/clipped-by-viewport.html
compositing/plugins/no-backing-store.html
compositing/iframes/connect-compositing-iframe.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/composited-parent-iframe.html
compositing/visible-rect/clipped-visible-rect.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/visible-rect/animated.html
compositing/iframes/resizer.html
compositing/layer-creation/animation-overlap-with-children.html
compositing/visible-rect/iframe-and-layers.html

[Alok Priyadarshi]

Created attachment 187420 [details]
Avoided assert and rebaselined platform/chromium

[Build Bot]

Comment on attachment 187420 [details]
Avoided assert and rebaselined platform/chromium

Attachment 187420 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16476174

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/visible-rect/3d-transform-style.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/2d-transformed.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/iframes/connect-compositing-iframe2.html
compositing/visible-rect/3d-transformed.html
compositing/visible-rect/animated-from-none.html
compositing/iframes/overlapped-iframe.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/page-cache-layer-tree.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/visible-rect/clipped-by-viewport.html
compositing/plugins/no-backing-store.html
compositing/iframes/connect-compositing-iframe.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/composited-parent-iframe.html
compositing/visible-rect/clipped-visible-rect.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/visible-rect/animated.html
compositing/iframes/resizer.html
compositing/layer-creation/animation-overlap-with-children.html
compositing/visible-rect/iframe-and-layers.html

[Build Bot]

Comment on attachment 187420 [details]
Avoided assert and rebaselined platform/chromium

Attachment 187420 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16466429

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/visible-rect/3d-transform-style.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/iframes/enter-compositing-iframe.html
compositing/iframes/iframe-resize.html
compositing/visible-rect/2d-transformed.html
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/rtl/rtl-fixed-overflow.html
compositing/iframes/connect-compositing-iframe2.html
compositing/visible-rect/3d-transformed.html
compositing/visible-rect/animated-from-none.html
compositing/iframes/overlapped-iframe.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/iframes/page-cache-layer-tree.html
compositing/rtl/rtl-fixed.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/tiling/tile-cache-zoomed.html
compositing/plugins/no-backing-store.html
compositing/iframes/connect-compositing-iframe.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/contents-opaque/contents-opaque-layer-opacity.html
compositing/visible-rect/animated.html
compositing/iframes/resizer.html
compositing/layer-creation/animation-overlap-with-children.html

[WebKit Review Bot]

Comment on attachment 187420 [details]
Avoided assert and rebaselined platform/chromium

Attachment 187420 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16470479

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
platform/chromium/compositing/rounded-corners.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[WebKit Review Bot]

Comment on attachment 187420 [details]
Avoided assert and rebaselined platform/chromium

Attachment 187420 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16467597

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
platform/chromium/compositing/rounded-corners.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[Alok Priyadarshi]

Created attachment 189336 [details]
Updated mac baselines

[WebKit Review Bot]

Comment on attachment 189336 [details]
Updated mac baselines

Attachment 189336 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16651456

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[Build Bot]

Comment on attachment 189336 [details]
Updated mac baselines

Attachment 189336 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16650662

New failing tests:
compositing/contents-opaque/contents-opaque-layer-transform.html
compositing/rtl/rtl-fixed.html
compositing/rtl/rtl-fixed-overflow.html
compositing/contents-opaque/contents-opaque-background-color.html
compositing/contents-opaque/contents-opaque-layer-opacity.html

[Alok Priyadarshi]

Committed r143626: <http://trac.webkit.org/changeset/143626>

[Simon Fraser (smfr)]

This causes garbage to show up in tiles at http://chainlove.com on Mac.

[Simon Fraser (smfr)]

(In reply to comment #130)
> This causes garbage to show up in tiles at http://chainlove.com on Mac.

I filed bug 112043 to cover this.

[Simon Fraser (smfr)]

I'm going to roll this out. It's too hard to fix easily.

What this needs to do is to take account of paint phases all the way from GraphicsLayers down to RenderObjects. The code needs to mirror the painting code.

[Simon Fraser (smfr)]

Ugh, the rollout is hard with all the test changes.

Alok, please take care of this.

[Alok Priyadarshi]

(In reply to comment #133)
> Ugh, the rollout is hard with all the test changes.
> 
> Alok, please take care of this.

Yes rollout will be really painful. I do not see why it needs to mirror the paint code. Can you provide a simple test case? Also why is it mac only? I cannot reproduce it on chromium windows and linux. I still need to check chromium mac.

[James Robinson]

(In reply to comment #134)
> (In reply to comment #133)
> > Ugh, the rollout is hard with all the test changes.
> > 
> > Alok, please take care of this.
> 
> Yes rollout will be really painful. I do not see why it needs to mirror the paint code. Can you provide a simple test case? Also why is it mac only? I cannot reproduce it on chromium windows and linux. I still need to check chromium mac.

(In reply to comment #134)
> (In reply to comment #133)
> > Ugh, the rollout is hard with all the test changes.
> > 
> > Alok, please take care of this.
> 
> Yes rollout will be really painful. I do not see why it needs to mirror the paint code. Can you provide a simple test case? Also why is it mac only? I cannot reproduce it on chromium windows and linux. I still need to check chromium mac.

Alok, let's figure that out with the patch rolled out.  There are a number of bugs chromium-side as well about this change.

[Simon Fraser (smfr)]

On chainlove.com there's a negative z-index child behind the main content, which makes the body compositing layer get an m_foregroundLayer. This means that the m_graphicsLayer doesn't paint everything, but we've told it that it's opaque, so we get garbage. This is most apparent if you load the page with JS disabled.

You need to take paint phases in to account because you have to be able to answer the "is opaque" question for each GraphicsLayer, and different GraphicsLayers paint different phases.

[Alok Priyadarshi]

Trying rollout.

[Alok Priyadarshi]

Created attachment 193067 [details]
simplified chainlove.com

I created a simplified version of chainlove.com to understand the problem. The issue here is very simple. The body-element layer is getting composited due to various CSS settings. The body layer is getting marked as opaque because it has an explicit background. And we optimize out painting of the body background.

See RenderBox::paintBackground.
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBox.cpp#L1127

If we always paint the body background, everything paints perfectly. There are two ways to fix the problem:
1. Do not mark the body-element layer as opaque. This seems like a band-aid.
2. Fix the optimization in RenderBox::paintBackground. It assumes that body will either never be composited or always drawn with blending enabled.

Also see crbug.com/181015 which has a even simpler example.

[Simon Fraser (smfr)]

(In reply to comment #138)
> Created an attachment (id=193067) [details]
> simplified chainlove.com
> 
> I created a simplified version of chainlove.com to understand the problem. The issue here is very simple. The body-element layer is getting composited due to various CSS settings. The body layer is getting marked as opaque because it has an explicit background. And we optimize out painting of the body background.
> 
> See RenderBox::paintBackground.
> http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBox.cpp#L1127
> 
> If we always paint the body background, everything paints perfectly. There are two ways to fix the problem:
> 1. Do not mark the body-element layer as opaque. This seems like a band-aid.
> 2. Fix the optimization in RenderBox::paintBackground. It assumes that body will either never be composited or always drawn with blending enabled.
> 
> Also see crbug.com/181015 which has a even simpler example.

I think you're missing the fact that a single RenderLayer can be painted by multiple GraphicsLayers, with each GL painting a different phase. On chainlove.com, an element with negative z-index is causing the body layer to get "background" and "foreground" GraphicsLayer, so your opaqueness logic has to compute opaqueness separately for each phase, in order to mark those two layers opaque correctly.

[Alok Priyadarshi]

Created attachment 193174 [details]
Patch

[Alok Priyadarshi]

Actually on chainlove.com. I see the body layer getting only the foreground GraphicsLayer, not the background. The main issue was <body>'s background not getting painted.

Anyways you are right about the need to compute opaqueness separately given that different GraphicsLayers paint different phases. The new patch fixes these issues. The patch still needs tests and a proper ChangeLog. PTAL.

[WebKit Review Bot]

Comment on attachment 193174 [details]
Patch

Attachment 193174 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17209111

New failing tests:
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
http/tests/cache/subresource-failover-to-network.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/geometry/preserve-3d-switching.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/columns/composited-in-paginated.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/geometry/limit-layer-bounds-transformed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html

[WebKit Review Bot]

Comment on attachment 193174 [details]
Patch

Attachment 193174 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17136285

New failing tests:
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
http/tests/cache/subresource-failover-to-network.html
compositing/geometry/fixed-position-composited-switch.html
compositing/geometry/limit-layer-bounds-transformed-overflow.html
compositing/iframes/become-overlapped-iframe.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/geometry/preserve-3d-switching.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/columns/composited-in-paginated.html
compositing/iframes/become-composited-nested-iframes.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/geometry/limit-layer-bounds-transformed.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html

[Simon Fraser (smfr)]

Comment on attachment 193174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193174&action=review

> Source/WebCore/rendering/RenderBox.cpp:1134
> +        if (documentElementRenderer && !documentElementRenderer->hasBackground() && documentElementRenderer == parent() && !isComposited)
>              return;

This is wrong. The change is causing the body background to be rendered twice sometimes (once into the page layer, once into the body layer), and you're only doing this to work around deficiencies in the "is opaque" logic.

[Alok Priyadarshi]

Comment on attachment 193174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193174&action=review

>> Source/WebCore/rendering/RenderBox.cpp:1134
>>              return;
> 
> This is wrong. The change is causing the body background to be rendered twice sometimes (once into the page layer, once into the body layer), and you're only doing this to work around deficiencies in the "is opaque" logic.

What is the deficiency in "is opaque" logic? The primary/background GraphicsLayer for the <body> layer is being marked opaque because it IS opaque.

I would argue that this optimization is wrong. It assumes that either <body> will never be composited or always blended. Yes there is redundant painting but that redundancy should be optimized at the page layer level, not the body layer. Page layer is the one that is occluded.

BTW in case it is not clear, redundant painting will happen only when the body layer is composited, not always. Not sure how common this case is.

[Simon Fraser (smfr)]

(In reply to comment #145)
> (From update of attachment 193174 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193174&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:1134
> >>              return;
> > 
> > This is wrong. The change is causing the body background to be rendered twice sometimes (once into the page layer, once into the body layer), and you're only doing this to work around deficiencies in the "is opaque" logic.
> 
> What is the deficiency in "is opaque" logic? The primary/background GraphicsLayer for the <body> layer is being marked opaque because it IS opaque.

As I keep saying, the "is opaque" logic needs to know about paint phases.

The fact that this patch touches painting code shows that it's wrong.

[Alok Priyadarshi]

> As I keep saying, the "is opaque" logic needs to know about paint phases.
> 
> The fact that this patch touches painting code shows that it's wrong.

I still do not understand why it needs to know about paint phases when "is opaque" logic is making a decision based on just the background and marking the layer that paints background as opaque/not.

The logic seems fine except for the composited body element that does not paint the background under certain conditions. I thought that composited <body> element not painting background is wrong, but I was told that this weirdness is expected according to the CSS spec. This particular issue can be fixed independent of this bug and I have opened a separate bug:
https://bugs.webkit.org/show_bug.cgi?id=112454

Once the other bug is fixed, I would not need to touch any paint code or handle the body element specially here.

[Simon Fraser (smfr)]

Comment on attachment 193174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193174&action=review

> Source/WebCore/rendering/RenderLayerBacking.cpp:701
> +    if (!m_isMainFrameRenderViewLayer) {
> +        bool backgroundIsOpaque = m_owningLayer->backgroundIsKnownToBeOpaqueInRect(localCompositingBounds);
> +        GraphicsLayer* backgroundPaintingLayer = m_backgroundLayer ? m_backgroundLayer.get() : m_graphicsLayer.get();
> +        backgroundPaintingLayer->setContentsOpaque(backgroundIsOpaque);

Currently m_isMainFrameRenderViewLayer is the only layer that will have a m_backgroundLayer, so this logic isn't really needed.

[Simon Fraser (smfr)]

(In reply to comment #147)
> > As I keep saying, the "is opaque" logic needs to know about paint phases.
> > 
> > The fact that this patch touches painting code shows that it's wrong.
> 
> I still do not understand why it needs to know about paint phases when "is opaque" logic is making a decision based on just the background and marking the layer that paints background as opaque/not.
> 
> The logic seems fine except for the composited body element that does not paint the background under certain conditions. I thought that composited <body> element not painting background is wrong, but I was told that this weirdness is expected according to the CSS spec. This particular issue can be fixed independent of this bug and I have opened a separate bug:
> https://bugs.webkit.org/show_bug.cgi?id=112454
> 
> Once the other bug is fixed, I would not need to touch any paint code or handle the body element specially here.

I think you are correct. Your Changelog should make it clear that this is just a first step that only considers the primary graphics layer, and only looks at backgrounds. Ideally we'd be able to set other layers as being opaque if we had enough knowledge of their contents, and at that point we would have to consider paint phases.

[Alok Priyadarshi]

Created attachment 193895 [details]
Patch

[Alok Priyadarshi]

I decided to fix bug 112454 here itself because it was much easier to test. Notice two new test cases in the new patch - body-background-skipped and body-background-painted. Rest of the test cases are the same as original patch.

Most of the code is also from the original patch except for the case handled by skipBodyBackground().

[WebKit Review Bot]

Comment on attachment 193895 [details]
Patch

Attachment 193895 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17212516

New failing tests:
compositing/geometry/bounds-ignores-hidden.html
compositing/geometry/clip.html
compositing/contents-opaque/body-background-painted.html
compositing/geometry/limit-layer-bounds-positioned.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
http/tests/cache/subresource-failover-to-network.html
compositing/contents-opaque/background-color.html
compositing/geometry/fixed-position-composited-switch.html
compositing/contents-opaque/layer-transform.html
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/absolute-inside-out-of-view-fixed.html
compositing/geometry/limit-layer-bounds-overflow-root.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/limit-layer-bounds-positioned-transition.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/columns/composited-in-paginated.html
compositing/geometry/bounds-ignores-hidden-dynamic.html
compositing/contents-opaque/layer-opacity.html
compositing/contents-opaque/body-background-skipped.html
compositing/clip-child-by-non-stacking-ancestor.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html

[Alok Priyadarshi]

smfr: Could you please review. I will rebaseline all the failing tests before landing.

[Simon Fraser (smfr)]

Comment on attachment 193895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193895&action=review

> Source/WebCore/rendering/RenderBox.cpp:95
> +static bool skipBodyBackground(const RenderBox* bodyElementRenderer)

This function name is unclear. Skip for what?

> Source/WebCore/rendering/RenderLayer.cpp:5539
> +    // It is somehow getting triggered during style update.

The "somehow" is a bit mysterious. You should investigate and file a bug.

[Alok Priyadarshi]

Comment on attachment 193895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193895&action=review

>> Source/WebCore/rendering/RenderBox.cpp:95
>> +static bool skipBodyBackground(const RenderBox* bodyElementRenderer)
> 
> This function name is unclear. Skip for what?

Would skipsPaintingBodyBackground be better? I was following the same convention as skipRootBackground.

>> Source/WebCore/rendering/RenderLayer.cpp:5539
>> +    // It is somehow getting triggered during style update.
> 
> The "somehow" is a bit mysterious. You should investigate and file a bug.

Will do.

[Alok Priyadarshi]

Committed r146531: <http://trac.webkit.org/changeset/146531>

[Simon Fraser (smfr)]

This caused bug 114825.

[Jer Noble]

This also cause bug #115216.