1375767 - stylo: Eliminate calling nsCSSPseudoElements::GetPseudoType for animations in ServoBinding.cpp

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(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

[Boris Zbarsky [:bzbarsky]]

Exactly what I was thinking.  ;)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1375767 - Don't use nsCSSPseudoElements::GetPseudoType()

It iterates over all pseudo types if a given element is not pseudo.
We should use IsGeneratedContentContainerForBefore() or
IsGeneratedContentContainerForAfter instead.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1375767 - Pass CSSPseudoElementType to GetAnimationCollection() directly.

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

Comment 6

[Brian Birtles (:birtles)]

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.

Comment 7

[Brian Birtles (:birtles)]

(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

[Brian Birtles (:birtles)]

Comment on attachment 8880720 [details]
Bug 1375767 - Pass CSSPseudoElementType to GetAnimationCollection() directly.

https://reviewboard.mozilla.org/r/152080/#review157038

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Thank you Brian for the quick review!  And thank you bz for checking the code!

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I made a silly mistake there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c22176143b7896a260ee088207ed827db90684

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8880719 [details]
Bug 1375767 - Don't use nsCSSPseudoElements::GetPseudoType()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152078/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8880720 [details]
Bug 1375767 - Pass CSSPseudoElementType to GetAnimationCollection() directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152080/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bfdb2703860
https://hg.mozilla.org/mozilla-central/rev/33005f196e10