[SVG] Implement 'discard' element

[SVG] Implement 'discard' element

Handled 'discard' element according to SVG2 specification:
http://www.w3.org/TR/SVG2/struct.html#DiscardElement

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org, fmalita@chromium.org
BUG=327339
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164939

[rwlbuis, 2013-12-16 21:31:33]

I am not a reviewer but did a short informal review.

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
Source/core/svg/animation/SMILTimeContainer.cpp:328: for (unsigned n = 0; n <
animationsToApply.size(); n++) {
Better use prefix ++ variant, it is not slower but may be quicker than postfix
++.
Also it looks like you can reuse animationsToApplySize here.

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
Source/core/svg/animation/SMILTimeContainer.cpp:329: if
(animationsToApply[n]->isDiscardable()) {
Do you need a flag for isDiscardable? I am not an expert with the anim system,
but isn't isDiscardable always true for the case at the time when the <discard>
can be applied, i.e. when begin time has been reached?

[pavane, 2013-12-16 22:03:50]

Thanks Rob for your review

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
Source/core/svg/animation/SMILTimeContainer.cpp:328: for (unsigned n = 0; n <
animationsToApply.size(); n++) {
On 2013/12/16 21:31:34, rwlbuis wrote:
> Better use prefix ++ variant, it is not slower but may be quicker than postfix
> ++.
> Also it looks like you can reuse animationsToApplySize here.

Sure, i missed it. I am not changing the size of the vector here after deleting
the discard element. So, i can use animationsToApplySize as well

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
Source/core/svg/animation/SMILTimeContainer.cpp:329: if
(animationsToApply[n]->isDiscardable()) {
On 2013/12/16 21:31:34, rwlbuis wrote:
> Do you need a flag for isDiscardable? I am not an expert with the anim system,
> but isn't isDiscardable always true for the case at the time when the
<discard>
> can be applied, i.e. when begin time has been reached?

you are right. only animations which are already started are added into
animationsToApply and we need to remove discard and it's target as soon as the
discard element is started. So, isDiscardable is not needed. We just need a way
to check whether it is a discard element.

[pdr., 2013-12-18 22:40:33]

A good start! Extensive tests, please :)

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/SVGDiscardEle...
File Source/core/svg/SVGDiscardElement.cpp (right):

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/SVGDiscardEle...
Source/core/svg/SVGDiscardElement.cpp:49: bool
SVGDiscardElement::hasValidAttributeType()
I feel like we should move some of these to SVGAnimateElement or
SVGAnimationElement since they are no longer applicable to all animation types.

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
Source/core/svg/animation/SMILTimeContainer.cpp:328: for (unsigned n = 0; n <
animationsToApply.size(); n++) {
I don't think this is safe.

Can't the remove() call in this for loop change the vector itself? For example,
if the remove target was a later element in this vector.

[pavane, 2013-12-19 07:17:03]

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/SVGDiscardEle...
File Source/core/svg/SVGDiscardElement.cpp (right):

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/SVGDiscardEle...
Source/core/svg/SVGDiscardElement.cpp:49: bool
SVGDiscardElement::hasValidAttributeType()
On 2013/12/18 22:40:33, pdr wrote:
> I feel like we should move some of these to SVGAnimateElement or
> SVGAnimationElement since they are no longer applicable to all animation
types.

Don't you think it will be a problem. Because all operations in
SMILTimeContainer are happening on SVGSMILElement type and you need to type cast
to SVGAnimationElement type every time you want to use hasValidAttributeType()

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
Source/core/svg/animation/SMILTimeContainer.cpp:328: for (unsigned n = 0; n <
animationsToApply.size(); n++) {
On 2013/12/18 22:40:33, pdr wrote:
> I don't think this is safe.
> 
> Can't the remove() call in this for loop change the vector itself? For
example,
> if the remove target was a later element in this vector.

This is the exact problem i am facing currently with one use case i have with
me. This code is producing dangling pointers. I am planning to check whether the
target element exists in the vector and if it exists, i want to set it to NULL
before removing the target element. But this looks to be inefficient as it needs
to search the vector every time. Any other better way to do this? Should i
change this to a vector of references and check for object to be inDocument() on
every iteration?

[pdr., 2013-12-19 22:01:44]

On 2013/12/19 07:17:03, pavane wrote:
>
https://codereview.chromium.org/99533004/diff/1/Source/core/svg/SVGDiscardEle...
> File Source/core/svg/SVGDiscardElement.cpp (right):
> 
>
https://codereview.chromium.org/99533004/diff/1/Source/core/svg/SVGDiscardEle...
> Source/core/svg/SVGDiscardElement.cpp:49: bool
> SVGDiscardElement::hasValidAttributeType()
> On 2013/12/18 22:40:33, pdr wrote:
> > I feel like we should move some of these to SVGAnimateElement or
> > SVGAnimationElement since they are no longer applicable to all animation
> types.
> 
> Don't you think it will be a problem. Because all operations in
> SMILTimeContainer are happening on SVGSMILElement type and you need to type
cast
> to SVGAnimationElement type every time you want to use hasValidAttributeType()

Up to you, but one way to do this would be to move setAttributeName onto
SVGAnimationElement as well, then there are only two callers in SVGSMILElement.
This may not be worth the effort, but please consider it.

> 
>
https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
> File Source/core/svg/animation/SMILTimeContainer.cpp (right):
> 
>
https://codereview.chromium.org/99533004/diff/1/Source/core/svg/animation/SMI...
> Source/core/svg/animation/SMILTimeContainer.cpp:328: for (unsigned n = 0; n <
> animationsToApply.size(); n++) {
> On 2013/12/18 22:40:33, pdr wrote:
> > I don't think this is safe.
> > 
> > Can't the remove() call in this for loop change the vector itself? For
> example,
> > if the remove target was a later element in this vector.
> 
> This is the exact problem i am facing currently with one use case i have with
> me. This code is producing dangling pointers. I am planning to check whether
the
> target element exists in the vector and if it exists, i want to set it to NULL
> before removing the target element. But this looks to be inefficient as it
needs
> to search the vector every time. Any other better way to do this? Should i
> change this to a vector of references and check for object to be inDocument()
on
> every iteration?

I would recommend solving using two techniques:
1) Change the vector to be of type Vector<RefPtr<SVGSMILElement>> instead of
Vector<SVGSMILElement*>. This will prevent use-after-frees by preventing the
freeing of these objects until animationsToApply goes out of scope. See
http://www.webkit.org/coding/RefPtr.html and
http://en.wikipedia.org/wiki/Reference_count for more information.

2) #1 only keeps the object from being destructed but we still have the issue of
elements being removed already when we go to remove them again. For example, two
<discard> elements could cause this situation. I think you can just check
element->inDocument() to check if it has been removed already, but please verify
that inDocument() is true before removal, and false after removal.

[pavane, 2013-12-20 13:31:07]

More test cases and some refactoring coming soon.

[pavane, 2013-12-20 21:12:21]

Please have a look at this patch.

pdr, the clean up you mentioned doesn't seem to be proper. If i move
attributeName to SVGAnimationElement, it doesn't look proper to schedule and
unschedule animations in SVGAnimationElement based on changes to attributeName
because m_timeContainer is maintained at SVGSMILElement.
It looks proper to handle scheduling operations at SVGSMILElement.

Apart from discard, there is SVGAnimateMotionElement which doesn't use
attributeName and it is handled in similar fashion.

I fear we end up making the code more uglier if we proceed with such clean up.
Please give your opinion on this.

[pdr., 2013-12-20 23:49:11]

This is starting to shape up!

I think we may have a very tricky security issue though:
<!DOCTYPE HTML>
<html>
<body>
<svg width="400" height="400" xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink">
  <svg id="inner" width="300" height="300">
    <rect id="rect" width="100" height="100" fill="green">
      <animate attributeName="x" from="0" to="300" begin="0s" dur="10s"/>
      <discard xlink:href="#inner" begin="2s"/>
    </rect>
  </svg>
</svg>
</body>
</html>

The issue here is that SMILTimeContainer is owned by SVGSVGElement and the above
code will destroy both the SVG element and the time container. I'm not sure how
to handle this; do you have any ideas?

https://codereview.chromium.org/99533004/diff/70001/LayoutTests/svg/animation...
File LayoutTests/svg/animations/discard-check-delete.svg (right):

https://codereview.chromium.org/99533004/diff/70001/LayoutTests/svg/animation...
LayoutTests/svg/animations/discard-check-delete.svg:17:
setTimeout(function(){testRunner.notifyDone();}, 100);
Most of these setTimeout(...) calls can have values less than 100 for the delay.
I think 0 may work, or 20 or 30.

https://codereview.chromium.org/99533004/diff/70001/LayoutTests/svg/animation...
File LayoutTests/svg/animations/discard-rect-with-anim-child-a.svg (right):

https://codereview.chromium.org/99533004/diff/70001/LayoutTests/svg/animation...
LayoutTests/svg/animations/discard-rect-with-anim-child-a.svg:7: <set
attributeName="fill" to="red" begin="2s" fill="freeze" />
nit: indentation

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/SVGDiscar...
File Source/core/svg/SVGDiscardElement.h (right):

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/SVGDiscar...
Source/core/svg/SVGDiscardElement.h:57: inline bool isSVGDiscardElement(const
Node& node)
Because this is called for every animation element on every frame, we should try
to make it fast.

Please define this on SVGSMILElement.h:
virtual bool isSVGDiscardElement() const { return false; } 

And add this to SVGDiscardElement.h:
virtual bool isSVGDiscardElement() const OVERRIDE FINAL { return true; }

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
Source/core/svg/animation/SMILTimeContainer.cpp:330: if (animationsToApply[n] &&
animationsToApply[n]->inDocument() &&
isSVGDiscardElement(*animationsToApply[n])) {
Can animationsToApply[n] ever be null? I don't think that it can, so you can
remove the animationsToApply[n] check.

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
Source/core/svg/animation/SMILTimeContainer.cpp:333:
animDiscard->remove(IGNORE_EXCEPTION);
I think the ordering here matters. Can you remove the target first, then the
discard?

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
Source/core/svg/animation/SMILTimeContainer.cpp:335:
targetElement->remove(IGNORE_EXCEPTION);
After both of these removal calls, can you add:
ASSERT(!element->inDocument())?

[pavane, 2013-12-21 12:28:10]

Changes done.


> I think we may have a very tricky security issue though:
> <!DOCTYPE HTML>
> <html>
> <body>
> <svg width="400" height="400" xmlns="http://www.w3.org/2000/svg"
> xmlns:xlink="http://www.w3.org/1999/xlink">
>   <svg id="inner" width="300" height="300">
>     <rect id="rect" width="100" height="100" fill="green">
>       <animate attributeName="x" from="0" to="300" begin="0s" dur="10s"/>
>       <discard xlink:href="#inner" begin="2s"/>
>     </rect>
>   </svg>
> </svg>
> </body>
> </html>
> 
> The issue here is that SMILTimeContainer is owned by SVGSVGElement and the
above
> code will destroy both the SVG element and the time container. I'm not sure
how
> to handle this; do you have any ideas?

If i understood the problem correctly, i think it should not be a problem as
m_timeContainer is also maintained as a refCounted object in every
SVGSMILElement. So, timeContainer is not destroyed till the time there is last
SVGSMILElement.

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
Source/core/svg/animation/SMILTimeContainer.cpp:333:
animDiscard->remove(IGNORE_EXCEPTION);
On 2013/12/20 23:49:11, pdr wrote:
> I think the ordering here matters. Can you remove the target first, then the
> discard?

I think the current order is logical. If discard is a child of the target, then
it is better to remove the child first and then the parent. It will avoid us
another inDocument() check.

[pdr., 2013-12-21 22:19:28]

On 2013/12/21 12:28:10, pavane wrote:
> > I think we may have a very tricky security issue though:
> > <!DOCTYPE HTML>
> > <html>
> > <body>
> > <svg width="400" height="400" xmlns="http://www.w3.org/2000/svg"
> > xmlns:xlink="http://www.w3.org/1999/xlink">
> >   <svg id="inner" width="300" height="300">
> >     <rect id="rect" width="100" height="100" fill="green">
> >       <animate attributeName="x" from="0" to="300" begin="0s" dur="10s"/>
> >       <discard xlink:href="#inner" begin="2s"/>
> >     </rect>
> >   </svg>
> > </svg>
> > </body>
> > </html>
> > 
> > The issue here is that SMILTimeContainer is owned by SVGSVGElement and the
> above
> > code will destroy both the SVG element and the time container. I'm not sure
> how
> > to handle this; do you have any ideas?
> 
> If i understood the problem correctly, i think it should not be a problem as
> m_timeContainer is also maintained as a refCounted object in every
> SVGSMILElement. So, timeContainer is not destroyed till the time there is last
> SVGSMILElement.

Your reasoning is sound but there's a case you're missing with pending tasks
that depend on the time container being around. Please add the above testcase to
your patch.

Here's the backtrace after the crash:
Program received signal SIGABRT, Aborted.
0x93a68952 in __pthread_kill ()
(gdb) bt
#0  0x93a68952 in __pthread_kill ()
#1  0x96612167 in pthread_kill ()
#2  0x909c3340 in abort ()
#3  0x990f9869 in abort_message ()
#4  0x99118f8d in __cxa_pure_virtual ()
#5  0x1ebc6333 in WebCore::ThreadTimers::sharedTimerFiredInternal
(this=0x7a61d090) at
../../third_party/WebKit/Source/platform/ThreadTimers.cpp:134
#6  0x1ebc601d in WebCore::ThreadTimers::sharedTimerFired () at
../../third_party/WebKit/Source/platform/ThreadTimers.cpp:108
#7  0x1fb640d9 in webkit_glue::WebKitPlatformSupportImpl::DoTimeout
(this=0x7a7600a0) at webkitplatformsupport_impl.h:143
#8  0x1fb65694 in base::internal::RunnableAdapter<void
(webkit_glue::WebKitPlatformSupportImpl::*)()>::Run (this=0xbff5eee0,
object=0x7a7600a0) at bind_internal.h:134
#9  0x1fb655a4 in base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void
(webkit_glue::WebKitPlatformSupportImpl::*)()>, void
()(webkit_glue::WebKitPlatformSupportImpl*)>::MakeItSo (runnable=Unexpected type
(16) encountered for integer constant.) at bind_internal.h:871
#10 0x1fb654e4 in base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(webkit_glue::WebKitPlatformSupportImpl::*)()>, void
()(webkit_glue::WebKitPlatformSupportImpl*), void
()(base::internal::UnretainedWrapper<webkit_glue::WebKitPlatformSupportImpl>)>,
void ()(webkit_glue::WebKitPlatformSupportImpl*)>::Run (base=0x7c06f100) at
bind_internal.h:1166
#11 0x0a4b46eb in base::Callback<void ()()>::Run (this=0xbff5ef78) at
callback.h:401
#12 0x0a683ce8 in base::Timer::RunScheduledTask (this=0x7a7600a8) at
../../base/timer/timer.cc:193
#13 0x0a683ee9 in base::BaseTimerTaskInternal::Run (this=0x7c071bf0) at
../../base/timer/timer.cc:49
#14 0x0a684764 in base::internal::RunnableAdapter<void
(base::BaseTimerTaskInternal::*)()>::Run (this=0xbff5f040, object=0x7c071bf0) at
bind_internal.h:134
#15 0x0a684674 in base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void (base::BaseTimerTaskInternal::*)()>, void
()(base::BaseTimerTaskInternal*)>::MakeItSo (runnable=Unexpected type (16)
encountered for integer constant.) at bind_internal.h:871
#16 0x0a6845be in base::internal::Invoker<1,
base::internal::BindState<base::internal::RunnableAdapter<void
(base::BaseTimerTaskInternal::*)()>, void ()(base::BaseTimerTaskInternal*), void
()(base::internal::OwnedWrapper<base::BaseTimerTaskInternal>)>, void
()(base::BaseTimerTaskInternal*)>::Run (base=0x7c06f450) at bind_internal.h:1166
#17 0x0a4b46eb in base::Callback<void ()()>::Run (this=0xbff5f334) at
callback.h:401
#18 0x0a5944d1 in base::MessageLoop::RunTask (this=0xbff61210,
pending_task=@0xbff5f320) at ../../base/message_loop/message_loop.cc:511
#19 0x0a594992 in base::MessageLoop::DeferOrRunPendingTask (this=0xbff61210,
pending_task=@0xbff5f320) at ../../base/message_loop/message_loop.cc:523
#20 0x0a594fd9 in base::MessageLoop::DoDelayedWork (this=0xbff61210,
next_delayed_work_time=0xbff5f400) at
../../base/message_loop/message_loop.cc:675
#21 0x0a4876c5 in base::MessagePumpCFRunLoopBase::RunWork (this=0x7a610070) at
../../base/message_loop/message_pump_mac.mm:492
#22 0x0a486c61 in base::MessagePumpCFRunLoopBase::RunWorkSource
(info=0x7a610070) at ../../base/message_loop/message_pump_mac.mm:463
#23 0x91240e3f in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ ()
#24 0x91231ccb in __CFRunLoopDoSources0 ()
#25 0x912313ce in __CFRunLoopRun ()
#26 0x91230d5a in CFRunLoopRunSpecific ()
#27 0x91230bbb in CFRunLoopRunInMode ()

I think what is happening is that SMILTimeContainer creates a timer with a raw
pointer to itself. I think that queues a task to call a function on the time
container, except the task is firing after the time container has been
destructed. I'm not sure the best way to solve this; I would recommend looking
at other users of Timer and seeing if they handle a similar situation.

>
https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
> File Source/core/svg/animation/SMILTimeContainer.cpp (right):
> 
>
https://codereview.chromium.org/99533004/diff/70001/Source/core/svg/animation...
> Source/core/svg/animation/SMILTimeContainer.cpp:333:
> animDiscard->remove(IGNORE_EXCEPTION);
> On 2013/12/20 23:49:11, pdr wrote:
> > I think the ordering here matters. Can you remove the target first, then the
> > discard?
> 
> I think the current order is logical. If discard is a child of the target,
then
> it is better to remove the child first and then the parent. It will avoid us
> another inDocument() check.

I agree with you that your approach is the most logical. Unfortunately, the spec
is pretty clear here:
"It [the discard element] must also be discarded following the target element
removal."

Please add a test such as the following to prove the removal order matches the
spec:
<!DOCTYPE HTML>
<html>
<body onload="startTest()">
  <script>
    var removedNodes = new Array();
    document.addEventListener("DOMNodeRemoved", function(evt) {
      removedNodes.push(evt.target.nodeName);
    });

    function startTest() {
      if (window.testRunner)
        testRunner.waitUntilDone();
      setTimeout(function() {
        document.getElementById('removeOrder').textContent = 'Discard node
removal order: ' + removedNodes.toString();
        if (window.testRunner) {
          testRunner.dumpAsText();
          testRunner.notifyDone();
        }
      }, 0);
    }
  </script>
  This tests for the remove order of discard elements.<br/>
  <div id="removeOrder"></div>
  <svg width="400" height="400" xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink">
    <rect id="rect" width="100" height="100" fill="green">
      <set id="set" attributeName="y" to="300" begin="3s" />
      <animate attributeName="x" from="0" to="300" begin="0s" dur="10s" />
      <discard xlink:href="#set" begin="0s" />
      <discard begin="0s" />
    </rect>
  </svg>
</body>
</html>

[pavane, 2013-12-23 10:30:53]

Changes done.

Unfortunately i was not able to reproduce the crash on my debug build. But the
above patch should solve the problem.

> > > I think the ordering here matters. Can you remove the target first, then
the
> > > discard?
> > 
> > I think the current order is logical. If discard is a child of the target,
> then
> > it is better to remove the child first and then the parent. It will avoid us
> > another inDocument() check.
> 
> I agree with you that your approach is the most logical. Unfortunately, the
spec
> is pretty clear here:
> "It [the discard element] must also be discarded following the target element
> removal."
> 
> Please add a test such as the following to prove the removal order matches the
> spec:
> <!DOCTYPE HTML>
> <html>
> <body onload="startTest()">
>   <script>
>     var removedNodes = new Array();
>     document.addEventListener("DOMNodeRemoved", function(evt) {
>       removedNodes.push(evt.target.nodeName);
>     });
> 
>     function startTest() {
>       if (window.testRunner)
>         testRunner.waitUntilDone();
>       setTimeout(function() {
>         document.getElementById('removeOrder').textContent = 'Discard node
> removal order: ' + removedNodes.toString();
>         if (window.testRunner) {
>           testRunner.dumpAsText();
>           testRunner.notifyDone();
>         }
>       }, 0);
>     }
>   </script>
>   This tests for the remove order of discard elements.<br/>
>   <div id="removeOrder"></div>
>   <svg width="400" height="400" xmlns="http://www.w3.org/2000/svg"
> xmlns:xlink="http://www.w3.org/1999/xlink">
>     <rect id="rect" width="100" height="100" fill="green">
>       <set id="set" attributeName="y" to="300" begin="3s" />
>       <animate attributeName="x" from="0" to="300" begin="0s" dur="10s" />
>       <discard xlink:href="#set" begin="0s" />
>       <discard begin="0s" />
>     </rect>
>   </svg>
> </body>
> </html>

pdr, no matter what order we put the remove statements, we cannot control the
order of removal as they are refCounted. I tried to see the order of removal
both ways, it doesn't make much difference. Also, DOMNodeRemoved is not getting
fired on every removed node as you expected. I think it is getting fired only
for tip of the removed subtree and remaining nodes are getting cleaned up by
garbage collector. This is what i found out after debugging. Please correct me
if i am wrong.

[krit, 2013-12-24 07:03:35]

Will this feature behind a flag? It looks like we plan some more changes on the
element. From the spec:

""
Need to define SVGDiscardElement DOM interface for ‘discard’ element.
""

and

""
Would this element be better as part of the Animation chapter? It also needs to
be a member of the element categories that other animation elements are, and an
IDL interface needs to be written for it.
""

[pavane, 2013-12-26 13:12:28]

On 2013/12/24 07:03:35, krit wrote:
> Will this feature behind a flag? It looks like we plan some more changes on
the
> element. From the spec:
> 
> ""
> Need to define SVGDiscardElement DOM interface for ‘discard’ element.
> ""
> 
> and
> 
> ""
> Would this element be better as part of the Animation chapter? It also needs
to
> be a member of the element categories that other animation elements are, and
an
> IDL interface needs to be written for it.
> ""

Dirk, this implementation is in line with the upcoming changes in the spec. As
and when those interfaces are ready, we just need to add them here. Do you
really feel the need of putting it behind the flag?

pdr, your opinion on this?

[pavane, 2014-01-06 20:20:33]

ping

[pdr., 2014-01-06 20:36:53]

Back from 2 weeks of vacation and slowly catching up on reviews :) If you have
any others I've missed, please do bug me.

Dirk, can you share the status of this feature in the SVG WG? Are we just
waiting for the interface to be defined or are you aware of active, planned
changes in this area?

> pdr, no matter what order we put the remove statements, we cannot control the
> order of removal as they are refCounted. I tried to see the order of removal
> both ways, it doesn't make much difference. Also, DOMNodeRemoved is not
getting
> fired on every removed node as you expected. I think it is getting fired only
> for tip of the removed subtree and remaining nodes are getting cleaned up by
> garbage collector. This is what i found out after debugging. Please correct me
> if i am wrong.

Refcounting and dom removal are two different things. Refcounting allows us to
delete a c++ object once there are no references to it, whereas we directly
control the removal of elements from the dom. Please investigate this further as
it should be trivial.

[krit, 2014-01-07 18:29:01]

On 2014/01/06 20:36:53, pdr wrote:
> Back from 2 weeks of vacation and slowly catching up on reviews :) If you have
> any others I've missed, please do bug me.
> 
> Dirk, can you share the status of this feature in the SVG WG? Are we just
> waiting for the interface to be defined or are you aware of active, planned
> changes in this area?
> 

