Closed Bug 1463605 Opened Last year Closed Last year

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

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.
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.
Depends on: 1459776
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
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
Blocks: 1459776
No longer depends on: 1459776
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 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!
Attachment #8979870 - Flags: review?(mstange) → review+
Comment on attachment 8979865 [details]
Bug 1463605 - Call MarkNeedsDisplayItemRebuild() for IB split siblings too.

https://reviewboard.mozilla.org/r/246056/#review252428
Attachment #8979865 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8979866 [details]
Bug 1463605 - Check continuation or IB split sibling frames in nsDOMWindowUtils::CheckAndClearDisplayListState.

https://reviewboard.mozilla.org/r/246058/#review252430
Attachment #8979866 - Flags: review?(matt.woodrow) → review+
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
Attachment #8979867 - Flags: review?(matt.woodrow) → review+
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
Attachment #8979863 - Flags: review?(bbirtles) → review+
Comment on attachment 8979864 [details]
Bug 1463605 - Check mMayHaveOpacityAnimation in nsFrame::HasOpacityInternal().

https://reviewboard.mozilla.org/r/246054/#review252436
Attachment #8979864 - Flags: review?(bbirtles) → review+
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.