1166500 - [Power] Invisible Paint-only CSS animations are still restyled every refresh driver tick

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Flanagan [:djf]]

This is a followup to bug 962594 and bug 1163789.

I've found that (on FirefoxOS, at least) it is possible to have the CPU busy because of a CSS animation that is not visible.

This was happening for the FirefoxOS media apps (Gallery, Music, and Video), and on our Flame reference devices, the CPU usage would never go below 2%. See bug 1163789 for details.)

In all of these apps we had spinners or animated progress bars that were nested inside of another container.  Here's what I found when preparing a gaia-based workaround for those apps:

1) Hiding (with display:none) the container element is not enough by itself to stop the animation on the contained element. Even though the contained element is no longer visible, the animation keeps running.

2) Hiding the contained element, or clearing the CSS animation property on the contained element does stop the animation, if that is all I do...

3) But (and this is the main bug I'm complaining about here) if I hide the container element and at the same time I also hide the contained element or remove the CSS animation property from the contained element, then the animation does not stop.  In order to get the animation to stop and the CPU to go down to 0% usage, I have to hide the contained element, or remove the CSS animation property, then use a setTimeout to wait 100ms and then hide the container element.

I think I've worked around this for our core media apps in gaia, but I suspect it will still affect web content, and should really be fixed.  This affects FirefoxOS 2.2, so I'm going to nominate it as a blocker.

Comment 1

[David Flanagan [:djf]]

Kats: you worked on bug 962594. Cameron: you reviewed the patch for that bug. Could either of you take a look at this one?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For paint-only animations we can fix this.  It would also fix bug 1105509.  I have ideas on how to do so, but in a meeting now.

Comment 3

[David Flanagan [:djf]]

Attachment: reduced test case

The attached test file demonstrates the bug in Firefox 38 (and presumably others) on  Mac desktop.

STR:

- Save the attachment to your /tmp dir
- Open a new instance of Firefox and load /tmp/bug.html into its single tap
- Open Activity Monitor or some similar tool and verify that the Firefox  process CPU usage drops to 0 or near 0

- Click on the "Start animation" button and see an animation. Verify that CPU usage goes way up.

- Click on the "Stop animation" button and see the animation stop. Verify that CPU usage goes back down to 0. In this case the animation was stopped by removing the CSS animation property. It works. The bug occurs when we want to stop the animation and also hide the animated element.

- Start the animation again, then click on "Hide the container". This sets hidden=true on the parent element of the animation. Note that the animation disappears, but that CPU usage does not go to 0. I suspect that you already have some other bug filed for this case. Maybe this is bug 1105509?

- Start the animation again, then click on "Hide the colors". This button sets hidden=true on the animated element itself. In firefox 38 the CPU usage does not go to zero. I suspect that this is bug 962594, and that if I tried this in Nightly I'd find that the CPU usage does go to zero in this case. But I haven't tried yet. This isn't really the point of this bug.

- Start the animation again, then click the "Bug: stop then hide container" button, and note that the CPU usage does not go to zero. This button stops the animation just like the "Stop animation" button does, but it then hides the container element. Somehow that combination of actions does not allow the CPU to go to 0. This is the main point of this bug report.

- Finally, start the animation again, then click the "Workaround..." button. This button demonstrates the setTimeout() workaround I used in bug 1163789.

Comment 4

[David Flanagan [:djf]]

Ignore the first couple of steps in the STR above. You don't have to save the attachment to /tmp and then load it. You can just click on the attachment link to run it, of course.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Unfortunately I don't have cycles to look into this right now, and I don't really know this area of code - for the other bug I fixed I was mostly just following instructions from the layout folks. dbaron/cameron should be able to help you out. Thanks for the reduced test case!

Comment 6

[Josh Cheng [:josh]]

Hi David, Hi Cameron,
Please help to shed some light here when you are available. Thanks!

Comment 7

[Josh Cheng [:josh]]

[Blocking Requested - why for this release]:
2.2 CCed. Continue investigate in next release.

Comment 8

[Cameron McCormack (:heycam)]

David's probably in a better position to work on this than me.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

What I've observed in the failure case is that AnimationCollection which is corresponding to the animated element stays in mElementCollections because nsAnimationManager::CheckAnimationRules is not called for the animated element in the failure case. nsAnimationManager::CheckAnimationRules is called only for the outer element (id = container element).

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What I was thinking was something like the following:

(1) We need to keep track of what the associated style change is for an animation.  There are two possible approaches here:

    (1a) Call CalcDifference for two style contexts, one with the start value of the animation applied, and one with the end value, and store the resulting change hint.  This is tricky because we need to be careful about what the basis for computation is (and probably redo that computation when there's a style change).  But it also requires the least duplicated code and yields the most exact optimization.

    (1b) Maintain a separate list of properties for which animations are always (or sometimes) paint-only.

There might be some interesting edge cases with properties that are paint-only for all of the animation except for the start or end.  (I'm not actually sure if there are any like that, though.)

We probably also want to consider animations that are paint-and-overflow-area only (transform, filter), and update overflow areas only at lower frequency like we do for transforms being run on the compositor.  Though this might be harder if they might be about to move into the visible area.


(2) Rework the mess around AnimationCollection::CanThrottleAnimation, its callers (FlushAnimations/FlushTransitions), and the EnsureStyleRuleFlags so that we can throttle things that don't run on the compositor thread.  This is currently annoyingly different for animations and transitions.

(3) Mechanism for detecting whether the element is being painted.  I'm not really sure what the right approach is here; it's a little tricky since I'd like to be able to do this for elements that don't have layers at all.


(4) Actually turn on throttling of paint-only animations, based on (1), (2) and (3).  Given (1a) this would involve comparing against a mask of change hints that we consider paint-only, which probably includes at least nsChangeHint_RepaintFrame | SyncFrameView (??) | UpdateOpacityLayer | UpdateTransformLayer | SchedulePaint | NeutralChange, though this list would need to be considered more closely than I just did.


I think that's enough work for one bug, although it only gets us to where we're throttling the animations (which will substantially reduce CPU usage) but it doesn't actually get us to where the refresh driver isn't running.  We should probably have a separate bug or bugs about at least one of:
 (1) not needing to tick the refresh driver at all for throttled animations -- at least for those not involving updating overflow areas.  This would probably involve keeping a list of any throttled animations so that we fix them up the next time we update.  If we do this, we probably also want to avoid doing any work on refresh driver ticks for any animations that wouldn't cause refresh driver ticks, so that we don't have extra codepaths to test.
 (2) (maybe) telling the refresh driver that a new tick isn't needed for X period of time

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (busy Aug. 8-Aug. 30) from comment #10)
> (2) Rework the mess around AnimationCollection::CanThrottleAnimation, its
> callers (FlushAnimations/FlushTransitions), and the EnsureStyleRuleFlags so
> that we can throttle things that don't run on the compositor thread.  This
> is currently annoyingly different for animations and transitions.

I have patches for this part for bug 1188251 which will hopefully be ready for review by the end of today.

There are also some ideas in bug 1190235 for reducing style updates which might tie into this.

>  (1) not needing to tick the refresh driver at all for throttled animations
> -- at least for those not involving updating overflow areas.  This would
> probably involve keeping a list of any throttled animations so that we fix
> them up the next time we update.  If we do this, we probably also want to
> avoid doing any work on refresh driver ticks for any animations that
> wouldn't cause refresh driver ticks, so that we don't have extra codepaths
> to test.
>  (2) (maybe) telling the refresh driver that a new tick isn't needed for X
> period of time

We need to remember events too. Either these optimizations would not apply when we have registered event listeners or we'd need (2) to cover that.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I had a bit of a think about what else we need to do for (2) from comment 10
after discussing with hiro yesterday, and I think the following approach might
make sense:

a. Land bug 1188251

b. Fix bug 1151694 - in particular:
   i.  Store mIsRunningOnCompositor per-property on KeyframeEffect
   ii. Clear mIsRunningOnCompositor when we take an animation off a layer

c. Remove Animation::CanThrottle and Animation::mFinishedAtLastComposeStyle
   and remove calls to AnimationCollection::PostRestyle from Animation::Tick

d. Post restyles from KeyframeEffect::SetParentTime using something like this:

   void
   KeyframeEffectReadOnly::SetParentTime(Nullable<TimeDuration> aParentTime)
   {
     PostRestyleForUpdatedTime(mParentTime, aParentTime);
     mParentTime = aParentTime;
   }

   void
   KeyframeEffectReadOnly::PostRestyleForUpdatedTime(
     Nullable<TimeDuration> aOldTime, Nullable<TimeDuration> aNewTime)
   {
     // Check for animations without a target or properties
     if (!mTarget || mProperties.IsEmpty()) {
       return;
     }

     ComputedTiming timingBefore = GetComputedTimingAt(aOldTime, mTiming);
     ComputedTiming timingAfter  = GetComputedTimingAt(aNewTime, mTiming);

     // Look for newly-finished compositor animations
     // (HasPropertyRunningOnCompositor simply iterates over mProperties
     //  and returns true if it finds mIsRunningOnCompositor on any of the
     //  properties)
     if (timingAfter.mPhase == AnimationPhase_After &&
         HasPropertyRunningOnCompositor()) {
       GetCollection()->PostRestyle(RestyleType::Layer);
       return;
     }

     // Look for newly-started compositor-able animations
     // (This should fix a bug where we don't send animations to the
     //  compositor as soon as we otherwise could.)
     // (HasCompositorAnimatableComponent is basically
     //  AnimationCollection::CanPerformOnCompositorThread but only operating on
     //  this animation and without the checks for whether the animation is
     //  playing or not. Also it should return true if we have one or more
     //  properties that can be animated on the compositor. See (e) below)
     if (timingAfter.mPhase == AnimationPhase_Active &&
         timingBefore.mPhase != AnimationPhase_Active &&
         HasCompositorAnimatableComponent(mTarget)) {
       GetCollection()->PostRestyle(RestyleType::Layer);
       return;
     }

     // I think the above is enough. If we pause/resume an animation we also need
     // need to update layers. For pausing/resuming using the API the
     // corresponding API method already does the PostRestyle(RestyleType::Layer).
     // For pausing/resuming from style CheckAnimationRule will update the layer
     // generation (which we'll check below) as well as clearing other state that
     // should mean we end up updating layers but we might run up against the
     // "value hasn't changed" optimization in RestyleManager that means we don't
     // update the layer until the next tick. If that's the case, we might need to
     // pass down some extra state from the Animation to detect that case.

     // Look for no-change animations
     // (The mCurrentIteration comparison is needed for animations that
     //  have an iteration composite operation other than "replace")
     if (timingAfter.mProgress == timingBefore.mProgress &&
         timingAfter.mCurrentIteration == timingBefore.mCurrentIteration) {
       return;
     }

     // Check if all animations are running on the compositor
     bool allPropertiesAreOnCompositor = true;
     for (auto iter = mProperties.cbegin();
          allPropertiesAreOnCompositor &&
            iter != collection->mAnimations.cend();
          ++iter) {
       allPropertiesAreOnCompositor &= (*iter)->mIsRunningOnCompositor;
     }
     if (allPropertiesAreOnCompositor) {
       // Pull in additional checks from
       // AnimationCollection::CanThrottleAnimation to check that:
       // - there is a layer (needed? Presumably if mIsRunningOnCompositor is
       //   accurate this is already the case? What if we just set display:none?)
       // - the layer generation is up-to-date
       // - we don't need to update the transform overflow region
       //   (basically AnimationCollection::CanThrottleTransformChanges)
       GetCollection()->PostRestyle(RestyleType::Throttled);
       return;
     }

     // TODO: Do similar checks for no paint animations here
     // PostRestyle(RestyleType::Throttled) if all properties
     // produce no paint animations

     GetCollection()->PostRestyle(RestyleType::Animation);
   }

   One issue here is that currently there is no KeyframeEffect::GetCollection.
   We'll need to add this eventually, perhaps we'll add a pointer back to the
   owning Animation and then fetch its collection. I'm not sure yet.

e. Clean up AnimationCollection

   * Drop the calls to CanPerformOnCompositorThread and CanThrottleAnimation
     from AnimationCollection::RequestRestyle and the "upgrading" logic there
   * Drop AnimationCollection::CanThrottleAnimation altogether
   * Rework AnimationCollection::GetAnimationsForCompositor to reuse some of the
     logic from the above-mentioned
     KeyframeEffect::HasCompositorAnimatableComponent method.

     We're basically reworking AnimationCollection::CanPerformOnCompositorThread
     and AnimationCollection::CanAnimatePropertyOnCompositor into something
     like:

       KeyframeEffect::CanAnimatePropertyOnCompositor
         -- Based on AnimationCollection::CanAnimatePropertyOnCompositor but
            *doesn't* include the "allow partial" behavior--it simply checks
            if the passed-in property can be compositor-animated or not.
            Also checks if we have a geometric property (previously performed by
            AnimationCollection::CanPerformOnCompositorThread).
            Also includes the check for IsCompositorAnimationDisabledForFrame?

       KeyframeEffect::HasCompositorAnimatableComponent
         -- Calls CanAnimatePropertyOnCompositor and returns true if one or
            more property is compositor-animatable.
            Unlike AnimationCollection::CanAnimatePropertyOnCompositor does
            *not* check if the animation is playing or not.

       AnimationCollection::GetAnimationsForCompositor
         -- Goes through all the animations and looks for at least one where
            HasCompositorAnimatableComponent returns true AND is playing.

   * Drop AnimationCollection::CanPerformOnCompositorThread.

I think this arrangement would be more sensible that what we currently have. It
would also allow us to skip updating style altogether for animations that are
finished, or waiting to start, or which are using a step timing function where
the output doesn't change between ticks.

The main problem I see is that the knowledge that, "Animations that are not
playing do not run on the compositor" is still shared between the
AnimationCollection and KeyframeEffect. It would be good to make one class
responsible for defining that behavior.

Also this doesn't help with stopping the refresh driver. I'm considering that
a separate bug. For that we'd need to ensure not only that we have no event
listeners registered but also no Promise listeners. We might be able to do it
initially for animations that run indefinitely since they're fairly common.

Even just making the refresh driver wait a specified period of time is probably
a fair bit of work and would have to take into account animationiteration event
listeners.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

David, thank for the detailed descriptions. Brian, thank you for helping me understand the descriptions.

I have two questions:

1) Can we skip style updates for paint-only and offscreen animations at all?
 I think if the animation does not cause overflow we can skip style updates for the animation. Am I missing something? (I would like to leave overflow animations optimization as another bug.)

2) I think we should ensure that building layer process has to be done at least once in order to decide whether the animation is offscreen with the check of corresponding layer existence.  That's because if the building layer process has not done yet, the corresponding layer does not exist. Is this right?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I should note about transform animations.
For optimization of transform animations we need to handle cases that the determinant of transform matrix becomes 0[1] or backface-visibility:hidden[2].

[1] http://hg.mozilla.org/mozilla-central/file/01a75ffad102/layout/base/FrameLayerBuilder.cpp#l4956
[2] http://hg.mozilla.org/mozilla-central/file/01a75ffad102/layout/base/nsDisplayList.cpp#l5243

In my current implementations KeyframeEffectReadOnly has a flag named mHasSingularTransforms to represent these cases because KeyframeEffectReadOnly stores change hints on each segment then it's easy to use them in the class.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> 1) Can we skip style updates for paint-only and offscreen animations at all?
>  I think if the animation does not cause overflow we can skip style updates
> for the animation. Am I missing something? (I would like to leave overflow
> animations optimization as another bug.)

Yes, if it is both paint-only and offscreen we can skip style updates.  (But we still need to keep the refresh driver active to send events at the right time.)

However, we need to update the style when there's a flush, just like we do for compositor-thread animations; see RestyleManager::UpdateOnlyAnimationStyles.  So we still need to run the refresh driver (although if we do bug 1207014, which I just filed, we could slow it down -- but that's a longer term project).

