Summary: | Make CachedSVGDocument independent of CSS Filters | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | allan.jensen, buildbot, commit-queue, dino, esprehn+autocc, glenn, gyuyoung.kim, japhet, kling, koivisto, kondapallykalyan, macpherson, menard, rniwa, sabouhallawa, sam | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=127151 | ||||||||||||||||
Attachments: |
|
Description
Dirk Schulze
2013-12-22 05:15:10 PST
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. |