1416957 - use MayHaveOpacityAnimation in nsIFrame to look up if a frame may have opacity

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P2)

Description

[Ya-Chieh Wu (inactive)]

Per Bug 1381153 Comment 85, for |opacity|, we might need to come up where we can make sure that we also update the continuous frame or maybe we can just redirect the look-up to the prime frame.

Comment 1

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1416957-part-1-look-up-MayHaveOpacityAnimation-i.patch

There are two places where we might be able to check the opacity status from frame. 
1. https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/layout/generic/nsFrame.cpp#1564
2. https://searchfox.org/mozilla-central/rev/137f1b2f434346a0c3756ebfcbdbee4069e15dc8/layout/generic/nsFrame.cpp#2757

I fix the first one. However for the second one, it seems that the only way is to check the opacity status from primary frame but it doesn't look more efficient to get the primary frame from here. I might be wrong?

Comment 2

[Ya-Chieh Wu (inactive)]

Attachment: 0002-Bug-1416957-part-2-remove-unused-effectset.patch

Comment 3

[Ya-Chieh Wu (inactive)]

try looks ok : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb11aa8956b03c5034f29d5082f06355514a6742

Comment 4

[Ya-Chieh Wu (inactive)]

sorry that I am leaving moz so untake this.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8942563 [details] [diff] [review]
0001-Bug-1416957-part-1-look-up-MayHaveOpacityAnimation-i.patch

Brian, you probably have a better understanding of the HasAnimationOfProperty /
EffectSet stuff than I do -- can you take this review?
(or delegate to someone more appropriate)

Comment 6

[Brian Birtles (:birtles)]

Hiro, are you able to look at this?

I'm a little bit busy this week and I think you're probably a more suitable reviewer for this code anyway.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942563 [details] [diff] [review]
0001-Bug-1416957-part-1-look-up-MayHaveOpacityAnimation-i.patch

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

r=me with comments addressed.

::: layout/generic/nsFrame.cpp
@@ +1536,4 @@
>    }
>  
>    EffectSet* effects =
>      aEffectSet ? aEffectSet : EffectSet::GetEffectSet(this);

I'd prefer to drop this in this patch instead of in a subsequent patch, it's not a big deal though.

@@ +1544,5 @@
> +  nsIFrame* frame = nsLayoutUtils::FirstContinuationOrIBSplitSibling(this);
> +  return (IsPrimaryFrame() &&
> +           nsLayoutUtils::HasAnimationOfProperty(this, eCSSProperty_opacity)) ||
> +         (frame->IsPrimaryFrame() &&
> +           nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_opacity));

In the case the frame has no continuation and has no animation (it's a common case), we call nsLayoutUtils::HasAnimationOfProperty() for the same frame twice.  We should check 'frame != this'?  Also it would be nice to avoid calling nsLayoutUtils::FirstContinuationOrIBSplitSibling in non continuation cases.  So something like this;

 if (IsPrimaryFrame() && nsLayoutUtils::HasAnimationOfProperty(this, eCSSProperty_opacity)) {
   return true;
 }

 nsIFrame* frame = nsLayoutUtils::FirstContinuationOrIBSplitSibling(this);
 return (frame != this && ...);

?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942564 [details] [diff] [review]
0002-Bug-1416957-part-2-remove-unused-effectset.patch

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

This patch looks good to me.  But I noticed that bug 1381153 is still incomplete since we don't yet use mMayHaveTransformAnimation, that means that we still call EffectSet::GetEffectSet() for transform case.

Also eventually we could eliminate EffectSet::mMayHaveOpacityAnim and EffectSet::mMayHaveTransformAnim.  So in any way, there are still some works in a later bug or in this bug.

Oh, I just realized this patch is two thirds of the patch set.  There must be one more patch.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Anyway, I will take over this.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:hiro, could you have a look please?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I did unintentionally do the same change as attachment 8942563 [details] [diff] [review] in https://hg.mozilla.org/mozilla-central/rev/9dacb05350d80c4e5591de4d7f10c766b969103e .

And bug 1518816 significantly and properly changed relevant stuff in attachment 8942564 [details] [diff] [review], so I think there is no more need to do there.