1471174 - Don't send invisible animations that we already throttled on the main thread

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

This avoids opacity animations on visibility:hidden elements keep running on the compositor, thus we can drop 'opacity: 0' style from some test cases in file_restyles.html.  Also this can avoid trying to send transform animations on visibility:hidden elements to the compositor, thus this will reduce the frequency of bug 1471106.  Also, this will fix bug 1456389 for some particular cases.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=827f7f12a19ebabb007c0ce4be26fc05eb5b9aa6

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1471174 - Constify nsIFrame::IsScrolledOutOfView.

The reason why const_cast is used for nsLayoutUtils::GetNearestScrollableFrame
is that if we changed the function as well, it ends up scattering const_cast
in most call sites of the function.  That's because GetNearestScrollableFrame
has a do_QueryFrame call for the given nsIFrame* and returns the queried frame,
so it will be like this;

 const nsIScrollableFrame* GetNearestScrollableFrame(const nsIFrame*, ..)

Most call sites of this function are then calls do_QueryFrame for the returned
nsIScrollableFrame*.

Review commit: https://reviewboard.mozilla.org/r/253248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253248/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1471174 - Don't try to send invisible animations that we already throttled on the main thread.

Even if we unthrottled the invisbile animations to update the overflow region,
we don't need to send the animations to the compositor since the scroll bar
updates caused by the overflow should have been finished before sending
animations to the compositor.

Review commit: https://reviewboard.mozilla.org/r/253250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253250/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1471174 - Drop opacity value that was for preventing the animations run on the compositor and check the animation is NOT running on the compositor.

Because now we don't try to send the animations on visibility:hidden element.

Review commit: https://reviewboard.mozilla.org/r/253252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253252/

Comment 4

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

Comment on attachment 8987984 [details]
Bug 1471174 - Constify nsIFrame::IsScrolledOutOfView.

https://reviewboard.mozilla.org/r/253248/#review259872

Comment 5

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

Comment on attachment 8987985 [details]
Bug 1471174 - Don't try to send invisible animations that we already throttled on the main thread.

https://reviewboard.mozilla.org/r/253250/#review259870

::: dom/animation/EffectCompositor.cpp:111
(Diff revision 1)
> +  // If we know that the animation is not visible, we don't need to send the
> +  // animation to the compositor.
> +  if (!aFrame->IsVisibleOrMayHaveVisibleDescendants() ||
> +      aFrame->IsScrolledOutOfView()) {
> +    return MatchForCompositor::No;
> +  }

I was wondering if, as an optimization, we should make this MatchForCompositor::NoAndBlockThisProperty. Since this check is only related to the frame, it's impossible to return Yes or IfNeeded from this function for any of the other effects targeting this frame.

Then again, that sort of optimization is probably not important so MatchForCompositor::No is fine.

Comment 6

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

Comment on attachment 8987986 [details]
Bug 1471174 - Drop opacity value that was for preventing the animations run on the compositor and check the animation is NOT running on the compositor.

https://reviewboard.mozilla.org/r/253252/#review259874

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> Comment on attachment 8987985 [details]
> Bug 1471174 - Don't try to send invisible animations that we already
> throttled on the main thread.
> 
> https://reviewboard.mozilla.org/r/253250/#review259870
> 
> ::: dom/animation/EffectCompositor.cpp:111
> (Diff revision 1)
> > +  // If we know that the animation is not visible, we don't need to send the
> > +  // animation to the compositor.
> > +  if (!aFrame->IsVisibleOrMayHaveVisibleDescendants() ||
> > +      aFrame->IsScrolledOutOfView()) {
> > +    return MatchForCompositor::No;
> > +  }
> 
> I was wondering if, as an optimization, we should make this
> MatchForCompositor::NoAndBlockThisProperty. Since this check is only related
> to the frame, it's impossible to return Yes or IfNeeded from this function
> for any of the other effects targeting this frame.

Oh right, NoAndBlockThisProperty makes much more sense.  Thanks, I will use it.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8987984 [details]
Bug 1471174 - Constify nsIFrame::IsScrolledOutOfView.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253248/diff/1-2/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8987985 [details]
Bug 1471174 - Don't try to send invisible animations that we already throttled on the main thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253250/diff/1-2/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8987986 [details]
Bug 1471174 - Drop opacity value that was for preventing the animations run on the compositor and check the animation is NOT running on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253252/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6b24ba30090
Constify nsIFrame::IsScrolledOutOfView. r=birtles
https://hg.mozilla.org/integration/autoland/rev/41dabd41e63e
Don't try to send invisible animations that we already throttled on the main thread. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c1bab94cd24f
Drop opacity value that was for preventing the animations run on the compositor and check the animation is NOT running on the compositor. r=birtles

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b6b24ba30090
https://hg.mozilla.org/mozilla-central/rev/41dabd41e63e
https://hg.mozilla.org/mozilla-central/rev/c1bab94cd24f