No, I am not aware of any active development and would not count on it any time
soon. However, to the future itself: the WG members just added features that the
WG agreed on having in the next level. Even if I am skeptical on the priority of
this particular element, you can expect it to be in the spec but maybe without
an interface. (Which isn't that bad in my opinion.)

[pavane, 2014-01-08 19:42:26]

Just to prove my point on current behavior of DOM node removal :)

Below is a simple test case for removal of node without involving discard:

<!DOCTYPE HTML>
<html>
<body>
  <script>
    var removedNodes = new Array();
    document.addEventListener("DOMNodeRemoved", function(evt) {
      removedNodes.push(evt.target.nodeName);
    });

    function startTest() {
      var rect1 = document.getElementById("rect1");
	  rect1.parentNode.removeChild(rect1);
      if (window.testRunner)
        testRunner.waitUntilDone();
      setTimeout(function() {
        document.getElementById('removeOrder').textContent = 'Discard node
removal order: ' + removedNodes.toString();
        if (window.testRunner) {
          testRunner.dumpAsText();
          testRunner.notifyDone();
        }
      }, 10);
    }
  </script>
  This tests for the remove order of elements.<br/>
  <div id="removeOrder"></div>
  <svg onclick="startTest()" width="400" height="400"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <rect id="rect1" width="100" height="100" fill="green">
      <set id="set1" attributeName="y" to="300" begin="3s" />
      <animate attributeName="x" from="0" to="300" begin="0s" dur="10s" />
    </rect>
  </svg>
