Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(541)

Issue 298873003: SVG: SVGAnimateElement should not cache |m_animatedElements| (Closed)

Created:
6 years, 7 months ago by kouhei (in TOK)
Modified:
6 years, 7 months ago
Reviewers:
pdr., esprehn
CC:
blink-reviews, ed+blinkwatch_opera.com, shans, rjwright, Mike Lawther (Google), blink-reviews-animation_chromium.org, rwlbuis, fs, kouhei+svg_chromium.org, dstockwell, Timothy Loh, krit, f(malita), gyuyoung.kim_webkit.org, darktears, Stephen Chennney, Steve Block, Eric Willigers
Visibility:
Public.

Description

SVG: SVGAnimateElement should not cache |m_animatedElements| Before this change, SVGAnimateElement cached resolved element instances in |m_animatedElements|. However, this is not updated synchronously when <use> shadow dom/instance tree is torn down or new instances are added. The patch changes SVGAnimateElement to resolve its target instances on the fly when needed. As |m_animatedElements| are no longer kept, we do not guarantee that animationStarted hook is called before each animation. animationEnded hook is guaranteed to be called for elements stayed in the tree when the animation ended, but it may not be called for the animated element |removedFrom| the document tree while animation. BUG=369860 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174923

Patch Set 1 #

Patch Set 2 : maybe target observer is optional? #

Patch Set 3 : minimized_change #

Patch Set 4 : added LayoutTests #

Patch Set 5 : remove_unused_line #

Total comments: 10

Patch Set 6 : address comments / remove old JSC specific hack #

Patch Set 7 : fix crash #

Patch Set 8 : rebased #

Total comments: 2

Patch Set 9 : rebased #

Patch Set 10 : replace SMIL specific logic -> exclude <use> #

Patch Set 11 : remove notification from detach #

Total comments: 1

Patch Set 12 : revert change on SVGElementInstance.cpp #

Total comments: 1

Patch Set 13 : don't cache m_animatedElements #

Patch Set 14 : remove m_isAnimating flag and new |isAnimating()| impl #

Patch Set 15 : update comments #

