[chromium] add back-face visibility check for renderSurfaces

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

This patch is related to https://bugs.webkit.org/show_bug.cgi?id=84195, but it might be convenient if this lands first, to reduce conflicts just in case we want to merge it back to m20.
I'm currently testing this patch, both by itself and in conjunction with https://bugs.webkit.org/show_bug.cgi?id=84195, and its passing all unit tests and layout tests everywhere.

The bug was that when a parent has preserves3D, but the layer does not, then it creates a renderSurface, and we do not correctly check backface visibility in that case.  This caused a few pages that use flip mechanisms to break

[Shawn Singh]

Created attachment 142760 [details]
Patch

[Shawn Singh]

Comment on attachment 142760 [details]
Patch

forgot to update the other unit test, will submit new patch in a bit

[Vangelis Kokkevis]

Comment on attachment 142760 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:386
> +        if (layer->parent() && layer->parent()->preserves3D() && !layer->doubleSided() && combinedTransform.isBackFaceVisible())

Do we have to worry about combinedTransform's that are animated on the thread? It seems from:

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

that we always need to have a RS on the main tree for layers that could get one on the impl side.

[Shawn Singh]

Created attachment 142777 [details]
added transformIsKnown check

ah, thanks... that is what was going wrong.  This patch now passes all unit tests, no obvious regression on layout tests.

[Adrienne Walker]

Comment on attachment 142777 [details]
added transformIsKnown check

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:388
> +        // If this surface flattens its entire subtree, then we need to check back-face visibility before continuing with this surface.
> +        // Cannot early exit here, however, if the transform is animating and not known on the main thread.
> +        if (layer->parent() && layer->parent()->preserves3D() && transformToParentIsKnown(layer) && !layer->doubleSided() && combinedTransform.isBackFaceVisible())

Don't you also need to check !layer->preserves3D() to know that this layer flattens its subtree? I think a "flattensSubtreeToSurface()" helper might improve readability too.

[Shawn Singh]

(In reply to comment #5)
> (From update of attachment 142777 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142777&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:388
> > +        // If this surface flattens its entire subtree, then we need to check back-face visibility before continuing with this surface.
> > +        // Cannot early exit here, however, if the transform is animating and not known on the main thread.
> > +        if (layer->parent() && layer->parent()->preserves3D() && transformToParentIsKnown(layer) && !layer->doubleSided() && combinedTransform.isBackFaceVisible())
> 
> Don't you also need to check !layer->preserves3D() to know that this layer flattens its subtree? I think a "flattensSubtreeToSurface()" helper might improve readability too.

Actually the code is correct, its my comment that is misleading and wrong.   The comment should say "if this surface is already in a w3c rendering context, then we should check whether the back face is visible"

The full situation can be understood in the context of https://bugs.webkit.org/show_bug.cgi?id=84195.  Different transforms need to be used to check back-face visibility, depending on whether we're in a 3d rendering context or not.

I'll add the appropriate helper function and fix the comment.

[Shawn Singh]

Created attachment 142784 [details]
Fixed comment and added useful helper function

Tested again manually, unit, layout, all seems good.

[Adrienne Walker]

Comment on attachment 142784 [details]
Fixed comment and added useful helper function

R=me.

[Shawn Singh]

Committed r117645: <http://trac.webkit.org/changeset/117645>

[Shawn Singh]

(In reply to comment #8)
> (From update of attachment 142784 [details])
> R=me.

Thanks!