[webkit-reviews] review granted: [Bug 68479] Hardware acceleration of W3C Filter Effects : [Attachment 119705] Patch with implementation for Mac using CI

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 68479: Hardware acceleration of W3C Filter Effects
https://bugs.webkit.org/show_bug.cgi?id=68479

Attachment 119705: Patch with implementation for Mac using CI
https://bugs.webkit.org/attachment.cgi?id=119705&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119705&action=review


> Source/WebCore/platform/graphics/GraphicsLayer.h:316
> +    const FilterOperations& filter() const { return m_filter; }
> +    virtual bool setFilter(const FilterOperations& filter) { m_filter =
filter; return true; }

I could use "filters" plural everywhere.

A comment should say what the boolean return value to setFilter() means.

> Source/WebCore/platform/graphics/GraphicsLayer.h:452
> +    FilterOperations m_filter;

m_filters

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1256
> +	   
> +    }

Extra blank line.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1287
> +	       updateFilter();

updateFilters()

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:348
> +	   FilterChanged = 1 << 25,

FiltersChanged

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:707
> +    // Assume filterCanBeComposited was called and it returned true

Can you ASSERT that here too?

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:714
> +	       // FIXME: For now assume drop shadow is the last filter, put it
on the layer

Should file a bug to fix this.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:769
> +	       //for ( ; amount < 0; amount += 360)
> +	       //    ;
> +	       //for ( ; amount >= 360; amount -= 360)
> +	       //    ;
> +	       amount *= M_PI * 2 / 360;
> +	       //amount -= M_PI;

Commented out code here!

> Source/WebCore/rendering/RenderLayer.cpp:272
> +bool RenderLayer::rendersFilter() const

'renders' is a bit vague here. Elsewhere we use the "paintsWith" terminology.