Use-count when SMIL animation renders a frame

Use-count when SMIL animation renders a frame

To gain a better understanding of the usage of particular features of SMIL
animations, this patch adds a use counter for when a SMIL animation renders a
frame, thus creating a user-visible effect.

Committed: https://crrev.com/011d85f10af924ece4c58d02c67f8716d61664d4
Cr-Commit-Position: refs/heads/master@{#408327}

[suzyh_UTC10 (ex-contributor), 2016-07-21 07:02:28]

suzyh@chromium.org changed reviewers:
+ shans@chromium.org

[fs, 2016-07-21 10:17:32]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2016-07-21 10:17:33]

Drive-by LGTM for third_party/Webkit w/ some suggestions.

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html
(right):

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html:14:
var testHandle = async_test("Count when SMIL animation has rendered a frame");
Maybe s/rendered a frame/been applied/ to more closely match counter name (and
location?)

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html:16:
requestAnimationFrame(function() {
Optionally I think you could pause the timeline (SVGSVGElement.pauseAnimations)
and setCurrentTime to something within the relevant interval of the animation
element. (Could also specify the <animate> in the markup and give it a suitable
interval or use beginElement.)

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp:519:
UseCounter::count(&document(), UseCounter::SVGSMILAnimationAppliedEffect);
You could ditch the m_hasUpdatedAnimation - once the "counter" (flag) is set, it
is set. I don't think the additional overhead is something worth caring about
(unless prompted.)

[suzyh_UTC10 (ex-contributor), 2016-07-25 03:16:00]

fs: I'm struggling a bit with adding additional tests. Any pointers?

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html
(right):

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html:14:
var testHandle = async_test("Count when SMIL animation has rendered a frame");
On 2016/07/21 at 10:17:32, fs (OoO until Aug 15) wrote:
> Maybe s/rendered a frame/been applied/ to more closely match counter name (and
location?)

Done.

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html:16:
requestAnimationFrame(function() {
On 2016/07/21 at 10:17:32, fs (OoO until Aug 15) wrote:
> Optionally I think you could pause the timeline
(SVGSVGElement.pauseAnimations) and setCurrentTime to something within the
relevant interval of the animation element. (Could also specify the <animate> in
the markup and give it a suitable interval or use beginElement.)

I tried the following test, but it crashed in debug mode when unpausing the
animations, with "ASSERTION FAILED: m_activeState != Active". Have I done
something stupid, or is this a bug?

<svg id="target" width="400" height="400">
    <rect x="10" y="10" width="80" height="80">
        <animate attributeType="XML" attributeName="y" from="10" to="200"
begin="2", dur="20" />
    </rect>
</svg>
...
test(function() {
    target.pauseAnimations();
    assert_false(internals.isUseCounted(document,
SVGSMILAnimationAppliedEffect));
    target.setCurrentTime(3);
    target.unpauseAnimations();
    assert_true(internals.isUseCounted(document,
SVGSMILAnimationAppliedEffect));
}, ...);

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp (right):

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/animation/SMILTimeContainer.cpp:519:
UseCounter::count(&document(), UseCounter::SVGSMILAnimationAppliedEffect);
On 2016/07/21 at 10:17:33, fs (OoO until Aug 15) wrote:
> You could ditch the m_hasUpdatedAnimation - once the "counter" (flag) is set,
it is set. I don't think the additional overhead is something worth caring about
(unless prompted.)

Done.

[Mike Lawther (Google), 2016-07-25 04:34:03]

\o/ super excited to see this happening!

(sorry, not a review, just excited :) )

[fs, 2016-07-25 14:50:41]

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html
(right):

https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html:16:
requestAnimationFrame(function() {
On 2016/07/25 at 03:15:59, suzyh wrote:
> On 2016/07/21 at 10:17:32, fs (OoO until Aug 15) wrote:
> > Optionally I think you could pause the timeline
(SVGSVGElement.pauseAnimations) and setCurrentTime to something within the
relevant interval of the animation element. (Could also specify the <animate> in
the markup and give it a suitable interval or use beginElement.)
> 
> I tried the following test, but it crashed in debug mode when unpausing the
animations, with "ASSERTION FAILED: m_activeState != Active". Have I done
something stupid, or is this a bug?

I've hard time seeing that it wouldn't be a bug. Could you file one and attach
that attempt (or c'n'p your example into the bug?)
 
> <svg id="target" width="400" height="400">
>     <rect x="10" y="10" width="80" height="80">
>         <animate attributeType="XML" attributeName="y" from="10" to="200"
begin="2", dur="20" />
>     </rect>
> </svg>
> ...
> test(function() {
>     target.pauseAnimations();
>     assert_false(internals.isUseCounted(document,
SVGSMILAnimationAppliedEffect));
>     target.setCurrentTime(3);
>     target.unpauseAnimations();

This call shouldn't be needed (I guess this is what ends up triggering the
assert?) so it'd be something like:

async_test(t => {
  target.pauseAnimations();
  assert_false(...);
  target.setCurrentTime(3);
  window.onload = () => {
    // 'load' starts the timeline and applies the animations at 3s. 
    setTimeout(t.step_func_done(() => {
      assert_true(...);
    }), 0);
  });
}, ...);

but no need to dwell on it really. (I.e the existing test works.) 

>     assert_true(internals.isUseCounted(document,
SVGSMILAnimationAppliedEffect));
> }, ...);

[suzyh_UTC10 (ex-contributor), 2016-07-27 06:23:25]

On 2016/07/25 at 14:50:41, fs wrote:
>
https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html
(right):
> 
>
https://codereview.chromium.org/2164233002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/svg/animations/animation-started-use-counter.html:16:
requestAnimationFrame(function() {
> On 2016/07/25 at 03:15:59, suzyh wrote:
> > On 2016/07/21 at 10:17:32, fs (OoO until Aug 15) wrote:
> > > Optionally I think you could pause the timeline
(SVGSVGElement.pauseAnimations) and setCurrentTime to something within the
relevant interval of the animation element. (Could also specify the <animate> in
the markup and give it a suitable interval or use beginElement.)
> > 
> > I tried the following test, but it crashed in debug mode when unpausing the
animations, with "ASSERTION FAILED: m_activeState != Active". Have I done
something stupid, or is this a bug?
> 
> I've hard time seeing that it wouldn't be a bug. Could you file one and attach
that attempt (or c'n'p your example into the bug?)

Filed crbug.com/631879


> > <svg id="target" width="400" height="400">
> >     <rect x="10" y="10" width="80" height="80">
> >         <animate attributeType="XML" attributeName="y" from="10" to="200"
begin="2", dur="20" />
> >     </rect>
> > </svg>
> > ...
> > test(function() {
> >     target.pauseAnimations();
> >     assert_false(internals.isUseCounted(document,
SVGSMILAnimationAppliedEffect));
> >     target.setCurrentTime(3);
> >     target.unpauseAnimations();
> 
> This call shouldn't be needed (I guess this is what ends up triggering the
assert?) so it'd be something like:
> 
> async_test(t => {
>   target.pauseAnimations();
>   assert_false(...);
>   target.setCurrentTime(3);
>   window.onload = () => {
>     // 'load' starts the timeline and applies the animations at 3s. 
>     setTimeout(t.step_func_done(() => {
>       assert_true(...);
>     }), 0);
>   });
> }, ...);
> 
> but no need to dwell on it really. (I.e the existing test works.) 
> 
> >     assert_true(internals.isUseCounted(document,
SVGSMILAnimationAppliedEffect));
> > }, ...);

Shane had suggested that adding the additional test would be useful but I guess
I'll just go with this one. Thanks!

[suzyh_UTC10 (ex-contributor), 2016-07-27 06:24:46]

suzyh@chromium.org changed reviewers:
+ dcheng@chromium.org, isherman@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-07-27 06:25:10]

+dcheng, isherman for UseCounter/histograms OWNERS

[suzyh_UTC10 (ex-contributor), 2016-07-27 06:25:17]

The CQ bit was checked by suzyh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 06:25:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 06:27:02]

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

[commit-bot: I haz the power, 2016-07-27 06:27:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[suzyh_UTC10 (ex-contributor), 2016-07-27 06:43:24]

The CQ bit was checked by suzyh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 06:43:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-07-27 08:01:45]

RS LGMT for UseCounter changes

[dcheng, 2016-07-27 08:01:55]

LGTM even

[commit-bot: I haz the power, 2016-07-27 08:07:29]

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

[commit-bot: I haz the power, 2016-07-27 08:07:30]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-07-27 16:27:34]

