Bug 72539 – FilterOperation* should stay in rendering/style, because it is directly referenced from RenderStyle

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

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.

[Alexandru Chiculita]

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.

[Alexandru Chiculita]

Created attachment 115555 [details]
Patch v1

[Gyuyoung Kim]

Comment on attachment 115555 [details]
Patch v1

Attachment 115555 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10509132

[Simon Fraser (smfr)]

(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.

[Dean Jackson]

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.

[Dean Jackson]

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.

[Alexandru Chiculita]

(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.

[Dean Jackson]

It definitely seems more simple to move the filter operations into style/rendering. I'm ok with it.

[Alexandru Chiculita]

Created attachment 116948 [details]
Patch for landing

[WebKit Review Bot]

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

[Alexandru Chiculita]

Created attachment 116965 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 116965 [details]
Patch for landing

Clearing flags on attachment: 116965

Committed r101458: <http://trac.webkit.org/changeset/101458>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.