1463605 - nsIFrame::HasOpacityInternal() should check mMayHaveOpacityAnimation flag before calling EffectSet::GetEffectSet

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

In the profile in bug 1421611 comment 6, I noticed that EffectSet::GetEffectSet() appeared in the top functions in invert view.

In the profile case, GetEffectSet is called from nsIFrame::HasOpacityInternal(),  I believe we can check mMayHaveOpacityAnimation and bail out from there if the flag is false.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=358c35f235c502d2f58eab0a4ddff244e5f1ef47&selectedJob=179739345

ib-split-sibling-opacity.html failed on this try.  So we don't properly set mMayHaveOpacityAnimation for the split frame.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

A problem here in KeyframeEffect::UpdateEffectSet() [1].  We should also set the flag to split siblings. 

[1] https://hg.mozilla.org/mozilla-central/file/9055d9d89a4b/dom/animation/KeyframeEffect.cpp#l1789

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I believe MarkNeedsDisplayItemRebuild() should also be called for IB split siblings too.  Here https://hg.mozilla.org/mozilla-central/file/9055d9d89a4b/dom/animation/KeyframeEffect.cpp#l778

Also I did try to write a reftest for this, but it seems we have to do the similar thing in nsDOMWindowUtils::CheckAndClearDisplayListState().

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1463605 - Set mMayHaveOpacityAnimation and mMayHaveTransformAnimation flag to continuation or IB split sibling frames too.

Without this fix layout/reftests/css-animations/ib-split-sibling-opacity.html
would have failed if the next change in this patch series is applied.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1463605 - Check mMayHaveOpacityAnimation in nsFrame::HasOpacityInternal().

If the mMayHaveOpacityAnimation is false there, we don't need to check opacity
animations at all.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1463605 - Call MarkNeedsDisplayItemRebuild() for IB split siblings too.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1463605 - Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearDisplayListState.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1463605 - A reftest that IB sibling frames are correctly marked as 'NeedsDisplayItemRebuild' when there is an animation on the frames.

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1463605 - Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearPaintedState.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8979870 [details]
Bug 1463605 - Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearPaintedState.

https://reviewboard.mozilla.org/r/246062/#review252090

I have never found any issue without this change, but I think theoritically this is the right thing to do just like what we do for checkAndClearDisplayList in the other patch.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8979870 [details]
Bug 1463605 - Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearPaintedState.

https://reviewboard.mozilla.org/r/246062/#review252318

Makes sense!

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8979865 [details]
Bug 1463605 - Call MarkNeedsDisplayItemRebuild() for IB split siblings too.

https://reviewboard.mozilla.org/r/246056/#review252428

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8979866 [details]
Bug 1463605 - Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearDisplayListState.

https://reviewboard.mozilla.org/r/246058/#review252430

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8979867 [details]
Bug 1463605 - A reftest that IB sibling frames are correctly marked as 'NeedsDisplayItemRebuild' when there is an animation on the frames.

https://reviewboard.mozilla.org/r/246060/#review252432

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8979863 [details]
Bug 1463605 - Set mMayHaveOpacityAnimation and mMayHaveTransformAnimation flag to continuation or IB split sibling frames too.

https://reviewboard.mozilla.org/r/246052/#review252434

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8979864 [details]
Bug 1463605 - Check mMayHaveOpacityAnimation in nsFrame::HasOpacityInternal().

https://reviewboard.mozilla.org/r/246054/#review252436

Comment 17

[Hiroyuki Ikezoe (:hiro)]

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd784e2729a84d4a66443b8c68727da36a521b3c

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3025063ca800
Set mMayHaveOpacityAnimation and mMayHaveTransformAnimation flag to continuation or IB split sibling frames too. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9dacb05350d8
Check mMayHaveOpacityAnimation in nsFrame::HasOpacityInternal(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/abf79f352837
Call MarkNeedsDisplayItemRebuild() for IB split siblings too. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fb535bef1dd0
Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearDisplayListState. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fe67559663ef
A reftest that IB sibling frames are correctly marked as 'NeedsDisplayItemRebuild' when there is an animation on the frames. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c411ccb6bb4a
Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearPaintedState. r=mstange

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3025063ca800
https://hg.mozilla.org/mozilla-central/rev/9dacb05350d8
https://hg.mozilla.org/mozilla-central/rev/abf79f352837
https://hg.mozilla.org/mozilla-central/rev/fb535bef1dd0
https://hg.mozilla.org/mozilla-central/rev/fe67559663ef
https://hg.mozilla.org/mozilla-central/rev/c411ccb6bb4a