Summary: | CSS Shaders: Custom filters painting is broken | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||||
Component: | Layout and Rendering | Assignee: | Alexandru Chiculita <achicu> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dino, eric, jchaffraix, kbr, senorblanco | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 84460 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2012-03-30 16:08:26 PDT
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. |