[SVG] Handle repeat(n) event for svg animations

[SVG] Handle repeat(n) event for svg animations

Handled repeat(n) event for svg animations according to http://www.w3.org/TR/SMIL3/smil-timing.html#q135

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

[pavane, 2013-09-02 09:24:01]

WIP patch. Handled repeat(n) by firing the event synchronously. Need ideas on
how to do it asynchronously.

[pavane, 2013-09-02 11:53:02]

On 2013/09/02 09:24:01, pavane wrote:
> WIP patch. Handled repeat(n) by firing the event synchronously. Need ideas on
> how to do it asynchronously.

Just a note that this event is first of it's type where it has what you can call
it arguments. 
It doesn't work if we do it as below for known reasons
smilRepeatNEventSender(repeatEventCount).dispatchEventSoon(this);
DEFINE_STATIC_LOCAL(SMILEventSender, sender, ("repeat(" +
String::number(repeatEventCount) + ")"));

[pavane, 2013-09-03 15:50:29]

Please have a look at this patch

[pdr., 2013-09-03 17:48:14]

Looks good, lets do one more round.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1a-expected.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1a-expected.svg:1: <?xml version="1.0"
encoding="UTF-8"?>
I'd like to see an additional test with the following scenario:
1) add an animation to the document, let it run and repeat
2) make sure the repeat fires
3) remove the animation from the document
4) re-add the animation, and ensure that the correct number of repeat events
occur

This will, sadly, have to be a javascript test using the animation framework.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1a.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1a.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
indentation

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1a.svg:16:
setTimeout(function(){testRunner.notifyDone();}, 100);
each of these tests will take 100ms each. Not a huge deal, but can you change
this to be lower; possibly 1?

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1b.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1b.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
indentation

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1c.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1c.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
indentation

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1d.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1d.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
indentation

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1d.svg:6: <set attributeName="width"
to="100" begin="anim.repeat(3)"/>
Can you add an additional test that repeat(4) doesn't fire?

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-2a.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-2a.svg:6: <rect x="0" y="0" width="100"
height="100" fill="green">
Does this pass? Should this first one be fill="red" and the set to="green"?

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:68:
DEFINE_STATIC_LOCAL(SMILEventSender, sender, ("repeatn"));
repeatNEvent?

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1124:
smilRepeatEventSender().dispatchEventSoon(this);
This code is duplicated 3 times. Can you move it to a helper function?

dispatchRepeatEvents(repeatNum)?

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1245:
m_repeatEventCountList.remove(0);
Because this is the only place you remove the events, I think there's a bug if
you remove the animation and re-add it. This is why I've asked for the big test
in my first comment.

[pavane, 2013-09-03 19:00:51]

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1a-expected.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1a-expected.svg:1: <?xml version="1.0"
encoding="UTF-8"?>
On 2013/09/03 17:48:14, pdr wrote:
> I'd like to see an additional test with the following scenario:
> 1) add an animation to the document, let it run and repeat
> 2) make sure the repeat fires
> 3) remove the animation from the document
> 4) re-add the animation, and ensure that the correct number of repeat events
> occur
> 
> This will, sadly, have to be a javascript test using the animation framework.

Working on it

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1a.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1a.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
On 2013/09/03 17:48:14, pdr wrote:
> indentation

Done.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1a.svg:16:
setTimeout(function(){testRunner.notifyDone();}, 100);
On 2013/09/03 17:48:14, pdr wrote:
> each of these tests will take 100ms each. Not a huge deal, but can you change
> this to be lower; possibly 1?

I think it is better to put atleast 10ms to avoid flakiness. Anyways let's try
with 1ms
Done.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1b.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1b.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
On 2013/09/03 17:48:14, pdr wrote:
> indentation

Done.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1c.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1c.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
On 2013/09/03 17:48:14, pdr wrote:
> indentation

Done.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-1d.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1d.svg:5: <animate id="anim"
attributeName="visibility" to="visible" begin="0s" dur="2s" repeatCount="4"/>
On 2013/09/03 17:48:14, pdr wrote:
> indentation

Done.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-1d.svg:6: <set attributeName="width"
to="100" begin="anim.repeat(3)"/>
On 2013/09/03 17:48:14, pdr wrote:
> Can you add an additional test that repeat(4) doesn't fire?

Done.

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeatn-event-2a.svg (right):

https://codereview.chromium.org/23823006/diff/4001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeatn-event-2a.svg:6: <rect x="0" y="0" width="100"
height="100" fill="green">
On 2013/09/03 17:48:14, pdr wrote:
> Does this pass? Should this first one be fill="red" and the set to="green"?

repeat(0) should not be fired. So, i added a negative test. Green first and then
if it turns red, it should fail.

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:68:
DEFINE_STATIC_LOCAL(SMILEventSender, sender, ("repeatn"));
On 2013/09/03 17:48:14, pdr wrote:
> repeatNEvent?

yes, i felt this name is the closest to represent repeat(n). Any suggestions? :)

Also, hope you don't have problem with this dummy string "repeatn" for later
comparison at SVGSMILElement::dispatchPendingEvent.

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1124:
smilRepeatEventSender().dispatchEventSoon(this);
On 2013/09/03 17:48:14, pdr wrote:
> This code is duplicated 3 times. Can you move it to a helper function?
> 
> dispatchRepeatEvents(repeatNum)?

