SVG: no need to call |clearAnimatedType| at SVGAnimate{,Motion}Element dtor.

SVG: no need to call |clearAnimatedType| at SVGAnimate{,Motion}Element dtor.

The deleted code was used to ensure the ASSERT(!isAnimating()) on ~SVGAnimatedProperty.
The assert checks that animation is always stopped when the element is dead. However, the assert is removed now, so we don't need to ensure it.

The deleted code |clearAnimatedType| in ~SVGAnimateElement was triggered when a document was unloaded and its <animate> element was destructed before its target svg element. This stopped animation on the target svg elements before the ASSERT run.

BUG=370834
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175989

[kouhei (in TOK), 2014-06-09 06:51:20]

PTAL.

oilpan-reviews: This removes the last FIXME comments from SVG Node hierarchy.

[haraken, 2014-06-09 07:10:23]

On 2014/06/09 06:51:20, kouhei wrote:
> PTAL.
> 
> oilpan-reviews: This removes the last FIXME comments from SVG Node hierarchy.

Discussed offline; and we decided to think about this CL more.

[kouhei (in TOK), 2014-06-09 10:03:51]

On 2014/06/09 07:10:23, haraken wrote:
> On 2014/06/09 06:51:20, kouhei wrote:
> > PTAL.
> > 
> > oilpan-reviews: This removes the last FIXME comments from SVG Node
hierarchy.
> 
> Discussed offline; and we decided to think about this CL more.

I think this CL is correct. Even in the case of <use> tree.

haraken@ was worrying that <use> tree triggering removeChildren() may not always
invoke removedFrom.
I double checked the code and |Container::removeChildren()| is guaranteed to
call |removedFrom|:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The only case left was from SVGElementInstance holding the last reference to
this <animate{,Motion}>, but the SVGElementInstance tree is gone since r175681.

[kouhei (in TOK), 2014-06-09 10:04:59]

> haraken@ was worrying that <use> tree triggering removeChildren() may not
always
> invoke removedFrom.
> I double checked the code and |Container::removeChildren()| is guaranteed to
> call |removedFrom|:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
FYI: It's |ContainerNode::detach()| that which do not always call removedFrom()
on its children.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[rwlbuis, 2014-06-09 16:53:58]

On 2014/06/09 10:04:59, kouhei wrote:
> > haraken@ was worrying that <use> tree triggering removeChildren() may not
> always
> > invoke removedFrom.
> > I double checked the code and |Container::removeChildren()| is guaranteed to
> > call |removedFrom|:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> FYI: It's |ContainerNode::detach()| that which do not always call
removedFrom()
> on its children.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Does the patch need testcase? It seems so from the bug.

[kouhei (in TOK), 2014-06-11 03:35:42]

Sorry, the patch is same, but the reasoning was wrong. I updated the CL
description. This isn't related to the SVGElementInstance deprecation.

The deleted code was used to ensure the ASSERT(!isAnimating()) on
~SVGAnimatedProperty.
The assert checks that animation is always stopped when the element is dead.
However, the assert is removed now, so we don't need to ensure it.

The deleted code |clearAnimatedType| in ~SVGAnimateElement was triggered when a
document was unloaded and its <animate> element was destructed before its target
svg element. This stopped animation on the target svg elements before the ASSERT
run.

The ASSERT(!isAnimating()) on ~SVGAnimatedProperty was removed in r174923,
because we no longer guarantee calling |animationEnded| on elements inside <use>
shadow-tree.
Before r174923, we used to have |SVGAnimatedProperty::m_animatedElements| that
had references to all element instances which it was animating, but these
references being a raw-ptr to avoid reference cycles caused issues. We removed
|SVGAnimatedProperty::m_animatedElements| in r174923 so we only call
|animationEnded| on elements alive at the time.

Suppose that we have:
<g id='#tgt'>
-<rect id='#rect'>
<use xlink:href='#tgt'>
-#instance-root
--<g>
---<rect>
<animate xlink:href='#rect'>

If we remove the <use> element, the <rect> in the <use> shadow instance tree is
destructed while |isAnimating()| == true without |animationEnded| being called.

> Does the patch need testcase? It seems so from the bug.
Added.

[rwlbuis, 2014-06-11 19:54:22]

LGTM with nits.

https://codereview.chromium.org/327463002/diff/20001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/animate-teardown.html (right):

https://codereview.chromium.org/327463002/diff/20001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/animate-teardown.html:1: <svg>
Usually we have a more complete document element, i.e. specifying svg ns would
be nice.

https://codereview.chromium.org/327463002/diff/20001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/animate-teardown.html:9: <p>Test pass if unloading
this document does not fail ASSERTs on debug builds.</p>
"Test passes"

[kouhei (in TOK), 2014-06-12 01:24:53]

Thanks for reviewing!

https://codereview.chromium.org/327463002/diff/20001/LayoutTests/svg/animatio...
File LayoutTests/svg/animations/animate-teardown.html (right):

https://codereview.chromium.org/327463002/diff/20001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/animate-teardown.html:1: <svg>
On 2014/06/11 19:54:21, rwlbuis wrote:
> Usually we have a more complete document element, i.e. specifying svg ns would
> be nice.

This was meant to be html5 inline svg tag. Added html5 DOCTYPE instead.

https://codereview.chromium.org/327463002/diff/20001/LayoutTests/svg/animatio...
LayoutTests/svg/animations/animate-teardown.html:9: <p>Test pass if unloading
this document does not fail ASSERTs on debug builds.</p>
On 2014/06/11 19:54:21, rwlbuis wrote:
> "Test passes"

Done.

[kouhei (in TOK), 2014-06-12 01:24:56]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-06-12 01:25:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/327463002/40001

[commit-bot: I haz the power, 2014-06-12 05:36:20]

Change committed as 175989