Currently a FilterOperation is needed to request and get CachedSVGDocument resources. This need to be more generic and independent of filters.
Created attachment 219870 [details] Patch
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 on attachment 219870 [details] Patch Attachment 219870 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/48758036
Comment on attachment 219870 [details] Patch Attachment 219870 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/46198021
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?
(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.
Created attachment 219880 [details] Patch
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 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 on attachment 219880 [details] Patch Attachment 219880 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/51638040
Comment on attachment 219880 [details] Patch Attachment 219880 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/48298030
(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.
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.
Created attachment 221140 [details] Patch See previous comment
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.
Created attachment 221144 [details] Patch with style updates
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);
Created attachment 221161 [details] Patch for landing
Comment on attachment 221161 [details] Patch for landing Clearing flags on attachment: 221161 Committed r161967: <http://trac.webkit.org/changeset/161967>
All reviewed patches have been landed. Closing bug.
Looks like this caused a regression = see 127151.