Bug 126133 - Make CachedSVGDocument independent of CSS Filters
Summary: Make CachedSVGDocument independent of CSS Filters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-22 05:15 PST by Dirk Schulze
Modified: 2014-01-21 20:58 PST (History)
15 users (show)

See Also:


Attachments
Patch (14.29 KB, patch)
2013-12-22 05:26 PST, Dirk Schulze
koivisto: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (15.07 KB, patch)
2013-12-22 11:46 PST, Dirk Schulze
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (11.04 KB, patch)
2014-01-14 04:19 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (10.16 KB, patch)
2014-01-14 04:24 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch with style updates (10.16 KB, patch)
2014-01-14 05:13 PST, Dirk Schulze
koivisto: review+
Details | Formatted Diff | Diff
Patch for landing (9.94 KB, patch)
2014-01-14 07:20 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2013-12-22 05:15:10 PST
Currently a FilterOperation is needed to request and get CachedSVGDocument resources. This need to be more generic and independent of filters.
Comment 1 Dirk Schulze 2013-12-22 05:26:07 PST
Created attachment 219870 [details]
Patch
Comment 2 Build Bot 2013-12-22 06:04:27 PST
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
Comment 3 Build Bot 2013-12-22 06:56:12 PST
Comment on attachment 219870 [details]
Patch

Attachment 219870 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48758036
Comment 4 Build Bot 2013-12-22 07:39:11 PST
Comment on attachment 219870 [details]
Patch

Attachment 219870 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46198021
Comment 5 Antti Koivisto 2013-12-22 10:01:10 PST
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?
Comment 6 Dirk Schulze 2013-12-22 11:03:13 PST
(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.
Comment 7 Dirk Schulze 2013-12-22 11:46:21 PST
Created attachment 219880 [details]
Patch
Comment 8 WebKit Commit Bot 2013-12-22 11:49:03 PST
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.
Comment 9 Build Bot 2013-12-22 12:24:29 PST
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
Comment 10 Build Bot 2013-12-22 13:04:07 PST
Comment on attachment 219880 [details]
Patch

Attachment 219880 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/51638040
Comment 11 Build Bot 2013-12-22 13:20:11 PST
Comment on attachment 219880 [details]
Patch

Attachment 219880 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48298030
Comment 12 Antti Koivisto 2013-12-23 12:41:50 PST
(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.
Comment 13 Dirk Schulze 2014-01-14 04:19:44 PST
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.
Comment 14 Dirk Schulze 2014-01-14 04:24:41 PST
Created attachment 221140 [details]
Patch

See previous comment
Comment 15 WebKit Commit Bot 2014-01-14 04:26:08 PST
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.
Comment 16 Dirk Schulze 2014-01-14 05:13:09 PST
Created attachment 221144 [details]
Patch with style updates
Comment 17 Antti Koivisto 2014-01-14 06:47:09 PST
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);
Comment 18 Dirk Schulze 2014-01-14 07:20:27 PST
Created attachment 221161 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2014-01-14 07:54:14 PST
Comment on attachment 221161 [details]
Patch for landing

Clearing flags on attachment: 221161

Committed r161967: <http://trac.webkit.org/changeset/161967>
Comment 20 WebKit Commit Bot 2014-01-14 07:54:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Sam Weinig 2014-01-21 20:58:48 PST
Looks like this caused a regression = see 127151.