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.
Also the shaders are not loaded anymore because the FilterEffectRenderer dies before any load event can be received.
Created attachment 137791 [details] Patch V1
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.
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.
Comment on attachment 137791 [details] Patch V1 OK, sounds good. Please upload a new patch when available.
Created attachment 138617 [details] Patch V2 Moved the implementation to RenderLayerFilterInfo
Created attachment 138619 [details] Patch V3 Fixed the previous review comment about the WEBGL macro.
Comment on attachment 138619 [details] Patch V3 Attachment 138619 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12525317
(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.
Created attachment 138630 [details] Patch V4 Fixed the ifdef.
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.
(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.
Landed in http://trac.webkit.org/changeset/115123.
Thumbs up here.