Bug 109866

Summary: Should not create backing store for frame borders
Product: WebKit Reporter: Tien-Ren Chen <trchen>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: eric, ojan.autocc, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch simon.fraser: review-

Description Tien-Ren Chen 2013-02-14 14:50:48 PST
Should not create backing store for frame borders
Comment 1 Tien-Ren Chen 2013-02-14 14:54:04 PST
Created attachment 188433 [details]
Patch
Comment 2 Tien-Ren Chen 2013-02-14 14:57:53 PST
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.
Comment 3 Simon Fraser (smfr) 2013-02-14 15:17:42 PST
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.
Comment 4 Tien-Ren Chen 2013-02-14 15:36:55 PST
(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
Comment 5 Tien-Ren Chen 2013-02-14 15:44:34 PST
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;