CustomFilterOperation references a StyleShader, which is outside of platform. We'll need to copy the way Images are referenced, and probably implement an Observer interface.
It's the other way around. FilterOperations are referenced from RenderStyle, so their place is not in platform code. FilterOperations look very much like FillLayers. FilterEffect (and its derivates) and FilterEffectRenderer are actual platform counterparts for the FilterOperations. The RenderLayer object will mix FilterOperations with platform code. This patch will move all the FilterOperations to rendering/style.
Created attachment 115555 [details] Patch v1
Comment on attachment 115555 [details] Patch v1 Attachment 115555 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10509132
(In reply to comment #1) > It's the other way around. FilterOperations are referenced from RenderStyle, so their place is not in platform code I don't agree. RenderStyle references TransformOperations, which is in platform/graphics. This seems very similar.
This is really only a problem for the CustomShaderOperation which needs to reference a cached resource, which is why TransformOperations never run into this. I guess we have a few options, including: - have CustomShaderOperation keep the references as an AtomicString (this is what the url() form of ReferenceFilterOperation does). The the builder of the filter could fire off the reference machinery. - implement an observer protocol between the shader resource and this operation, similar to the way Image and CachedImage work. - move the FilterOperation* stuff as the patch does. I don't know which is best. This move seems pretty easy.
Comment on attachment 115555 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=115555&action=review If we agree this is the correct approach, then r=me (but r- for forgetting the CMakefile change) > Source/WebCore/ChangeLog:73 > + * rendering/style/CustomFilterOperation.h: Renamed from Source/WebCore/platform/graphics/filters/CustomFilterOperation.h. > + (WebCore::CustomFilterOperation::create): > + (WebCore::CustomFilterOperation::setVertexShader): > + (WebCore::CustomFilterOperation::vertexShader): > + (WebCore::CustomFilterOperation::setFragmentShader): > + (WebCore::CustomFilterOperation::fragmentShader): > + (WebCore::CustomFilterOperation::operator==): > + (WebCore::CustomFilterOperation::CustomFilterOperation): > + * rendering/style/FilterOperation.h: Renamed from Source/WebCore/platform/graphics/filters/FilterOperation.h. > + (WebCore::FilterOperation::~FilterOperation): > + (WebCore::FilterOperation::operator!=): > + (WebCore::FilterOperation::getOperationType): > + (WebCore::FilterOperation::isSameType): > + (WebCore::FilterOperation::FilterOperation): > + (WebCore::ReferenceFilterOperation::create): > + (WebCore::ReferenceFilterOperation::reference): > + (WebCore::ReferenceFilterOperation::operator==): > + (WebCore::ReferenceFilterOperation::ReferenceFilterOperation): > + (WebCore::BasicColorMatrixFilterOperation::create): > + (WebCore::BasicColorMatrixFilterOperation::amount): > + (WebCore::BasicColorMatrixFilterOperation::operator==): > + (WebCore::BasicColorMatrixFilterOperation::BasicColorMatrixFilterOperation): > + (WebCore::BasicComponentTransferFilterOperation::create): > + (WebCore::BasicComponentTransferFilterOperation::amount): > + (WebCore::BasicComponentTransferFilterOperation::operator==): > + (WebCore::BasicComponentTransferFilterOperation::BasicComponentTransferFilterOperation): > + (WebCore::GammaFilterOperation::create): > + (WebCore::GammaFilterOperation::amplitude): > + (WebCore::GammaFilterOperation::exponent): > + (WebCore::GammaFilterOperation::offset): > + (WebCore::GammaFilterOperation::operator==): > + (WebCore::GammaFilterOperation::GammaFilterOperation): > + (WebCore::BlurFilterOperation::create): > + (WebCore::BlurFilterOperation::stdDeviationX): > + (WebCore::BlurFilterOperation::stdDeviationY): > + (WebCore::BlurFilterOperation::operator==): > + (WebCore::BlurFilterOperation::BlurFilterOperation): > + (WebCore::SharpenFilterOperation::create): > + (WebCore::SharpenFilterOperation::amount): > + (WebCore::SharpenFilterOperation::radius): > + (WebCore::SharpenFilterOperation::threshold): > + (WebCore::SharpenFilterOperation::operator==): > + (WebCore::SharpenFilterOperation::SharpenFilterOperation): > + (WebCore::DropShadowFilterOperation::create): > + (WebCore::DropShadowFilterOperation::x): > + (WebCore::DropShadowFilterOperation::y): > + (WebCore::DropShadowFilterOperation::stdDeviation): > + (WebCore::DropShadowFilterOperation::color): > + (WebCore::DropShadowFilterOperation::operator==): > + (WebCore::DropShadowFilterOperation::DropShadowFilterOperation): > + * rendering/style/FilterOperations.cpp: Renamed from Source/WebCore/platform/graphics/filters/FilterOperations.cpp. > + (WebCore::FilterOperations::FilterOperations): > + (WebCore::FilterOperations::operator==): > + * rendering/style/FilterOperations.h: Renamed from Source/WebCore/platform/graphics/filters/FilterOperations.h. > + (WebCore::FilterOperations::operator!=): > + (WebCore::FilterOperations::clear): > + (WebCore::FilterOperations::operations): > + (WebCore::FilterOperations::size): > + (WebCore::FilterOperations::at): I think we can remove all the (WebCore::) bits here since it is just a rename.
(In reply to comment #5) > This is really only a problem for the CustomShaderOperation which needs to reference a cached resource, which is why TransformOperations never run into this. I guess we have a few options, including: > > - have CustomShaderOperation keep the references as an AtomicString (this is what the url() form of ReferenceFilterOperation does). The the builder of the filter could fire off the reference machinery. > CustomShaderOperation is referenced from RenderStyle directly. If we do this one, it means that the CSSStyleSelector would load the specified file and create a CustomShaderOperation with AtomicString. CSSStyleSelector has a short life, so I doubt loading code can stay in that class. > - implement an observer protocol between the shader resource and this operation, similar to the way Image and CachedImage work. The observer in Image is used for a different reason. It is used to update animated images like GIFs or SVG. Moreover, it can be used to track the load of progressive JPEG. Anyway, the RenderStyle references a FillLayer that keeps a reference to the CachedImage. The RenderObjects that need to paint the images register themselves as observers. > > - move the FilterOperation* stuff as the patch does. > > I don't know which is best. This move seems pretty easy. I think TransformOperation should also be part of the rendering/style folder. FilterOperation clases are part of the style of an element. RenderLayer will take that from the style and create the platform classes that will actually do the rendering. In this case the platform code is FilterEffectRenderer, plus all the filter effects. Moreover, the RenderLayer will be an observer of the CachedShader, so that it will be able to apply the shader filter as soon as the content of the specified URL is loaded.
It definitely seems more simple to move the filter operations into style/rendering. I'm ok with it.
Created attachment 116948 [details] Patch for landing
Comment on attachment 116948 [details] Patch for landing Rejecting attachment 116948 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: atching file Source/WebCore/platform/graphics/filters/FilterOperations.h rm 'Source/WebCore/platform/graphics/filters/FilterOperations.h' patching file Source/WebCore/rendering/style/CustomFilterOperation.h patching file Source/WebCore/rendering/style/FilterOperation.h patching file Source/WebCore/rendering/style/FilterOperations.cpp patching file Source/WebCore/rendering/style/FilterOperations.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/10682385
Created attachment 116965 [details] Patch for landing
Comment on attachment 116965 [details] Patch for landing Clearing flags on attachment: 116965 Committed r101458: <http://trac.webkit.org/changeset/101458>
All reviewed patches have been landed. Closing bug.