Implement url() filter function

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

Implement the url (reference to SVG filter) filter function.

[Radar WebKit Bug Importer]

<rdar://problem/10451971>

[Stephen White]

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

[Stephen White]

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.

[Stephen White]

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

[Stephen White]

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.

[Eric Seidel (no email)]

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.

[Stephen White]

Created attachment 143390 [details]
Patch

[Stephen White]

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.

[Dean Jackson]

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.

[Build Bot]

Comment on attachment 143390 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 143390 [details]
Patch

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

[Build Bot]

Comment on attachment 143390 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 143390 [details]
Patch

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

[Build Bot]

Comment on attachment 143390 [details]
Patch

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Comment on attachment 143390 [details]
Patch

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Gyuyoung Kim]

Comment on attachment 143390 [details]
Patch

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

[Nikolas Zimmermann]

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

[Stephen White]

Created attachment 144386 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 144386 [details]
Patch

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

[Build Bot]

Comment on attachment 144386 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144386 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144386 [details]
Patch

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Stephen White]

(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

[Build Bot]

Comment on attachment 144386 [details]
Patch

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

[Build Bot]

Comment on attachment 144386 [details]
Patch

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

[Stephen White]

Created attachment 144561 [details]
Patch

[Stephen White]

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

[Build Bot]

Comment on attachment 144561 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144561 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144561 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 144561 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 144561 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 144561 [details]
Patch

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

[Stephen White]

Created attachment 144617 [details]
Patch

[Simon Fraser (smfr)]

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.

[Stephen White]

(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

[Stephen White]

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

[Build Bot]

Comment on attachment 144617 [details]
Patch

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

[Stephen White]

Created attachment 144628 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 144628 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 144628 [details]
Patch

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

[Gyuyoung Kim]

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

[Stephen White]

Created attachment 144805 [details]
Speculative fix for EFL build

[Stephen White]

Created attachment 144934 [details]
Patch

[Build Bot]

Comment on attachment 144934 [details]
Patch

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

[Stephen White]

Created attachment 145162 [details]
Patch

[Build Bot]

Comment on attachment 145162 [details]
Patch

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

[Stephen White]

Created attachment 145174 [details]
Patch

[Stephen White]

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

[Tim Horton]

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)

[Stephen White]

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.

[Stephen White]

Created attachment 145595 [details]
Patch

[Stephen White]

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

[Stephen White]

Created attachment 148438 [details]
Patch

[Stephen White]

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

Updated to ToT.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Stephen White]

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

[Dean Jackson]

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?

[Simon Fraser (smfr)]

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.

[Stephen White]

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.

[Stephen White]

Created attachment 149050 [details]
Patch

[Dean Jackson]

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?

[Stephen White]

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?)

[Stephen White]

Created attachment 150030 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 150030 [details]
Patch for landing

Clearing flags on attachment: 150030

Committed r121513: <http://trac.webkit.org/changeset/121513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.