Bug 82803 – CSS Shaders: Custom filters painting is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexandru Chiculita]

After https://bugs.webkit.org/show_bug.cgi?id=80323 the custom shaders filters painting is broken.

That's because of the missing bool affectsOpacity() const and bool movesPixels() const on the CustomFilterOperation.

[Alexandru Chiculita]

Also the shaders are not loaded anymore because the FilterEffectRenderer dies before any load event can be received.

[Alexandru Chiculita]

Created attachment 137791 [details]
Patch V1

[Kenneth Russell]

Comment on attachment 137791 [details]
Patch V1

View in context: https://bugs.webkit.org/attachment.cgi?id=137791&action=review

One concern is that this adds more functionality to RenderLayer, which as discussed in today's WebKit meeting is probably not a good architectural direction.

The code otherwise seems fine, and I'd be willing to r+ it if there are no other reviewers with more experience in this area who want to look at it.

> Source/WebCore/rendering/RenderLayer.h:902
> +#if ENABLE(CSS_SHADERS) && ENABLE(WEBGL)

Did you mean to add ENABLE(WEBGL) here? It's the only #if clause in the new code referencing it.

[Alexandru Chiculita]

View in context: https://bugs.webkit.org/attachment.cgi?id=137791&action=review

> One concern is that this adds more functionality to RenderLayer, which as discussed in today's WebKit meeting is probably not a good architectural direction.

Yes, it does. I'm not sure about how functionality can be split, but at least memory can be allocated on demand instead. I think I can move m_filter and a couple of more related fields to a different structure and only allocate it when needed.

I've added the following bug:
https://bugs.webkit.org/show_bug.cgi?id=84460

>> Source/WebCore/rendering/RenderLayer.h:902
>> +#if ENABLE(CSS_SHADERS) && ENABLE(WEBGL)
> 
> Did you mean to add ENABLE(WEBGL) here? It's the only #if clause in the new code referencing it.

No this an error I will fix it. I don't want to limit the loading notifications, because platforms might choose to implement it without WebGL.

[Kenneth Russell]

Comment on attachment 137791 [details]
Patch V1

OK, sounds good. Please upload a new patch when available.

[Alexandru Chiculita]

Created attachment 138617 [details]
Patch V2

Moved the implementation to RenderLayerFilterInfo

[Alexandru Chiculita]

Created attachment 138619 [details]
Patch V3

Fixed the previous review comment about the WEBGL macro.

[Build Bot]

Comment on attachment 138619 [details]
Patch V3

Attachment 138619 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12525317

[Alexandru Chiculita]

(In reply to comment #8)
> (From update of attachment 138619 [details])
> Attachment 138619 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12525317

Looks like I'm missing an ifdef. I will post a new patch.

[Alexandru Chiculita]

Created attachment 138630 [details]
Patch V4

Fixed the ifdef.

[Kenneth Russell]

Comment on attachment 138630 [details]
Patch V4

This looks good to me; better than earlier revisions. Though I'm not an expert in the filter code, setting r=me. Alex, since you're a committer, I'm not touching the cq? bit, so that you can decide whether you want to get a review from someone else as well.

[Alexandru Chiculita]

(In reply to comment #11)
> (From update of attachment 138630 [details])
> This looks good to me; better than earlier revisions. Though I'm not an expert in the filter code, setting r=me. Alex, since you're a committer, I'm not touching the cq? bit, so that you can decide whether you want to get a review from someone else as well.

Thanks for the R+. It's mostly CSS shaders code moving from RenderLayer to RenderLayerFilterInfo. I think Dean already knows about this, so it should be ok to commit it as is.

[Alexandru Chiculita]

Landed in http://trac.webkit.org/changeset/115123.

[Dean Jackson]

Thumbs up here.