Closed
Bug 1232561
Opened 9 years ago
Closed 9 years ago
Add a ComposeAnimationRule method that works off the effect set
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
This covers the first part of bug 1230040:
* Add AnimationLevel to EffectCompositor (use with AppliesToTransitionsLevel)
* Move mStyleRule to the EffectSet
* Add static ComposeStyleRule to EffectCompositor and call from EnsureStyleRule
Assignee | ||
Comment 1•9 years ago
|
||
Introducing an enum will simplify further patches in this series by providing
a common vocabulary for this distinction.
Assignee | ||
Comment 2•9 years ago
|
||
This is needed in order to support script-generated animations since they do not
belong to any AnimationCollection.
This patch adopts the naming "animation rule" over "style rule". Currently we
are inconsistent about this (e.g. GetAnimationRule vs EnsureStyleRuleFor).
We don't do a mass rename here but just a few places near where we're touching.
Many of the other references to "style rule" will be revised in this bug or
related bugs so we can fix those references when we come to them.
Assignee | ||
Comment 3•9 years ago
|
||
As we gradually move logic from layout/style/AnimationCommon.cpp to
dom/animation/EffectSet and EffectCompositor it makes sense to let this class
live in its own file inside dom/animation where it is used.
Assignee | ||
Comment 4•9 years ago
|
||
This patch just moves a piece of functionality from
AnimationCollection::EnsureStyleRuleFor to the EffectCompositor. In subsequent
bugs we will moves more and more of this functionality across until this
logic is fully contained in the EffectCompositor.
Assignee | ||
Updated•9 years ago
|
Summary: Add a ComposeStyleRule method that works off the effect set → Add a ComposeAnimationRule method that works off the effect set
Assignee | ||
Comment 5•9 years ago
|
||
I was going to wait for bug 1228229 to be reviewed before putting this up for review (in case I need to do rebasing) but I've been waiting a long time for those reviews so I'm just going to go ahead with requesting review on this.
Assignee | ||
Updated•9 years ago
|
Attachment #8701684 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8701707 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8701708 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8701711 -
Flags: review?(cam)
Assignee | ||
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
Comment on attachment 8701684 [details] [diff] [review]
part 1 - Replace AppliesToTransitionsLevel() with a cascade level enumeration
Review of attachment 8701684 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/animation/Animation.h
@@ +10,5 @@
> #include "nsWrapperCache.h"
> #include "nsCycleCollectionParticipant.h"
> #include "mozilla/Attributes.h"
> #include "mozilla/LinkedList.h"
> +#include "mozilla/EffectCompositor.h" // For EffectCompositor::CascadeLevel
Up one line, if you want to keep the includes (approximately) sorted.
Attachment #8701684 -
Flags: review?(cam) → review+
Comment 8•9 years ago
|
||
Comment on attachment 8701707 [details] [diff] [review]
part 2 - Move the animation style rules from AnimationCollection to EffectSet
Review of attachment 8701707 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/animation/EffectCompositor.h
@@ +39,5 @@
> // The transitions sheet (CSS transitions that are tied to CSS markup)
> Transitions
> };
> + static const size_t kCascadeLevelCount =
> + static_cast<size_t>(CascadeLevel::Transitions) + 1;
This can be removed if we use an EnumeratedArray per my later comment.
::: dom/animation/EffectSet.h
@@ +7,5 @@
> #ifndef mozilla_EffectSet_h
> #define mozilla_EffectSet_h
>
> +#include "mozilla/EffectCompositor.h"
> +#include "AnimationCommon.h" // For AnimValuesStyleRule
Swap these around to keep these sorted (unless you are sorting by directory first, then file within directory).
@@ +126,5 @@
> }
> bool IsEmpty() const { return mEffects.IsEmpty(); }
>
> + RefPtr<AnimValuesStyleRule>& AnimationRule(EffectCompositor::CascadeLevel
> + aCascadeLevel)
Indenting is off.
@@ +152,5 @@
> + // style without animation, we need to not use them so that we can
> + // detect any new changes; if necessary we restyle immediately
> + // afterwards with animation.
> + RefPtr<AnimValuesStyleRule>
> + mAnimationRule[EffectCompositor::kCascadeLevelCount];
How about we use a mozilla::EnumeratedArray here instead of a C array? That will let us index into it with CascadeLevel values directly without needing to cast to an integer type.
::: layout/style/AnimationCommon.cpp
@@ +317,5 @@
>
> collection->EnsureStyleRuleFor(
> mPresContext->RefreshDriver()->MostRecentRefresh());
>
> + EffectSet *effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);
"*" next to type.
@@ +324,5 @@
> + }
> +
> + return IsAnimationManager() ?
> + effectSet->AnimationRule(EffectCompositor::CascadeLevel::Animations) :
> + effectSet->AnimationRule(EffectCompositor::CascadeLevel::Transitions);
Can you explain why we don't do the kind of check like in CSSTransition::CascadeLevel()? Are all the Animation objects in the manager tied to markup?
::: layout/style/nsAnimationManager.cpp
@@ -419,5 @@
> return nullptr;
> }
>
> if (collection) {
> - collection->mStyleRule = nullptr;
Why don't we need to do the equivalent thing on the EffectSet?
Comment 9•9 years ago
|
||
Comment on attachment 8701708 [details] [diff] [review]
part 3 - Move AnimValuesStyleRule to a separate file
Review of attachment 8701708 [details] [diff] [review]:
-----------------------------------------------------------------
r=me though I didn't check the moved code carefully.
Attachment #8701708 -
Flags: review?(cam) → review+
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away Dec 25-28) from comment #8)
> Comment on attachment 8701707 [details] [diff] [review]
> part 2 - Move the animation style rules from AnimationCollection to EffectSet
>
> Review of attachment 8701707 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/animation/EffectCompositor.h
> @@ +39,5 @@
> > // The transitions sheet (CSS transitions that are tied to CSS markup)
> > Transitions
> > };
> > + static const size_t kCascadeLevelCount =
> > + static_cast<size_t>(CascadeLevel::Transitions) + 1;
>
> This can be removed if we use an EnumeratedArray per my later comment.
Is it ok to keep this member but use an EnumeratedArray? The trouble with adding a Count/Max member to the enumeration is you end up with weaker static type checking.
For example, if you pass CascadeLevel::Count as an argument to a method taking a CascadeLevel parameter, the compiler won't complain and the call site will have to explicitly check for it. If you pass kCascadeLevelCount, however, the compiler will complain.
Likewise, if you use the enum type in a case statement, some compilers will complain if you don't have 'default' or 'case CascadeLevel::Count' branch. Suppose you add a default branch, then if we introduce another cascade level, the compiler won't warn you if you fail to update the case statement to handle the extra level. So I think if we keep the enum as only having valid values you get end up with better static type checking.
> ::: dom/animation/EffectSet.h
> @@ +7,5 @@
> > #ifndef mozilla_EffectSet_h
> > #define mozilla_EffectSet_h
> >
> > +#include "mozilla/EffectCompositor.h"
> > +#include "AnimationCommon.h" // For AnimValuesStyleRule
>
> Swap these around to keep these sorted (unless you are sorting by directory
> first, then file within directory).
Yeah, I've been grouping the mozilla/ ones first. I think I was told to do that once but I can't remember who told me that.
> @@ +152,5 @@
> > + // style without animation, we need to not use them so that we can
> > + // detect any new changes; if necessary we restyle immediately
> > + // afterwards with animation.
> > + RefPtr<AnimValuesStyleRule>
> > + mAnimationRule[EffectCompositor::kCascadeLevelCount];
>
> How about we use a mozilla::EnumeratedArray here instead of a C array? That
> will let us index into it with CascadeLevel values directly without needing
> to cast to an integer type.
Done.
> @@ +324,5 @@
> > + }
> > +
> > + return IsAnimationManager() ?
> > + effectSet->AnimationRule(EffectCompositor::CascadeLevel::Animations) :
> > + effectSet->AnimationRule(EffectCompositor::CascadeLevel::Transitions);
>
> Can you explain why we don't do the kind of check like in
> CSSTransition::CascadeLevel()? Are all the Animation objects in the manager
> tied to markup?
This isn't quite right, but we can't do any better until bug 1232577 where we completely rewrite this function. It will be correct for all currently supported cases (we only support animations tied to markup at the moment).
> ::: layout/style/nsAnimationManager.cpp
> @@ -419,5 @@
> > return nullptr;
> > }
> >
> > if (collection) {
> > - collection->mStyleRule = nullptr;
>
> Why don't we need to do the equivalent thing on the EffectSet?
It's been a while since I wrote this patch, but I think I worked out it's not needed since:
* We call collection->mStyleRuleRefreshTime = TimeStamp(); in the following line
* After the if {} block we call collection->mStyleChanging = true;
* Then we call:
TimeStamp refreshTime = mPresContext->RefreshDriver()->MostRecentRefresh();
collection->EnsureStyleRuleFor(refreshTime);
* In EnsureStyleRuleFor we have the following early returns:
1. if (!mStyleChanging) {
mStyleRuleRefreshTime = aRefreshTime;
return;
}
OK: We set mStyleChanging to true above
2. if (!mStyleRuleRefreshTime.IsNull() &&
mStyleRuleRefreshTime == aRefreshTime) {
// The style rule on the EffectSet may be null and valid, if we have no
// style to apply.
return;
}
OK: We set mStyleRuleRestyleTime to the null timestamp above
3. EffectSet* effectSet = EffectSet::GetEffectSet(mElement,
PseudoElementType());
if (!effectSet) {
return;
}
OK: If there is no effect set, there is no style rule to reset
* Then, after the early returns we run:
RefPtr<AnimValuesStyleRule>& styleRule =
IsForAnimations() ?
effectSet->AnimationRule(EffectCompositor::CascadeLevel::Animations) :
effectSet->AnimationRule(EffectCompositor::CascadeLevel::Transitions);
styleRule = nullptr;
So it will be reset anyway.
Assignee | ||
Comment 11•9 years ago
|
||
This is needed in order to support script-generated animations since they do not
belong to any AnimationCollection.
This patch adopts the naming "animation rule" over "style rule". Currently we
are inconsistent about this (e.g. GetAnimationRule vs EnsureStyleRuleFor).
We don't do a mass rename here but just a few places near where we're touching.
Many of the other references to "style rule" will be revised in this bug or
related bugs so we can fix those references when we come to them.
Attachment #8701729 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8701707 -
Attachment is obsolete: true
Attachment #8701707 -
Flags: review?(cam)
Assignee | ||
Comment 12•9 years ago
|
||
Put EnumeratedArray include in the right file
Attachment #8701888 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8701729 -
Attachment is obsolete: true
Attachment #8701729 -
Flags: review?(cam)
Assignee | ||
Comment 13•9 years ago
|
||
Rebase
Assignee | ||
Updated•9 years ago
|
Attachment #8701708 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
(In reply to Brian Birtles (:birtles, away 26 Dec~5 Jan) from comment #10)
> > > + static const size_t kCascadeLevelCount =
> > > + static_cast<size_t>(CascadeLevel::Transitions) + 1;
> >
> > This can be removed if we use an EnumeratedArray per my later comment.
>
> Is it ok to keep this member but use an EnumeratedArray? The trouble with
> adding a Count/Max member to the enumeration is you end up with weaker
> static type checking.
> [...]
OK. FWIW the example in the comment in mfbt/EnumeratedArray.h uses an explicit Count enum value, but your reasoning for not including one makes sense to me.
Updated•9 years ago
|
Attachment #8701888 -
Flags: review?(cam) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8701711 [details] [diff] [review]
part 4 - Add EffectCompositor::ComposeAnimationRule
Review of attachment 8701711 [details] [diff] [review]:
-----------------------------------------------------------------
s/moves/move/ in the commit message.
::: dom/animation/EffectCompositor.cpp
@@ +242,5 @@
> + if (effect->GetAnimation()->CascadeLevel() == aCascadeLevel) {
> + sortedEffectList.AppendElement(effect);
> + }
> + }
> + sortedEffectList.Sort(EffectCompositeOrderComparator());
I can't find where EffectCompositeOrderComparator is defined but I'll assume it's doing what we want here. :)
Attachment #8701711 -
Flags: review?(cam) → review+
Comment 16•9 years ago
|
||
Comment 17•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b329d19647b5
https://hg.mozilla.org/mozilla-central/rev/8ab3174e31a5
https://hg.mozilla.org/mozilla-central/rev/32728ff57fcf
https://hg.mozilla.org/mozilla-central/rev/df95b504dc3b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•