Rewind the timeline for SVG-in-<img> when resetAnimation() is called

Rewind the timeline for SVG-in-<img> when resetAnimation() is called

Attempt to mimic the behavior of animated raster images (BitmapImage)
more closely wrt when the timeline is started and reset/rewound.

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

[kouhei (in TOK), 2014-03-20 15:27:29]

non-owner lgtm with nits

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
File
LayoutTests/svg/as-background-image/animated-svg-animation-control-expected.html
(right):

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
LayoutTests/svg/as-background-image/animated-svg-animation-control-expected.html:2:
<div style="width: 100px; height: 100px; background-color: green"><div>
</div>

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
File LayoutTests/svg/as-background-image/animated-svg-animation-control.html
(right):

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
LayoutTests/svg/as-background-image/animated-svg-animation-control.html:13:
"</svg>\")";
Maybe we can have this as a svg file in LayoutTests/svg/as-image/resources/ dir.

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
LayoutTests/svg/as-background-image/animated-svg-animation-control.html:14:
div.style.background = imgurl;
There are LayoutTests/svg/as-image/ testing svg inside <img>. Maybe migrate this
there as <img src='foobar.svg'> test?

https://codereview.chromium.org/206033005/diff/1/Source/core/svg/graphics/SVG...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/206033005/diff/1/Source/core/svg/graphics/SVG...
Source/core/svg/graphics/SVGImage.cpp:286: startAnimation();
This line took me a while to understand. This call unpauses animation if
necessary. Makes sense.
Would you mind adding a comment describing why "startAnimation" here?

[fs, 2014-03-20 17:02:17]

Updated to address kouhei's comments.

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
File
LayoutTests/svg/as-background-image/animated-svg-animation-control-expected.html
(right):

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
LayoutTests/svg/as-background-image/animated-svg-animation-control-expected.html:2:
<div style="width: 100px; height: 100px; background-color: green"><div>
On 2014/03/20 15:27:29, kouhei wrote:
> </div>

Done.

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
File LayoutTests/svg/as-background-image/animated-svg-animation-control.html
(right):

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
LayoutTests/svg/as-background-image/animated-svg-animation-control.html:13:
"</svg>\")";
On 2014/03/20 15:27:29, kouhei wrote:
> Maybe we can have this as a svg file in LayoutTests/svg/as-image/resources/
dir.

Done. (But to as-background-image/resources/ - I generally prefer to keep tests
as self-contained as possible - and runnable in stock-browsers if possible.)

https://codereview.chromium.org/206033005/diff/1/LayoutTests/svg/as-backgroun...
LayoutTests/svg/as-background-image/animated-svg-animation-control.html:14:
div.style.background = imgurl;
On 2014/03/20 15:27:29, kouhei wrote:
> There are LayoutTests/svg/as-image/ testing svg inside <img>. Maybe migrate
this
> there as <img src='foobar.svg'> test?

The bug was reported against 'background-image' so that's why the test was put
there. It ought to be possible to have a <img>-based test too (but the
difference in code-paths shouldn't matter.)

I heard that TCs don't mind quantity, so I'll add one there as well.

https://codereview.chromium.org/206033005/diff/1/Source/core/svg/graphics/SVG...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/206033005/diff/1/Source/core/svg/graphics/SVG...
Source/core/svg/graphics/SVGImage.cpp:286: startAnimation();
On 2014/03/20 15:27:29, kouhei wrote:
> This line took me a while to understand. This call unpauses animation if
> necessary. Makes sense.
> Would you mind adding a comment describing why "startAnimation" here?

Added a comment to that effect.

[pdr., 2014-03-20 18:02:34]

LGTM with nits. Feel free to commit

https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
File LayoutTests/svg/as-background-image/animated-svg-animation-control.html
(right):

https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
LayoutTests/svg/as-background-image/animated-svg-animation-control.html:19: },
150);
Both 150 and 10 are suspicious to me.

For 150: can we bring the 150ms number down? Is this just waiting for the
animation to occur?

For 10: What is this waiting on? If it's layout, can we force it explicitly with
"document.body.offsetTop"? What about using 0 instead of 10?

[fs, 2014-03-20 18:12:28]

On 2014/03/20 18:02:34, pdr wrote:
> LGTM with nits. Feel free to commit
> 
>
https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
> File LayoutTests/svg/as-background-image/animated-svg-animation-control.html
> (right):
> 
>
https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
> LayoutTests/svg/as-background-image/animated-svg-animation-control.html:19: },
> 150);
> Both 150 and 10 are suspicious to me.

Yes, these are gnarly - but that's the business w/ SVGImage - it's difficult to
observe it because of the limited access.
 
