Fix the returned string of CSSPseudoElement::GetType()

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Animation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: boris, Assigned: boris)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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().
Summary: Fix CSSPseudoElement::mPseudoType return string → Fix the returned string of CSSPseudoElement::GetType()
Created attachment 8720725 [details] [diff] [review]
Prepend an extra colon to the pseudo type string
Attachment #8720725 - Flags: review?(bbirtles)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cd82b2160fb
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 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
Attachment #8720725 - Flags: review?(bbirtles) → review+
(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.
Created attachment 8721139 [details] [diff] [review]
Prepend an extra colon to the pseudo type string. (v2, carry birtles' r+)
Attachment #8721139 - Flags: review+
Attachment #8720725 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=faedf5df87b2
Keywords: checkin-needed

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d30bece68d
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5d30bece68d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.