Closed Bug 1226091 Opened 4 years ago Closed 4 years ago

Set and use MayHaveAnimations flag in pseudo-elements too

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: birtles, Assigned: birtles)

Details

Attachments

(2 files)

Olli points out in bug 1225699 comment 7 that we could probably use the MayHaveAnimations element flag on pseudo-elements too and that would speed up a few places like UnbindOnTree.

I'm not sure if there's any reason we don't currently set that.
Presumably you'd set the flag on the real element content node rather than the pseudo-element content node?
You would set the flag on whatever element has one or more of
nsGkAtoms::animationsProperty | nsGkAtoms::animationsOfBeforeProperty | nsGkAtoms::animationsOfAfterProperty | nsGkAtoms::transitionsProperty | 
nsGkAtoms::transitionsOfBeforeProperty | nsGkAtoms::transitionsOfAfterProperty
properties.

Then UnbindFromTree could check the flag before trying to delete any of those properties.

Currently the flag seems to be used only in CommonAnimationManager::GetAnimationCollection
which would then check the flag before trying to get the property.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Comment on attachment 8708195 [details] [diff] [review]
Use MayHaveAnimations flag for animations on pseudo elements too

(Not about this bug, but mumbling about Pair, such a horrible API :) first and second are just so random names for anything.)

I wonder if we could have somewhere DEBUG code asserting that whenever
we have effect properties, MayHaveAnimations is true.
Perhaps in  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN
Something like
#ifdef DEBUG
  nsIAtom** effectProps = EffectSet::GetEffectSetPropertyAtoms();
  for (uint32_t i = 0; effectProps[i]; ++i) {
    MOZ_ASSERT_IF(tmp->GetProperty(effectProps[i]), tmp->MayHaveAnimations());
  }
#endif
Attachment #8708195 - Flags: review?(bugs) → review+
I noticed we can use this in UnbindFromTree too
Attachment #8708902 - Flags: review?(bugs)
Comment on attachment 8708902 [details] [diff] [review]
Use MayHaveAnimations in Element::UnbindFromTree

Now that I think of this, might be nice to rename MayHaveAnimations
to MayHaveAnimationsOrTransitions or some such, but looks like we do mix
transitions to animations elsewhere too, like 
nsAnimationManager::GetAnimationsAtom() is about animation, 
nsTransitionManager::GetAnimationsAtom() is about transition.
So if those aren't fixed too, no need to fix the MayHave* method name, and definitely no need to fix naming in this bug.

I admit this is just a view of someone who doesn't work on transitions and animations. Perhaps it is clear to everyone else that transitions are sort-of animations.
Attachment #8708902 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/55298ac22503
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8708902 [details] [diff] [review]
> Use MayHaveAnimations in Element::UnbindFromTree
> 
> Now that I think of this, might be nice to rename MayHaveAnimations
> to MayHaveAnimationsOrTransitions or some such, but looks like we do mix
> transitions to animations elsewhere too, like 
> nsAnimationManager::GetAnimationsAtom() is about animation, 
> nsTransitionManager::GetAnimationsAtom() is about transition.
> So if those aren't fixed too, no need to fix the MayHave* method name, and
> definitely no need to fix naming in this bug.
> 
> I admit this is just a view of someone who doesn't work on transitions and
> animations. Perhaps it is clear to everyone else that transitions are
> sort-of animations.

Yes, I agree. We're moving towards "animations" being the generic term and using "CSS animations" when we're specifically referring to them (and then "CSS transitions" instead of "transitions" for consistency). I'm doing some renaming in bug 1239945 to that effect.
You need to log in before you can comment on or make changes to this bug.