Bug 126133 – Make CachedSVGDocument independent of CSS Filters

• Component: CSS
• Status: RESOLVED FIXED

[Dirk Schulze]

Currently a FilterOperation is needed to request and get CachedSVGDocument resources. This need to be more generic and independent of filters.

[Dirk Schulze]

Created attachment 219870 [details]
Patch

[Build Bot]

Comment on attachment 219870 [details]
Patch

Attachment 219870 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48888009

[Build Bot]

Comment on attachment 219870 [details]
Patch

Attachment 219870 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48758036

[Build Bot]

Comment on attachment 219870 [details]
Patch

Attachment 219870 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46198021

[Antti Koivisto]

Comment on attachment 219870 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219870&action=review

> Source/WebCore/loader/SVGDocumentConsumer.h:46
> +class SVGDocumentConsumer {
> +public:
> +    CachedSVGDocumentReference* cachedSVGDocumentReference() const { return m_cachedSVGDocumentReference.get(); }
> +    void setCachedSVGDocumentReference(PassOwnPtr<CachedSVGDocumentReference> cachedSVGDocumentReference) { m_cachedSVGDocumentReference = cachedSVGDocumentReference; }
> +
> +private:
> +    OwnPtr<CachedSVGDocumentReference> m_cachedSVGDocumentReference;
> +};    

This class is just a pointer. I don't see why we are introducing it and making code more abstract. Also name seem off. It doesn't 'consume' anything.

We use unique_ptr not OwnPtr.