> 2) I think we should ensure that building layer process has to be done at
> least once in order to decide whether the animation is offscreen with the
> check of corresponding layer existence.  That's because if the building
> layer process has not done yet, the corresponding layer does not exist. Is
> this right?

see comment 10, point (3).  I think we want to make the offscreen test not involve layers.  So I don't think we should do this.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> I should note about transform animations.
> For optimization of transform animations we need to handle cases that the
> determinant of transform matrix becomes 0[1] or
> backface-visibility:hidden[2].

Now that bug 1097464 has landed, shouldn't the handling of the transform on the compositor handle this?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #15)
> > 2) I think we should ensure that building layer process has to be done at
> > least once in order to decide whether the animation is offscreen with the
> > check of corresponding layer existence.  That's because if the building
> > layer process has not done yet, the corresponding layer does not exist. Is
> > this right?
> 
> see comment 10, point (3).  I think we want to make the offscreen test not
> involve layers.  So I don't think we should do this.

Other way that I found is using FrameLayerBuilder::HasRetainedDataFor or setting a flag to descendant frmes which have animations at [1] in DisplayLine in nsBlockFrame.

The former looks essentially as the same as the check of layer existence to me. I will try the latter way.

[1] http://hg.mozilla.org/mozilla-central/file/737517ce8115/layout/generic/nsBlockFrame.cpp#l6402

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Retitling this bug to reflect the work that seems reasonable to do in one bug.  Bug 1207014 covers actually reducing the number of refresh driver ticks, which is somewhat harder and rather separate.

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Matt, what do you suggest is the best way to detect if an element is being painted. Specifically we're talking about point (3) from comment 10. It's easy enough to test if there is a corresponding layer for an element but we'd like to be able to apply this optimization to animation of color, border-color etc. which don't have layers.

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to Brian Birtles (:birtles) from comment #19)
> Matt, what do you suggest is the best way to detect if an element is being
> painted. Specifically we're talking about point (3) from comment 10. It's
> easy enough to test if there is a corresponding layer for an element but
> we'd like to be able to apply this optimization to animation of color,
> border-color etc. which don't have layers.

FrameLayerBuilder::IterateRetainedDataFor will probably work for this.

You could also add an overload of FrameLayerBuilder::HasRetainedDataFor that doesn't take a display item key parameter, and returns true if any are present.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> (In reply to Brian Birtles (:birtles) from comment #19)
> > Matt, what do you suggest is the best way to detect if an element is being
> > painted. Specifically we're talking about point (3) from comment 10. It's
> > easy enough to test if there is a corresponding layer for an element but
> > we'd like to be able to apply this optimization to animation of color,
> > border-color etc. which don't have layers.
> 
> FrameLayerBuilder::IterateRetainedDataFor will probably work for this.
> 
> You could also add an overload of FrameLayerBuilder::HasRetainedDataFor that
> doesn't take a display item key parameter, and returns true if any are
> present.

As far as I confirmed, FrameLayerBuilder::IterateRetainedDataFor can not be used for 'color' animation if the animation element does not have some kind of properties (e.g. background, border, etc.).

Comment 22

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)
> (In reply to Brian Birtles (:birtles) from comment #19)
> > Matt, what do you suggest is the best way to detect if an element is being
> > painted. Specifically we're talking about point (3) from comment 10. It's
> > easy enough to test if there is a corresponding layer for an element but
> > we'd like to be able to apply this optimization to animation of color,
> > border-color etc. which don't have layers.
> 
> FrameLayerBuilder::IterateRetainedDataFor will probably work for this.
> 
> You could also add an overload of FrameLayerBuilder::HasRetainedDataFor that
> doesn't take a display item key parameter, and returns true if any are
> present.

I will try this way even if some kind of 'color' animations can not be throttled.

Thanks, Matt!

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> As far as I confirmed, FrameLayerBuilder::IterateRetainedDataFor can not be
> used for 'color' animation if the animation element does not have some kind
> of properties (e.g. background, border, etc.).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> I will try this way even if some kind of 'color' animations can not be
> throttled.


Isn't the risk there that you will incorrectly throttle animations that are actually visible?  (For example, an animation of 'color' that only applies through inheriting to descendants.)  That doesn't seem acceptable.

Comment 24

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #23)

> Isn't the risk there that you will incorrectly throttle animations that are
> actually visible?  (For example, an animation of 'color' that only applies
> through inheriting to descendants.)  That doesn't seem acceptable.

Right, for a given animation we'll need the set of frames that might need to paint differently in response to it. It seems like this will often be more than just the primary frame for the element with the animation.

We also need to decide which properties just change existing painting, and which ones might paint new things, paint a larger/smaller area of pixels or move painted content.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #24)
> Right, for a given animation we'll need the set of frames that might need to
> paint differently in response to it.

That sounds like it will often be quite expensive.  It would be cheaper to do something with the visual overflow rect of the frame, if there's something we can do with it.

Comment 26

[Matt Woodrow (:mattwoodrow)]

We could definitely start with the overflow rect of the frame (maybe old+new combined) in order to quickly eliminate frames (and descendants) that definitely don't need to be repainted.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> > I should note about transform animations.
> > For optimization of transform animations we need to handle cases that the
> > determinant of transform matrix becomes 0[1] or
> > backface-visibility:hidden[2].
> 
> Now that bug 1097464 has landed, shouldn't the handling of the transform on
> the compositor handle this?

Unfortunately as far as I can confirm, bug 1097464 did not solve the singular transform issue.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> We could definitely start with the overflow rect of the frame (maybe old+new

Mattwoodrow, I'd like to clarify what the frame means and what the overflow means here.

I think the frame means primary frame (or pseudo frame for animations on pseudo element) and
the overflow rect means the rect of the primary frame.

A pseudo code like below:

bool WasRendered(aFrame, aRect) {
  if (HasRetainedDataFor(aFrame)) {
    return true;
  }

  for (nsIFrame *child : aFrame.GetChildren()) {
    if (!aRect.Intersects(child->GetVisialOverflowRect())) {
      continue;
    }
    if (WasRendered(child, aRect)) {
      return true;
    }
  }
  return false;
}

bool WasRendered(aFrame) {
  return WasRendered(aFrame, aFrame->GetVisualOverflowRect());
}

// offscreen check
if (!WasRendered(primaryFrame)) {
  // In case of offscreen, ...
}

Is this code what you has in mind?

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Reftests to detect regressions caused by offscreen optimization

I think it's worth to adding reftests which could detect regressions (i.e. offscreen optimization does not over-throttle visible animations) whatever optimization will be applied.

This patch has three test cases:

* offscreen-animation-partial-out-of-view.html
This test is a normal test case (i.e. a trivial one). An animation on an
element which is partially out of the view. If ofscreen optimization
is totally broken, this test will fail.

* offscreen-animation-pseudo-element.html
This test checks animation on visible pseudo element which is attached to
invisible element is not throttled.

* offscreen-animation.html
This test checks animation on visible element which is inherited from
invisible parent element is not throttled.


Note about automation tests for this issue. I was thinking we can provide automation tests with Frame Timing API but the Frame Timing spec has been changed, it can not be used for the test now. I have no idea how we can write any type of automation tests.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Not quite.

The visual overflow rect for a frame includes the visual overflow rects for all descendant frames already.

So we only need to take the overflow rect for the primary frame, convert it from coordinates relative to the current frame into coordinates relative to the window, and then see if it intersects what we're going to paint.

We do something similar for deciding if we should decode images, but I forget exactly where that code is. Adding ni?tn to point that out :)

Note that this is only sufficient for style changes that won't modify the overflow rect (like a colour changes). For ones that do, we will need to make the style change, run reflow (to recompute overflow rects), and then check again with the new overflow rect.

Comment 31

[Timothy Nikkel (:tnikkel)]

Visibility testing for images has one significant wrinkle: it also checks if the image is scrolled out of view (but only a limited amount).

nsLayoutUtils::UpdateImageVisibilityForFrame checks if one give image frame is visible:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=001f7d3139ce#7692

PresShell::MarkImagesInSubtreeVisible checks all image frames in a document tree.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=ac11fbd3b96a#5680

Also, I think Seth is working on expanding this work to work for CSS images (like background) as well.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Thank you both for the helps!

(In reply to Matt Woodrow (:mattwoodrow) from comment #30)

> So we only need to take the overflow rect for the primary frame, convert it
> from coordinates relative to the current frame into coordinates relative to
> the window, and then see if it intersects what we're going to paint.

If I understand what you are saying correctly, we have to care about animations on visibility:hidden element case separately.
I will try the combination of overflow rect check and StyleVisibility() check.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8673326 [details] [diff] [review]
Reftests to detect regressions caused by offscreen optimization

"offscreen" implies that the position of the element is out of the viewport; two of the three tests here are actually testing interaction of visibility:hidden and visibility:visible, so they should have names reflecting that.

I'd suggest:
offscreen-animation -> in-visibility-hidden-animation
offscreen-animation-pseudo-element -> in-visibility-hidden-animation-pseudo-element
offscreen-animation-partial-out-of-view -> partially-out-of-view animation

Also, in offscreen-animation-partial out-of-view, it would be better to use an explicit height rather than assume that "0123456789 0123456789" doesn't fit within width:10em (which might be possible with a narrow font)

r=dbaron with that

Comment 34

[Hiroyuki Ikezoe (:hiro)]

I've opened a new bug corresponding to #comment10 (2) and comment#12 e because that part is harder/bigger/cumbersome than I thought.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> I've opened a new bug corresponding to #comment10 (2) and comment#12 e
> because that part is harder/bigger/cumbersome than I thought.

This is bug 1216030, I assume.

Comment 36

[Mahendra Potharaju [:mahe]]

Hiro, 

Is this going to land soon, looks like the review is already done.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

It's just automation tests, not fix.

FWIW, here is a try with WIP patches.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ef3590cc20b

Comment 38

[Hiroyuki Ikezoe (:hiro)]

OK. Bug 1216030 will be landed soon.

Here in this bug, I'd like to handle throttling below two cases separately:

a) animations on visibility:hidden element
b) animations on element which is out of view

