1194028 - KeyframeEffectReadOnly needs SetTiming method

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Currently KeyframeEffect properties are copied (actually memmoved?) from a newly created effect's properties.
http://hg.mozilla.org/mozilla-central/file/7649ffe28b67/layout/style/nsAnimationManager.cpp#l452

If KeyframeEffectReadOnly has a setter method for the properties, KeyframeEffectReadOnly can control mIsRunningOnCompositor flags more easily in its class.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I am going to implement KeyEffectReadOnly::SetTiming() too in this bug.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Implement KeyFrameEffect SetProperties and use it in CheckAnimationRule

Comment 3

[Hiroyuki Ikezoe (:hiro)]

SetTiming() part is harder than I thought because we need to consider animation observer which is added/removed in Animation::UpdateRelevance().

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Implement Animation NotifyEffectTimingUpdate

This method will be called from KeyEffect when the KeyEffect timing is updated.

I gave up managing notifications for mutation observer in Animation class for now.
A subsequent patch will prevent animationChanged event after animationRemoved event was fired.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Implement KeyframeEffect SetTiming.

SetTiming() causes animationRemoved event to mutation observes in
some cases (test_animation_observers.html covers these cases).
Notifying animationChanged event after animationRemoved is problematic.
This patch does just check that animationRemoved event is not fired on
SetTiming to use IsRelevant(). Otherwise below assertion happens on
test_animation_observers.html.

!!! ASSERTION: shouldn't have observed an animation being changed after being removed: 'entry->mState == eState_RemainedPresent || entry->mState == eState_Added'

We should eventually handle these animation events in Animation(CSSAnimation?)
class or another new class.

This patch is independent from patches for bug1188251 for now. After applying the paches for bug1188251, we can remove oldAnim->SetEffect(oldEffect) line in CheckAnimationRule().

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8647330 [details] [diff] [review]
Part 3: Implement KeyframeEffect SetTiming.

Clearing review request flag since that assertion also happened on the try for bug 1188251.
https://treeherder.mozilla.org/logviewer.html#?job_id=10266187&repo=try

I will be waiting for being bug 1188251 closed first.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8647313 [details] [diff] [review]
Part 1: Implement KeyFrameEffect SetProperties and use it in CheckAnimationRule

Why do we need this? It doesn't seem necessary. Once we encapsulate all the other usage of Properties() we will need it, but it doesn't seem to add anything now?

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8647328 [details] [diff] [review]
Part 2: Implement Animation NotifyEffectTimingUpdate

>+void
>+Animation::NotifyEffectTimingUpdated()
>+{
>+  MOZ_ASSERT(mEffect);
>+  UpdateTiming(Animation::SeekFlag::NoSeek,
>+               Animation::SyncNotifyFlag::Async);
>+}

We should add a message to the assertion, e.g. "We should only update effect timing when we have a target effect"

>diff --git a/dom/animation/Animation.h b/dom/animation/Animation.h
>--- a/dom/animation/Animation.h
>+++ b/dom/animation/Animation.h
>@@ -292,16 +292,20 @@ public:
>    * properties that are changed are added to |aSetProperties|.
>    * |aNeedsRefreshes| will be set to true if this animation expects to update
>    * the style rule on the next refresh driver tick as well (because it
>    * is running and has an effect to sample).
>    */
>   void ComposeStyle(nsRefPtr<AnimValuesStyleRule>& aStyleRule,
>                     nsCSSPropertySet& aSetProperties,
>                     bool& aNeedsRefreshes);
>+
>+  // Should be called only from KeyframeEffect.
>+  void NotifyEffectTimingUpdated();

This comment seems to suggest that bad things will happen if something else call this but I think it's quite safe to call. I think no comment would be fine. (If we have a number of these methods later we could group them together and say 'Interface for target effect' or 'Called by target effect:' or something like that.)

>+void
>+KeyframeEffectReadOnly::SetTiming(const AnimationTiming& aTiming,
>+                                  Animation* aOwningAnimation)
>+{
>+  MOZ_ASSERT(aOwningAnimation,
>+    "Do not call KeyframeEffectReadOnly::SetTiming without animation");

The assertion message should describe the expected behavior, e.g. "SetTiming should be passed an owning animation".

If a parameter should never be null, my preference is to make it a reference, then we don't need an assertion at all (and it forces the call site to ensure it is not null). I know not everyone agrees, however.

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
>@@ -237,16 +237,20 @@ public:
> 
>   const AnimationTiming& Timing() const {
>     return mTiming;
>   }
>   AnimationTiming& Timing() {
>     return mTiming;
>   }
> 
>+  // The Animation instance pointer will be removed once KeyFrameEffect
>+  // owns parent animation.
>+  void SetTiming(const AnimationTiming& aTiming, Animation* aOwningAnimtion);

I think we can drop the comment or make it say, "FIXME: Drop |aOwningAnimation| once we make AnimationEffects track their owning animation".

r=me with comments addressed

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #7)
> Comment on attachment 8647313 [details] [diff] [review]
> Part 1: Implement KeyFrameEffect SetProperties and use it in
> CheckAnimationRule
> 
> Why do we need this? It doesn't seem necessary. Once we encapsulate all the
> other usage of Properties() we will need it, but it doesn't seem to add
> anything now?

I had a misunderstanding there.
I am planning to remove ClearIsRunningOnCompositor in nsAnimationManager::CheckAnimationRule() (bug 1151694). To perform it I was thinking KeyframeEffect needs to know its property changes (e.g. from or to styles). I was misunderstanding that KeyframeEffect style properties can be directly modified from script.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Implement Animation NotifyEffectTimingUpdate and KeyframeEffect SetTiming v2

I thought I was splitting the part of implementation of KeyframeEffectReadOnly::SetTiming into part 3, but didn't....

Anyway this patch addressed all review comments.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Use KeyFrameEffect SetTiming.

Now KeyframeEffect.SetTiming() updates the owning animation timing and relavance, so we don't need to call each methods respectively for the animation any more.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8648963 [details] [diff] [review]
Part 2: Use KeyFrameEffect SetTiming.

I think we can also remove the comment at the start of Animation::SetEffect and add an early-return of mEffect == aEffect?

https://dxr.mozilla.org/mozilla-central/rev/9673c75864beafca2f6c8b117b98503128bf2e56/dom/animation/Animation.cpp#51

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Use KeyFrameEffect SetTiming v2

(In reply to Brian Birtles (:birtles) from comment #12)
> Comment on attachment 8648963 [details] [diff] [review]
> Part 2: Use KeyFrameEffect SetTiming.
> 
> I think we can also remove the comment at the start of Animation::SetEffect
> and add an early-return of mEffect == aEffect?
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 9673c75864beafca2f6c8b117b98503128bf2e56/dom/animation/Animation.cpp#51

Exactly!

Comment 14

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b76ea7479d

Thank you, Brian!

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b76ea7479d

I think you can add:

Animation::SetEffect(KeyframeEffectReadOnly* aEffect)
{
  if (mEffect == aEffect) {
    return;
  }

?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #15)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b76ea7479d
> 
> I think you can add:
> 
> Animation::SetEffect(KeyframeEffectReadOnly* aEffect)
> {
>   if (mEffect == aEffect) {
>     return;
>   }

Thanks! I missed the later half of your comments.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Use KeyFrameEffect SetTiming v3

Comment 18

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbc2c9aedac5

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bf7a483a20
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffca89296de

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a8bf7a483a20
https://hg.mozilla.org/mozilla-central/rev/2ffca89296de