[webkit-reviews] review granted: [Bug 88908] [chromium] webkit-backface-visibility doesn't work with <video> : [Attachment 148511] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has granted Christopher Cameron
<ccameron at chromium.org>'s request for review:
Bug 88908: [chromium] webkit-backface-visibility doesn't work with <video>
https://bugs.webkit.org/show_bug.cgi?id=88908

Attachment 148511: Patch
https://bugs.webkit.org/attachment.cgi?id=148511&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=148511&action=review


I have a few comment nits, and then this is good to go.

> Source/Platform/chromium/public/WebLayer.h:118
> +    WEBKIT_EXPORT void setUseParentBackfaceVisibility(bool);

Can you also mention the limitations of this function that if this is true,
this layer must have a parent and that parent can't have this flag set?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:246
> +    // If this is a contents layer, then we should evaluate double-sidedness
based

Hrm.  Contents layer means a bit of a different thing in CCLayerTreeHostCommon
than it does in GraphicsLayerChromium.	I could imagine somebody thinking this
meant an instance of ContentLayerChromium and not
GraphicsLayerChromium::m_contentsLayer.  I think the property speaks for itself
and this comment only makes it confusing.  Can you remove it?