[CI] Restructure SVGResourcesCycleSolver [chromium/src : master]
Fredrik Söderquist (Gerrit)
This change is ready for review.
To view, visit change 966661. To unsubscribe, or for help writing mail filters, visit settings.
Philip Rogers (Gerrit)
2 comments:
File third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.h:
Patch Set #1, Line 33:
class SVGResourcesCycleSolver {WYDT about adding a short class comment about how this works? It took me a few minutes to figure out what this is doing.
Something like:
// SVGResourcesCycleSolver breaks resource cycles. It maintains a list of active resources and
// traverses the resource graph looking for references that are already in the active list.
// After |ResolveCycles| is called, any resources with cycles will be removed.WDYT of refactoring this class to be a static function that, internally, creates the class and runs the cycle detection? I think that would make it more clear that this class should only be for a single run of cycle detection. (I initially thought you had introduced a bug because you forgot to clear active_resources_).
File third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.cpp:
Patch Set #1, Line 108:
if (auto* container_with_cycle = TraverseResources(resources_))TraverseResources returns the first container with a cycle which is then cleared. What if there are multiple containers with cycles? Don't we need to call resources_->ClearReferencesTo(...) for each container that has a cycle?
To view, visit change 966661. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
2 comments:
Patch Set #1, Line 33:
class SVGResourcesCycleSolver {
WYDT about adding a short class comment about how this works? It took me a few minutes to figure out […]
I'll look into doing that.
File third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.cpp:
Patch Set #1, Line 108:
if (auto* container_with_cycle = TraverseResources(resources_))
TraverseResources returns the first container with a cycle which is then cleared. […]
Yes, you're right. I guess this doesn't clean up as nicely as I'd initially hoped. Should add a test like that (because apparently there aren't one...)
To view, visit change 966661. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
2 comments:
File third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.h:
Patch Set #1, Line 33:
// This class traverses the graph formed by SVGResources of
I'll look into doing that.
I ended up keeping the stateful interface (for now [1]) and instead hoisting the side-effects. Some form of documentation added.
[1] To preserve the "DAG cache". I'm thinking of perhaps "distributing" this cache around the layout tree in which case the "stateless" interface might be possible again.
File third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.cpp:
Yes, you're right. I guess this doesn't clean up as nicely as I'd initially hoped. […]
Because of how we do cycle breaking [1] this turns out to be more non-obvious than one would think. Added a additional test regardless. (Maybe some day we'll have predictable/deterministic cycle breaking.)
[1] I.e when resolving cycles, there may not be a (noticeable) cycle because it has already been broken.
To view, visit change 966661. To unsubscribe, or for help writing mail filters, visit settings.
Fredrik Söderquist (Gerrit)
Fredrik Söderquist uploaded patch set #3 to this change.
[CI] Restructure SVGResourcesCycleSolver
Restructure the code so that traversing a LayoutObject's resources is
shared, and rename ResourceContainsCycles to TraverseResourceContainer.
Hoist the side-effects (clearing out resources with cycles) to the
caller.
Bug: 534817, 769774
Change-Id: Icd8200a2cd9073487479e32827a9fdeb1d8d04c4
---
A third_party/WebKit/LayoutTests/svg/custom/multiple-resource-cycles-expected.html
A third_party/WebKit/LayoutTests/svg/custom/multiple-resource-cycles.html
M third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp
M third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.cpp
M third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.h
5 files changed, 69 insertions(+), 39 deletions(-)
To view, visit change 966661. To unsubscribe, or for help writing mail filters, visit settings.
Philip Rogers (Gerrit)
Thanks for the explanations and update. LGTM
Patch set 4:Code-Review +1
Fredrik Söderquist (Gerrit)
Patch set 4:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
[CI] Restructure SVGResourcesCycleSolver
Restructure the code so that traversing a LayoutObject's resources is
shared, and rename ResourceContainsCycles to TraverseResourceContainer.
Hoist the side-effects (clearing out resources with cycles) to the
caller.
Bug: 534817, 769774
Change-Id: Icd8200a2cd9073487479e32827a9fdeb1d8d04c4
Reviewed-on: https://chromium-review.googlesource.com/966661
Reviewed-by: Philip Rogers <p...@chromium.org>
Commit-Queue: Fredrik Söderquist <f...@opera.com>
Cr-Commit-Position: refs/heads/master@{#544037}
---
A third_party/WebKit/LayoutTests/svg/custom/multiple-resource-cycles-expected.html
A third_party/WebKit/LayoutTests/svg/custom/multiple-resource-cycles.html
M third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp
M third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.cpp
M third_party/WebKit/Source/core/layout/svg/SVGResourcesCycleSolver.h
5 files changed, 71 insertions(+), 40 deletions(-)