Bug 76328 – [chromium] Draw gutter quads outside root content layer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexandre Elias]

[chromium] Draw gutter quads outside root content layer

[Alexandre Elias]

Created attachment 122532 [details]
Patch

[Alexandre Elias]

Created attachment 122831 [details]
Patch

[James Robinson]

Comment on attachment 122831 [details]
Patch

Possibly dumb idea: can we just make the NonCompositedContentHost's m_graphicsLayer be as big as the viewport always even if the content size is smaller and rely on checkerboarding?

[Alexandre Elias]

I tried that before taking this approach.  The problem was that content size doesn't necessarily lie on tile boundaries, so there was a stretch of debug color between on the last content tile.  Nor is it desirable to rely on painting background color onto the tiles themselves.

[Adrienne Walker]

@jamesr: I think painting outside what WebKit thinks is the bounds on an element will just get you "debug red" or nothing at all, so setting the size on just the graphics layer probably won't do the right thing.  And, I'm not sure this can only be solved on the main thread side; don't we need to handle this at draw time because of a pinch-zoom on the impl thread?

Although, looking at this patch, I'm kind of sad at code that has to look for a layer with isNonCompositedContent set on it.  I'm really hoping we can get rid of that flag eventually.

[Alexandre Elias]

(In reply to comment #5)
> Although, looking at this patch, I'm kind of sad at code that has to look for a layer with isNonCompositedContent set on it.  I'm really hoping we can get rid of that flag eventually.

I suppose instead I could add a function to CCLayerImpl that walks the tree and returns the first layer with drawsContent() == true.  Would that be guaranteed to be equivalent?

[James Robinson]

That doesn't seem any better.

What about having a layer that draws to infinity regardless of the content size, but doesn't issue paints outside its contentBounds?  Not sure if that's a ton better but I'd like to somehow express this within the compositor code in a way that's not so tightly bound to the details of what we're doing in the WebKit side.

[Alexandre Elias]

Created attachment 122873 [details]
Patch

[Alexandre Elias]

OK, I moved it to the layer side.  It does seem more natural to put it there.

It's still using isNonCompositedContent() for now; should I add a new property called like hasInfiniteBackground pushed through the commit process, or is there something else I could piggyback on?

[James Robinson]

Looks like this should be pretty easily testable, no?

[Alexandre Elias]

Created attachment 123008 [details]
Patch

[Alexandre Elias]

Added a test and a new property called "backgroundCoversViewport" (to avoid using isNonCompositedContent).

[James Robinson]

Comment on attachment 123008 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:94
> +    void setBackgroundCoversViewport(bool);
> +    bool backgroundCoversViewport() const { return m_backgroundCoversViewport; }

this flag appears to only be supported on tiled layers - can we move the bit down to TiledLayerChromium or have some sort of comment at least indicating that it's a no-op for other layer types?

[Alexandre Elias]

Created attachment 123039 [details]
Patch

[Alexandre Elias]

OK, I moved up the implementation to CCLayerImpl as there is nothing specific to TiledLayerChromium about it.  The test is still against TiledLayer as that test file has more utilities for rect comparison.

Also, I added support for drawing a fullscreen rect when the visibleLayerRect is empty (which seems like the correct thing to do).

[W. James MacLean]

Perhaps this is not what you are looking for, but the SolidColorLayer bug (https://bugs.webkit.org/show_bug.cgi?id=75732) is designed to help with a similar problem in Aura ... what if one just puts a solid color layer behind everything else?

[Alexandre Elias]

I talked it over a bit with James, and we agreed that we should probably have both this backgroundCoversViewport feature and your layer type.  Although each feature could be shoehorned to also address the other case, the details are different so it would be ugly.  In my case I want the color to always fill the viewport (instead of being fixed in advance), and to be tied to the root content layer's background color.

[James Robinson]

You've got merge conflicts!

[James Robinson]

Comment on attachment 123039 [details]
Patch

Looks good, R=me.  Will need to fix merge issues before landing.

[W. James MacLean]

(In reply to comment #17)
> I talked it over a bit with James, and we agreed that we should probably have both this backgroundCoversViewport feature and your layer type.  Although each feature could be shoehorned to also address the other case, the details are different so it would be ugly.  In my case I want the color to always fill the viewport (instead of being fixed in advance), and to be tied to the root content layer's background color.

Sounds fair!

[Alexandre Elias]

Created attachment 123202 [details]
Patch

[James Robinson]

Comment on attachment 123202 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 123202 [details]
Patch

Clearing flags on attachment: 123202

Committed r105470: <http://trac.webkit.org/changeset/105470>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.