[webkit-reviews] review denied: [Bug 80046] [chromium] Background filters for composited layers : [Attachment 131883] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 2 15:16:20 PDT 2012


Adrienne Walker <enne at google.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 80046: [chromium] Background filters for composited layers
https://bugs.webkit.org/show_bug.cgi?id=80046

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

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


Needs tests.

Unfortunately, I don't think this is something that's easily testable in a unit
test, since correctness is really about final pixel results and less about what
calls get made on the way there.  One option might be exposing background
filters to window.internals somehow and writing some layout tests.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:117
> +    // regions in this layer. They are used through the WebLayer interface,
and are not exposed to HTML.

Can you expose them to the WebLayer interface in this patch?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:497
> +    bool drawingToRootRenderSurface = m_currentRenderSurface ==
m_defaultRenderSurface;
> +    if (!drawingToRootRenderSurface)
> +	   return;

I kind of wish this case could be handled better and not silently fail, but I
am unsure about how to do that.  Even an assert might be overkill, since
different content could fall down this path in a way that's unexpected. 
Perhaps a comment on WebLayer::setBackgroundFilters about when this is valid to
be used and a comment here about why we're not handling this case would help?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:512
> +    if (applyBackgroundFilters) {

style nit: early out here instead of indenting.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:516
> +	   if (filteredDeviceBackground.getTexture()) {

style nit: early out here instead of indenting.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1211
> +    if (m_currentRenderSurface == renderSurface && !m_currentManagedTexture)


There's at least one other place in LRC that makes this same equality check,
and we should be consistent in how we check for this state.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:116
> +    if (!m_contentsBackgroundTexture->reserve(requiredSize,
GraphicsContext3D::RGBA)) {
> +	   m_skipsDraw = true;
> +	   return false;
> +    }

I'm not entirely convinced that we should skip drawing the surface here.  If we
drew the rest of the surface we'd not have the filter, but would still be able
to show all the rest of the content.  You don't skip the surface if
getFramebufferTexture fails, for example.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:129
> +bool CCRenderSurface::prepareReplicaBackgroundTexture(LayerRendererChromium*
layerRenderer)

This looks a lot like prepareContentsBackgroundTexture.  For the sake of "don't
repeat yourself", can you combine these somehow?


More information about the webkit-reviews mailing list