In this patch I will add the window.layoutTestController.overridePreference("WebKitCSSCustomFilterEnabled", "1"); and make the custom shader filter tests run on Chromium.
Created attachment 132432 [details] Patch V1
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 132432 [details] Patch V1 Attachment 132432 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11970376 New failing tests: css3/filters/custom/effect-custom-parameters.html css3/filters/custom/custom-filter-property-computed-style.html css3/filters/custom/custom-filter-shader-cache.html css3/filters/custom/effect-custom-combined-missing.html css3/filters/custom/custom-filter-property-parsing.html css3/filters/custom/missing-custom-filter-shader.html css3/filters/custom/effect-custom.html
Comment on attachment 132432 [details] Patch V1 Attachment 132432 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11960974 New failing tests: css3/filters/custom/effect-custom-parameters.html css3/filters/custom/custom-filter-property-computed-style.html css3/filters/custom/custom-filter-shader-cache.html css3/filters/custom/effect-custom-combined-missing.html css3/filters/custom/custom-filter-property-parsing.html css3/filters/custom/missing-custom-filter-shader.html css3/filters/custom/effect-custom.html
Comment on attachment 132432 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=132432&action=review > Source/WebKit/chromium/public/WebSettings.h:101 > + virtual void setExperimentalCSSCustomFilterEnabled(bool) = 0; Chromium WebKit API change LGTM
Created attachment 132730 [details] Patch V2 Updated the test results for linux.
Comment on attachment 132730 [details] Patch V2 One thing I've noticed (not new to this patch, but would be brought into Chrome with it) is that FECustomFilter.cpp introduces a dependency on Document.h. This is a layering violation, since the platform/ layer is not supposed to depend on files in dom/. You should probably either pass down the document's HostWindow directly (this is ok since HostWindow is in the platform/ layer), or for Chromium you can just pass a NULL HostWindow to GraphicsContext3D::create() for an offscreen context (not sure if you can do that in Safari, though). Similarly, the dependencies on CachedShader and StyleCachedShader are problematic, but I'm less sure how to remove them.
(In reply to comment #7) > (From update of attachment 132730 [details]) > One thing I've noticed (not new to this patch, but would be brought into Chrome with it) is that FECustomFilter.cpp introduces a dependency on Document.h. This is a layering violation, since the platform/ layer is not supposed to depend on files in dom/. You should probably either pass down the document's HostWindow directly (this is ok since HostWindow is in the platform/ layer), or for Chromium you can just pass a NULL HostWindow to GraphicsContext3D::create() for an offscreen context (not sure if you can do that in Safari, though). > In the end I will want to use the same GraphicsContext3D for all the shaders rendered in software, so that we can cache compiled shaders and mesh vertex/index buffers between elements. So it's just a matter of time until we have Document.h removed from this file. Is this something that can wait? I think it's also a good idea to use the same GraphicsContext3D that Skia is using. With that in place we can have a special implementation that will just use the texture allocated by Skia instead of reading it back from GPU just to move it back there. > Similarly, the dependencies on CachedShader and StyleCachedShader are problematic, but I'm less sure how to remove them. Thanks for catching this. We already fixed that dependency, but the headers are still included with no real reason. Do you mind if I remove those part of this patch?
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 132730 [details] [details]) > > One thing I've noticed (not new to this patch, but would be brought into Chrome with it) is that FECustomFilter.cpp introduces a dependency on Document.h. This is a layering violation, since the platform/ layer is not supposed to depend on files in dom/. You should probably either pass down the document's HostWindow directly (this is ok since HostWindow is in the platform/ layer), or for Chromium you can just pass a NULL HostWindow to GraphicsContext3D::create() for an offscreen context (not sure if you can do that in Safari, though). > > > In the end I will want to use the same GraphicsContext3D for all the shaders rendered in software, so that we can cache compiled shaders and mesh vertex/index buffers between elements. So it's just a matter of time until we have Document.h removed from this file. Is this something that can wait? > > I think it's also a good idea to use the same GraphicsContext3D that Skia is using. With that in place we can have a special implementation that will just use the texture allocated by Skia instead of reading it back from GPU just to move it back there. > Actually, I'm trying to fix that right now. What about passing the FrameView instead of the Document. Is that still a violation? I want to avoid passing a fixed value that might change without a notice (ie. layers moved to a different host window maybe).
Created attachment 132938 [details] Patch V3 1. Removed the Document.h and used the ScrollView.h instead 2. Removed references to CachedShader.h and StyleCachedShader.h as they were not used in that file anymore
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
(In reply to comment #11) > Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. It's the same API from before -> https://bugs.webkit.org/show_bug.cgi?id=81435#c5
> Actually, I'm trying to fix that right now. What about passing the FrameView instead of the Document. Is that still a violation? I want to avoid passing a fixed value that might change without a notice (ie. layers moved to a different host window maybe). FrameView would also be a layering violation.
Comment on attachment 132938 [details] Patch V3 ScrollView is in platform/ so it's not a layering violation, but it doesn't make very much sense Filters don't have anything to do with scrolling. Why not HostWindow? What are filters tied to - I'm assuming that they can't move between documents / windows?
(In reply to comment #14) > (From update of attachment 132938 [details]) > ScrollView is in platform/ so it's not a layering violation, but it doesn't make very much sense Filters don't have anything to do with scrolling. Why not HostWindow? > > What are filters tied to - I'm assuming that they can't move between documents / windows? Filters are tied to the RenderLayer, so I assumed that the RenderLayers will not be recreated when for example the HostWindow is changed, that's why I want to avoid giving the HostWindow to the filters. Anyway it seems like something is still wrong, because it creates the context and reuses it even though the HostWindow of the ScrollView might have changed, so I still need to find a way to invalidate the filters when that happens. I will also check if the HostWindow is needed at all when creating offscreen GraphicsContext3D's. On Chromium I understand that it is not needed, so I will check on other platforms. If it is not needed, I will just remove the reference and post a new patch.
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 132938 [details] [details]) > > ScrollView is in platform/ so it's not a layering violation, but it doesn't make very much sense Filters don't have anything to do with scrolling. Why not HostWindow? > > > > What are filters tied to - I'm assuming that they can't move between documents / windows? > > Filters are tied to the RenderLayer, so I assumed that the RenderLayers will not be recreated when for example the HostWindow is changed, that's why I want to avoid giving the HostWindow to the filters. > RenderLayers will be recreated when the HostWindow changes, so no problem there. > Anyway it seems like something is still wrong, because it creates the context and reuses it even though the HostWindow of the ScrollView might have changed, so I still need to find a way to invalidate the filters when that happens. > No you don't. > I will also check if the HostWindow is needed at all when creating offscreen GraphicsContext3D's. On Chromium I understand that it is not needed, so I will check on other platforms. If it is not needed, I will just remove the reference and post a new patch. It is not in Chromium. I'm not sure about other platforms.
(In reply to comment #8) > In the end I will want to use the same GraphicsContext3D for all the shaders rendered in software, so that we can cache compiled shaders and mesh vertex/index buffers between elements. So it's just a matter of time until we have Document.h removed from this file. Is this something that can wait? I'd prefer to fix it now, if possible. We have some code reorg plans that this dependency would prevent. > I think it's also a good idea to use the same GraphicsContext3D that Skia is using. With that in place we can have a special implementation that will just use the texture allocated by Skia instead of reading it back from GPU just to move it back there. Actually, I don't think this is a good idea. Skia makes assumptions about GL context state, which means you'd have to call a reset on Skia each time the custom filters changed GL state, and vice versa, which could impact performance. That said, it shouldn't be too much trouble to create a context for custom filter use which shares textures with the skia context. That way, you wouldn't need a readback. Just set "shareResources" to true in your GraphicsContext3D::Attributes (all contexts which set this flag are in the same share group). > > Similarly, the dependencies on CachedShader and StyleCachedShader are problematic, but I'm less sure how to remove them. > > Thanks for catching this. We already fixed that dependency, but the headers are still included with no real reason. Do you mind if I remove those part of this patch? That would be great.
Created attachment 133082 [details] Patch V4 In this patch I change the Document reference to HostWindow instead. As per previous comments the RenderLayers will get recreated anyway so there's no need to make sure the Filters are invalidated when the HostWindow changes. Also addressed all the other comments (removed unused headers from FECustomFilter.cpp).
Comment on attachment 133082 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=133082&action=review > LayoutTests/platform/chromium/test_expectations.txt:-3081 > -// CSS custom() filters are not currently supported > -BUGWK71392 SKIP : css3/filters/custom = FAIL > - Since your change only includes mac and linux results, I'd leave this line in, but change the SKIP to WIN. That way the win bots won't go red on your change.
Created attachment 133103 [details] Patch V5 Done. I've added the skip for Windows.
Comment on attachment 133103 [details] Patch V5 OK. Thanks for the extra effort. r=me
Comment on attachment 133103 [details] Patch V5 Clearing flags on attachment: 133103 Committed r111610: <http://trac.webkit.org/changeset/111610>
All reviewed patches have been landed. Closing bug.