Patch Set 16 : remove more asserts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -43 lines) Patch
A LayoutTests/svg/dom/remove-use-target-element-indirectly.html View 1 2 3 4 5 1 chunk +46 lines, -0 lines 0 comments Download
A + LayoutTests/svg/dom/remove-use-target-element-indirectly-expected.txt View 1 2 3 1 chunk +7 lines, -1 line 0 comments Download
A LayoutTests/svg/dom/resources/svg-with-animate-use.svg View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/svg/SVGAnimateElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/svg/SVGAnimateElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +10 lines, -14 lines 0 comments Download
M Source/core/svg/SVGAnimatedAngle.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGAnimatedInteger.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/svg/SVGAnimatedNumber.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/svg/SVGAnimatedTypeAnimator.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -3 lines 0 comments Download
M Source/core/svg/SVGStaticStringList.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/svg/SVGStaticStringList.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/svg/properties/SVGAnimatedProperty.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +6 lines, -9 lines 0 comments Download
M Source/core/svg/properties/SVGAnimatedProperty.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +1 line, -11 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
kouhei (in TOK)
The patch is ready for review now. Would you take a look? esprehn: I think ...
6 years, 7 months ago (2014-05-23 02:07:32 UTC) #1
kouhei (in TOK)
+cc: oilpan-reviews JFYI
6 years, 7 months ago (2014-05-23 02:09:39 UTC) #2
esprehn
https://codereview.chromium.org/298873003/diff/70001/LayoutTests/svg/dom/remove-use-target-element-indirectly.html File LayoutTests/svg/dom/remove-use-target-element-indirectly.html (right): https://codereview.chromium.org/298873003/diff/70001/LayoutTests/svg/dom/remove-use-target-element-indirectly.html#newcode1 LayoutTests/svg/dom/remove-use-target-element-indirectly.html:1: <script src="../../resources/js-test.js"></script> Missing <!DOCTYPE html> https://codereview.chromium.org/298873003/diff/70001/LayoutTests/svg/dom/remove-use-target-element-indirectly.html#newcode27 LayoutTests/svg/dom/remove-use-target-element-indirectly.html:27: setTimeout(moveFromLocalToOther, 0); ...
6 years, 7 months ago (2014-05-23 04:38:17 UTC) #3
kouhei (in TOK)
PTAL. https://codereview.chromium.org/298873003/diff/70001/LayoutTests/svg/dom/remove-use-target-element-indirectly.html File LayoutTests/svg/dom/remove-use-target-element-indirectly.html (right): https://codereview.chromium.org/298873003/diff/70001/LayoutTests/svg/dom/remove-use-target-element-indirectly.html#newcode1 LayoutTests/svg/dom/remove-use-target-element-indirectly.html:1: <script src="../../resources/js-test.js"></script> On 2014/05/23 04:38:17, esprehn wrote: > ...
6 years, 7 months ago (2014-05-23 07:17:14 UTC) #4
kouhei (in TOK)
PTAL. The tests are passing.
6 years, 7 months ago (2014-05-24 00:59:14 UTC) #5
pdr.
Synchronously tearing down the generated use tree seems like the right approach, but the SMIL-specific ...
6 years, 7 months ago (2014-05-24 06:13:06 UTC) #6
kouhei (in TOK)
On 2014/05/24 06:13:06, pdr wrote: > Synchronously tearing down the generated use tree seems like ...
6 years, 7 months ago (2014-05-26 03:44:57 UTC) #7
kouhei (in TOK)
On 2014/05/24 06:13:06, pdr wrote: > Synchronously tearing down the generated use tree seems like ...
6 years, 7 months ago (2014-05-26 03:44:57 UTC) #8
kouhei (in TOK)
PTAL. On 2014/05/26 03:44:57, kouhei wrote: > On 2014/05/24 06:13:06, pdr wrote: > > Synchronously ...
6 years, 7 months ago (2014-05-26 04:35:44 UTC) #9
pdr.
> On 2014/05/24 06:13:07, pdr wrote: > > Is this needed? > Yes. The element ...
6 years, 7 months ago (2014-05-27 00:45:32 UTC) #10
kouhei (in TOK)
On 2014/05/27 00:45:32, pdr wrote: > > On 2014/05/24 06:13:07, pdr wrote: > > > ...
6 years, 7 months ago (2014-05-27 01:34:31 UTC) #11
pdr.
Kouhei and I talked offline for a bit and he is going to explore a ...
6 years, 7 months ago (2014-05-27 03:08:01 UTC) #12
kouhei (in TOK)
On 2014/05/27 03:08:01, pdr wrote: > Kouhei and I talked offline for a bit and ...
6 years, 7 months ago (2014-05-27 03:33:43 UTC) #13
kouhei (in TOK)
> For most part, it is only used for sanity check so it should be ...
6 years, 7 months ago (2014-05-27 03:34:36 UTC) #14
esprehn
That won't work then, you'd need to force synchronization of the flag before reading isAnimating(). ...
6 years, 7 months ago (2014-05-27 03:39:27 UTC) #15
kouhei (in TOK)
On 2014/05/27 03:39:27, esprehn wrote: > That won't work then, you'd need to force synchronization ...
6 years, 7 months ago (2014-05-27 03:52:28 UTC) #16
pdr.
LGTM! This is the right approach. Please update the change description before landing. I think ...
6 years, 7 months ago (2014-05-27 04:47:46 UTC) #17
kouhei (in TOK)
On 2014/05/27 04:47:46, pdr wrote: > LGTM! This is the right approach. Please update the ...
6 years, 7 months ago (2014-05-27 05:32:22 UTC) #18
esprehn
ah, this looks good. thanks! lgtm
6 years, 7 months ago (2014-05-27 20:31:10 UTC) #19
kouhei (in TOK)
The CQ bit was checked by kouhei@chromium.org
6 years, 7 months ago (2014-05-28 01:09:54 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/298873003/230009
6 years, 7 months ago (2014-05-28 01:10:13 UTC) #21
commit-bot: I haz the power
6 years, 7 months ago (2014-05-28 02:40:35 UTC) #22
Message was sent while issue was closed.
Change committed as 174923

Powered by Google App Engine
This is Rietveld 408576698