Closed
Bug 1194028
Opened 9 years ago
Closed 9 years ago
KeyframeEffectReadOnly needs SetTiming method
Categories
(Core :: DOM: Animation, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(2 files, 5 obsolete files)
3.90 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
3.66 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
I am going to implement KeyEffectReadOnly::SetTiming() too in this bug.
Summary: KeyframeEffectReadOnly needs SetProperties method → KeyframeEffectReadOnly needs SetProperties/SetTiming method
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8647313 -
Flags: review?(bbirtles)
Assignee | ||
Comment 3•9 years ago
|
||
SetTiming() part is harder than I thought because we need to consider animation observer which is added/removed in Animation::UpdateRelevance().
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8647313 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
(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+
Assignee | ||
Comment 14•9 years ago
|
||
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
Comment 15•9 years ago
|
||
(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; } ?
Assignee | ||
Comment 16•9 years ago
|
||
(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 | ||
Comment 17•9 years ago
|
||
Assignee: nobody → hiikezoe
Attachment #8649136 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8649202 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbc2c9aedac5
Keywords: checkin-needed
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8bf7a483a20 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffca89296de
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8bf7a483a20 https://hg.mozilla.org/mozilla-central/rev/2ffca89296de
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•