histograms.xml lgtm

[Ilya Sherman, 2016-07-27 16:27:51]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2016-07-27 16:27:52]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2164233002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-07-27 16:28:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 16:33:49]

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

[commit-bot: I haz the power, 2016-07-27 16:33:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[suzyh_UTC10 (ex-contributor), 2016-07-27 21:24:50]

The CQ bit was checked by suzyh@chromium.org

[commit-bot: I haz the power, 2016-07-27 21:25:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 21:30:09]

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

[commit-bot: I haz the power, 2016-07-27 21:30:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[suzyh_UTC10 (ex-contributor), 2016-07-28 00:45:31]

The CQ bit was checked by suzyh@chromium.org

[suzyh_UTC10 (ex-contributor), 2016-07-28 00:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, fs@opera.com, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2164233002/#ps60001
(title: "Rebase again")

[commit-bot: I haz the power, 2016-07-28 00:45:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 03:27:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-28 03:28:55]

Description was changed from

==========
Use-count when SMIL animation renders a frame

To gain a better understanding of the usage of particular features of SMIL
animations, this patch adds a use counter for when a SMIL animation renders a
frame, thus creating a user-visible effect.
==========

to

==========
Use-count when SMIL animation renders a frame

To gain a better understanding of the usage of particular features of SMIL
animations, this patch adds a use counter for when a SMIL animation renders a
frame, thus creating a user-visible effect.

Committed: https://crrev.com/011d85f10af924ece4c58d02c67f8716d61664d4
Cr-Commit-Position: refs/heads/master@{#408327}
==========

[commit-bot: I haz the power, 2016-07-28 03:28:57]

Patchset 4 (id:??) landed as
https://crrev.com/011d85f10af924ece4c58d02c67f8716d61664d4
Cr-Commit-Position: refs/heads/master@{#408327}