[webkit-reviews] review denied: [Bug 75842] CSS Filters: apply the filters in RenderLayerBacking::paintIntoLayer when filters cannot be composited in hardware : [Attachment 121670] Patch V2

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 75842: CSS Filters: apply the filters in RenderLayerBacking::paintIntoLayer
when filters cannot be composited in hardware
https://bugs.webkit.org/show_bug.cgi?id=75842

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=121670&action=review


r- for the multiple calls to filtersCanBeComposited. I'd also like Simon's take
on the rest...

> Source/WebCore/ChangeLog:6
> +	   Layers have two possible states: composited or not. When composited
the RenderLayerBacking::paintIntoLayer is used

You should say "RenderLayers have two possible states", to avoid confusion with
the underlying composited layer implementations

> Source/WebCore/ChangeLog:7
> +	   to render the result inside a memory buffer. When not composited the
RenderLayer::paintLayer method will draw

Painting doesn't necessarily go into a memory buffer. It is painted into the
passed GraphicsContext, which paints to the approrpiate platform specific
place.

> Source/WebCore/ChangeLog:13
> +	   Reviewed by NOBODY (OOPS!).

Please put this at the top, right under the bug URL.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:711
> +    if (!filtersCanBeComposited(filters)) {

This will cause filtersCanBeComposited() to be called ar least twice for
hardware accelerated layers: once in GraphicsLayerCA::setFilters and once here.
This function is non-trivial now and will get more expensive as we handle more
cases. I don't see why this is needed. If filtersCanBeComposited() returned
false from GraphicsLayerCA then this call should be made with a null filter
list and the check at the top of the function should take care of getting rid
of any leftover filters. If that is not happening (setFilters(null) is not
getting called when filters can't be composited) then the error is up in
GraphicsLayer or RenderLayerBacking and should be dealt with there.

Either way, you should avoid multiple calls to filtersCanBeComposited() per
style change.

> Source/WebCore/rendering/RenderLayer.cpp:2768
> +	  && (!isComposited() || backing()->paintingGoesToWindow() ||
shouldDoSoftwarePaint(this, paintFlags & PaintLayerPaintingReflection))) {

This is an awfully confusing if test. I'd like Simon's take on how it can be
better structured.