Bug 94728

I'm currently looking at this.

Created attachment 162132 [details] Patch

Comment on attachment 162132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162132&action=review Looks good to me. > Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:286 > + CustomFilterGlobalContext m_programValidator; It looks like we need to extract the validator out of CustomFilterGlobalContext. Do you mind adding a bug for that and assign it to me? > Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:287 > + HashSet<LayerFilterMesh*> m_meshSet; I think it would be awesome if m_meshSet could also go into the CustomFilterGlobalContext, but that's ok for now. There's a different bug for that https://bugs.webkit.org/show_bug.cgi?id=88429 . It may be worth adding a fixme linking to the bug. > Source/WebCore/rendering/FilterEffectRenderer.cpp:207 > +#if !PLATFORM(BLACKBERRY) This will also disable FilterOperation::REFERENCE. Is that what you want?

Comment on attachment 162132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162132&action=review Thanks for looking into this, even though it's port specific =) >> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:286 >> + CustomFilterGlobalContext m_programValidator; > > It looks like we need to extract the validator out of CustomFilterGlobalContext. Do you mind adding a bug for that and assign it to me? Will do, the goal here is to not have any object which could possibly create a GraphicsContext3D on our compositing thread. >> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:287 >> + HashSet<LayerFilterMesh*> m_meshSet; > > I think it would be awesome if m_meshSet could also go into the CustomFilterGlobalContext, but that's ok for now. There's a different bug for that https://bugs.webkit.org/show_bug.cgi?id=88429 . It may be worth adding a fixme linking to the bug. In this case, that's in opposition to the previous comment - we don't want a CustomFilterGlobalContext at all on our compositing thread, since GraphicsContext3D is WebKit-thread specific. CustomFilterGlobalContext should have a CustomFilterProgramValidator and a list of meshes, but we won't be reusing that infrastructure in the BlackBerry LayerFilterRenderer - instead, it will just have it's own separate CustomFilterProgramValidator and list of meshes. >> Source/WebCore/rendering/FilterEffectRenderer.cpp:207 >> +#if !PLATFORM(BLACKBERRY) > > This will also disable FilterOperation::REFERENCE. Is that what you want? I've been trying to figure out what Joshua was trying to accomplish here, but I'm not certain why he disabled this code...

Comment on attachment 162132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162132&action=review >>> Source/WebCore/platform/graphics/blackberry/LayerFilterRenderer.h:286 >>> + CustomFilterGlobalContext m_programValidator; >> >> It looks like we need to extract the validator out of CustomFilterGlobalContext. Do you mind adding a bug for that and assign it to me? > > Will do, the goal here is to not have any object which could possibly create a GraphicsContext3D on our compositing thread. I created https://bugs.webkit.org/show_bug.cgi?id=95804

Comment on attachment 162132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162132&action=review >>> Source/WebCore/rendering/FilterEffectRenderer.cpp:207 >>> +#if !PLATFORM(BLACKBERRY) >> >> This will also disable FilterOperation::REFERENCE. Is that what you want? > > I've been trying to figure out what Joshua was trying to accomplish here, but I'm not certain why he disabled this code... Mystery solved, the only thing standing between us and FECustomFilter is the Texture naming conflict described in https://bugs.webkit.org/show_bug.cgi?id=95781. I'm gonna re-enable this code in anticipation of a patch for https://bugs.webkit.org/show_bug.cgi?id=95781.

Created attachment 162142 [details] Patch

Comment on attachment 162142 [details] Patch We plan to enable CSS shaders a little bit later when the implementation is updated to match the latest spec. I'll rename this bug slightly.

(In reply to comment #8) > (From update of attachment 162142 [details]) > We plan to enable CSS shaders a little bit later when the implementation is updated to match the latest spec. I'll rename this bug slightly. I thought that we are following the spec closely. The remaining issues should be around the filter margins, the filter area used by the feCustomFilter and implementing the remaining parameter types. We are also working to remove the texture access. Are you thinking about some specific issue?

Created attachment 162880 [details] Patch

(In reply to comment #10) > Created an attachment (id=162880) [details] > Patch As a bonus, this one actually applies to upstream! There was a conflict in platformBlackBerry.cmake in the other patches.

Comment on attachment 162880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162880&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:573 > +#if ENABLE(CSS_SHADERS) This looks wrong. I think it's a bad merge...

Comment on attachment 162880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162880&action=review >> Source/WebKit/blackberry/Api/WebPage.cpp:573 >> +#if ENABLE(CSS_SHADERS) > > This looks wrong. I think it's a bad merge... This was actually how Josh put it in his patch. I think the reasoning was, if we don't have webgl, we won't have graphicscontext3d, and css shaders can't work. However, the build system should make sure to enable css_shaders only if webgl is on, so moving it out of te webgl #ifdef would make sense.

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 162142 [details] [details]) > > We plan to enable CSS shaders a little bit later when the implementation is updated to match the latest spec. I'll rename this bug slightly. > > I thought that we are following the spec closely. The remaining issues should be around the filter margins, the filter area used by the feCustomFilter and implementing the remaining parameter types. We are also working to remove the texture access. > > Are you thinking about some specific issue? I was thinking of "Renoving the texture access", specifically.