> Source/WebCore/platform/graphics/filters/FilterOperation.h:160
> +class ReferenceFilterOperation : public FilterOperation, public SVGDocumentConsumer {

Why is ReferenceFilterOperation inheriting SVGDocumentConsumer instead of having one as a member?

[Dirk Schulze]

(In reply to comment #5)
> (From update of attachment 219870 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219870&action=review
> 
> > Source/WebCore/loader/SVGDocumentConsumer.h:46
> > +class SVGDocumentConsumer {
> > +public:
> > +    CachedSVGDocumentReference* cachedSVGDocumentReference() const { return m_cachedSVGDocumentReference.get(); }
> > +    void setCachedSVGDocumentReference(PassOwnPtr<CachedSVGDocumentReference> cachedSVGDocumentReference) { m_cachedSVGDocumentReference = cachedSVGDocumentReference; }
> > +
> > +private:
> > +    OwnPtr<CachedSVGDocumentReference> m_cachedSVGDocumentReference;
> > +};    
> 
> This class is just a pointer. I don't see why we are introducing it and making code more abstract. Also name seem off. It doesn't 'consume' anything.

I think there is a misunderstanding. I do not try to fix the layer violation with this patch. Right now, CachedSVGDocumentReference can just be used by FilterOperation (specifically ReferencedFilterOperation). We have more situations like clip-path, SVG patterns and gradients that can be referenced from external resources as well. These operators need access to CachedSVGDocumentReference as well. See next paragraph...

> 
> We use unique_ptr not OwnPtr.

Takes some time to adapt to "new" C++ rules. Thanks.

> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.h:160
> > +class ReferenceFilterOperation : public FilterOperation, public SVGDocumentConsumer {
> 
> Why is ReferenceFilterOperation inheriting SVGDocumentConsumer instead of having one as a member?

The abstraction SVGDocumentConsumer is used to access the CachedSVGDocumentReference. All inheriting classes are "consuming" CachedSVGDocumentReference that is why I think SVGDocumentConsumer is the right name.

[Dirk Schulze]

Created attachment 219880 [details]
Patch

[WebKit Commit Bot]

Attachment 219880 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', u'Source/WebCore/loader/SVGDocumentConsumer.h', u'Source/WebCore/platform/graphics/filters/FilterOperation.cpp', u'Source/WebCore/platform/graphics/filters/FilterOperation.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/loader/SVGDocumentConsumer.h:38:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 219880 [details]
Patch

Attachment 219880 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/50938286

[Build Bot]

Comment on attachment 219880 [details]
Patch

Attachment 219880 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/51638040

[Build Bot]

Comment on attachment 219880 [details]
Patch

Attachment 219880 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48298030

[Antti Koivisto]

(In reply to comment #6)
> > This class is just a pointer. I don't see why we are introducing it and making code more abstract. Also name seem off. It doesn't 'consume' anything.
> 
> I think there is a misunderstanding. I do not try to fix the layer violation with this patch. Right now, CachedSVGDocumentReference can just be used by FilterOperation (specifically ReferencedFilterOperation). We have more situations like clip-path, SVG patterns and gradients that can be referenced from external resources as well. These operators need access to CachedSVGDocumentReference as well. See next paragraph...

I didn't say anything about layer violation (which is bad too). It just doesn't make sense to me to introduce a class that is nothing but a unique_ptr. You could replace the whole things with a typedef (not that you should). Usually something like this means that the design is off.

> > Why is ReferenceFilterOperation inheriting SVGDocumentConsumer instead of having one as a member?
> 
> The abstraction SVGDocumentConsumer is used to access the CachedSVGDocumentReference. All inheriting classes are "consuming" CachedSVGDocumentReference that is why I think SVGDocumentConsumer is the right name.

That didn't really answer my question. 

SVGDocumentConsumer does not do any consuming. It is a smart pointer without any additional functionality. Would you inherit from unique_ptr?

The SVG implementation is full of unnecessary abstractions which contribute to it being so hard to follow. Lets not add more.

[Dirk Schulze]

Created attachment 221139 [details]
Patch for landing

This patch uses a different approach. CachedSVGDocumentReference is now responsible for requesting the SVG document.

Note to the original code:

-            if (SVGURIReference::isExternalURIReference(svgDocumentValue->url(), m_state.document())) {
-                if (!svgDocumentValue->loadRequested())
-                    m_state.pendingSVGDocuments().set(operation.get(), svgDocumentValue);
-                else if (svgDocumentValue->cachedSVGDocument())
-                    operation->setCachedSVGDocumentReference(adoptPtr(new CachedSVGDocumentReference(svgDocumentValue->cachedSVGDocument())));
-            }

I wrote a bunch of test cases and the else if clause was never called. Independent if element style, document style... setting filter for a collection of elements.

[Dirk Schulze]

Created attachment 221140 [details]
Patch

See previous comment

[WebKit Commit Bot]

Attachment 221140 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', u'Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp', u'Source/WebCore/loader/cache/CachedSVGDocumentReference.h', u'Source/WebCore/platform/graphics/filters/FilterOperation.cpp', u'Source/WebCore/platform/graphics/filters/FilterOperation.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/loader/cache/CachedSVGDocumentReference.cpp:55:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Created attachment 221144 [details]
Patch with style updates

[Antti Koivisto]

Comment on attachment 221144 [details]
Patch with style updates

View in context: https://bugs.webkit.org/attachment.cgi?id=221144&action=review

r=me

> Source/WebCore/css/StyleResolver.cpp:3433
> +    for (auto it = state.pendingSVGDocuments().begin(), end = state.pendingSVGDocuments().end(); it != end; ++it)
> +        (*it)->load(cachedResourceLoader);

Please use range-for syntax:

for (auto pendingDocument : state.pendingSVGDocuments())

> Source/WebCore/loader/cache/CachedSVGDocumentReference.h:44
> -    CachedSVGDocumentReference(CachedSVGDocument*);
> +    static std::unique_ptr<CachedSVGDocumentReference> create(const String& url)
> +    {
> +        return std::unique_ptr<CachedSVGDocumentReference>(new CachedSVGDocumentReference(url));
> +    }

No need to add create(). Please just keep the constructor public and use std::make_unique<CachedSVGDocumentReference>(url);

[Dirk Schulze]

Created attachment 221161 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 221161 [details]
Patch for landing

Clearing flags on attachment: 221161

Committed r161967: <http://trac.webkit.org/changeset/161967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Looks like this caused a regression = see 127151.