</body>
</html>


You can see here that DOMNodeRemoved event is fired only for rect (tip of the
removed sub tree) and not it's children.

One more test case:

<!DOCTYPE HTML>
<html>
<body>
  <script>
    var removedNodes = new Array();
    document.addEventListener("DOMNodeRemoved", function(evt) {
      removedNodes.push(evt.target.nodeName);
    });

    function startTest() {
      var set1 = document.getElementById("set1");
	  set1.parentNode.removeChild(set1);
      var rect1 = document.getElementById("rect1");
	  rect1.parentNode.removeChild(rect1);
      if (window.testRunner)
        testRunner.waitUntilDone();
      setTimeout(function() {
        document.getElementById('removeOrder').textContent = 'Discard node
removal order: ' + removedNodes.toString();
        if (window.testRunner) {
          testRunner.dumpAsText();
          testRunner.notifyDone();
        }
      }, 10);
    }
  </script>
  This tests for the remove order of elements.<br/>
  <div id="removeOrder"></div>
  <svg onclick="startTest()" width="400" height="400"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <rect id="rect1" width="100" height="100" fill="green">
      <set id="set1" attributeName="y" to="300" begin="3s" />
      <animate attributeName="x" from="0" to="300" begin="0s" dur="10s" />
    </rect>
  </svg>
