[SVG] Handle repeatEvent for svg animations

[SVG] Handle repeatEvent for svg animations

Handled repeatEvent 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=269715
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157037

[pavane, 2013-08-31 02:05:28]

Please have a look at this patch

[pdr., 2013-08-31 02:28:07]

This change looks good.

I have just one nit, and one request for another test.

https://codereview.chromium.org/23621018/diff/2001/LayoutTests/svg/animations...
File LayoutTests/svg/animations/repeat-event.svg (right):

https://codereview.chromium.org/23621018/diff/2001/LayoutTests/svg/animations...
LayoutTests/svg/animations/repeat-event.svg:1: <?xml version="1.0"
encoding="UTF-8"?>
This is a good test but I wonder if you could add an additional test that the
repeat event fires multiple times?
You may be able to use additive animations to do this, where an animation begins
on anim.repeatEvent and runs twice. You may have to use JS to do this, I'm not
sure.

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

https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1142: // Triggering all the pending
events if the animation timeline is changed.
Does the order that you fire these matter?

https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1150: while (count < repeat) {
A for loop would be cleaner here:
for (int repeatEventCount = 1; repeatEventCount < repeat; repeatEventCount++)
  smilRepeatEventSender().dispatchEventSoon(this);

[pavane, 2013-08-31 03:30:11]

changes done as mentioned except the multiple repeat events testcase

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

https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1142: // Triggering all the pending
events if the animation timeline is changed.
On 2013/08/31 02:28:08, pdr wrote:
> Does the order that you fire these matter?

I don't think so. Specification doesn't define anything about this. I
implemented these events based on the behavior of Opera and Firefox and based on
whatever i could think of logically. For safe side i will put these in order.

https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
Source/core/svg/animation/SVGSMILElement.cpp:1150: while (count < repeat) {
On 2013/08/31 02:28:08, pdr wrote:
> A for loop would be cleaner here:
> for (int repeatEventCount = 1; repeatEventCount < repeat; repeatEventCount++)
>   smilRepeatEventSender().dispatchEventSoon(this);

done.

[pdr., 2013-08-31 03:54:31]

On 2013/08/31 03:30:11, pavane wrote:
> changes done as mentioned except the multiple repeat events testcase
> 
>
https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
> File Source/core/svg/animation/SVGSMILElement.cpp (right):
> 
>
https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
> Source/core/svg/animation/SVGSMILElement.cpp:1142: // Triggering all the
pending
> events if the animation timeline is changed.
> On 2013/08/31 02:28:08, pdr wrote:
> > Does the order that you fire these matter?
> 
> I don't think so. Specification doesn't define anything about this. I
> implemented these events based on the behavior of Opera and Firefox and based
on
> whatever i could think of logically. For safe side i will put these in order.
> 
>
https://codereview.chromium.org/23621018/diff/2001/Source/core/svg/animation/...
> Source/core/svg/animation/SVGSMILElement.cpp:1150: while (count < repeat) {
> On 2013/08/31 02:28:08, pdr wrote:
> > A for loop would be cleaner here:
> > for (int repeatEventCount = 1; repeatEventCount < repeat;
repeatEventCount++)
> >   smilRepeatEventSender().dispatchEventSoon(this);
> 
> done.

LGTM.

Lets test this further when more of this is built out.

[commit-bot: I haz the power, 2013-08-31 04:05:57]

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

[commit-bot: I haz the power, 2013-08-31 04:34:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-31 04:35:58]

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

[commit-bot: I haz the power, 2013-08-31 04:46:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pdr., 2013-08-31 04:48:28]

https://codereview.chromium.org/23621018/diff/2/Source/core/svg/animation/SVG...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/23621018/diff/2/Source/core/svg/animation/SVG...
Source/core/svg/animation/SVGSMILElement.cpp:1148: for (int repeatEventCount =
1; repeatEventCount < repeat; repeatEventCount++) {
Looks like this is failing to compile on windows.

Can you change "int" here to "unsigned" to match the type of repeat?

[commit-bot: I haz the power, 2013-08-31 05:49:55]

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

[commit-bot: I haz the power, 2013-08-31 18:21:22]

Change committed as 157037