a) can be throttled paint-only and transform(UpdatePoseTransformOverflow), and transform animations are unthrottled every 200ms like visible transform animations do.

b) can be throttled paint-only. To throttle transform animations we need to calculate over flow rect on post transform before restyling process. I'd like to leave this issue as another bug.

'paint-only' means that the same as dbaron describied in comment #10:
 NS_STYLE_HINT_VISUAL(RepaintFrame,SyncFrameView,SchedulePaint), NeutralChange, UpdateOpacityLayer and UpdateTransformLayer.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 0: Reftests to detect regressions caused by offscreen optimization

Addressed comment #33.

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Add KeyframeEffectReadOnly::AppendSegment

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Store change hints between from and to for each animation segment.

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Add KeyframeEffectReadOnly::SetProperties.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Add ElementPropertyTransition::SetTransitionProperty

This is needed for calculation of cumulative change hints

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Calculate cumulative change hints

Note that CalculateCumulativeChangeHints in nsAnimationManager::BuildAnimations.
For newly-generated animations in BuildAnimations there is no trigger to
calculate cumulative hints in KeyframeEffectReadOnly class. So in this patch
CalculateCumulativeChangeHints is called after all CSS properties are processed.
(i.e. After all of animation's properties are generated from CSS properties)

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: Add KeyframeEffectReadOnly::IsPaintOnly()

nsChangeHint_UpdatePostTransformOverflow will be supported in a subsequent
patch.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 7: Add nsIFrame::IsOutOfView

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 8: Throttle paint-only animations on element which is out of view

Comment 48

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 9: Add nsIFrame::IsInvisible

This method checks all descendant frames' style visibility.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 10: Throttle animations on invisble element

Comment 50

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8684188 [details] [diff] [review]
Part 7: Add nsIFrame::IsOutOfView

Review of attachment 8684188 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +9048,5 @@
> +nsIFrame::IsOutOfView()
> +{
> +  nsRect rect = GetVisualOverflowRect();
> +
> +  nsIFrame* rootFrame = nsLayoutUtils::GetContainingBlockForClientRect(this);

I think you want to use nsLayoutUtils::GetDisplayRootFrame for this, it returns the frame that will be the root frame for the window we paint.

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8684183 [details] [diff] [review]
Part 3: Add KeyframeEffectReadOnly::SetProperties.

Review of attachment 8684183 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/KeyframeEffect.cpp
@@ +158,5 @@
>  }
>  
> +void
> +KeyframeEffectReadOnly::SetProperties(
> +  const InfallibleTArray<AnimationProperty>& aProperties)

Should this take a universal reference?

  Then you could do effect.SetProperties(MyFuncThatReturnsAPropArray());

But given how this is used, I think we should just define:

  KeyframeEffectReadOnly::SwapProperties(KeyframeEffectReadOnly* aEffect)

What do you think?

We could also replace SetTiming with SwapTiming but the AnimationTiming structure is less expensive so it's probably fine as-is.

Comment 52

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8684180 [details] [diff] [review]
Part 1: Add KeyframeEffectReadOnly::AppendSegment

Review of attachment 8684180 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine with the change to making AppendSegment a regular method on AnimationProperty. I don't know if the style contexts we're passing make sense but I presume David will check that when he reviews part 2.

::: dom/animation/KeyframeEffect.cpp
@@ +518,5 @@
>    }
>  }
>  
> +/* static */ void
> +KeyframeEffectReadOnly::AppendSegment(AnimationProperty* aAnimationProperty,

As discussed, we could just make this a regular property of AnimationProperty.

@@ +527,5 @@
> +                                      float aToKey,
> +                                      nsStyleContext* aToContext,
> +                                      ComputedTimingFunction& aTimingFunction)
> +{
> +  MOZ_ASSERT(aAnimationProperty, "AnimationProperty should be valid");

Then we wouldn't need this assertion.

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684182 [details] [diff] [review]
Part 2: Store change hints between from and to for each animation segment.

>+
>+  if (!aFromContext || !aToContext) {
>+    // FIXME! In case of KeyframeEffect constructor we need to calculate
>+    // change hint when target element is set.
>+    return;
>+  }

What do you plan to do about this?  (I don't really know what the callers that pass null contexts do.  Could you explain?)

>+  uint32_t equalStructs = 0;
>+  uint32_t samePointerStructs = 0;
>+  segment->mChangeHint =
>+    aFromContext->CalcStyleDifference(aToContext,
>+                                      nsChangeHint(0),
>+                                      &equalStructs,
>+                                      &samePointerStructs);

So we need to worry about two things here:
 (1) whether this CalcStyleDifference call will produce all the hints we need
 (2) whether this CalcStyleDifference call will produce extra hints that we don't want

I think it's clear that (2) is a problem for both transitions and animations:

It's a problem for transitions because you're directly comparing the old and new style contexts, without any filtering for which properties are transitionable.  The change hints for the separate properties should also be associated with the correct animations rather than all of them being associated with all animations resulting from a particular style change.

It's a problem for animations for the same two reasons, although the issue of filtering for interpolable properties might go away when we support animations of all properties rather than only interpolable ones (if that turns out to be Web-compatible).

What makes (1) interesting is that CalcStyleDifference is only *guaranteed* to calculate the difference for structs that have been retrieved for the context.  It is allowed (although not required -- though I briefly made it so in bug 1209603 but undid that in bug 1216431) to skip reporting differences for structs that have never been retrieved, since if nobody has asked for the data, nobody cares about the result.  I think it looks like (1) is OK, because nsAnimationManager::BuildSegment and nsTransitionManager::ConsiderStartingTransition both call ExtractComputedValueForTransition before they append the segment.  However, you should clearly document this assumption, since it's important and non-obvious.



Instead of this, I think you should probably construct a new style context with ResolveStyleByAddingRules, by adding a freshly created AnimValuesStyleRule that accounts for only the difference you care about.

I don't *think* there should be problems with the unions of hints from different animations needing to produce a more significant hint than just unioning the bits together -- though I could be wrong.  It might be worth checking.

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684185 [details] [diff] [review]
Part 4: Add ElementPropertyTransition::SetTransitionProperty

Doesn't seem to add much, but it's fine.

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684186 [details] [diff] [review]
Part 5: Calculate cumulative change hints

># HG changeset patch
># User Hiroyuki Ikezoe <[e-mail]>
># Parent  c265b3b148eca009367dbf6cecec2bc6a6be0525
>Bug 1166500 - Part 5: Calculate cumulative change hints. r=dbaron
>
>Note that CalculateCumulativeChangeHints in nsAnimationManager::BuildAnimations.

This sentence is missing a verb.

>For newly-generated animations in BuildAnimations there is no trigger to
>calculate cumulative hints in KeyframeEffectReadOnly class. So in this patch
>CalculateCumulativeChangeHints is called after all CSS properties are processed.
>(i.e. After all of animation's properties are generated from CSS properties)

This doesn't seem like a problem.  But if you leave it, lowercase "a" following "i.e.", and maybe a comma, i.e., "i.e., after...".  Though I think the FIXME in the code comment is enough, and you don't need to mention it again here.

>+  , mCumulativeChangeHints(nsChangeHint(0))

I think this should be called mCumulativeChangeHint (no "s" at the end)

>+  CalculateCumulativeChangeHints();

Same with this.

>+    // FIXME: This calculation should be done in KeyframeEffectReadOnly.
>+    // This calculation will be removed once KeyframeEffectReadOnly
>+    // has a method to create aniamtion properties from KeyframeData.

It's not clear to me why this needs fixing, but I guess this part at least proposes a solution.

"aniamtion" -> "animation"

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684186 [details] [diff] [review]
Part 5: Calculate cumulative change hints

>+      NS_UpdateHint(mCumulativeChangeHints, segment.mChangeHint);

Oh, also, this should just use |=.  I'd like to remove NS_UpdateHint, etc.:

  mCumulativeChangeHint |= segment.mChangeHint;

Comment 57

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684187 [details] [diff] [review]
Part 6: Add KeyframeEffectReadOnly::IsPaintOnly()

># HG changeset patch
># User Hiroyuki Ikezoe <[e-mail]>
># Parent  aa5d5b3177cda05c4c0ccedfe94aa4d9ff9bb8e6
>Bug 1166500 - Part 6: Add KeyframeEffectReadOnly::IsPaintOnly(). r=dbaron

I think I'd prefer using "CanIgnoreIfNotVisible" rather than "IsPaintOnly" throughout, for both the method and the nsChangeHint.

And for the nsChangeHint, I think it should be nsChangeHint_Hints_CanIgnoreIfNotVisible, to indicate it's a set of hints.  (That's the convention we use for nsChangeHint_Hints_NotHandledForDescendants.)  nsChangeHint_PaintOnly is not a good name because somebody might return it rather than test against it.


>nsChangeHint_UpdatePostTransformOverflow will be supported in a subsequent
>patch.

Maybe mention that in a FIXME in the definition of nsChangeHint_Hints_CanIgnoreIfNotVisible instead of the commit message?

r=dbaron with that

Comment 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684190 [details] [diff] [review]
Part 9: Add nsIFrame::IsInvisible

>This method checks all descendant frames' style visibility.

It should, but it currently doesn't!

>+      if (child->StyleVisibility()->IsVisible()) {

This should recursively call itself on the child (and negate the result), rather than calling this!

>diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
>+  /**
>+   * Returns true if the frame and all descendants frame are
>+   * visibility:hidden.
>+   */
>+  bool IsInvisible();

This should have a better name to indicate that it's expensive, since it walks all descendants.  The comment should also say explicitly that it can be expensive, since it will walk all descendants before returning true.

I'd suggest IsSubtreeInvisible.


However, this isn't quite sufficient, since what you need to know is whether there are frames that inherit style from the element that are visible.  In order to know that you need to traverse through placeholder frames to their out-of-flows, but only when those out-of-flow frames are outside the subtree root that you started from.  (See nsFrame::Destroy and nsFrame::DestroyFrom for an example of the mechanics of this.)

I'm actually not sure this is even worth doing.  I think it's far more valuable to optimize animations on offscreen elements and background tabs than it is to worry about visibility:hidden.

So I tend to think it's better not to bother with this at all.

Comment 59

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #57)
> >nsChangeHint_UpdatePostTransformOverflow will be supported in a subsequent
> >patch.
> 
> Maybe mention that in a FIXME in the definition of
> nsChangeHint_Hints_CanIgnoreIfNotVisible instead of the commit message?

Actually, given the way you handle this in the later patch, it probably shouldn't be a FIXME, but instead a note that callers may also want to consider nsChangeHint_UpdatePostTransformOverflow, but they need to do so separately.

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So a few thoughts on patches 8 and 10 before I review them more closely:

One thing about throttling animations is that we need to have code that flushes throttled animations when the condition for which we throttled them is no longer true.  We need to check that this is the case.

Second, I think the important case being missed here is animations in background tabs, or other cases where PresShell::IsVisible returns false (or at least cases where it's false because mIsActive is false, because we can clearly find out when that changes).  Although I think the work in part 10 should not be used on top of the work in part 9, I think you'll be able to reuse the code from part 10 to handle the background tab case.

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684189 [details] [diff] [review]
Part 8: Throttle paint-only animations on element which is out of view

So before you do this, you need to make some other changes that I think aren't in this patch series.  This patch will lead to CanThrottle returning true even when compositor-thread animations are not enabled.  This means that you need to remove:

 * the test of nsLayoutUtils::AreAsyncAnimationsEnabled in PresShell::HandleEvent

 * the test of nsLayoutUtils::AreAsyncAnimationsEnabled in the assertion in nsTransitionManager::StyleContextChanged

(I also don't currently understand the test in nsRefreshDriver::Tick.)

That should be a separate patch before this one.



But there's also an issue with this patch, as I alluded to in my previous comment.  Or at least I think there is.  Is it possible for a frame to become in-view (e.g., via scrolling (maybe even APZC scrolling), or resizing, or movement of an iframe that contains it) and for a very old state to paint?  It seems like if we are throttling animations because frames are out-of-view, we need a mechanism to unthrottle them immediately if the frames in question move into view.  That might mean maintaining a list of frames that are throttled for being out-of-view, or something like that.

Or is there a reason you're confident we won't see a flash of incorrect data before the next tick?

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8684191 [details] [diff] [review]
Part 10: Throttle animations on invisble element

So as I said in my comments on patch 9 (comment 58), I don't think you should bother trying to do optimizations based on the 'visibility' property.  Doing it correctly is somewhat complicated, and it's not clear that the cost of the optimization will even be faster than what it's avoiding, in many cases.

However, I think you could reuse most of this patch (including the transform tests) to optimization based on hidden tabs, as I suggested in comment 60.

However, all of the issues I mentioned in comment 61 also apply here, although you probably don't need to maintain a list of frames throttled for this reason, since flushing all throttled animations (or at least all that are not out-of-view) when a tab becomes visible is easy enough.

Comment 63

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #53)

> Instead of this, I think you should probably construct a new style context
> with ResolveStyleByAddingRules, by adding a freshly created
> AnimValuesStyleRule that accounts for only the difference you care about.
> 
> I don't *think* there should be problems with the unions of hints from
> different animations needing to produce a more significant hint than just
> unioning the bits together -- though I could be wrong.  It might be worth
> checking.

Thanks for the detailed explanations! I will do it.

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #58)
> Comment on attachment 8684190 [details] [diff] [review]
> Part 9: Add nsIFrame::IsInvisible
> 
> However, this isn't quite sufficient, since what you need to know is whether
> there are frames that inherit style from the element that are visible.  In
> order to know that you need to traverse through placeholder frames to their
> out-of-flows, but only when those out-of-flow frames are outside the subtree
> root that you started from.  (See nsFrame::Destroy and nsFrame::DestroyFrom
> for an example of the mechanics of this.)
> 
> I'm actually not sure this is even worth doing.  I think it's far more
> valuable to optimize animations on offscreen elements and background tabs
> than it is to worry about visibility:hidden.
> 
> So I tend to think it's better not to bother with this at all.

OK. I thought visibility:hidden elements is the one of the main targets of optimizations in this bug, but I will leave the optimization for visibility:hidden as a future bug. I guess we could at least throttle in case of the frame has no children and the frame itself is visibility:hidden.

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Add AnimationProperty::AppendSegment v2

Addressed comment #52.
Arguments of style context have been removed because the style contexts is not needed for calculation of change hint. A pres context will be added instead in part 2.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Store change hints between from and to for each animation segment v2

Addressed comment #53.

(In reply to David Baron [:dbaron] ✈ from comment #53)
> Comment on attachment 8684182 [details] [diff] [review]
> Part 2: Store change hints between from and to for each animation segment.
> 
> >+
> >+  if (!aFromContext || !aToContext) {
> >+    // FIXME! In case of KeyframeEffect constructor we need to calculate
> >+    // change hint when target element is set.
> >+    return;
> >+  }
> 
> What do you plan to do about this?  (I don't really know what the callers
> that pass null contexts do.  Could you explain?)

In the previous patch I supposed that current implementation of KeyframeEffect constructor does not support passing target element but actually it does.

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Add KeyframeEffectReadOnly::SwapProperties.

Added KeyframeEffectReadOnly::SwapProperties instead of KeyframeEffectReadOnly::SetProperties.

Comment 67

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #61)
> Comment on attachment 8684189 [details] [diff] [review]
> Part 8: Throttle paint-only animations on element which is out of view
> 
> So before you do this, you need to make some other changes that I think
> aren't in this patch series.  This patch will lead to CanThrottle returning
> true even when compositor-thread animations are not enabled.  This means
> that you need to remove:
> 
>  * the test of nsLayoutUtils::AreAsyncAnimationsEnabled in
> PresShell::HandleEvent
> 
>  * the test of nsLayoutUtils::AreAsyncAnimationsEnabled in the assertion in
> nsTransitionManager::StyleContextChanged

Thanks for pointing it out. I will remove those checks in a separate patch.

> (I also don't currently understand the test in nsRefreshDriver::Tick.)

According to the commit message[1] when the check was added, the check is 
exactly for throttled animations in case of async animation is disable just like
we do in this bug. So I am going to leave the check there.

[1] https://hg.mozilla.org/mozilla-central/rev/163dbef74332

> But there's also an issue with this patch, as I alluded to in my previous
> comment.  Or at least I think there is.  Is it possible for a frame to
> become in-view (e.g., via scrolling (maybe even APZC scrolling), or
> resizing, or movement of an iframe that contains it) and for a very old
> state to paint?  It seems like if we are throttling animations because
> frames are out-of-view, we need a mechanism to unthrottle them immediately
> if the frames in question move into view.  That might mean maintaining a
> list of frames that are throttled for being out-of-view, or something like
> that.
> 
> Or is there a reason you're confident we won't see a flash of incorrect data
> before the next tick?

No, I am not. I thought it's OK because the next tick will come soon basically. So it will be hardly noticeable.  But you are right, once bug 1207014 is fixed (e.g. less ticks for less frequent animations), it will be a problem.

Comment 68

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8687141 [details] [diff] [review]
Part 3: Add KeyframeEffectReadOnly::SwapProperties.

>+void
>+KeyframeEffectReadOnly::SwapProperties(KeyframeEffectReadOnly* aEffect)
>+{
>+  if (mProperties == aEffect->Properties()) {
>+    return;
>+  }

I don't think this check is necessary.

r=birtles with the check removed.

Comment 69

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8684188 [details] [diff] [review]
Part 7: Add nsIFrame::IsOutOfView

Removing r?, waiting on changes based on comment #50.

Comment 70

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away 17 Nov) from comment #68)
> Comment on attachment 8687141 [details] [diff] [review]
> Part 3: Add KeyframeEffectReadOnly::SwapProperties.
> 
> >+void
> >+KeyframeEffectReadOnly::SwapProperties(KeyframeEffectReadOnly* aEffect)
> >+{
> >+  if (mProperties == aEffect->Properties()) {
> >+    return;
> >+  }
> 
> I don't think this check is necessary.

I was thinking the check will be necessary to fix bug 1219543. In that bug I thought animation properties should not be swapped if isRunningOnCompositor does not equal.  But anyway I will remove the check in this bug, and handle the check in bug 1219543.

Comment 71

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #60)
> Second, I think the important case being missed here is animations in
> background tabs, or other cases where PresShell::IsVisible returns false (or
> at least cases where it's false because mIsActive is false, because we can
> clearly find out when that changes).  Although I think the work in part 10
> should not be used on top of the work in part 9, I think you'll be able to
> reuse the code from part 10 to handle the background tab case.

In the case of background tabs (I think mIsActive is false at the same time), refresh driver just becomes an inactive state (i.e. less tick), nsIFrame::IsOutOfView() can be worked as well as on foreground tab I confirmed.
I don't know nsPresShell::IsVisible case yet, I will handle the case.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8687140 [details] [diff] [review]
Part 2: Store change hints between from and to for each animation segment v2

>+static already_AddRefed<nsStyleContext>
>+CreateTemporaryStyleContextForProperty(nsCSSProperty aProperty,
>+                                       StyleAnimationValue aValue,
>+                                       nsPresContext& aPresContext)
>+{
>+  RefPtr<nsStyleContext> emptyContext =
>+    NS_NewStyleContext(nullptr,
>+                       nullptr,
>+                       nsCSSPseudoElements::ePseudo_NotPseudoElement,
>+                       aPresContext.StyleSet()->GetRuleTree(),
>+                       true);
>+
>+  RefPtr<AnimValuesStyleRule> styleRule = new AnimValuesStyleRule();
>+  styleRule->AddValue(aProperty, aValue);
>+
>+  nsCOMArray<nsIStyleRule> rules;
>+  rules.AppendObject(styleRule);
>+
>+  RefPtr<nsStyleContext> tempContext =
>+    aPresContext.StyleSet()->ResolveStyleByAddingRules(emptyContext,
>+                                                       rules);
>+
>+  // We need to call StyleData to generate cached data for the style context.
>+  // Otherwise CalcStyleDifference returns no meaningful result.
>+  tempContext->StyleData(nsCSSProps::kSIDTable[aProperty]);
>+
>+  return tempContext.forget();
>+}

This is not at all what I was suggesting.

I believe you need to use the real style context for the element as a basis to add to, not this empty context, because properties have interactions that can change the way differences are computed.

Comment 73

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #71)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #60)
> > Second, I think the important case being missed here is animations in
> > background tabs, or other cases where PresShell::IsVisible returns false (or
> > at least cases where it's false because mIsActive is false, because we can
> > clearly find out when that changes).  Although I think the work in part 10
> > should not be used on top of the work in part 9, I think you'll be able to
> > reuse the code from part 10 to handle the background tab case.
> 
> In the case of background tabs (I think mIsActive is false at the same
> time), refresh driver just becomes an inactive state (i.e. less tick),

There are fewer ticks, but there are still some, and we don't need to restyle.

> nsIFrame::IsOutOfView() can be worked as well as on foreground tab I
> confirmed.

But it's also not as strong an optimization, since you need to deal with overflow updating.

> I don't know nsPresShell::IsVisible case yet, I will handle the case.

The cases where IsVisible returns false for reasons other than mIsActive are hard to handle because you don't know when that state changes.  Don't try to do that here.

Comment 74

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #72)

> This is not at all what I was suggesting.
> 
> I believe you need to use the real style context for the element as a basis
> to add to, not this empty context, because properties have interactions that
> can change the way differences are computed.

Hmm, but in CSS transition cases, I thought the basis style context does involve unwanted style changes as you mentioned in comment 43. I will re-check it.

Comment 75

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #72)
> 
> > This is not at all what I was suggesting.
> > 
> > I believe you need to use the real style context for the element as a basis
> > to add to, not this empty context, because properties have interactions that
> > can change the way differences are computed.
> 
> Hmm, but in CSS transition cases, I thought the basis style context does
> involve unwanted style changes as you mentioned in comment 43. I will
> re-check it.

I confirmed the basis style context involves unwanted style changes.
I did use below example.

<style>
  div {
    width: 100px;
    height: 100px;
    margin-left: 0px;
    background: orange;
    transition: margin 1s;
  }
</style>
<script>
  document.getElementById('target').style.marginLeft = '100px';
  document.getElementById('target').style.opacity = 0.5;
</script>

In this case nsChangeHint_UpdateOpacityLayer is included in the change hint.

I think using empty style context in CreateTemporaryStyleContextForProperty should not be a problem because the StyleAnimationValue passed to CreateTemporaryStyleContextForProperty is a result of ExtractComputedValueForTransition.

Or am I totally misunderstanding something?

Comment 76

[Hiroyuki Ikezoe (:hiro)]

:dbaron, do you mind commenting the basis style context in comment #75?
Should we use the real style context?

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what I was suggesting is that, if you want to figure out what the change hint for animating opacity:0 to opacity:0.5 is, you compare two style contexts:
 (a) the real style context for the element, modified with ResolveStyleByAddingRules to add the 'opacity: 0'
 (b) the real style context for the element, modified with ResolveStyleByAddingRules to add the 'opacity: 0.5'

I don't think that should generate extra changes.

I'm not sure what you were saying I said in comment 43.  Was that a reference to a different bug?

Comment 78

[Hiroyuki Ikezoe (:hiro)]

I am sorry I've been vacant for a while.  I had been investigating throtteled animations are immediately unthrotteled when target elements become in-view state.

Although I could not find any firm evidences for it, but I could write some automation tests to confirm that restyling process　is immediately done when the target element gets back in view.
Unfortunately some of those tests don't pass on Android (Async animations don't work　correctly in the first place?).  I will handle them in bug 1247800.

I am going to upload updated patches now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98858c6c6e2a

Comment 79

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Add AnimationProperty::AppendSegment v3

Adapted to the latest changes. Mainly TimingFunction became Maybe<>.

Comment 80

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add KeyframeEffectReadOnly::GetStyleContextForTarget

This one is a brand new patch.

GetStyleContextForTarget needs for calculation change hints for animations
which are created by the Constructor of KeyframeEffectReadOnly.
This patch also introduces a static method named
KeyframeEffectReadOnly::GetAnimationFrameForElement to get animation frame
from the Constructor.

Comment 81

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Store change hints between from and to for each animation segment

This was 'part 2' patch before. 

AnimationProperty::AppendSegment now needs nsPresContext and base
nsStyleContext to calculate the change hints.

This patch also introduces KeyframeEffectReadOnly::AppendSegment
which gets nsPresContext and base nsStyleContext and passes them
to AnimationProperty::AppendSegment when KeyframeEffectReadOnly is
newly created by the constructor of KeyframeEffectReadOnly.

This patch fixes problems that dbaron commented in comment#77.

Comment 82

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Add ElementPropertyTransition::SetTransitionProperty v2

Adapted to the latest Maybe<ComputedTimingFunction> change.

Comment 83

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Calculate cumulative change hints

Unrotten.

Comment 84

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: Add KeyframeEffectReadOnly::CanIgnoreIfNotVisible().

Addressed comment #57.

Comment 85

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 7: Add nsIFrame::IsOutOfView v2

This patch uses ancestor nsIScrollableFrame to check whether the target frame is scrolled out or not.

Comment 86

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 8: Remove some AreAsyncAnimationsEnabled checks

This is the patch that dbaron pointed me out in comment #61.

Now we can throttle some sort of animations which run on the main thread
as well as animations on the compositor. So, AreAsyncAnimationsEnabled checks
should be removed.

The check in PresShell::HandleEvent might be replaced with checking
nsIDocument::mNeedStyleFlush, but I am not sure.

Comment 87

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 9: Throttle paint-only animations on element which is out of view

Comment 88

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 10: Throttle paint-only animations if the presShell is not active

Comment 89

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Now automation tests for bug 1166500 can pass expect on Android

Comment 90

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 12: onFrame process should be called even if frameCount is 1

Comment 91

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Test that animation on an element which gets back to in view

This is the tests what I comment in comment #78.

Comment 92

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8721859 [details] [diff] [review]
Part 11: Now automation tests for bug 1166500 can pass expect on Android

Review of attachment 8721859 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/chrome/test_restyles.html
@@ +202,5 @@
>  
>      var markers = yield observeStyling(5);
>  
> +    is(markers.length, 0,
> +       'Bug 1166500: Animations running on the compositor in out of ' +

We can drop the bug number now that this passes.

Nit: This would be easier to read if you hyphenate 'out of view' as 'out-of-view' (better still, 'an out-of-view').

@@ +216,5 @@
>      yield animation.ready;
>      var markers = yield observeStyling(5);
>  
> +    is(markers.length, 0,
> +       'Bug 1166500: Animations running on the main-thread in out of ' +

Likewise here.

@@ +225,3 @@
>    add_task_if_omta_enabled(function* no_restyling_compositor_animations_in_scrolled_out_element() {
> +    /*
> +     Disabled for now since, on Android, the opacity animation runs on the

Nit: Disabled on Android, for now, since ...

@@ +244,5 @@
>  
>      var markers = yield observeStyling(5);
>  
> +    is(markers.length, 0,
> +       'Bug 1166500: Animations running on the compositor in elements ' +

Drop the bug now.

Nit: 'running on the compositor for element'

@@ +252,4 @@
>  
>    add_task(function* no_restyling_main_thread_animations_in_scrolled_out_element() {
> +    /*
> +     Disabled for now since, on Android, throttled animations are left behind

Nit: Disabled on Android, for now, since ...

@@ +269,5 @@
>      yield animation.ready;
>      var markers = yield observeStyling(5);
>  
> +    is(markers.length, 0,
> +       'Bug 1166500: Animations running on the main-thread in elements ' +

Drop the bug number.

Nit: 'for elements'

@@ +278,3 @@
>    add_task_if_omta_enabled(function* no_restyling_compositor_animations_in_visiblily_hidden_element() {
> +    /*
> +     Disabled for now since, on Android and B2G, the opacity animation runs on the

Disabled on Android, for now, since...

Comment 93

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8721855 [details] [diff] [review]
Part 7: Add nsIFrame::IsOutOfView v2

Review of attachment 8721855 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +9145,5 @@
> +bool
> +nsIFrame::IsOutOfView()
> +{
> +  nsIScrollableFrame* scrollableFrame =
> +    nsLayoutUtils::GetNearestScrollableFrame(this,

This isn't going to be correct if there are nested scrollframe in the document. You might want to repeat this until we run out of scrollframes.

'IsScrolledOutOfView' might be better given the implementation.

Comment 94

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 7: Add nsIFrame::IsScrolledOutOfView v3

Thank you, Matt, for pointing it out.

I will add test cases for nested scrollable frame in part 13.

Comment 95

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Now automation tests for bug 1166500 can pass expect on Android v2

Addressed review comments.

Comment 96

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 13: Test that animation on an element which gets back to in view

Added two test cases for the nested scrollable frame.

Unfortunately a test which checks that restyling process is done immediately after the animation element gets back in view from the nested scrolled frame causes *TWO* restylings in a subsequent frame.  I think it's basically harmless but of course we should reduce a restyling there.  I would like to tackle this new issue in a later bug, if it's OK with you, dbaron and Brian.

Comment 97

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8722245 [details] [diff] [review]
Part 7: Add nsIFrame::IsScrolledOutOfView v3

Review of attachment 8722245 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +9166,5 @@
> +    return true;
> +  }
> +
> +  nsIFrame* parent = aFrame->GetParent();
> +  if (parent == aFrame) {

Frames should never have themselves as a parent.

I think you should be calling IsFrameScrolledOutOfView(scrollableParent) anyway.

Comment 98

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #97)
> Comment on attachment 8722245 [details] [diff] [review]
> Part 7: Add nsIFrame::IsScrolledOutOfView v3
> 
> Review of attachment 8722245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsFrame.cpp
> @@ +9166,5 @@
> > +    return true;
> > +  }
> > +
> > +  nsIFrame* parent = aFrame->GetParent();
> > +  if (parent == aFrame) {
> 
> Frames should never have themselves as a parent.
> 
> I think you should be calling IsFrameScrolledOutOfView(scrollableParent)
> anyway.

Oh my.. I intended to pass scrollableParent->GetParent() to IsFrameScrolledOutOfView.
Passing scrollableParent itself will lead to a result that GetNearestScrollableFrame returns the same scrollable frame.  I will revise the patch soon. Thanks!

Comment 99

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 7: Add nsIFrame::IsScrolledOutOfView v4

Comment 100

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8722266 [details] [diff] [review]
Part 7: Add nsIFrame::IsScrolledOutOfView v4

Review of attachment 8722266 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +9169,5 @@
> +  if (!transformedRect.Intersects(scrollableRect)) {
> +    return true;
> +  }
> +
> +  return IsFrameScrolledOutOfView(scrollableParent->GetParent());

Actually, I'd prefer if you null checked scrollableParent->GetParent() here, and get rid of the aFrame null check at the start.

Comment 101

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8721850 [details] [diff] [review]
Part 2: Add KeyframeEffectReadOnly::GetStyleContextForTarget

>+KeyframeEffectReadOnly::GetAnimationFrameForElement(
>+  Element& aTarget,
>+  CSSPseudoElementType aPseudoType)

In Gecko code, we don't use * vs. & to distinguish when something is allowed to be null; we use comments and assertions.  A mozilla::dom::Element is always passed as a pointer and never as a reference.

So please change this reference to a pointer, and add a MOZ_ASSERT() that it's non-null.

>+  static nsStyleContext* GetStyleContextForTarget(
>+    Element* aTarget,
>+    CSSPseudoElementType aPseudoType);
>+  static nsIFrame* GetAnimationFrameForElement(
>+    Element& aTarget,
>+    CSSPseudoElementType aPseudoType);

It feels a little odd that one of these is called ...ForTarget and the other is ...ForElement.  They should probably be consistent, one way or the other.

r=dbaron with that

Comment 102

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add KeyframeEffectReadOnly::GetStyleContextForTarget v2

Addressed comments.

Comment 103

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Store change hints between from and to for each animation segment v2

Pass pointers instead of references for nsPresContext and nsStyleContext.

Comment 104

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8724508 [details] [diff] [review]
Part 3: Store change hints between from and to for each animation segment v2

My previous reviews of this patch were:
 comment 53, of attachment 8684182 [details] [diff] [review]
 comment 72, of attachment 8687140 [details] [diff] [review] (... through comment 77)

So, looking at these null checks:

>+  nsPresContext* presContext = aTarget ?
>+    nsContentUtils::GetContextForContent(aTarget) : nullptr;

>+  if (!aPresContext || !aStyleContext) {
>+    // In case of a valid pres context is not passed to, that means the
>+    // keyframe effect is created without target element, we don't need
>+    // to calculate change hints for optimization because there is no need
>+    // to paint for the animation which has no target element.
>+    return;
>+  }

It's not clear to me why you need to null-check aTarget or aPresContext.
I'd hope you can find a way to guarantee that they're non-null.  (But
below I propose just eliminating aPresContext, so don't even worry about
figuring that out.)

I'm more worried about the null-check of the style context.  It's clear
that the style context might be null in some cases, e.g., if the element
doesn't currently have a frame.

At the very least, if you're not setting the change hint usefully,
you should set it to a value that indicates that it is uninitialized,
and that we can assert about later, to assert that the assumption that
we won't need it is correct.

(I guess this assumption is somewhat believable... at least after bug
1254424, since the element won't get a frame without restyling
happening.  But I'm really not convinced now.)

That said, in most cases, we get into this code because we already have
a style context.  Perhaps it would be possible to pass that style
context through instead of recomputing it.  That would also avoid
having to deal with the possibility that it's null.  This seems like the
best option if it's possible.

In the worst case, we could fall back to
fall back to nsComputedDOMStyle::GetStyleContextForElementNoFlush,
but I hope that's not needed.

>+  already_AddRefed<nsStyleContext> CreateStyleContextForAnimationValue(
>+    StyleAnimationValue aValue,
>+    nsStyleContext* aBaseStyleContext,
>+    nsPresContext* aPresContext);

Please format as:
+  already_AddRefed<nsStyleContext>
+  CreateStyleContextForAnimationValue(StyleAnimationValue aValue,
+                                      nsStyleContext* aBaseStyleContext,
+                                      nsPresContext* aPresContext);

Instead of adding an aPresContext parameter to
nsAnimationManager::BuildSegment, you should just get the pres context
from the style context that you already have there.

The same for AnimationProperty::AppendSegment (which means you don't need
to compute the pres context in KeyframeEffectReadOnly::AppendSegment or
null-check it in AnimationProperty::AppendSegment.

Comment 105

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #104)
> Comment on attachment 8724508 [details] [diff] [review]
> Part 3: Store change hints between from and to for each animation segment v2
> 
> My previous reviews of this patch were:
>  comment 53, of attachment 8684182 [details] [diff] [review]
>  comment 72, of attachment 8687140 [details] [diff] [review] (... through
> comment 77)
> 
> So, looking at these null checks:
> 
> >+  nsPresContext* presContext = aTarget ?
> >+    nsContentUtils::GetContextForContent(aTarget) : nullptr;
> 
> >+  if (!aPresContext || !aStyleContext) {
> >+    // In case of a valid pres context is not passed to, that means the
> >+    // keyframe effect is created without target element, we don't need
> >+    // to calculate change hints for optimization because there is no need
> >+    // to paint for the animation which has no target element.
> >+    return;
> >+  }
> 
> It's not clear to me why you need to null-check aTarget or aPresContext.
> I'd hope you can find a way to guarantee that they're non-null.  (But
> below I propose just eliminating aPresContext, so don't even worry about
> figuring that out.)

My intention about aTarget was that I think we will eventually support null target for KeyframeEffectReadOnly constructor [1].  Do you mean that we don't need to worry about it as for now?

https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/dom/animation/KeyframeEffect.cpp#660

As for nsPresContext, you are right.  I can remove the null check.  Thanks.

> I'm more worried about the null-check of the style context.  It's clear
> that the style context might be null in some cases, e.g., if the element
> doesn't currently have a frame.
> 
> At the very least, if you're not setting the change hint usefully,
> you should set it to a value that indicates that it is uninitialized,
> and that we can assert about later, to assert that the assumption that
> we won't need it is correct.

It sounds really great.  nsChangeHint(0) is a suitable value as the uninitialized state?

> (I guess this assumption is somewhat believable... at least after bug
> 1254424, since the element won't get a frame without restyling
> happening.  But I'm really not convinced now.)
> 
> That said, in most cases, we get into this code because we already have
> a style context.  Perhaps it would be possible to pass that style
> context through instead of recomputing it.  That would also avoid
> having to deal with the possibility that it's null.  This seems like the
> best option if it's possible.
> 
> In the worst case, we could fall back to
> fall back to nsComputedDOMStyle::GetStyleContextForElementNoFlush,
> but I hope that's not needed.
> 
> >+  already_AddRefed<nsStyleContext> CreateStyleContextForAnimationValue(
> >+    StyleAnimationValue aValue,
> >+    nsStyleContext* aBaseStyleContext,
> >+    nsPresContext* aPresContext);
> 
> Please format as:
> +  already_AddRefed<nsStyleContext>
> +  CreateStyleContextForAnimationValue(StyleAnimationValue aValue,
> +                                      nsStyleContext* aBaseStyleContext,
> +                                      nsPresContext* aPresContext);
> 
> Instead of adding an aPresContext parameter to
> nsAnimationManager::BuildSegment, you should just get the pres context
> from the style context that you already have there.
> 
> The same for AnimationProperty::AppendSegment (which means you don't need
> to compute the pres context in KeyframeEffectReadOnly::AppendSegment or
> null-check it in AnimationProperty::AppendSegment.

Thank you.  I will do it.

Comment 106

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #105)
> My intention about aTarget was that I think we will eventually support null
> target for KeyframeEffectReadOnly constructor [1].  Do you mean that we
> don't need to worry about it as for now?

Don't worry about it for now. I'm completely rewriting this code in bug 1245748 so that we only generate the properties when we have a target and a style context.

Comment 107

[Hiroyuki Ikezoe (:hiro)]

Thank you, dbaron and Brian.

Now I found that StyleAnimationValue::ComputeValues calls LookupStyleContext and LookupStyleContext calls GetStyleContextForElementNoFlush eventually.  In case that StyleAnimationValue::ComputeValues fails when it's called from KeyframeEffectReadOnly::Constructor, AnimationProperty::AppendSegment will never be called.  So I can use MOZ_ASSERT(aStyleContext) in AnimationProperty::AppendSegment.  Thank you both of you!

[1] https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/layout/style/StyleAnimationValue.cpp#2667

Comment 108

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Store change hints between from and to for each animation segment v3

Changes from v3:

* Pass nsStyleContext* to some functions which call AnimationProperty::AppendSegment eventually (previously Element* and CSSPseudoElementType were passed to)
* Drop KeyframeEffectReadOnly::AppendSegment
* Pass nsStyleContext* to AnimationProperty::AppendSegment.
* Add MOZ_ASSERT(aStyleContext) in AnimationProperty::AppendSegment

Thanks.

Comment 109

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8728189 [details] [diff] [review]
Part 3: Store change hints between from and to for each animation segment v3

My previous reviews of this patch were:
 comment 53, of attachment 8684182 [details] [diff] [review]
 comment 72, of attachment 8687140 [details] [diff] [review] (... through comment 77)
 comment 104, of attachment 8724508 [details] [diff] [review]

In this patch, you compute a style context in
BuildAnimationPropertyListFromPropertyIndexedKeyframes
using KeyframeEffectReadOnly::GetStyleContextForTarget, which
returns null if the element doesn't have a frame, so this
means you might be passing AppendSegment a null style context.

(Those methods look like you use them from Web Animations APIs -- in
particular from the KeyframeEffect constructor -- although maybe I'm
misunderstanding how they're used.)

Then AppendSegment just has a MOZ_ASSERT that aStyleContext is non-null.

You either need a way to guarantee that you don't pass null, or you need
to handle null as I described in comment 104.  This doesn't look like
it succeeds at either.  (In other words, I don't think the "Perhaps
it would be possible" in comment 104 is possible, although maybe it is,
and I just don't sufficiently understand what the methods that I think
can pass null are used for.)

(And, to respond to your question in comment 105, nsChangeHint(0) probably
isn't sufficient since it's a real value.  You probably need a new
bit in nsChangeHint for this.)

In other words, I think this will just hit the MOZ_ASSERT if you use
the KeyframeEffect constructor on an element inside a display:none
subtree, or something else that makes an element not have a frame.

Comment 110

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8721856 [details] [diff] [review]
Part 8: Remove some AreAsyncAnimationsEnabled checks

This patch is as requested in comment 61.

The use in nsRefreshDriver::Tick seems to have been removed since
I made that comment.

r=dbaron

Comment 111

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8721857 [details] [diff] [review]
Part 9: Throttle paint-only animations on element which is out of view

I previously reviewed this patch in:
  comment 61, of attachment 8684189 [details] [diff] [review] (response in comment 67)

While you addressed the first half of my comment there, I don't see how
this addresses the out-of-date animation issue.

I have mixed feelings about whether I'm ok with allowing this patch to
land without adding code that brings animations up-to-date when a frame
is brought back into view.  I guess I'll allow it, though.

I think you should definitely have a followup bug on doing that, though,
since I think we do want to reduce the ticking of low-frequency
animations.

Comment 112

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8721858 [details] [diff] [review]
Part 10: Throttle paint-only animations if the presShell is not active

comment 111 applies here as well

It also seems odd to treat a null pres shell as more active than an inactive one.  It seems that if the pres shell is null, you also should be able to throttle the animation.  (That said, it's not clear to me how that could possibly happen; perhaps it makes sense to just assume the pres shell is non-null.)

r=dbaron with that, although I'd like to see a followup about flushing the animations

Also, you should add another patch to make this feature pref-controlled.  The pref can be enabled for nightly, but we ought to be able to turn it off, both for testing, an in case we decide it's problematic.  (You should be able to do the standard cached preference stuff, and test that preference in CanIgnoreIfNotVisible.)

Comment 113

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8722249 [details] [diff] [review]
Part 13: Test that animation on an element which gets back to in view

>Bug 1166500 - Part 13: Test that animation on an element which gets back to in view. r?dbaron

I'm not sure what this commit message means.  "Test that" should be
following by a complete sentence; here it's followed only by a noun.

Instead, I'd suggest:
  Test throttling and unthrottling of paint-only animations on elements that are scrolled out of view.

>+    /*
>+     Disabled on Android, for now, since throttled animations are left behind
>+     on the main thread in some frames, We will fix this in bug 1247800.
>+     */
>+    if (isAndroid) {
>+      return;
>+    }

Does this really need to be disabled?

You should always prefer having a test be annotated as failing (e.g.,
by calling it with (isAndroid ? todo_is : is)) than skipping it entirely,
since then we'll notice and enable the test when we fix the bug.

>+    // FIXME: We should reduce a redundant restylings here.

"a redundant restylings" should be either:
  "redundant restyling" (if there's possibly more than one)
  "a redundant restyle" (if there's exactly one)

>+       'Animations running on the main-thread which was in nested scrolled ' +

"which was" -> "which were"

>+       'out elements should update restyling soon after the element moved ' +

I'm not sure what you mean by "update restyling"?  Perhaps "update
style" or "restyle"?

>+    var parentRect = parentElement.getBoundingClientRect();
>+    var centerX = parentRect.left + parentRect.width / 2;
>+    var centerY = parentRect.top + parentRect.height / 2;

Wouldn't it be better to do this before the "yield animation.ready",
since this causes a layout flush?

r=dbaron


These tests seem good, but I wonder if it's worth also adding some tests
that make elements offscreen in a simple way, and using the
scrollbar on the viewport, since that's the important case.

It might also be good to have a test that restyling happens
multiple times for a normal, in-view animation.

Comment 114

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Add KeyframeEffectReadOnly::GetStyleContextForTarget v3

(In reply to David Baron [:dbaron] ⌚️UTC-4 (less responsive until April 4) from comment #109)
> Comment on attachment 8728189 [details] [diff] [review]
> Part 3: Store change hints between from and to for each animation segment v3
> 
> My previous reviews of this patch were:
>  comment 53, of attachment 8684182 [details] [diff] [review]
>  comment 72, of attachment 8687140 [details] [diff] [review] (... through
> comment 77)
>  comment 104, of attachment 8724508 [details] [diff] [review]
> 
> In this patch, you compute a style context in
> BuildAnimationPropertyListFromPropertyIndexedKeyframes
> using KeyframeEffectReadOnly::GetStyleContextForTarget, which
> returns null if the element doesn't have a frame, so this
> means you might be passing AppendSegment a null style context.
> 
> (Those methods look like you use them from Web Animations APIs -- in
> particular from the KeyframeEffect constructor -- although maybe I'm
> misunderstanding how they're used.)
> 
> Then AppendSegment just has a MOZ_ASSERT that aStyleContext is non-null.
> 
> You either need a way to guarantee that you don't pass null, or you need
> to handle null as I described in comment 104.  This doesn't look like
> it succeeds at either.  (In other words, I don't think the "Perhaps
> it would be possible" in comment 104 is possible, although maybe it is,
> and I just don't sufficiently understand what the methods that I think
> can pass null are used for.)

I've decided that GetStyleContextForTarget uses the same logic that  StyleAnimationValue::ComputeValues are using so that it does not return null because GetStyleContextForTarget is called only after StyleAnimationValue::ComputeValues succeeded.

Comment 115

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Store change hints between from and to for each animation segment v4

Comment 116

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 14: Add a preference for offscreen throttling.

Comment 117

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚️UTC-4 (less responsive until April 4) from comment #113)

> >+    /*
> >+     Disabled on Android, for now, since throttled animations are left behind
> >+     on the main thread in some frames, We will fix this in bug 1247800.
> >+     */
> >+    if (isAndroid) {
> >+      return;
> >+    }
> 
> Does this really need to be disabled?
> 
> You should always prefer having a test be annotated as failing (e.g.,
> by calling it with (isAndroid ? todo_is : is)) than skipping it entirely,
> since then we'll notice and enable the test when we fix the bug.

A hard part of using todo_is on Android is that results of those tests are unstable.  Using todo_is will cause intermittent success there.  I will use todo_is as possible but there might be some leftovers. 

FYI: a try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=96d4f9170210&selectedJob=18449228

> These tests seem good, but I wonder if it's worth also adding some tests
> that make elements offscreen in a simple way, and using the
> scrollbar on the viewport, since that's the important case.
> 
> It might also be good to have a test that restyling happens
> multiple times for a normal, in-view animation.

Thanks!  Added those test cases locally.

Comment 118

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Store change hints between from and to for each animation segment v5

Bug 1245748 has been landed.  We don't need part 2 patch here any more.

Now AnimationProperty::AppendSegment is called only if |aStyleContext| is not nullptr.
http://hg.mozilla.org/mozilla-central/file/24c5fbde4488/dom/animation/KeyframeEffect.cpp#l728

Comment 119

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8733288 [details] [diff] [review]
Part 2: Add KeyframeEffectReadOnly::GetStyleContextForTarget v3

I reviewed this patch on the airplane; it appears to have been obsoleted since, but here are my comments.

At the very least this needs to use GetStyleContextForElementNoFlush
rather than GetStyleContextForElement.

But this is less efficient for ::before and ::after since
GetStyleContextForElement doesn't use their frames.

It's also a substantively different behavior for elements inside
of ::first-letter and ::first-line pseudo-elements, since you're
computing style differences as though they're not inside such 
pseudo-elements.  This seems less than ideal given that it's
inconsistent with the animation that we're actually going to be doing.

I think I would prefer continuing with the previous approach rather
than this new one, and adding a new nsChangeHint bit as I previously
suggested as a way to fix the issues I raised with the previous approach.

Comment 120

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8733289 [details] [diff] [review]
Part 3: Store change hints between from and to for each animation segment v4

This patch also appears to have been obsoleted, but I reviewed it on the airplane while offline.

review-; see previous comment

Comment 121

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8733290 [details] [diff] [review]
Part 14: Add a preference for offscreen throttling.

This patch is as requested in comment 112.

Comment 122

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8734537 [details] [diff] [review]
Part 2: Store change hints between from and to for each animation segment v5

Cancelling review request.  Please re-request if it still makes sense after reading my review comments on the previous iteration, which I reviewed while offline on an airplane today.

Comment 123

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚️UTC (less responsive until April 4) from comment #122)
> Comment on attachment 8734537 [details] [diff] [review]
> Part 2: Store change hints between from and to for each animation segment v5
> 
> Cancelling review request.  Please re-request if it still makes sense after
> reading my review comments on the previous iteration, which I reviewed while
> offline on an airplane today.

OK. I will add a new nsChangeHint bit named nsChangeHint_Uninitialized and check it in CalculateCumulativeChangeHint.  I will those related patches.

Comment 124

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Store change hints between from and to for each animation segment v6

Comment 125

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Calculate cumulative change hints v2

Changes from previous (part 5 actually):

* assert if the change hints are not set

Comment 126

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8734578 [details] [diff] [review]
Part 4: Calculate cumulative change hints v2

>+      MOZ_ASSERT(segment.mChangeHint & ~nsChangeHint_Uninitialized);

Could you explain why you believe this assert won't fire?  What happens if an animation starts when an element is display:none or in a display:none subtree, but the element later becomes displayed, and you call them methods that trigger this.

Comment 127

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(It seems OK to be pessimistic in that case and assume that the animation is not paint-only.  But I'm not convinced it's ok to just assert that it doesn't happen.)


Also, in hindsight, I probably should have advised you to use Maybe<nsChangeHint> instead of adding a new nsChangeHint_Uninitialized.  I guess it's probably OK as is, but it's also very simple to switch to Maybe<>, which I think would be cleaner.

Comment 128

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... although maybe it's not a bit cleaner, since the easy way to handle the Uninitialized bit might be to | things together into the combined mCumulativeChangeHint.

Comment 129

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8734577 [details] [diff] [review]
Part 2: Store change hints between from and to for each animation segment v6

My previous reviews of this were:
 comment 53, of attachment 8684182 [details] [diff] [review]
 comment 72 through comment 77, of attachment 8687140 [details] [diff] [review]
 comment 104, of attachment 8724508 [details] [diff] [review]
 comment 109, of attachment 8728189 [details] [diff] [review]
 comment 119 and comment 120, of attachment 8733289 [details] [diff] [review], in which I asked
   you to work from the previous approach reviewed in comment 109

So in my previous review I suggested that you needed to handle aStyleContext being null in AnimationProperty::AppendSegment, since it would be null in some cases.

To handle that comment you removed a MOZ_ASSERT(aStyleContext).

But you didn't actually change anything else about it.

In particular, you continued to just pass aStyleContext to AnimationProperty::CreateStyleContextForAnimationValue, which has the exact same MOZ_ASSERT (except the parameter there is now called aBaseStyleContext).

The point was that if aStyleContext was null, instead of doing that, you should set segment->mChangeHint to the uninitialized bit (or maybe the none value of a Maybe<>) and return.



On the other hand, it looks like this patch no longer contains the callsites from which the style context could be null.

So maybe the whole issue is no longer relevant now.  (It would have been nice if you'd mentioned that explicitly rather than having me figure it out.)

Although I'm curious what happens if you set up Web animations on an element that doesn't have a frame.  Where is the style context resolved in that case?

>+  nsChangeHint mChangeHint = nsChangeHint_Uninitialized;

Please don't use initializers inconsistently like this.  I also don't think this use makes sense; I think it should be set in AppendSegment.

But given my comments above, I think you can remove this, and also remove the introduction of the new bit.

>diff --git a/layout/base/nsChangeHint.h b/layout/base/nsChangeHint.h
>--- a/layout/base/nsChangeHint.h
>+++ b/layout/base/nsChangeHint.h
>@@ -189,16 +189,26 @@ enum nsChangeHint {
>    *
>    * Used as extra data for handling UpdateOpacityLayer hints.
>    *
>    * Note that we do not send this hint if the non-1 value was 0.99 or
>    * greater, since in that case we send a RepaintFrame hint instead.
>    */
>   nsChangeHint_UpdateUsesOpacity = 1 << 25,
> 
>+  /**
>+   * Indicates that this change hint is not initialized yet.
>+   *
>+   * Note that we use this bit only for checking whether the changehint used
>+   * for throttling offscreen animations is valid or not.
>+   * Do not use this along with other bits.
>+   * See Bug 1166500.
>+   */
>+  nsChangeHint_Uninitialized = 1 << 31,
>+
>   // IMPORTANT NOTE: When adding new hints, consider whether you need to
>   // add them to NS_HintsNotHandledForDescendantsIn() below.  Please also
>   // add them to RestyleManager::ChangeHintToString.
> };

Remove this, since it appears it's no longer needed.



r=dbaron with that

Comment 130

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8734578 [details] [diff] [review]
Part 4: Calculate cumulative change hints v2

Given my comments on part 2, I think you can ignore my previous question on this patch, and also remove the added MOZ_ASSERT.

However, I'm still concerned about where you're calling CalculateCumulativeChangeHint.  What makes sure that you're initializing mCumulativeChangeHint in all cases?

Comment 131

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #129)
> Although I'm curious what happens if you set up Web animations on an element
> that doesn't have a frame.  Where is the style context resolved in that case?

IIUC, nsComputedDOMStyle::GetStyleContextForElement returns a valid nsStyleContext even when the target element does not have a frame.
https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/KeyframeEffect.cpp#466

(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #130)
> Comment on attachment 8734578 [details] [diff] [review]
> Part 4: Calculate cumulative change hints v2
> 
> Given my comments on part 2, I think you can ignore my previous question on
> this patch, and also remove the added MOZ_ASSERT.
> 
> However, I'm still concerned about where you're calling
> CalculateCumulativeChangeHint.  What makes sure that you're initializing
> mCumulativeChangeHint in all cases?

Because UpdateProperties() is called whenever properties causing style changes are changed.  But yes, there is a case that mCumulativeChangeHint is not initialized.  The case is when mProperties is empty.  We are going to handle the case in bug 1235002.  Once bug 1235002 is fixed, we can add an assertion that checks mCumulativeChangeHint is surely assigned in KeyframeEffectReadOnly::CanIgnoreIfNotVisible().  Should we fix bug 1235002 first to add the assertion?

Comment 132

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store change hints between from and to for each animation segment v7

Part 1 patch is no longer useful, now this patch became part 1, actually this was part 2 before. Addressed review comments and adapted to current tip.  Using Maybe<> looks better! Thanks!

Comment 133

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In comment 129 I wrote:
> On the other hand, it looks like this patch no longer contains the callsites
> from which the style context could be null.
> 
> So maybe the whole issue is no longer relevant now.  (It would have been
> nice if you'd mentioned that explicitly rather than having me figure it out.)
> 
> Although I'm curious what happens if you set up Web animations on an element
> that doesn't have a frame.  Where is the style context resolved in that case?

so I'm curious why you're still using Maybe<>.


(In reply to Hiroyuki Ikezoe (:hiro) from comment #131)
> (In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #129)
> > Although I'm curious what happens if you set up Web animations on an element
> > that doesn't have a frame.  Where is the style context resolved in that case?
> 
> IIUC, nsComputedDOMStyle::GetStyleContextForElement returns a valid
> nsStyleContext even when the target element does not have a frame.
> https://dxr.mozilla.org/mozilla-central/rev/
> fc15477ce628599519cb0055f52cc195d640dc94/dom/animation/KeyframeEffect.cpp#466

It is, but in that case it's quite expensive (potentially O(D*S) where D is the depth in the tree and S is the number of selectors in the style sheet) and something we should really avoid doing unless script asks for the style.

> Because UpdateProperties() is called whenever properties causing style
> changes are changed.  But yes, there is a case that mCumulativeChangeHint is
> not initialized.  The case is when mProperties is empty.  We are going to
> handle the case in bug 1235002.  Once bug 1235002 is fixed, we can add an
> assertion that checks mCumulativeChangeHint is surely assigned in
> KeyframeEffectReadOnly::CanIgnoreIfNotVisible().  Should we fix bug 1235002
> first to add the assertion?

I need to look into this part later.

Comment 134

[Hiroyuki Ikezoe (:hiro)]

I've decided I am going to fix bug 1235002 first.

Comment 135

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #131)
> (In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #130)
> > Comment on attachment 8734578 [details] [diff] [review]
> > Part 4: Calculate cumulative change hints v2

> > However, I'm still concerned about where you're calling
> > CalculateCumulativeChangeHint.  What makes sure that you're initializing
> > mCumulativeChangeHint in all cases?
> 
> Because UpdateProperties() is called whenever properties causing style
> changes are changed.  But yes, there is a case that mCumulativeChangeHint is
> not initialized.  The case is when mProperties is empty.  We are going to
> handle the case in bug 1235002.  Once bug 1235002 is fixed, we can add an
> assertion that checks mCumulativeChangeHint is surely assigned in
> KeyframeEffectReadOnly::CanIgnoreIfNotVisible().  Should we fix bug 1235002
> first to add the assertion?

I don't really understand the question -- or what the assertion would be -- but I guess if that's what you think is needed to ensure mCumulativeChangeHint is initialized in all cases, sure.

Comment 136

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store change hints between from and to for each animation segment v8

Maybe<> was dropped for mChangeHint on each segment.

Comment 137

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Calculate cumulative change hint.

Now mCumulativeChangeHint became Maybe<>.

In order to raise an assertion in CanIgnoreIfNotVisible() which will be         
introduced in part 3 when the cumulative hint is not properly, we should skip   
the calculation when mProperties is empty.

Comment 138

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Add KeyframeEffectReadOnly::CanIgnoreIfNotVisible().

Added an assertion if mCUmulativeChangeHint it not properly calculated.

Comment 139

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Add nsIFrame::IsScrolledOutOfView v4

unrotten.

Comment 140

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Remove some AreAsyncAnimationsEnabled checks

unrotten.

Comment 141

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 6: Throttle paint-only animations on element which is out of view.

unrotten.

Comment 142

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 7: Throttle paint-only animations if the presShell is not active

unrotten.

Comment 143

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 8: Now automation tests for bug 1166500 can pass expect on Android

unrotten.

Comment 144

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 9: onFrame process should be called even if frameCount is 1

unrotten.

Comment 145

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 10: Test throttling and unthrottling of paint-only animations on elements that are scrolled out of view

Unrotten.

Comment 146

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 11: Add a preference for offscreen throttling

Unrotten.

Comment 147

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8750518 [details] [diff] [review]
Part 3: Add KeyframeEffectReadOnly::CanIgnoreIfNotVisible().

This patch got review+ once, but I think it should be reviewed once again because an assertion has been added at the top of KeyframeEffectReadOnly::CanIgnoreIfNotVisible() to ensure mCumulativeChangeHint is calculated.

Comment 148

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you explain what changed since I last granted review and why you're requesting review again?

Comment 149

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(for patch 2)

Comment 150

[Hiroyuki Ikezoe (:hiro)]

I'd like you to take a look into the patch again in respond to your comment 133, which is "I'm curious why you're still using Maybe<>".

Thanks for taking your time again.

Comment 151

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8750517 [details] [diff] [review]
Part 2: Calculate cumulative change hint.

>+void
>+KeyframeEffectReadOnly::CalculateCumulativeChangeHint()
>+{
>+  mCumulativeChangeHint.reset();
>+
>+  if (mProperties.IsEmpty()) {
>+    return;
>+  }

I don't see why you should have special handling for mProperties being empty.  If the animation has no properties, then it should have nsChangeHint(0) as the change hint.  I think you should just remove the 5 lines above.

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>+  Maybe<nsChangeHint> mCumulativeChangeHint;

This should just be nsChangeHint rather than Maybe<nsChangeHint>.

(You didn't answer the question as to why you're still using Maybe<>, and I don't see a good reason.)

r=dbaron with that

Comment 152

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8750518 [details] [diff] [review]
Part 3: Add KeyframeEffectReadOnly::CanIgnoreIfNotVisible().

Previous review of this patch was in comment 57 and comment 59.

>+  MOZ_ASSERT(mCumulativeChangeHint,
>+             "The cumulative change hint should be calculated");

Per comment 151, please remove this.

r=dbaron with that

Comment 153

[Hiroyuki Ikezoe (:hiro)]

Thank you, dbaron!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9176d766cfcd
Try result with several retriggers on Android looks good.  I am really happy to land this.

Comment 154

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1939cbae14f943f239ca3d1839dc85516ca6ef
Bug 1166500 - Part 0: Test that offscreen animation optimization does not throttle visible animations. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3e5b026e23da81b232ee22de96b1772f6d0247
Bug 1166500 - Part 1: Store change hints between from and to for each animation segment. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bcaebb5ae5f0f8f2efd1266c2fea29462a1848c
Bug 1166500 - Part 2: Calculate cumulative change hint. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/3596a8ab01f351ab487761e20c4c4f99e9c9215c
Bug 1166500 - Part 3: Add KeyframeEffectReadOnly::CanIgnoreIfNotVisible(). r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/60bc26930e6abd1f381ab26c75cd663d2661b44c
Bug 1166500 - Part 4: Add nsIFrame::IsScrolledOutOfView. r=mattwoodrow

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3772b6ff68963dae9147772412c5968f8a036d9
Bug 1166500 - Part 5: Remove some AreAsyncAnimationsEnabled checks. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da9fb921d2c34d356d579366210a1d74be51ad4
Bug 1166500 - Part 6: Throttle paint-only animations on element which is out of view. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/db40d92f57f8de665f3334739e87a7df85ced95e
Bug 1166500 - Part 7: Throttle paint-only animations if the presShell is not active. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/e72c3fad984aeb7c50b5a3dd550da27e005ba566
Bug 1166500 - Part 8: Now automation tests for bug 1166500 can pass expect on Android. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d3369a34d98d8ecfb0a39a0f4a88f50cdf28510
Bug 1166500 - Part 9: onFrame process should be called even if frameCount is 1. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/6494ca2e43c7e19dc76daa8674d27c43f2df56a9
Bug 1166500 - Part 10: Test throttling and unthrottling of paint-only animations on elements that are scrolled out of view. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe7766352e3eb84c9da204d866e8a25d814a09d8
Bug 1166500 - Part 11: Add a preference for offscreen throttling. r=dbaron

Comment 155

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c1939cbae14
https://hg.mozilla.org/mozilla-central/rev/2a3e5b026e23
https://hg.mozilla.org/mozilla-central/rev/1bcaebb5ae5f
https://hg.mozilla.org/mozilla-central/rev/3596a8ab01f3
https://hg.mozilla.org/mozilla-central/rev/60bc26930e6a
https://hg.mozilla.org/mozilla-central/rev/c3772b6ff689
https://hg.mozilla.org/mozilla-central/rev/1da9fb921d2c
https://hg.mozilla.org/mozilla-central/rev/db40d92f57f8
https://hg.mozilla.org/mozilla-central/rev/e72c3fad984a
https://hg.mozilla.org/mozilla-central/rev/6d3369a34d98
https://hg.mozilla.org/mozilla-central/rev/6494ca2e43c7
https://hg.mozilla.org/mozilla-central/rev/fe7766352e3e

Comment 156

[Nicholas Nethercote [inactive]]

I took some measurements on Mac before and after this landed, but it didn't make any noticeable difference on microsoft.com, buzzfeed.com, and businessinsider.com.au, which are three sites mentioned in bugs blocked by this bug.

Comment 157

[Hiroyuki Ikezoe (:hiro)]

Actually this bug just optimizes animations in elements which are scrolled out of view.  I guess those sites have animations in visibility:hidden elements, it will be handled in bug 1237454.