</body>
</html>

Try to debug above test case with a break point in SVGElement::removedFrom() Run
the test and click the rect after some time. Reload the test again. You will see
that removedFrom() will be called from garbage collector for some elements of
previous page (<svg>) while in the mid of animation of the current page
(although not always reproducable). This shows that currently dom node removal
is not controlled by the user.

[pdr., 2014-01-08 21:10:45]

On 2014/01/08 19:42:26, pavane wrote:
> Just to prove my point on current behavior of DOM node removal :)
> 
> Below is a simple test case for removal of node without involving discard:
> 
> <!DOCTYPE HTML>
> <html>
> <body>
>   <script>
>     var removedNodes = new Array();
>     document.addEventListener("DOMNodeRemoved", function(evt) {
>       removedNodes.push(evt.target.nodeName);
>     });
> 
>     function startTest() {
>       var rect1 = document.getElementById("rect1");
> 	  rect1.parentNode.removeChild(rect1);
>       if (window.testRunner)
>         testRunner.waitUntilDone();
>       setTimeout(function() {
>         document.getElementById('removeOrder').textContent = 'Discard node
> removal order: ' + removedNodes.toString();
>         if (window.testRunner) {
>           testRunner.dumpAsText();
>           testRunner.notifyDone();
>         }
>       }, 10);
>     }
>   </script>
>   This tests for the remove order of elements.<br/>
>   <div id="removeOrder"></div>
>   <svg onclick="startTest()" width="400" height="400"
> xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
>     <rect id="rect1" width="100" height="100" fill="green">
>       <set id="set1" attributeName="y" to="300" begin="3s" />
>       <animate attributeName="x" from="0" to="300" begin="0s" dur="10s" />
>     </rect>
>   </svg>
> </body>
> </html>
> 
> 
> You can see here that DOMNodeRemoved event is fired only for rect (tip of the
> removed sub tree) and not it's children.
> 
> One more test case:
> 
> <!DOCTYPE HTML>
> <html>
> <body>
>   <script>
>     var removedNodes = new Array();
>     document.addEventListener("DOMNodeRemoved", function(evt) {
>       removedNodes.push(evt.target.nodeName);
>     });
> 
>     function startTest() {
>       var set1 = document.getElementById("set1");
> 	  set1.parentNode.removeChild(set1);
>       var rect1 = document.getElementById("rect1");
> 	  rect1.parentNode.removeChild(rect1);
>       if (window.testRunner)
>         testRunner.waitUntilDone();
>       setTimeout(function() {
>         document.getElementById('removeOrder').textContent = 'Discard node
> removal order: ' + removedNodes.toString();
>         if (window.testRunner) {
>           testRunner.dumpAsText();
>           testRunner.notifyDone();
>         }
>       }, 10);
>     }
>   </script>
>   This tests for the remove order of elements.<br/>
>   <div id="removeOrder"></div>
>   <svg onclick="startTest()" width="400" height="400"
> xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
>     <rect id="rect1" width="100" height="100" fill="green">
>       <set id="set1" attributeName="y" to="300" begin="3s" />
>       <animate attributeName="x" from="0" to="300" begin="0s" dur="10s" />
>     </rect>
>   </svg>
> </body>
> </html>
> 
> Try to debug above test case with a break point in SVGElement::removedFrom()
Run
> the test and click the rect after some time. Reload the test again. You will
see
> that removedFrom() will be called from garbage collector for some elements of
> previous page (<svg>) while in the mid of animation of the current page
> (although not always reproducable). This shows that currently dom node removal
> is not controlled by the user.

