Bug 82803 - CSS Shaders: Custom filters painting is broken
Summary: CSS Shaders: Custom filters painting is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on: 84460
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-30 16:08 PDT by Alexandru Chiculita
Modified: 2012-04-24 15:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch V1 (21.90 KB, patch)
2012-04-18 15:51 PDT, Alexandru Chiculita
kbr: review-
Details | Formatted Diff | Diff
Patch V2 (24.16 KB, patch)
2012-04-24 12:14 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V3 (24.14 KB, patch)
2012-04-24 12:18 PDT, Alexandru Chiculita
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch V4 (24.17 KB, patch)
2012-04-24 13:27 PDT, Alexandru Chiculita
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2012-03-30 16:08:26 PDT
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 Alexandru Chiculita 2012-04-18 10:56:28 PDT
Also the shaders are not loaded anymore because the FilterEffectRenderer dies before any load event can be received.
Comment 2 Alexandru Chiculita 2012-04-18 15:51:15 PDT
Created attachment 137791 [details]
Patch V1
Comment 3 Kenneth Russell 2012-04-19 17:21:16 PDT
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.
Comment 4 Alexandru Chiculita 2012-04-20 09:15:39 PDT
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 Kenneth Russell 2012-04-20 09:24:08 PDT
Comment on attachment 137791 [details]
Patch V1

OK, sounds good. Please upload a new patch when available.
Comment 6 Alexandru Chiculita 2012-04-24 12:14:41 PDT
Created attachment 138617 [details]
Patch V2

Moved the implementation to RenderLayerFilterInfo
Comment 7 Alexandru Chiculita 2012-04-24 12:18:47 PDT
Created attachment 138619 [details]
Patch V3

Fixed the previous review comment about the WEBGL macro.
Comment 8 Build Bot 2012-04-24 12:49:36 PDT
Comment on attachment 138619 [details]
Patch V3

Attachment 138619 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12525317
Comment 9 Alexandru Chiculita 2012-04-24 13:13:16 PDT
(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.
Comment 10 Alexandru Chiculita 2012-04-24 13:27:45 PDT
Created attachment 138630 [details]
Patch V4

Fixed the ifdef.
Comment 11 Kenneth Russell 2012-04-24 13:49:19 PDT
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.
Comment 12 Alexandru Chiculita 2012-04-24 13:59:02 PDT
(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.
Comment 13 Alexandru Chiculita 2012-04-24 15:46:22 PDT
Landed in http://trac.webkit.org/changeset/115123.
Comment 14 Dean Jackson 2012-04-24 15:48:17 PDT
Thumbs up here.