Brian Birtles (:birtles) Assignee
	Description • 9 years ago

We need to make our style updating include *all* animations not just those in AnimationCollections. This bug tracks that.

This is possibly the last part of bug 1190235 but it's massive so I might split off more parts yet.

	Brian Birtles (:birtles) Assignee
	Comment 1 • 9 years ago

WIP patch of roughly what I intend to do. This patch doesn't actually work (it compiles and runs, but animations are broken) but it outlines the structure of what I think this will end being.

	Brian Birtles (:birtles) Assignee
	Comment 2 • 9 years ago

I'm going to try to break this up into steps that can be verified and landed individually since this is pretty tricky code full of all sorts of dirty flags and timestamps just begging for regressions. At very least, if I can avoid too many big changes we can bisect any regressions that do pop up down to something fairly easy to understand.

  Goal: Add ComposeStyleRule that works off the effect set
  * Add AnimationLevel to EffectCompositor (use with AppliesToTransitionsLevel)
  * Move mStyleRule to the EffectSet
  * Add static ComposeStyleRule to EffectCompositor and call from EnsureStyleRule
    -- convert as many call sites as possible to using ComposeStyleRule directly

  Goal: Get rid of mStyleChanging (and fix the layer update problem at the same time)
  * Move request restyle down to the effect
  * Store previous progress and don't request restyle if it hasn't changed
  * Try removing mStyleChanging
  * Post a layer update whenever an Animation is newly finished
    (Rename Animation::PostUpdate?)
  VERIFY: We are still skipping style updates for compositor animations
    
  Goal [[BIG]]: Get rid of the style rule refresh time
  * Add a compositor per pres context
  * Define a hashset keyed on RefPtr<Element> and pseudo-type
    Add a hashset: mElementsToRestyle and add to it from
    QUESTION: Do we need to add two maps here? One for each level?
  * In RequestRestyle::Standard, add elements to the hashset.
    Try to drop mHasPendingAnimationRestyle flag at this point by using the IsEmpty status of the map instead
  * Instead of checking the rule refresh time, check if it is in the set or not.
    If it's not, it doesn't need updating.
    Once we've updated the rule, remove the animation from the hashset.
  * Move RequestRestyle to EffectCompositor
  * Update end of UpdateCascadeResults to call the local RequestRestyle
  VERIFY: We are still skipping style updates for compositor animations
    
  Goal: Drop EnsureStyleRule and just call ComposeStyleRule directly
    Do the checks inside ComposeStyleRule? Call it UpdateStyleRule?
  VERIFY: We are still skipping style updates for compositor animations
    
  Goal: Move GetAnimationRule to EffectCompositor
  
  Goal: Track throttled elements in the compositor
  * Add another hashset for throttled animations? (One for each level?)
  * Update hashset in RequestRestyle
  VERIFY: This updating works as expected
  * Implement AddThrottledStyleUpdates
  * Use AddThrottledStyleUpdates
  * Drop old AddStyleUpdates
  VERIFY: We only add the throttled updates
  
  Goal: Move FlushAnimations to the compositor
  (Need to work out what it is actually doing!)
  
  Goal: Move style rule processors to the compositor
  
  Goal: Tidy up
  * Move ClearIsRunningOnCompositor to EffectCompositor?
  * Redo ContentOrAncestorHasAnimation
  * Remove any other redundant methods (including empty NeedRefreshes())
  * Remove linked-list from collections?
  * Move AnimStyleValue stuff to dom/animation?
  * Rename nsAnimationManager -> mozilla::CSSAnimationBuilder
  * Likewise nsTransitionManager -> mozilla::CSSTransitionBuilder
  * Separate out files

	Hiroyuki Ikezoe (:hiro)
	Comment 3 • 9 years ago

Do you have a plan to expose the status which indicates the animation is finished but not composed the last frame yet?

https://bugzilla.mozilla.org/attachment.cgi?id=8695113&action=diff#a/dom/animation/Animation.cpp_sec3

If we expose the status, we can check it in AddStyleUpdatesTo to skip unnecessary styling processes in bug1219236.

	Brian Birtles (:birtles) Assignee
	Comment 4 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Do you have a plan to expose the status which indicates the animation is
> finished but not composed the last frame yet?
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8695113&action=diff#a/dom/
> animation/Animation.cpp_sec3
> 
> If we expose the status, we can check it in AddStyleUpdatesTo to skip
> unnecessary styling processes in bug1219236.

The idea in that patch is that AddStyleUpdatesTo is replaced with AddThrottledStyleUpdatesTo. The EffectCompositor maintains a list of elements with throttled. When AddThrottledStyleUpdatesTo is called it adds only the elements in that list and then clears the list. It also clears that list whenever RequestRestyle > Standard is called on the element.

That's the idea, anyway, I'm not sure if it really works. But if it does it would mean we never add unnecessary style updates in AddStyleUpdatesTo.

	Brian Birtles (:birtles) Assignee
	Comment 5 • 9 years ago

All dependent bugs are now fixed, marking resolved.