You're overthinking this.
All I'm trying to do is match this part of the spec: "It [the discard element]
must also be discarded following the target element removal."

If you can make a test where the following happens:
target->remove(IGNORE_EXCEPTION);
animDiscard->remove(IGNORE_EXCEPTION);
and you see target being removed AFTER animDiscard, then we have a bug. I
strongly suspect you will see target removed before animDiscard every time.

[pavane, 2014-01-11 16:14:28]

Please have a look at this patch

[Inactive, 2014-01-11 16:31:25]

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
File Source/core/svg/SVGDiscardElement.h (right):

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
Source/core/svg/SVGDiscardElement.h:2: * Copyright (C) 2013 Samsung Electronics.
All rights reserved.
2014?

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
Source/core/svg/SVGDiscardElement.h:42: virtual bool isSVGDiscardElement() const
OVERRIDE FINAL { return true; }
No need for FINAL as the class is already FINAL

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
Source/core/svg/SVGDiscardElement.h:46: virtual bool isAdditive() const { return
false; }
You're missing quite a lot of OVERRIDE

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:269: Vector<
RefPtr<SVGSMILElement> >  animationsToApply;
We usually add an extra white space only on the right side:
Vector<RefPtr<SVGSMILElement> >

(I cannot wait for C++11)

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:327: for (unsigned n = 0; n <
animationsToApplySize; ++n) {
Any reason you're not using i as loop counter, like the other loop 5 lines
above?

[pavane, 2014-01-11 17:14:16]

Changes done.

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
File Source/core/svg/SVGDiscardElement.h (right):

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
Source/core/svg/SVGDiscardElement.h:2: * Copyright (C) 2013 Samsung Electronics.
All rights reserved.
On 2014/01/11 16:31:26, Chris Dumez wrote:
> 2014?

I liked it. :)
Done.

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
Source/core/svg/SVGDiscardElement.h:42: virtual bool isSVGDiscardElement() const
OVERRIDE FINAL { return true; }
On 2014/01/11 16:31:26, Chris Dumez wrote:
> No need for FINAL as the class is already FINAL

