Closed Bug 1463605 Opened 6 years ago Closed 6 years ago

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.

Attachment

General

Created:
Updated:
Size: