Bug 72443

<rdar://problem/10451971>

*** Bug 75704 has been marked as a duplicate of this bug. ***

Unless there are any objections, I'd like to take this on. That said, I'm wading into areas of the code I know little about, so apologies in advance for the newbie questions. Starting from the loading side, it seems that there is already a CachedSVGDocument class that the loader knows about (equivalent of CachedImage; I'm making analogies to the Image case). I'm guessing that we could use this to load and reference the document from which the filters will be loaded. If the filters are in the current document (#bar), will the loader automatically handle that case, or should we special-case it to avoid double-parsing? What we don't seem to have is some kind of representation of an SVG document (or set of nodes?) in rendering/style/. Do we need this? Something like a StyleSVGDocument (equivalent to a StyleImage), and StyledCachedSVGDocument (equivalent to StyleCachedImage), in order to get feedback from the loader? Or perhaps StyleSVGFilter and StyleCachedSVGFilter, representing only the relevant filter network, extracted from the SVG document? Keeping it restricted to Filters could be a bit neater, but OTOH if there are other upcoming changes to allow other SVG nodes referenced from CSS, it might be useful to keep it more general. Over in css/, WebKitCSSFilterValue already seems to parse the url() syntax, so I presume there are no changes needed on the idl side. Can we just leave the URL as a string as far as the idl is concerned, or do we need to create another subclass here too (say, CSSSVGDocumentValue), to connect the url to the (proposed) StyleCachedSVGDocument, similar to how WebKitCSSShaderValue connects its url to a StyleCachedShader? Then over on the processing side, I was thinking of adding a RefPtr<Filter> to ReferenceFilterOperation, so that it could hold the SVG filter network representing that stage of the CSS filter chain. Thanks in advance for any comments you may have.

+mikelawther, to shine his CSS brilliance in the dark cave of my ignorance

OK, so I've gotten as far as parsing the filter URL, triggering a CachedResourceLoader to load me a CachedSVGDocument. I'm currently stuck on how to notify the element (or style?) that the load is complete. Presumably when I've figured that out, I trigger a SyntheticStyleChange, and create the appropriate filter network from the SVG nodes on a style change (handwave handwave). On the Image side, it seems this is done by making RenderObject a CachedImageClient (at least, I'm assuming that's how it works for background image styles, etc, but I'm not certain). This seems pretty Image-specific, though, and just hanging yet another CachedResourceClient subclass onto RenderObject seems pretty crude. One possible solution would be to make RenderObject derived from CachedResourceClient instead, so it could handle notifications from any CachedResource. I have no idea if that's really a workable solution (ie., if there are image-specific notifications which need to remain). Another thing I'm not too happy about: I'm paralleling the Image/Shader CSS style classes, which means I've created three style classes: StyleCachedSVGDocument, StylePendingSVGDocument, StyleSVGDocument. This seems pretty heavy-weight, unless it's there just to please some layering issues. It seems like it could be one class, which mutates from pending to cached as the style resolve completes, but perhaps I'm misunderstanding the purpose.

It appears you'll need something similar to StyleCachedImage to hold onto the CachedSVGDocument from the RenderStyle object. It's possible one might make the existing StylePendingImage stuff more generic so we can avoid loading unused svg filters.

Created attachment 143390 [details] Patch

This is a really early (raw) patch. There are no tests, there are some troublesome FIXMEs, and the setData()/data() thing on ReferenceFilterOperation is a big hack. It does load internally or externally-referenced SVG files, though, and it does produce pixels: it will process an SVG filter network in a CSS filter on load. I'm putting it up to get some feedback from people who know this area of the code better than me while I continue to refine it.

Comment on attachment 143390 [details] Patch Thanks for starting this. Yeah, it's a little annoying that so much has to be copied from the shader path.

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12777019

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12773050

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12771054

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12774056

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12776038

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12773068 New failing tests: css3/filters/filter-property-computed-style.html css3/filters/filter-property-parsing.html css3/filters/filter-property.html

Created attachment 143445 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12768068

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12772095 New failing tests: css3/filters/filter-property-computed-style.html css3/filters/filter-property-parsing.html css3/filters/filter-property.html

Created attachment 143459 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 143390 [details] Patch Attachment 143390 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12769101

Thanks Stephen for doing the important prototyping! I'll check it out in more detail during the day.

Created attachment 144386 [details] Patch

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12842273

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12852217

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12842277

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12848262

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12850234 New failing tests: css3/filters/filter-property-computed-style.html css3/filters/filter-property-parsing.html css3/filters/filter-property.html

Created attachment 144391 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #22) > Created an attachment (id=144386) [details] > Patch Another checkpoint: - added new source files to Mac build - put all relevant code behind ENABLE(SVG) (haven't tried to compile without it yet, but it should be almost there) - refactored all external resource loading into loadPendingResources() - removed m_filter and m_lastEffect from ReferenceFilterOperation (for now; may have to add it back for composited path later, but this patch will focus on software processing) - added plumbing to invalidate CSS style & redraw filters when SVG render nodes are invalidated: RenderSVGResourceContainer now maintains a list of RenderLayer clients

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12839397

Comment on attachment 144386 [details] Patch Attachment 144386 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12844313

Created attachment 144561 [details] Patch

(In reply to comment #32) > Created an attachment (id=144561) [details] > Patch Mostly EWS-food: - more fixes for Mac - removed spurious "url()" added by WebKitCSSSVGDocumentValue to fix two filters tests - fixed getComputedStyle for reference filters by retaining the full URL (for getComputedStyle) as well as the fragment (for node lookups)

Comment on attachment 144561 [details] Patch Attachment 144561 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12852464

Comment on attachment 144561 [details] Patch Attachment 144561 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12850494

Comment on attachment 144561 [details] Patch Attachment 144561 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12846562

Comment on attachment 144561 [details] Patch Attachment 144561 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12842573

Comment on attachment 144561 [details] Patch Attachment 144561 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12850540

Comment on attachment 144561 [details] Patch Attachment 144561 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12840684

Created attachment 144617 [details] Patch

Comment on attachment 144617 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144617&action=review > Source/WebCore/css/StyleResolver.cpp:5464 > + if (!cachedDocument) > + > + // Stash the CachedSVGDocument on the reference filter. > + referenceFilter->setData(cachedDocument); Something is missing here.

(In reply to comment #40) > Created an attachment (id=144617) [details] > Patch Getting closer now (not setting r? 'cos it still needs tests and polish) - fixed all build files (I think?) - ripped out StyleCachedSVGDocument/StyleSVGDocument/StylePendingSVGDocument (useless boilerplate); moved everything into WebKitCSSSVGDocumentValue

(In reply to comment #41) > (From update of attachment 144617 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144617&action=review > > > Source/WebCore/css/StyleResolver.cpp:5464 > > + if (!cachedDocument) > > + > > + // Stash the CachedSVGDocument on the reference filter. > > + referenceFilter->setData(cachedDocument); > > Something is missing here. Indeed. Thanks.

Comment on attachment 144617 [details] Patch Attachment 144617 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12848613

Created attachment 144628 [details] Patch

Comment on attachment 144628 [details] Patch Attachment 144628 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12839820

Comment on attachment 144628 [details] Patch Attachment 144628 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12841787

Comment on attachment 144628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144628&action=review EFL port doesn't enable CSS_FILTERS yet. So, could you consider it as I mention. > Source/WebCore/css/StyleResolver.cpp:5444 > +#if ENABLE(SVG) It looks loadPendingSVGDocuments() needs to use ENABLE(CSS_FILTERS) as well. > Source/WebCore/css/StyleResolver.cpp:5947 > +#if ENABLE(SVG) ditto. > Source/WebCore/css/StyleResolver.h:519 > + HashMap<FilterOperation*, WebKitCSSSVGDocumentValue*> m_pendingSVGDocuments; Don't you need to add FilterOperation class to namespace ? There is a build break on EFL port. WebKit/Source/WebCore/css/StyleResolver.h:520:13: error: ‘FilterOperation’ was not declared in this scope > Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:114 > + Don't you need to wrap CSS_FILTERS macro ? filterNeedsRepaint() is able to be enabled when CSS_FILTERS is ON. http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayer.h#L578 577 #if ENABLE(CSS_FILTERS) 578 virtual void filterNeedsRepaint(); 579 bool hasFilter() const { return renderer()->hasFilter(); } 580 #else

Created attachment 144805 [details] Speculative fix for EFL build

Created attachment 144934 [details] Patch

Comment on attachment 144934 [details] Patch Attachment 144934 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12861082

Created attachment 145162 [details] Patch

Comment on attachment 145162 [details] Patch Attachment 145162 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12862407

Created attachment 145174 [details] Patch

(In reply to comment #54) > Created an attachment (id=145174) [details] > Patch I think this patch is now ready for review. All known bugs have been fixed, and I've added tests which exercise the major operational modes. Dean, would you mind taking a look at the CSS and filters bits? Niko or Dirk, could you look at the SVG bits?

Comment on attachment 145174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145174&action=review > Source/WebCore/css/CSSParser.cpp:82 > +#if ENABLE(CSS_FILTERS) && ENABLE(SVG) > +#include "WebKitCSSSVGDocumentValue.h" > +#endif This should be in its own block below. > Source/WebCore/css/StyleResolver.cpp:5738 > + // FIXME: There's probably a better way to detect if this is > + // a fragment relative to this document. Some combination of completeURL and equalIgnoringFragmentIdentifier? SVGURIReference.cpp has some vaguely related code. > LayoutTests/css3/filters/effect-reference-ordering.html:1 > +<svg xmlns="http://www.w3.org/2000/svg" width="0" height="0" version="1.1" color-interpolation-filters="sRGB"> I don't think we do color-interpolation-filters yet; is this here for a reason? (https://bugs.webkit.org/show_bug.cgi?id=6033)

Comment on attachment 145174 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145174&action=review Thanks for your comments. >> Source/WebCore/css/CSSParser.cpp:82 >> +#endif > > This should be in its own block below. I moved it into the ENABLE(CSS_FILTERS) block for grouping and conciseness, but if you'd still prefer it in its own block, let me know. >> Source/WebCore/css/StyleResolver.cpp:5738 >> + // a fragment relative to this document. > > Some combination of completeURL and equalIgnoringFragmentIdentifier? SVGURIReference.cpp has some vaguely related code. Thanks! SVGURIReference::isExternalURIReference() seems to be just what I need. >> LayoutTests/css3/filters/effect-reference-ordering.html:1 >> +<svg xmlns="http://www.w3.org/2000/svg" width="0" height="0" version="1.1" color-interpolation-filters="sRGB"> > > I don't think we do color-interpolation-filters yet; is this here for a reason? (https://bugs.webkit.org/show_bug.cgi?id=6033) I was probably trying to get Firefox's results to look like ours, since we do sRGB by default and ignore this attribute. Removed for now.

Created attachment 145595 [details] Patch

(In reply to comment #58) > Created an attachment (id=145595) [details] > Patch Addresses Tim's comments, and changes the SVG morphology filter in the tests into a hue rotation (so it's easier to see what the correct results should be).

Created attachment 148438 [details] Patch

(In reply to comment #60) > Created an attachment (id=148438) [details] > Patch Updated to ToT.

Comment on attachment 148438 [details] Patch Attachment 148438 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12996090 New failing tests: css3/filters/effect-reference-ordering.html css3/filters/effect-reference-hw.html css3/filters/effect-reference-external.html css3/filters/effect-reference.html

Created attachment 148486 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 148586 [details] Fixed image results (minor pixel diffs due to intervening skia change)

Comment on attachment 148586 [details] Fixed image results (minor pixel diffs due to intervening skia change) View in context: https://bugs.webkit.org/attachment.cgi?id=148586&action=review Looking pretty good. Just some small points and questions. > Source/WebCore/ChangeLog:5 > + Implement filter url() function. > + https://bugs.webkit.org/show_bug.cgi?id=72443 > + Please add some description of the implementation here, at least at a high-level. I can read the details for each file below, but without much context. > Source/WebCore/ChangeLog:80 > + * platform/mac/ScrollbarThemeMac.mm: > + Fix mac build for the GraphicsLayer/PlatformLayer change. I guess you mean just include PlatformLayer.h? > Source/WebCore/Target.pri:1687 > + css/WebKitCSSSVGDocumentValue.h \ > css/WebKitCSSShaderValue.h \ Shouldn't the new PlatformLayer.h be listed in this file too? > Source/WebCore/WebCore.gypi:2585 > + 'css/WebKitCSSSVGDocumentValue.cpp', > + 'css/WebKitCSSSVGDocumentValue.h', > 'css/WebKitCSSTransformValue.cpp', ditto on PlatformLayer.h > Source/WebCore/css/StyleResolver.cpp:1801 > + // Start loading resources referenced by this style. > + loadPendingResources(); > > -#if ENABLE(CSS_SHADERS) > - // Start loading the shaders referenced by this style. > - loadPendingShaders(); > -#endif > - > + // Start loading all pending resources referenced by this style. This last comment is a duplicate. > Source/WebCore/css/StyleResolver.cpp:5676 > + if (!m_style->hasFilter() || m_pendingSVGDocuments.isEmpty()) > + return; This might show me up as a moron, but shouldn't this be && ? If there is no filter AND nothing pending? I understand the no filter bit. I assume this is because the item would never have been put into the pending list if it was already in the process of being loaded? > Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp:14 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE I think you have the wrong license here. > Source/WebCore/css/WebKitCSSSVGDocumentValue.h:15 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR ditto here > Source/WebCore/css/WebKitCSSSVGDocumentValue.h:49 > + WebKitCSSSVGDocumentValue(const String& url); Since you have inline functions above, any reason why you don't put the constructor here as well? I have zero feelings about this - just asking. Same with customCssText. > Source/WebCore/platform/graphics/filters/FilterOperation.h:190 > + void* m_data; It's annoying we can't use a CachedSVGDocument here :( > Source/WebCore/rendering/FilterEffectRenderer.cpp:129 > + if (!document) > + return 0; Would this case be anything other than an error? Worth putting an ASSERT? > Source/WebCore/rendering/FilterEffectRenderer.cpp:141 > + // FIXME: Figure out what to do with SourceAlpha. Right now, we're > + // using the alpha of the original input layer, which is obviously > + // wrong. We should probably be extracting the alpha from the > + // previousEffect, but this requires some more processing. > + // This may need a spec clarification. Good point. I'm not sure the input element alpha is definitely wrong, but this is the first time we've had this in a chain. > Source/WebCore/rendering/FilterEffectRenderer.cpp:352 > + if (filterOperation->getOperationType() != FilterOperation::REFERENCE) { > effect->inputEffects().append(previousEffect); > - m_effects.append(effect); > + m_effects.append(effect); Why is this skipped for url()? > Source/WebCore/rendering/RenderLayerFilterInfo.h:89 > + void updateReferenceFilter(ReferenceFilterOperation*); I don't see this used anywhere. Am I missing it? > LayoutTests/css3/filters/effect-reference-ordering.html:15 > +<img style="-webkit-filter: url(#blurY);" src="resources/reference.png"> > +<img style="-webkit-filter: contrast(500%) url(#blurY);" src="resources/reference.png"> > +<img style="-webkit-filter: url(#blurY) contrast(500%);" src="resources/reference.png"> could we add another that's shorthand + url + shorthand? > LayoutTests/css3/filters/resources/hueRotate.svg:5 > + <filter id="MyFilter"> > + <feColorMatrix type="hueRotate" values="180"/> > + </filter> Since we're replicating the filterBuilder functionality a bit, I think we also need a test that has a filter with nested elements and non-linear input (named inputs). > LayoutTests/platform/chromium-linux/css3/filters/effect-reference-expected.txt:10 > + [feColorMatrix type="HUEROTATE" values="180.00"] > + [SourceGraphic] Hmmm... I wonder if we should decorate our filtered elements like this too, or provide that option in WKTR?

Comment on attachment 148586 [details] Fixed image results (minor pixel diffs due to intervening skia change) View in context: https://bugs.webkit.org/attachment.cgi?id=148586&action=review >> Source/WebCore/platform/graphics/filters/FilterOperation.h:190 >> + void* m_data; > > It's annoying we can't use a CachedSVGDocument here :( Not just annoying, but it makes the code hard to understand and debug. When platform code needs a reference to something outside of platform, we normally do this by making an interface in platform that is implemented by something outside of it.

Comment on attachment 148586 [details] Fixed image results (minor pixel diffs due to intervening skia change) View in context: https://bugs.webkit.org/attachment.cgi?id=148586&action=review Thanks for the review. >> Source/WebCore/ChangeLog:5 >> + > > Please add some description of the implementation here, at least at a high-level. I can read the details for each file below, but without much context. Done. >> Source/WebCore/ChangeLog:80 >> + Fix mac build for the GraphicsLayer/PlatformLayer change. > > I guess you mean just include PlatformLayer.h? Actually, this file was relying on ImageBuffer.h to bring in GraphicsLayer.h. Since it no longer does (and only includes PlatformLayer.h), we have to #include it directly. Added this info to the ChangeLog. > Source/WebCore/Target.pri:1687 > css/WebKitCSSShaderValue.h \ Done. > Source/WebCore/WebCore.gypi:2585 > 'css/WebKitCSSTransformValue.cpp', Done. >> Source/WebCore/css/StyleResolver.cpp:1801 >> + // Start loading all pending resources referenced by this style. > > This last comment is a duplicate. Fixed, thanks. >> Source/WebCore/css/StyleResolver.cpp:5676 >> + return; > > This might show me up as a moron, but shouldn't this be && ? If there is no filter AND nothing pending? I understand the no filter bit. I assume this is because the item would never have been put into the pending list if it was already in the process of being loaded? No, I think you can have a filter, but no pending SVG documents (say, if you have no url() filters in the style, or if they're all internal references), in which case you have nothing to do. >> Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp:14 >> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > > I think you have the wrong license here. I've been told this is correct: 2-clause, Google in the (c) line, Apple in the license text. Most of the files in platform/graphics/chromium have this, for example. Or is there something else I got wrong? >> Source/WebCore/css/WebKitCSSSVGDocumentValue.h:49 >> + WebKitCSSSVGDocumentValue(const String& url); > > Since you have inline functions above, any reason why you don't put the constructor here as well? I have zero feelings about this - just asking. Same with customCssText. I tend to inline accessors, but not much else unless the profiler tells me to. Here's quote from a coworker smarter than me: --- Constructors and destructors are often significantly more complex than you think they are, especially if your class has any non-POD data members. Many STL classes have inlined constructors/destructors which may be copied into your function body. Because the bodies of these appear to be empty, they often seem like trivial functions that can safely be inlined. Don't give in to this temptation. Define them in the implementation file unless you really _need_ them to be inlined. Even if they do nothing now, someone could later add something seemingly-trivial to the class and make your hundreds of inlined destructors much more complex. For more information, read Item 30 in Effective C++. >>> Source/WebCore/platform/graphics/filters/FilterOperation.h:190 >>> + void* m_data; >> >> It's annoying we can't use a CachedSVGDocument here :( > > Not just annoying, but it makes the code hard to understand and debug. > > When platform code needs a reference to something outside of platform, we normally do this by making an interface in platform that is implemented by something outside of it. Yeah, I'm not a fan of this either, although I will note that the custom filter classes do this too. They just define CustomFilterProgram down in platform/graphics/filters, but still do an unconditional static_cast up to StyleCustomFilterProgram in CSS-land. I really would rather have this data stored in a hash table at a higher level, but I seem to be hamstrung by data hiding. StyleResolver knows about the WebKitCSSSVGDocumentValue (where the CachedSVGDocument lives), and uses it to load the SVG document, but RenderLayerFilterInfo and FilterEffectRenderer only get the FilterOperations, not the WebKitCSSSVGDocumentValue. This void* is only to get the pointer from one place to the other two, so there's no real interface to be defined down at the FilterOperation level, since it doesn't do anything with it. If there's a better way to do that, I'm definitely interested. >> Source/WebCore/rendering/FilterEffectRenderer.cpp:129 >> + return 0; > > Would this case be anything other than an error? Worth putting an ASSERT? Actually, this function can be called with an external document which has not been loaded yet, in which case cachedSVGDocument->document() is NULL. >> Source/WebCore/rendering/FilterEffectRenderer.cpp:352 >> + m_effects.append(effect); > > Why is this skipped for url()? buildReferenceFilter() adds all the FilterEffects it creates in the build loop, and connects it to its input in the call to SVGFilterBuilder::create(), so there's no need to do either here. >> Source/WebCore/rendering/RenderLayerFilterInfo.h:89 >> + void updateReferenceFilter(ReferenceFilterOperation*); > > I don't see this used anywhere. Am I missing it? Good catch. Removed. >> LayoutTests/css3/filters/effect-reference-ordering.html:15 >> +<img style="-webkit-filter: url(#blurY) contrast(500%);" src="resources/reference.png"> > > could we add another that's shorthand + url + shorthand? Done.

Created attachment 149050 [details] Patch

Comment on attachment 149050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149050&action=review Thanks Stephen. Small comments. > Source/WebCore/GNUmakefile.list.am:1760 > + Source/WebCore/css/WebKitCSSSVGDocumentValue.cpp \ > + Source/WebCore/css/WebKitCSSSVGDocumentValue.h \ > Source/WebCore/css/WebKitCSSShaderValue.cpp \ Should PlatformLayer.h be here too? (Missed this the first time) > Source/WebCore/WebCore.vcproj/WebCore.vcproj:37570 > + <File > + RelativePath="..\css\WebKitCSSShaderValue.cpp" And in this file too? > Source/WebCore/platform/graphics/filters/FilterOperation.h:190 > + void* m_data; This is the bit I'm most worried about, but as you mentioned, there are not any easy solutions. Maybe we could file a followup to clean this up? > Source/WebCore/rendering/FilterEffectRenderer.cpp:141 > + // FIXME: Figure out what to do with SourceAlpha. Right now, we're > + // using the alpha of the original input layer, which is obviously > + // wrong. We should probably be extracting the alpha from the > + // previousEffect, but this requires some more processing. > + // This may need a spec clarification. Could you file a spec bug on this and link from here?

Comment on attachment 149050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149050&action=review Thanks again for your diligence. > Source/WebCore/GNUmakefile.list.am:1760 > Source/WebCore/css/WebKitCSSShaderValue.cpp \ Done. >> Source/WebCore/WebCore.vcproj/WebCore.vcproj:37570 >> + RelativePath="..\css\WebKitCSSShaderValue.cpp" > > And in this file too? Done. >> Source/WebCore/platform/graphics/filters/FilterOperation.h:190 >> + void* m_data; > > This is the bit I'm most worried about, but as you mentioned, there are not any easy solutions. Maybe we could file a followup to clean this up? Done. https://bugs.webkit.org/show_bug.cgi?id=90213 >> Source/WebCore/rendering/FilterEffectRenderer.cpp:141 >> + // This may need a spec clarification. > > Could you file a spec bug on this and link from here? Sure, but I can't seem to find it at https://www.w3.org/Bugs/Public/enter_bug.cgi?product=FXTF (seems to only have the Web Animations spec?)

Created attachment 150030 [details] Patch for landing

Comment on attachment 150030 [details] Patch for landing Clearing flags on attachment: 150030 Committed r121513: <http://trac.webkit.org/changeset/121513>

All reviewed patches have been landed. Closing bug.