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

[bugzilla-daemon at webkit.org]

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 136353: Patch
https://bugs.webkit.org/attachment.cgi?id=136353&action=review

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


Just so I'm reading this right:

framebuffer => (readback) deviceBackgroundTexture
deviceBackgroundTexture => (filter) filteredDeviceBackground
filteredDeviceBackground => (copy) drawingSurface->backgroundTexture()
drawingSurface->backgroundTexture() => (copy) targetRenderSurface

It kind of seems like there's an extra copy (or two?) here.  Is there a reason
you can't just copy directly from the filteredBackgroundTexture into the
targetRenderSurface?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:564
> +	   drawingSurface->releaseBackgroundTexture();

I'm a little confused why you only release the background texture here, but not
on either of the two returns above this.

> Source/WebCore/testing/Internals.cpp:81
> +#if USE(ACCELERATED_COMPOSITING) && PLATFORM(CHROMIUM)

Chromium always uses accelerated compositing.

> LayoutTests/ChangeLog:13
> +	   * compositing/filters/background-filter-blur-outsets.html: Added.
> +	   * compositing/filters/background-filter-blur.html: Added.

I think these html files should be in platform/chromium/compositing/filters/,
since they're not really testing anything for non-Chromium platforms.

> LayoutTests/compositing/filters/background-filter-blur-outsets.html:22
> +  .yellow-parent {
> +    width: 600px;
> +    height:	600px;
> +    -webkit-filter: blur(10px);
> +  }
> +  .centered {
> +    width: 200px;
> +    height: 200px;
> +    position: absolute;
> +    left: 100px;
> +    top: 100px;
> +  }

Unused?

> LayoutTests/compositing/filters/background-filter-blur-outsets.html:33
> +	 window.setTimeout(setBlur, 0);

Can you write these tests without using setTimeout?

> LayoutTests/compositing/filters/background-filter-blur-outsets.html:47
> +<div class="composited green-border" style="position:absolute; left:50px;
top:50px; width:200px; height:200px;"></div>
> +<div id="blur" style="position:absolute; left:59px; top:59px; width:200px;
height:200px; border:1px solid green;"></div>

Can you remove the border from the blur node, so that it's clear that it's not
using its own border?