Closed
Bug 1375767
Opened 7 years ago
Closed 7 years ago
stylo: Eliminate calling nsCSSPseudoElements::GetPseudoType for animations in ServoBinding.cpp
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
We can write a similar function to PseudoTagAndCorrectElementForAnimation which returns CSSPseudoElementType instead. nsCSSPseudoElements::GetPseudoType is not suitable for animation element since it iterates over all pseudo types.
Comment 1•7 years ago
|
||
Actually, you should just change PseudoTagAndCorrectElementForAnimation to do PseudoTypeAndCorrectElementForAnimation and return the type. For the consumers that want the tag, gettingthe tag from the type is an O(1) operation. But actually, none of the consumers want the tag anyway. They all want the type. At first glance the ones that call AnimationCollection<AnimationType>::GetAnimationCollection look like they want a tag, but that just converts to a type and calls the type overload anyway. So we could just call that directly, with a type.
Updated•7 years ago
|
Blocks: stylo-perf
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #1) > Actually, you should just change PseudoTagAndCorrectElementForAnimation to > do PseudoTypeAndCorrectElementForAnimation and return the type. For the > consumers that want the tag, gettingthe tag from the type is an O(1) > operation. > > But actually, none of the consumers want the tag anyway. They all want the > type. At first glance the ones that call > AnimationCollection<AnimationType>::GetAnimationCollection look like they > want a tag, but that just converts to a type and calls the type overload > anyway. So we could just call that directly, with a type. Yes, something like this? https://treeherder.mozilla.org/#/jobs?repo=try&revision=9700bb181f291f4f67d86a7859fa2c0dbfe74b65
Comment 3•7 years ago
|
||
Exactly what I was thinking. ;)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8880719 [details] Bug 1375767 - Don't use nsCSSPseudoElements::GetPseudoType() https://reviewboard.mozilla.org/r/152078/#review157036 ::: layout/style/ServoBindings.cpp:513 (Diff revision 1) > static nsIAtom* > PseudoTagAndCorrectElementForAnimation(const Element*& aElementOrPseudo) { > if (aElementOrPseudo->IsGeneratedContentContainerForBefore()) { > aElementOrPseudo = aElementOrPseudo->GetParent()->AsElement(); > return nsCSSPseudoElements::before; > } > > if (aElementOrPseudo->IsGeneratedContentContainerForAfter()) { > aElementOrPseudo = aElementOrPseudo->GetParent()->AsElement(); > return nsCSSPseudoElements::after; > } > > return nullptr; > } > > +static CSSPseudoElementType > +GetPseudoTypeFromElementForAnimation(const Element*& aElementOrPseudo) { > + if (aElementOrPseudo->IsGeneratedContentContainerForBefore()) { > + aElementOrPseudo = aElementOrPseudo->GetParent()->AsElement(); > + return CSSPseudoElementType::before; > + } > + > + if (aElementOrPseudo->IsGeneratedContentContainerForAfter()) { > + aElementOrPseudo = aElementOrPseudo->GetParent()->AsElement(); > + return CSSPseudoElementType::before; > + } > + > + return CSSPseudoElementType::NotPseudo; > +} It seems like we could fairly easily templatize these two to remove the redundant code, but it's up to you.
Attachment #8880719 -
Flags: review?(bbirtles) → review+
Comment 7•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #6) > It seems like we could fairly easily templatize these two to remove the > redundant code, but it's up to you. Oh never mind. I see it goes away in the next patch.
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8880720 [details] Bug 1375767 - Pass CSSPseudoElementType to GetAnimationCollection() directly. https://reviewboard.mozilla.org/r/152080/#review157038
Attachment #8880720 -
Flags: review?(bbirtles) → review+
Updated•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•7 years ago
|
||
Thank you Brian for the quick review! And thank you bz for checking the code!
Assignee | ||
Comment 10•7 years ago
|
||
I made a silly mistake there. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c22176143b7896a260ee088207ed827db90684
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8bfdb2703860 Don't use nsCSSPseudoElements::GetPseudoType() r=birtles https://hg.mozilla.org/integration/autoland/rev/33005f196e10 Pass CSSPseudoElementType to GetAnimationCollection() directly. r=birtles
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8bfdb2703860 https://hg.mozilla.org/mozilla-central/rev/33005f196e10
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•