Done.

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/SVGDisca...
Source/core/svg/SVGDiscardElement.h:46: virtual bool isAdditive() const { return
false; }
On 2014/01/11 16:31:26, Chris Dumez wrote:
> You're missing quite a lot of OVERRIDE

Done.

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/animatio...
File Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:269: Vector<
RefPtr<SVGSMILElement> >  animationsToApply;
On 2014/01/11 16:31:26, Chris Dumez wrote:
> We usually add an extra white space only on the right side:
> Vector<RefPtr<SVGSMILElement> >
> 
> (I cannot wait for C++11)

Done.

https://codereview.chromium.org/99533004/diff/460001/Source/core/svg/animatio...
Source/core/svg/animation/SMILTimeContainer.cpp:327: for (unsigned n = 0; n <
animationsToApplySize; ++n) {
On 2014/01/11 16:31:26, Chris Dumez wrote:
> Any reason you're not using i as loop counter, like the other loop 5 lines
> above?

No specific reason. :)
Done.

[pavane, 2014-01-11 17:43:02]

Just noticed that isAdditive() is removed from the base class. So, removing it
from the derived class too.

[pdr., 2014-01-11 18:02:04]

On 2014/01/11 17:43:02, pavane wrote:
> Just noticed that isAdditive() is removed from the base class. So, removing it
> from the derived class too.

LGTM, but please fix the global-constructors-listing.html test that is failing.

[pavane, 2014-01-11 18:57:52]

On 2014/01/11 18:02:04, pdr wrote:
> On 2014/01/11 17:43:02, pavane wrote:
> > Just noticed that isAdditive() is removed from the base class. So, removing
it
> > from the derived class too.
> 
> LGTM, but please fix the global-constructors-listing.html test that is
failing.

Changes Done. Thanks for the reviews.

[commit-bot: I haz the power, 2014-01-11 18:58:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/99533004/610001

[commit-bot: I haz the power, 2014-01-11 20:26:28]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-12 02:55:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/99533004/780001

[commit-bot: I haz the power, 2014-01-13 00:47:28]

Change committed as 164939

[alph, 2014-01-13 07:58:01]

A revert of this CL has been created in
https://codereview.chromium.org/136313002/ by alph@chromium.org.

The reason for reverting is: svg/animations/discard-check-removal-order.html
test is flaky .