Done.

https://codereview.chromium.org/23823006/diff/4001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1245:
m_repeatEventCountList.remove(0);
On 2013/09/03 17:48:14, pdr wrote:
> Because this is the only place you remove the events, I think there's a bug if
> you remove the animation and re-add it. This is why I've asked for the big
test
> in my first comment.

This is not an event list. i am just storing the repeatEventCount in a queue in
the same order of adding repeatn events to m_dispatchEventSoonList. When we
remove the animation, the destructor has code to delete all the pending events
from m_dispatchEventSoonList if they exist and i guess m_repeatEventCountList
too will be cleared with the object itself automatically. I guess i need not
call m_repeatEventCountList.clear() explicitly as it is a member variable. am i
wrong?

[pavane, 2013-09-04 21:49:26]

On 2013/09/03 17:48:14, pdr wrote:
> I'd like to see an additional test with the following scenario:
> 1) add an animation to the document, let it run and repeat
> 2) make sure the repeat fires
> 3) remove the animation from the document
> 4) re-add the animation, and ensure that the correct number of repeat events
> occur
> 
> This will, sadly, have to be a javascript test using the animation framework.

pdr, i need some clarification here. what do you mean by correct number of
repeats here?
let us say we deleted the animation after repeat(2) is fired. So, basically
repeat(1) and repeat(2) are already fired. Now when we re-add the animation, it
will again start from the beginning. So, it will again fire repeat(1), repeat(2)
etc. My question is what we are trying to prove here?

pdr, also to my bad luck i am not yet successful in writing a working testcase
using js animation framework. Even a simple testcase of registering snapshots
for events fail as it requires a certain time delay. When i try to use
setTimeout() for any sample, it skips testing that sample and doesn't print that
sample's result.

[pdr., 2013-09-04 22:02:40]

On 2013/09/04 21:49:26, pavane wrote:
> On 2013/09/03 17:48:14, pdr wrote:
> > I'd like to see an additional test with the following scenario:
> > 1) add an animation to the document, let it run and repeat
> > 2) make sure the repeat fires
> > 3) remove the animation from the document
> > 4) re-add the animation, and ensure that the correct number of repeat events
> > occur
> > 
> > This will, sadly, have to be a javascript test using the animation
framework.
> 
> pdr, i need some clarification here. what do you mean by correct number of
> repeats here?
> let us say we deleted the animation after repeat(2) is fired. So, basically
> repeat(1) and repeat(2) are already fired. Now when we re-add the animation,
it
> will again start from the beginning. So, it will again fire repeat(1),
repeat(2)
> etc. My question is what we are trying to prove here?

I'd like to prove that if repeatCount=3, the following works:
1) add the animation to the document, make sure 2 repeats fire
2) remove the animation from the document before the 3rd repeat fires.
3) re-add the animation to the document and let it run all the way through, make
sure 3 repeats fire (and not 4).

This also tests that we re-set everything up correctly when adding and removing
repeating events from the document.
 
> pdr, also to my bad luck i am not yet successful in writing a working testcase
> using js animation framework. Even a simple testcase of registering snapshots
> for events fail as it requires a certain time delay. When i try to use
> setTimeout() for any sample, it skips testing that sample and doesn't print
that
> sample's result.

I recommend taking one of the existing tests and gradually changing it until it
matches what you want to do.

[pavane, 2013-09-10 15:16:07]

Please have a look at this patch.

Added test case for remove/re-add scenario.

We should not be checking for hasConnectedConditions() while calling
connectConditions() as it doesn't handle remove/re-add scenario. This means that
we need to remove/re-add listeners every time updateAnimations() is called.
Looks like there is no way to avoid it.

[pdr., 2013-09-11 04:04:04]

LGTM, nice change as always. Please click commit after updating the test :)

As we discussed on IRC, lets do a second patch (after this lands) to switch to
using SVGDocumentExtensions instead of re-connecting the conditions on every
update. As written, the conditions will be very slow for even moderately-sized
documents due to the getElementById call in the conditions loop.

I've uploaded an old document of mine that might help you understand
SVGDocumentExtensions:
https://docs.google.com/a/chromium.org/document/d/14n617cSM13UfQjVxREJ_jouJ8e...

https://codereview.chromium.org/23823006/diff/21001/LayoutTests/svg/animation...
File LayoutTests/svg/animations/repeatn-event-1d.svg (right):

https://codereview.chromium.org/23823006/diff/21001/LayoutTests/svg/animation...
LayoutTests/svg/animations/repeatn-event-1d.svg:9: if (window.testRunner)
Please make the indentation consistent here (2 or 4 is fine).

(Here, and in the other tests)

[commit-bot: I haz the power, 2013-09-11 06:05:11]

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

[commit-bot: I haz the power, 2013-09-11 08:27:20]

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

[commit-bot: I haz the power, 2013-09-11 08:34:30]

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

[commit-bot: I haz the power, 2013-09-11 10:51:28]

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

[commit-bot: I haz the power, 2013-09-11 20:44:45]

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

[commit-bot: I haz the power, 2013-09-11 23:08:06]

Change committed as 157624