Bug 109866 – Should not create backing store for frame borders

• Status: UNCONFIRMED
• Component: New Bugs

[Tien-Ren Chen]

Should not create backing store for frame borders

[Tien-Ren Chen]

Created attachment 188433 [details]
Patch

[Tien-Ren Chen]

Verified that <iframe> no longer generate unnecessary tiles for border.

All layout tests look nice. (with GraphicsLayer::drawsContent differences, and 2 pixel tests with sub-pixel difference)

<frame> is trickier since we still need backing on the main frame (which is just the <frameset>) to paint the borders. It is still better than creating separate backing for each frame.

[Simon Fraser (smfr)]

Comment on attachment 188433 [details]
Patch

I don't think this is correct for platforms that don't necessarily create backing store for the frame contents. I'm also suspicious that no test results have changed, which maybe be true but if this results in a behavior change you should add a new test.

[Tien-Ren Chen]

(In reply to comment #3)
> (From update of attachment 188433 [details])
> I don't think this is correct for platforms that don't necessarily create backing store for the frame contents.

I'm not sure how could that happen, but in that case, the frame contents will create its own backing store, since its compositing ancestor doesn't have one.

> I'm also suspicious that no test results have changed, which maybe be true but if this results in a behavior change you should add a new test.

Test result changes mentioned in last comment. This patch doesn't update test expectation yet. I'm waiting for the bots to give me the list I need to change.

--> with GraphicsLayer::drawsContent differences, and 2 pixel tests with sub-pixel difference

[Tien-Ren Chen]

By the way, there is a comment in the code I don't really understand though:
The comment says "The root layer always has a compositing layer, but it may not have backing." but code reads the opposite.

@ bool RenderLayerCompositor::requiresCompositingLayer(const RenderLayer* layer, RenderLayer::ViewportConstrainedNotCompositedReason* viewportConstrainedNotCompositedReason) const
    // The root layer always has a compositing layer, but it may not have backing.
    return requiresCompositingForTransform(renderer)
        || requiresCompositingForVideo(renderer)
        || requiresCompositingForCanvas(renderer)
        || requiresCompositingForPlugin(renderer)
        || requiresCompositingForFrame(renderer)
        || (canRender3DTransforms() && renderer->style()->backfaceVisibility() == BackfaceVisibilityHidden)
        || clipsCompositingDescendants(layer)
        || requiresCompositingForAnimation(renderer)
        || requiresCompositingForFilters(renderer)
        || requiresCompositingForPosition(renderer, layer, viewportConstrainedNotCompositedReason)
        || requiresCompositingForOverflowScrolling(layer)
        || requiresCompositingForBlending(renderer);

@ bool RenderLayerCompositor::requiresOwnBackingStore(const RenderLayer* layer, const RenderLayer* compositingAncestorLayer) const
    if (layer->isRootLayer()
        || layer->transform() // note: excludes perspective and transformStyle3D.
        || requiresCompositingForVideo(renderer)
        || requiresCompositingForCanvas(renderer)
        || requiresCompositingForPlugin(renderer)
        || (canRender3DTransforms() && renderer->style()->backfaceVisibility() == BackfaceVisibilityHidden)
        || requiresCompositingForAnimation(renderer)
        || requiresCompositingForFilters(renderer)
        || requiresCompositingForBlending(renderer)
        || requiresCompositingForPosition(renderer, layer)
        || requiresCompositingForOverflowScrolling(layer)
        || renderer->isTransparent()
        || renderer->hasMask()
        || renderer->hasReflection()
        || renderer->hasFilter())
        return true;

[Ahmad Saleem]

The attached patch is trying to modify following line, which is still present in Webkit Github source:

https://github.com/WebKit/WebKit/blob/e6bc0bb67a2e6fa4cc85aced20228dc00369033b/Source/WebCore/rendering/RenderLayerCompositor.cpp#L2684

Is this change needed anymore? It does not have any test case attached to it and even highlighted in Comment 03, so I cannot test whether it modify any functionality or have any impact across browsers.

Appreciate if someone can mark this appropriately, I think "RESOLVED LATER" to ponder on this would be appropriate or "RESOLVED WONTFIX". Thanks!