> For 150: can we bring the 150ms number down? Is this just waiting for the
> animation to occur?

This is waiting for the animation to end, yes - and some additional fudge to try
to account for the uncertainty at start/end (one 25ms frame-delay-sized bit on
each side basically.) It's possible that the amount of fudge could be reduced -
I'm not sure I'd go below 25ms (fudge) though.
 
> For 10: What is this waiting on? If it's layout, can we force it explicitly
with
> "document.body.offsetTop"? What about using 0 instead of 10?

This is to make sure the animation has cycled - with the additional setTimeout
added in PS2 it's quite possible this could be changed to 0. (Since it's the
state of the "image" - whose DOM we don't have access to - it's difficult to
affect in any more direct way.)

[pdr., 2014-03-20 18:14:01]

On 2014/03/20 18:12:28, fs wrote:
> On 2014/03/20 18:02:34, pdr wrote:
> > LGTM with nits. Feel free to commit
> > 
> >
>
https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
> > File LayoutTests/svg/as-background-image/animated-svg-animation-control.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
> > LayoutTests/svg/as-background-image/animated-svg-animation-control.html:19:
},
> > 150);
> > Both 150 and 10 are suspicious to me.
> 
> Yes, these are gnarly - but that's the business w/ SVGImage - it's difficult
to
> observe it because of the limited access.
>  
> > For 150: can we bring the 150ms number down? Is this just waiting for the
> > animation to occur?
> 
> This is waiting for the animation to end, yes - and some additional fudge to
try
> to account for the uncertainty at start/end (one 25ms frame-delay-sized bit on
> each side basically.) It's possible that the amount of fudge could be reduced
-
> I'm not sure I'd go below 25ms (fudge) though.
>  
> > For 10: What is this waiting on? If it's layout, can we force it explicitly
> with
> > "document.body.offsetTop"? What about using 0 instead of 10?
> 
> This is to make sure the animation has cycled - with the additional setTimeout
> added in PS2 it's quite possible this could be changed to 0. (Since it's the
> state of the "image" - whose DOM we don't have access to - it's difficult to
> affect in any more direct way.)

Could you try shooting for the lower bound of both? My concern is just test
cycle time (30k tests and all :)

[fs, 2014-03-20 18:29:36]

On 2014/03/20 18:14:01, pdr wrote:
> On 2014/03/20 18:12:28, fs wrote:
> > On 2014/03/20 18:02:34, pdr wrote:
> > > LGTM with nits. Feel free to commit
> > > 
> > >
> >
>
https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
> > > File
LayoutTests/svg/as-background-image/animated-svg-animation-control.html
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/206033005/diff/20001/LayoutTests/svg/as-backg...
> > >
LayoutTests/svg/as-background-image/animated-svg-animation-control.html:19:
> },
> > > 150);
> > > Both 150 and 10 are suspicious to me.
> > 
> > Yes, these are gnarly - but that's the business w/ SVGImage - it's difficult
> to
> > observe it because of the limited access.
> >  
> > > For 150: can we bring the 150ms number down? Is this just waiting for the
> > > animation to occur?
> > 
> > This is waiting for the animation to end, yes - and some additional fudge to
> try
> > to account for the uncertainty at start/end (one 25ms frame-delay-sized bit
on
> > each side basically.) It's possible that the amount of fudge could be
reduced
> -
> > I'm not sure I'd go below 25ms (fudge) though.
> >  
> > > For 10: What is this waiting on? If it's layout, can we force it
explicitly
> > with
> > > "document.body.offsetTop"? What about using 0 instead of 10?
> > 
> > This is to make sure the animation has cycled - with the additional
setTimeout
> > added in PS2 it's quite possible this could be changed to 0. (Since it's the
> > state of the "image" - whose DOM we don't have access to - it's difficult to
> > affect in any more direct way.)
> 
> Could you try shooting for the lower bound of both? My concern is just test
> cycle time (30k tests and all :)

I ended up reducing the duration of the animation to 75ms, and then dropped the
150 to 100 in both tests. The 10's had to stay (at least for debug.) So ~320 ->
~220.

[fs, 2014-03-20 18:30:13]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-20 18:30:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/206033005/60001

[commit-bot: I haz the power, 2014-03-20 18:31:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:31:29]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[fs, 2014-03-20 18:35:46]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-20 18:35:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/206033005/60001

[commit-bot: I haz the power, 2014-03-20 19:39:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 19:39:40]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-03-20 20:26:54]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:27:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/206033005/60001

[commit-bot: I haz the power, 2014-03-20 21:27:34]

Change committed as 169694