Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by fs Modified:
4 years, 1 month ago Reviewers:
pdr., esprehn CC:
ajuma+watch-canvas_chromium.org, darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-style_chromium.org, Rik, chromium-reviews, dglazkov+blink, dshwang, krit, eae+blinkwatch, f(malita), gavinp+loader_chromium.org, gyuyoung2, haraken, Nate Chapin, jchaffraix+rendering, Justin Novosad, kouhei+svg_chromium.org, leviw+renderwatch, loading-reviews+fetch_chromium.org, pdr+svgwatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, sof, szager+layoutwatch_chromium.org, tyoshino+watch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTracking reference filter mutation via SVGElementProxy
This introduces SVGElementProxy - a new piece with the functionality of
DocumentResourceReference and the ReferenceFilterBuilder merged. It
provides the means to track clients of a certain element (only
SVGFilterElements for now, but will likely be extended to other types if
it ends up sticking.) An SVGElementProxy is created, and primarily owned,
by CSSURIValue. The proxy also handles loading of a resource document, if
requested.
Clients are SVGResourceClients, like before, with methods/callbacks
renamed. Some of the old functionality of SVGResourceClient has either
been moved to clients, to the proxy or been replaced with different
solutions.
Mutations to the element/subtree is signaled separately from any
potential changes to the actual reference (anything that might invalidate
the element reference.)
BUG=439970
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991
Cr-Commit-Position: refs/heads/master@{#430550}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Change caching avoidance; fix (restore) canvas add/remove order. #Patch Set 4 : Rebase; fix DCHECK #Patch Set 5 : Rebase #
Total comments: 30
Patch Set 6 : Touchups #
Total comments: 4
Patch Set 7 : Extend documentation for SVGElementProxy #Patch Set 8 : referenceChanged -> proxiedElementChanged #
Total comments: 16
Patch Set 9 : Rewrite SVGElementProxy to eliminate generations; Other review feedback. #Patch Set 10 : Fix fullscreen #
Total comments: 11
Patch Set 11 : Rebase; Don't register TreeScope clients on external documents; Comment fixups #Patch Set 12 : Rebase; Add lifecycle checks #Patch Set 13 : Tweak lifecycle checks #
Total comments: 4
Patch Set 14 : Fix auto usage; drop possibly incorrect DCHECK. #Patch Set 15 : Remove DeprecatedScheduleStyleRecalcDuringLayout include #Patch Set 16 : Rebase #
Created: 4 years, 1 month ago
Messages
Total messages: 102 (76 generated)
Description was changed from ========== Tracking filter mutation via SVGElementProxy Remove ReferenceFilterBuilder. Don't keep PaintLayerFilterInfo around when no filter will be painted. Drop DocumentResourceReference. SVGElementProxy: SVGElementProxy as ResourceDocumentClient. Make the DocumentResource handling a detail internal to SVGElementProxy. BUG=439970 ========== to ========== Tracking filter mutation via SVGElementProxy Remove ReferenceFilterBuilder. Don't keep PaintLayerFilterInfo around when no filter will be painted. Drop DocumentResourceReference. SVGElementProxy: SVGElementProxy as ResourceDocumentClient. Make the DocumentResource handling a detail internal to SVGElementProxy. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Tracking filter mutation via SVGElementProxy Remove ReferenceFilterBuilder. Don't keep PaintLayerFilterInfo around when no filter will be painted. Drop DocumentResourceReference. SVGElementProxy: SVGElementProxy as ResourceDocumentClient. Make the DocumentResource handling a detail internal to SVGElementProxy. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. Clients are SVGResourceClients, like before, with methods/callbacks renamed. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. Clients are SVGResourceClients, like before, with methods/callbacks renamed. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) In the latter case, the generation of the proxy is bumped. This generation (id) is then used to check for changes in the element reference. This unfortunately still require caching to be disabled for these ComputedStyle objects. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) In the latter case, the generation of the proxy is bumped. This generation (id) is then used to check for changes in the element reference. This unfortunately still require caching to be disabled for these ComputedStyle objects. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, in a somewhat transparent fashion, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) In the latter case, the generation of the proxy is bumped. This generation (id) is then used to check for changes in the element reference. This unfortunately still require caching to be disabled for these ComputedStyle objects. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
fs@opera.com changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
I think I've noodled over this long enough, and it seems to settling as to what "smarts" I manage to come up with... so time to hit the fan. I've tried to shrink this to the bare essentials only, but it's still fairly large... I left out the removal of the "hack" code, but it should be dead with this change (i.e not called.) Will remove that if and when this has landed and stuck. Feel free to add additional reviewers as you see fit, I didn't think it'd be wise to have a too long list of reviewers given the size of this change.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:118: if (!computedStyle->hasFilter()) Looking at the comment about images in loadPendingImages, we can request a bunch of images but only end up needing one. For example: <style> div { background-image: url(a.png); } div { background-image: url(b.png); } div { background-image: none; } </style> <div></div> My understanding of the new code is that we'd create a bunch of SVGElementProxies but only resolve the used ones? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127: referenceOperation.elementProxy().resolve(*m_document); If the same url is requested multiple times by different references, will we create a request for each? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:136: SVGElementProxy* elementProxy = Is the CSSURIValue lifetime guaranteed to last as long as the filter operations? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h:89: void invalidateProxyClients(InvalidationMode); Is InvalidationMode needed (yet)? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/FilterOperation.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FilterOperation.h:164: String m_url; Is this url still needed or can we rely on m_elementProxy which also has the url (temporarily)? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:125: for (SVGElementProxy* proxy : m_elementProxies) Should we save off the proxies, clear them, then issue reference change notifications? This would protect against the case that a new proxy is synchronously added during this loop. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:57: // tracker. This also seems to be called from notifyFinished https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter) incremented with each change to the proxy reference. Are there any security issues here if a mean user spams proxies until this overflows? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:73: void notifyFinished(Resource*) override; Can you add a comment here about how this is called when the referenced document loads? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:76: void nextGeneration() { m_generation++; } Bikeshed nit: incrementCacheGeneration? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:82: String m_url; Can you add a comment about how this will be cleared after the request completes? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:90: class SVGElementProxyTracker : public GarbageCollected<SVGElementProxyTracker> { This name confused me for a bit because I thought it was more than just a set+helper. WDYT of renaming to just SVGElementProxySet? (further suggestions below, none required). https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:92: void attachElementProxy(SVGElementProxy&); Maybe just "add(SVGElementProxy&)"? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:97: void invalidateProxyClients(); Maybe just "notifyThatContentChanged"? https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGResourceClient.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGResourceClient.h:20: virtual void resourceReferenceChanged(SVGElementProxy*) = 0; Are the SVGElementProxy* arguments needed? Would it be safe to use them since the proxy could be freed when this is called?
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:118: if (!computedStyle->hasFilter()) On 2016/10/20 at 03:11:04, pdr. wrote: > Looking at the comment about images in loadPendingImages, we can request a bunch of images but only end up needing one. For example: > <style> > div { background-image: url(a.png); } > div { background-image: url(b.png); } > div { background-image: none; } > </style> > <div></div> > > My understanding of the new code is that we'd create a bunch of SVGElementProxies but only resolve the used ones? That is correct, yes. The option would be to create some form of placeholder (like StylePendingImage) and return that, but seeing that an (unresolved) proxy isn't too heavy I've opted against doing that. (It might also be worth noting that a CSSURIValue will only create one proxy and cache it, so createElementProxy will always return the same object. (For now, see first parenthesis below.) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/ElementStyleResources.cpp:127: referenceOperation.elementProxy().resolve(*m_document); On 2016/10/20 at 03:11:04, pdr. wrote: > If the same url is requested multiple times by different references, will we create a request for each? No. After the proxy is resolved the first time, this is a no-op. (Barring a fix for crbug.com/405315, which would require a reset of that state.) If multiple CSSURIValues reference the same resource (non-local document), the MemoryCache would provide the same Resource. (Excepting fetching restrictions which prevent re-use.) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/FilterOperationResolver.cpp:136: SVGElementProxy* elementProxy = On 2016/10/20 at 03:11:05, pdr. wrote: > Is the CSSURIValue lifetime guaranteed to last as long as the filter operations? Both have strong references, so technically speaking the lifetime of the CSSURIValue is irrelevant. (But in most, if not all, cases it is likely to outlive the latter.) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.h:89: void invalidateProxyClients(InvalidationMode); On 2016/10/20 at 03:11:05, pdr. wrote: > Is InvalidationMode needed (yet)? No. Dropped (for now.) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/FilterOperation.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/FilterOperation.h:164: String m_url; On 2016/10/20 at 03:11:05, pdr. wrote: > Is this url still needed or can we rely on m_elementProxy which also has the url (temporarily)? This is the URL we need for getComputedStyle. It's the unresolved/relative URL, while the one that is (temporarily) in m_elementProxy is a resolved/absolute URL. (This is another thing that will likely change when crbug.com/405315 is fixed.) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:125: for (SVGElementProxy* proxy : m_elementProxies) On 2016/10/20 at 03:11:05, pdr. wrote: > Should we save off the proxies, clear them, then issue reference change notifications? This would protect against the case that a new proxy is synchronously added during this loop. Sure. Reworked. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:57: // tracker. On 2016/10/20 at 03:11:05, pdr. wrote: > This also seems to be called from notifyFinished That's kind of an implementation detail that ought to be transparent to users of this interface. Technically speaking it can also be considered covered by "the id of an element changed". I'll add it to the list of events though. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter) incremented with each change to the proxy reference. On 2016/10/20 at 03:11:05, pdr. wrote: > Are there any security issues here if a mean user spams proxies until this overflows? What you can achieve by that is to make two operations compare equal when they should not. That would make the client(s) reference the wrong proxy/element (== failed invalidations), but nothing more severe that that I can think of. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:73: void notifyFinished(Resource*) override; On 2016/10/20 at 03:11:05, pdr. wrote: > Can you add a comment here about how this is called when the referenced document loads? Done. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:76: void nextGeneration() { m_generation++; } On 2016/10/20 at 03:11:05, pdr. wrote: > Bikeshed nit: incrementCacheGeneration? Made it incrementGeneration (there's nothing "cache" here really.) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:82: String m_url; On 2016/10/20 at 03:11:05, pdr. wrote: > Can you add a comment about how this will be cleared after the request completes? Done. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:90: class SVGElementProxyTracker : public GarbageCollected<SVGElementProxyTracker> { On 2016/10/20 at 03:11:05, pdr. wrote: > This name confused me for a bit because I thought it was more than just a set+helper. WDYT of renaming to just SVGElementProxySet? > > (further suggestions below, none required). Renamed to SVGElementProxySet. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:92: void attachElementProxy(SVGElementProxy&); On 2016/10/20 at 03:11:05, pdr. wrote: > Maybe just "add(SVGElementProxy&)"? Done. https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGElementProxy.h:97: void invalidateProxyClients(); On 2016/10/20 at 03:11:05, pdr. wrote: > Maybe just "notifyThatContentChanged"? Done (minus "That".) https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGResourceClient.h (right): https://codereview.chromium.org/2401343002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGResourceClient.h:20: virtual void resourceReferenceChanged(SVGElementProxy*) = 0; On 2016/10/20 at 03:11:05, pdr. wrote: > Are the SVGElementProxy* arguments needed? Would it be safe to use them since the proxy could be freed when this is called? I guess they aren't strictly needed (yet. Will be if/when a client is registered - and cares about - multiple proxies.) I can drop them for now. They are safe because the reference is GCd and on the stack.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Very nice! A couple more requests for comments to help future readers. I think this is ready to go but would like someone more familiar with CSS loading/resources/etc to review before landing. I'd recommend alancutter or esprehn who touched similar code in ElementStyleResources. https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:20: // A proxy to an SVGElement. Allows access to an element with a certain 'id', Do you mind expanding this comment to describe the overall design? These are the notes I took while reviewing. Maybe something along these lines? * SVGElements can be referenced using CSS (filter: url(foo.com/bar.svg#baz;) which may require loading external resources. SVGElementProxy is a persistent reference to an SVGElement both before and after the SVGElement has been found. * Filtered content has |CSSURIValue|s which create |SVGElementProxy|s which also maintain references back to the filtered content via SVGResourceClient. * SVGElementProxy looks up SVGElements by id (for remote resources, it will request the SVGDocument and lookup by id there). * When SVGElementProxy finds an SVGElement, it tells the SVGElement which in turn keeps a list of |SVGElementProxy|s referencing it. * These bi-directional references (filtered content <-> SVGElementProxy and SVGElementProxy <-> SVGFilterElement) are used for invalidation, change notifications, etc. https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter) incremented with each change to the proxy reference. Nit: "reference" was a little confusing to me when reviewing this because the proxy both references and is referenced. I don't have a great suggestion for making this clearer though. Maybe "proxied element" (and proxiedElementChanged(), above)?
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fs@opera.com changed reviewers: + esprehn@chromium.org
On 2016/10/20 at 19:03:42, pdr wrote: > Very nice! A couple more requests for comments to help future readers. Thanks! Comments further touched up/reworked/extended. > I think this is ready to go but would like someone more familiar with CSS loading/resources/etc to review before landing. I'd recommend alancutter or esprehn who touched similar code in ElementStyleResources. +esprehn for a look at these things. https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:20: // A proxy to an SVGElement. Allows access to an element with a certain 'id', On 2016/10/20 at 19:03:42, pdr. wrote: > Do you mind expanding this comment to describe the overall design? > > These are the notes I took while reviewing. Maybe something along these lines? > * SVGElements can be referenced using CSS (filter: url(foo.com/bar.svg#baz;) which may require loading external resources. SVGElementProxy is a persistent reference to an SVGElement both before and after the SVGElement has been found. > * Filtered content has |CSSURIValue|s which create |SVGElementProxy|s which also maintain references back to the filtered content via SVGResourceClient. > * SVGElementProxy looks up SVGElements by id (for remote resources, it will request the SVGDocument and lookup by id there). > * When SVGElementProxy finds an SVGElement, it tells the SVGElement which in turn keeps a list of |SVGElementProxy|s referencing it. > * These bi-directional references (filtered content <-> SVGElementProxy and SVGElementProxy <-> SVGFilterElement) are used for invalidation, change notifications, etc. Thanks, I tried to jot something down that I hope included all of the above. https://codereview.chromium.org/2401343002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:63: // number (or counter) incremented with each change to the proxy reference. On 2016/10/20 at 19:03:42, pdr. wrote: > Nit: "reference" was a little confusing to me when reviewing this because the proxy both references and is referenced. I don't have a great suggestion for making this clearer though. Maybe "proxied element" (and proxiedElementChanged(), above)? I tried to replace "reference" with "proxied element" (or add it for disambiguation) here and in a few other places. Also renamed the method (and the corresponding callback on SVGResourceClient.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pdr@chromium.org changed reviewers: - chrishtr@chromium.org
LGTM here, but please wait for a second review from someone more familiar with CSS resource tracking such as esprehn or alancutter.
This doesn't remove the usage of scheduleSVGFilterLayerUpdateHack() in the paint invalidation code. I'm worried we're depending on the bad side effects there, how will this patch remove that hack? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSURIValue.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSURIValue.cpp:19: SVGElementProxy* CSSURIValue::createElementProxy(Document& document) const { ensureElementProxy, this returns the same value every time after the first call. it should probably actually return a reference? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp:139: if (style.hasFilter() && style.filter().hasReferenceFilter()) Hmm, can you explain why you need to turn off matched properties caching here? I wonder why we need a strong reference inside the style value. It seems like the style should setup observations on the Document for things with the referenced filter id? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.cpp:3087: lastFilterEffect(); This is strange that it has side effects. Can we refactor it to call whatever code has the side effects directly instead? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/FilterOperation.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/FilterOperation.cpp:81: m_elementProxyGeneration == other.m_elementProxyGeneration; What changes the generation? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/FilterOperation.h (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/FilterOperation.h:167: unsigned m_elementProxyGeneration; Why do we need a generation instead of a single dirty bit? I'd prefer some setNeedsFilterUpdate() bit check instead of incrementing counters https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:41: ThreadState::current()->registerPreFinalizer(this); What about this feature needs a preFinalizer? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:93: SVGElement* element(TreeScope&); getElement or findElement? https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:113: unsigned generation() const { return m_generation; } I'd prefer a dirty bit specific to the subtree or something, my experience is the generation model is very hard to debug
(Haven't uploaded new PS, will do that when I've had the time to do something about the more structural change requests.) On 2016/10/25 at 01:18:43, esprehn wrote: > This doesn't remove the usage of scheduleSVGFilterLayerUpdateHack() in the paint invalidation code. I'm worried we're depending on the bad side effects there, how will this patch remove that hack? All the calls leading to that method is removed in this CL. I'd planned to remove the code in a separate CL to avoid bloating this one even more. It's a mere cherry-pick away though. Hopefully we should be able to migrate all resource invalidation to the same type of mechanism. https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/CSSURIValue.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/CSSURIValue.cpp:19: SVGElementProxy* CSSURIValue::createElementProxy(Document& document) const { On 2016/10/25 at 01:18:42, esprehn wrote: > ensureElementProxy, this returns the same value every time after the first call. it should probably actually return a reference? Sure, done. https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/MatchedPropertiesCache.cpp:139: if (style.hasFilter() && style.filter().hasReferenceFilter()) On 2016/10/25 at 01:18:42, esprehn wrote: > Hmm, can you explain why you need to turn off matched properties caching here? Because mutations to the DOM tree does not reflect in the MatchResults. I guess optionally this could be handled out-of-band of the style system by having clients register id observers? > I wonder why we need a strong reference inside the style value. It seems like the style should setup observations on the Document for things with the referenced filter id? There's only a strong reference to the proxy (and back.) The proxy is essentially the "observation" mechanism. I guess for the actual target element we could use IdTargetObservers and do away with the generation mechanism. https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.cpp:3087: lastFilterEffect(); On 2016/10/25 at 01:18:42, esprehn wrote: > This is strange that it has side effects. Can we refactor it to call whatever code has the side effects directly instead? Sure. That would mean that every time this is called we'd have to redo it though. lastFilterEffect() has some notion of validity, so it can avoid that. Note that this is nothing new in this CL though, rather just removing the indirection. Another option would be to call mapRect on the FilterEffect returned by lastFilterEffect(). https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/FilterOperation.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/FilterOperation.cpp:81: m_elementProxyGeneration == other.m_elementProxyGeneration; On 2016/10/25 at 01:18:42, esprehn wrote: > What changes the generation? All invalidations of the proxy (SVGElementProxy::proxiedElementChanged). https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/FilterOperation.h (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/FilterOperation.h:167: unsigned m_elementProxyGeneration; On 2016/10/25 at 01:18:42, esprehn wrote: > Why do we need a generation instead of a single dirty bit? I'd prefer some setNeedsFilterUpdate() bit check instead of incrementing counters (see similar comment in SVGElementProxy.h) https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:41: ThreadState::current()->registerPreFinalizer(this); On 2016/10/25 at 01:18:42, esprehn wrote: > What about this feature needs a preFinalizer? It's for calling removeClient on m_document (balancing the addClient on the line above here.) Given that ResourceClients are now weak, I suppose it's not strictly needed (registration without unregistration feels kind of iffy though.) https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:93: SVGElement* element(TreeScope&); On 2016/10/25 at 01:18:42, esprehn wrote: > getElement or findElement? Done. https://codereview.chromium.org/2401343002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:113: unsigned generation() const { return m_generation; } On 2016/10/25 at 01:18:42, esprehn wrote: > I'd prefer a dirty bit specific to the subtree or something, my experience is the generation model is very hard to debug This mechanism is only here for the case of the target element changing. With a different mechanism (IdTargetObserver?) I think it'd be possible to eliminate this. I don't see where we'd be served by a dirty bit, since there's no single point of validation where we'd be resetting it.
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, in a somewhat transparent fashion, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) In the latter case, the generation of the proxy is bumped. This generation (id) is then used to check for changes in the element reference. This unfortunately still require caching to be disabled for these ComputedStyle objects. BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've reworked things to try and address esprehn's comments. Primarily by using an IdTargetObserver to track the 'id' per tree scope. This allowed removing the style caching disablement. Client registration is more complicated than I'd like though. PTAL.
https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.h (left): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.h:476: void scheduleSVGFilterLayerUpdateHack(Element&); Woooooooooooooohoooooooooooooo!!! https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.cpp:3076: lastFilterEffect(); I agree with your and Elliott's review comments about this being a little weird. Your idea of calling mapRect on the FilterEffect sounded good to me. WDYT of adding a TODO above lastFilterEffect to clean this up? https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: Can you think of any race situations from using IdTargetObserver? For example, are the notifications limited to certain document lifecycle phases that could be asserted for safety? https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:17: class SVGElementProxy::IdObserver : public IdTargetObserver { I didn't know about IdTargetObserver... Would this also be useful for other SVG by-id tracking? https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:43: // For same document references, the proxy will track changes that could affect Is this comment still up to date with the IdTargetObserver change? For example, it looks like external document mutation is supported now (though not used)? https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:47: // et.c.) nit: etc.
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.cpp:3076: lastFilterEffect(); On 2016/11/02 at 06:35:22, pdr. wrote: > I agree with your and Elliott's review comments about this being a little weird. Your idea of calling mapRect on the FilterEffect sounded good to me. WDYT of adding a TODO above lastFilterEffect to clean this up? After a somewhat related discussion with senorblanco, I have some other ideas for how this could end up looking. Adding a general TODO (like above.) https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: On 2016/11/02 at 06:35:22, pdr. wrote: > Can you think of any race situations from using IdTargetObserver? For example, are the notifications limited to certain document lifecycle phases that could be asserted for safety? IIRC, Element::updateId is the primary hook (which is called by various DOM mutation methods), so I guess one can't say there's any explicit lifecycle dependencies? Implicitly though I guess we would expect DocumentLifecycle::stateAllowsTreeMutations() to be true, so maybe that would be something worth asserting? From the other side of things, I think we need <= Compositing or so. https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:17: class SVGElementProxy::IdObserver : public IdTargetObserver { On 2016/11/02 at 06:35:22, pdr. wrote: > I didn't know about IdTargetObserver... Would this also be useful for other SVG by-id tracking? Yes, I did get the same idea. I'll look in to it (if I remember.) https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.h (right): https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:43: // For same document references, the proxy will track changes that could affect On 2016/11/02 at 06:35:22, pdr. wrote: > Is this comment still up to date with the IdTargetObserver change? For example, it looks like external document mutation is supported now (though not used)? Added code to make the comment correct again. https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.h:47: // et.c.) On 2016/11/02 at 06:35:22, pdr. wrote: > nit: etc. Fixed.
@esprehn, can you give this another review too? > https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: > On 2016/11/02 at 06:35:22, pdr. wrote: > > Can you think of any race situations from using IdTargetObserver? For example, are the notifications limited to certain document lifecycle phases that could be asserted for safety? > > IIRC, Element::updateId is the primary hook (which is called by various DOM mutation methods), so I guess one can't say there's any explicit lifecycle dependencies? Implicitly though I guess we would expect DocumentLifecycle::stateAllowsTreeMutations() to be true, so maybe that would be something worth asserting? From the other side of things, I think we need <= Compositing or so. Those asserts SGTM. Could you add something along those lines?
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/02 at 20:51:30, pdr wrote: > @esprehn, can you give this another review too? > > > > https://codereview.chromium.org/2401343002/diff/180001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:16: > > On 2016/11/02 at 06:35:22, pdr. wrote: > > > Can you think of any race situations from using IdTargetObserver? For example, are the notifications limited to certain document lifecycle phases that could be asserted for safety? > > > > IIRC, Element::updateId is the primary hook (which is called by various DOM mutation methods), so I guess one can't say there's any explicit lifecycle dependencies? Implicitly though I guess we would expect DocumentLifecycle::stateAllowsTreeMutations() to be true, so maybe that would be something worth asserting? From the other side of things, I think we need <= Compositing or so. > > Those asserts SGTM. Could you add something along those lines? Added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/02 at 20:51:30, pdr wrote: > @esprehn, can you give this another review too? esprehn, could you PTA(nother)L?
lgtm w/ some nits and comments. I think that lifecycle assert might be wrong? https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:56: DCHECK(lifecycle().state() <= DocumentLifecycle::CompositingClean || I'm not sure this holds true, can't something like elementFromPoint() cause CompositingClean but not PaintClean and then you mutate the DOM which notifies this observer? https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:168: if (auto observer = m_observers.get(&treeScope)) We usually do auto* or auto& to make it clear you're not making a copy
LGTM here too
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGElementProxy.cpp (right): https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:56: DCHECK(lifecycle().state() <= DocumentLifecycle::CompositingClean || On 2016/11/04 at 23:58:07, esprehn wrote: > I'm not sure this holds true, can't something like elementFromPoint() cause CompositingClean but not PaintClean and then you mutate the DOM which notifies this observer? Since both of these are not strict less/greater than I think that should be handled, no? I guess we can settle for only the first DCHECK though, since it ought to be strict enough. https://codereview.chromium.org/2401343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGElementProxy.cpp:168: if (auto observer = m_observers.get(&treeScope)) On 2016/11/04 at 23:58:07, esprehn wrote: > We usually do auto* or auto& to make it clear you're not making a copy Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2401343002/#ps280001 (title: "Remove DeprecatedScheduleStyleRecalcDuringLayout include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 Cr-Commit-Position: refs/heads/master@{#430550} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 Cr-Commit-Position: refs/heads/master@{#430550}
Message was sent while issue was closed.
On 2016/11/08 at 08:50:36, commit-bot wrote: > Patchset 15 (id:??) landed as https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 > Cr-Commit-Position: refs/heads/master@{#430550} Thanks Fredrik! Rest in peace, scheduleSVGFilterLayerUpdateHack
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2482353002/ by pdr@chromium.org. The reason for reverting is: There are several reports of issues from this patch. Lets go ahead and roll out for now. BUG=663362,663444,663473,663362.
Message was sent while issue was closed.
On 2016/11/09 at 00:47:08, pdr wrote: > A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2482353002/ by pdr@chromium.org. > > The reason for reverting is: There are several reports of issues from this patch. Lets go ahead and roll out for now. > > BUG=663362,663444,663473,663362. Thanks. I've been suffering from limited (network) access recently, which has severely reduced my response time =(.
Description was changed from ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 Cr-Commit-Position: refs/heads/master@{#430550} ========== to ========== Reland of "Tracking reference filter mutation via SVGElementProxy" This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 Cr-Commit-Position: refs/heads/master@{#430550} ==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/09 at 00:47:08, pdr wrote: > A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2482353002/ by pdr@chromium.org. > > The reason for reverting is: There are several reports of issues from this patch. Lets go ahead and roll out for now. > > BUG=663362,663444,663473,663362. Looks like all of these was the same underlying cause - a bug in client unregistration. Hopefully fixed now (SVGElementProxy::removeClient), and I simplified local vs. remote handling a bit in addClient/removeClient while at it. Added two tests based on the bug reports (different crash stacks so figured it'd be good to keep one of each just in case.) Permission to reland?
On 2016/11/09 at 17:25:47, fs wrote: > On 2016/11/09 at 00:47:08, pdr wrote: > > A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2482353002/ by pdr@chromium.org. > > > > The reason for reverting is: There are several reports of issues from this patch. Lets go ahead and roll out for now. > > > > BUG=663362,663444,663473,663362. > > Looks like all of these was the same underlying cause - a bug in client unregistration. Hopefully fixed now (SVGElementProxy::removeClient), and I simplified local vs. remote handling a bit in addClient/removeClient while at it. Added two tests based on the bug reports (different crash stacks so figured it'd be good to keep one of each just in case.) > > Permission to reland? LGTM to reland. Do you mind creating a new review for it though, and post a link here? Sorry about making you go through the CQ twice :( I think the two reviews will help keep this separated if it has to be rolled out again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/09 at 18:42:23, pdr wrote: > On 2016/11/09 at 17:25:47, fs wrote: > > On 2016/11/09 at 00:47:08, pdr wrote: > > > A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2482353002/ by pdr@chromium.org. > > > > > > The reason for reverting is: There are several reports of issues from this patch. Lets go ahead and roll out for now. > > > > > > BUG=663362,663444,663473,663362. > > > > Looks like all of these was the same underlying cause - a bug in client unregistration. Hopefully fixed now (SVGElementProxy::removeClient), and I simplified local vs. remote handling a bit in addClient/removeClient while at it. Added two tests based on the bug reports (different crash stacks so figured it'd be good to keep one of each just in case.) > > > > Permission to reland? > > LGTM to reland. Do you mind creating a new review for it though, and post a link here? > > Sorry about making you go through the CQ twice :( I think the two reviews will help keep this separated if it has to be rolled out again. Done here: https://codereview.chromium.org/2490163002
Message was sent while issue was closed.
Description was changed from ========== Reland of "Tracking reference filter mutation via SVGElementProxy" This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 Cr-Commit-Position: refs/heads/master@{#430550} ========== to ========== Tracking reference filter mutation via SVGElementProxy This introduces SVGElementProxy - a new piece with the functionality of DocumentResourceReference and the ReferenceFilterBuilder merged. It provides the means to track clients of a certain element (only SVGFilterElements for now, but will likely be extended to other types if it ends up sticking.) An SVGElementProxy is created, and primarily owned, by CSSURIValue. The proxy also handles loading of a resource document, if requested. Clients are SVGResourceClients, like before, with methods/callbacks renamed. Some of the old functionality of SVGResourceClient has either been moved to clients, to the proxy or been replaced with different solutions. Mutations to the element/subtree is signaled separately from any potential changes to the actual reference (anything that might invalidate the element reference.) BUG=439970 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d36f8a8c9c4b7d757c0d8832e624be80c4465991 Cr-Commit-Position: refs/heads/master@{#430550} ==========
Message was sent while issue was closed.
Patchset #17 (id:320001) has been deleted |
