Bug 72539 - FilterOperation* should stay in rendering/style, because it is directly referenced from RenderStyle
Summary: FilterOperation* should stay in rendering/style, because it is directly refer...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 13:17 PST by Dean Jackson
Modified: 2011-11-30 00:58 PST (History)
4 users (show)

See Also:


Attachments
Patch v1 (60.51 KB, patch)
2011-11-17 03:44 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch for landing (61.85 KB, patch)
2011-11-29 04:59 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch for landing (61.82 KB, patch)
2011-11-29 07:29 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2011-11-16 13:17:29 PST
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.
Comment 1 Alexandru Chiculita 2011-11-17 00:57:28 PST
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.
Comment 2 Alexandru Chiculita 2011-11-17 03:44:14 PST
Created attachment 115555 [details]
Patch v1
Comment 3 Gyuyoung Kim 2011-11-17 03:52:24 PST
Comment on attachment 115555 [details]
Patch v1

Attachment 115555 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10509132
Comment 4 Simon Fraser (smfr) 2011-11-18 11:42:28 PST
(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.
Comment 5 Dean Jackson 2011-11-18 12:19:25 PST
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 6 Dean Jackson 2011-11-18 12:22:06 PST
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.
Comment 7 Alexandru Chiculita 2011-11-23 03:09:51 PST
(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.
Comment 8 Dean Jackson 2011-11-28 23:43:35 PST
It definitely seems more simple to move the filter operations into style/rendering. I'm ok with it.
Comment 9 Alexandru Chiculita 2011-11-29 04:59:40 PST
Created attachment 116948 [details]
Patch for landing
Comment 10 WebKit Review Bot 2011-11-29 07:19:48 PST
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
Comment 11 Alexandru Chiculita 2011-11-29 07:29:16 PST
Created attachment 116965 [details]
Patch for landing
Comment 12 WebKit Review Bot 2011-11-30 00:57:56 PST
Comment on attachment 116965 [details]
Patch for landing

Clearing flags on attachment: 116965

Committed r101458: <http://trac.webkit.org/changeset/101458>
Comment 13 WebKit Review Bot 2011-11-30 00:58:01 PST
All reviewed patches have been landed.  Closing bug.