Bug 82803

Summary: CSS Shaders: Custom filters painting is broken
Product: WebKit Reporter: Alexandru Chiculita <achicu@adobe.com>
Component: Layout and RenderingAssignee: Alexandru Chiculita <achicu@adobe.com>
Status: RESOLVED FIXED    
Severity: Normal CC: dino@apple.com, eric@webkit.org, jchaffraix@webkit.org, kbr@google.com, senorblanco@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 84460    
Bug Blocks:    
Attachments:
Description Flags
Patch V1
kbr: review-
Patch V2
none
Patch V3
buildbot: commit‑queue-
Patch V4 kbr: review+

Description From 2012-03-30 16:08:26 PST
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.
------- Comment #1 From 2012-04-18 10:56:28 PST -------
Also the shaders are not loaded anymore because the FilterEffectRenderer dies before any load event can be received.
------- Comment #2 From 2012-04-18 15:51:15 PST -------
Created an attachment (id=137791) [details]
Patch V1
------- Comment #3 From 2012-04-19 17:21:16 PST -------
(From update of attachment 137791 [details])
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.
------- Comment #4 From 2012-04-20 09:15:39 PST -------
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 #5 From 2012-04-20 09:24:08 PST -------
(From update of attachment 137791 [details])
OK, sounds good. Please upload a new patch when available.
------- Comment #6 From 2012-04-24 12:14:41 PST -------
Created an attachment (id=138617) [details]
Patch V2

Moved the implementation to RenderLayerFilterInfo
------- Comment #7 From 2012-04-24 12:18:47 PST -------
Created an attachment (id=138619) [details]
Patch V3

Fixed the previous review comment about the WEBGL macro.
------- Comment #8 From 2012-04-24 12:49:36 PST -------
(From update of attachment 138619 [details])
Attachment 138619 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12525317
------- Comment #9 From 2012-04-24 13:13:16 PST -------
(In reply to comment #8)
> (From update of attachment 138619 [details] [details])
> Attachment 138619 [details] [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.
------- Comment #10 From 2012-04-24 13:27:45 PST -------
Created an attachment (id=138630) [details]
Patch V4

Fixed the ifdef.
------- Comment #11 From 2012-04-24 13:49:19 PST -------
(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.
------- Comment #12 From 2012-04-24 13:59:02 PST -------
(In reply to comment #11)
> (From update of attachment 138630 [details] [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.
------- Comment #13 From 2012-04-24 15:46:22 PST -------
Landed in http://trac.webkit.org/changeset/115123.
------- Comment #14 From 2012-04-24 15:48:17 PST -------
Thumbs up here.