Closed
Bug 1416957
Opened 7 years ago
Closed 5 years ago
use MayHaveOpacityAnimation in nsIFrame to look up if a frame may have opacity
Categories
(Core :: DOM: Animation, enhancement, P2)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox59 | --- | affected |
People
(Reporter: ywu, Assigned: hiro)
References
Details
Attachments
(2 files)
1.40 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
14.21 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•6 years ago
|
||
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?
Attachment #8942563 -
Flags: review?(mats)
Reporter | ||
Comment 2•6 years ago
|
||
Attachment #8942564 -
Flags: review?(mats)
Reporter | ||
Comment 3•6 years ago
|
||
try looks ok : https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb11aa8956b03c5034f29d5082f06355514a6742
Comment 5•6 years ago
|
||
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)
Attachment #8942563 -
Flags: review?(mats) → review?(bbirtles)
Updated•6 years ago
|
Attachment #8942564 -
Flags: review?(mats) → review?(bbirtles)
Comment 6•6 years ago
|
||
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.
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 7•6 years ago
|
||
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 && ...); ?
Attachment #8942563 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 8•6 years ago
|
||
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.
Attachment #8942564 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 9•6 years ago
|
||
Anyway, I will take over this.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Comment 10•5 years ago
|
||
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?
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 11•5 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(hikezoe)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•