1249230 - Fix the returned string of CSSPseudoElement::GetType()

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Boris Chiou [:boris]]

According to Bug 1234403 Comment 14, the returned string of pseudoType is not correct. It should be "::after" or "::before", instead of ":after" or ":before". We have to prepend an extra ':' to the pseudoType returned from nsCSSPseudoElements::GetPseudoAtom().

Comment 1

[Boris Chiou [:boris]]

Attachment: Prepend an extra colon to the pseudo type string

Comment 2

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd82b2160fb

Comment 3

[Boris Chiou [:boris]]

Comment on attachment 8720725 [details] [diff] [review]
Prepend an extra colon to the pseudo type string

Review of attachment 8720725 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/CSSPseudoElement.h
@@ +44,5 @@
>                 "All pseudo-types allowed by this class should have a"
>                 " corresponding atom");
> +    aRetVal.Assign(char16_t(':'));
> +    aRetVal.Append(
> +      nsCSSPseudoElements::GetPseudoAtom(mPseudoType)->GetUTF16String());

I will add a comment for this part after the review:

// Our atoms use one colon and we would like to return two colons syntax for
// the returned pseudo type string, so serialize this to the non-deprecated
// two colon syntax.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8720725 [details] [diff] [review]
Prepend an extra colon to the pseudo type string

Review of attachment 8720725 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles with comments addressed.

::: dom/animation/CSSPseudoElement.h
@@ +44,5 @@
>                 "All pseudo-types allowed by this class should have a"
>                 " corresponding atom");
> +    aRetVal.Assign(char16_t(':'));
> +    aRetVal.Append(
> +      nsCSSPseudoElements::GetPseudoAtom(mPseudoType)->GetUTF16String());

This is a bit expensive. GetUTF16String() returns a char16ptr_t.[1] Then we pass that to Append without specifying a length.[2] As a result we end up in Replace with aLength == -1 [3] so we'll call nsCharTraits<char16_t>::length(aData)[4] which will make us iterate through the whole string to get its length even though the atom already knows its length.

At very least we could call:

  nsIAtom* pseudoAtom = nsCSSPseudoElement::GetPseudoAtom(mPseudoType);
  aRetVal.Append(pseudoAtom->GetUTF16String(), pseudoAtom()->GetLength());

Better still, we should do:

  aRetVal.Append(
    nsDependentAtomString(nsCSSPseudoElements::GetPseudoAtom(mPseudoType)));

[1] https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/xpcom/ds/nsIAtom.idl#47
[2] https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/xpcom/string/nsTSubstring.h#535
[3] https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/xpcom/string/nsTSubstring.cpp#535
[4] https://dxr.mozilla.org/mozilla-central/rev/0629918a09ae87808efdda432d7852371ba37db6/xpcom/string/nsCharTraits.h#292

Comment 5

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #4)
> Better still, we should do:
> 
>   aRetVal.Append(
>     nsDependentAtomString(nsCSSPseudoElements::GetPseudoAtom(mPseudoType)));
> 

Thanks for explaining the performance problem. Using nsDependentAtomString() is more concise.

Comment 6

[Boris Chiou [:boris]]

Attachment: Prepend an extra colon to the pseudo type string. (v2, carry birtles' r+)

Comment 7

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faedf5df87b2

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d30bece68d

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f5d30bece68d