Closed Bug 1194028 Opened 5 years ago Closed 5 years ago

KeyframeEffectReadOnly needs SetTiming method

Categories

(Core :: DOM: Animation, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
I am going to implement KeyEffectReadOnly::SetTiming() too in this bug.
Summary: KeyframeEffectReadOnly needs SetProperties method → KeyframeEffectReadOnly needs SetProperties/SetTiming method
SetTiming() part is harder than I thought because we need to consider animation observer which is added/removed in Animation::UpdateRelevance().
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.
Attachment #8647328 - Flags: review?(bbirtles)
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().
Attachment #8647330 - Flags: review?(bbirtles)
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.
Attachment #8647330 - Flags: review?(bbirtles)
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?
Attachment #8647313 - Flags: review?(bbirtles)
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
Attachment #8647328 - Flags: review?(bbirtles) → review+
(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.
Attachment #8647313 - Attachment is obsolete: true
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.
Attachment #8647328 - Attachment is obsolete: true
Attachment #8648962 - Flags: review+
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.
Attachment #8647330 - Attachment is obsolete: true
Attachment #8648963 - Flags: review?(bbirtles)
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
Attachment #8648963 - Flags: review?(bbirtles) → review+
(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!
Attachment #8648963 - Attachment is obsolete: true
Attachment #8649136 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b76ea7479d

Thank you, Brian!
Keywords: checkin-needed
Summary: KeyframeEffectReadOnly needs SetProperties/SetTiming method → KeyframeEffectReadOnly needs SetTiming method
(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;
  }

?
(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.
Keywords: checkin-needed
Assignee: nobody → hiikezoe
Attachment #8649136 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8649202 - Flags: review+
You need to log in before you can comment on or make changes to this bug.