602341 - remove nsIAtoms from nsCSSPseudoClassList

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Right now nsCSSPseudoClassList has an nsIAtom and an nsCSSPseudoClasses::Type reperesenting the same thing.  We should get rid of the nsIAtom if we can, or at least stop using it in as many places as possible.

Comment 1

[Boris Zbarsky [:bzbarsky]]

These atoms are only really used in three places:

1)  Serialization (can be replaced with |static const char*[]| indexed by mType)
2)  Parsing, to determine the pseudoclass type.  We could do this using the
    const char* array and string compares, but maybe atomizing plus atom
    compares is faster.
3)  Tree pseudoelements: the value in parens is parsed into a pseudoclass list.
    I see no reason not to use an AtomList here instead, right?

If we want to keep the atoms for #2, we could just make the actual atom storage static to nsCSSPseudoClasses.cpp if desired (as in, not expose the existence of those atoms to any other code at all, other than via the "get the type for a given atom" getter).

Comment 2

[Boris Zbarsky [:bzbarsky]]

I have patches for this; will attach on Monday.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Switch the pseudoclass Has*Arg functions from atoms to pseudoclass types.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch tree pseudoelements to using an nsAtomList, not an nsPseudoClassList, for their list of atoms.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Get rid of the mAtom member of nsPseudoClassList.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Drop the nsIAtom arguments to AddPSeudoClass.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Get rid of some pseudoclass atoms in the CSS parser.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Make pseudoclass atoms private to nsCSSPseudoClasses.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 484181 [details] [diff] [review]
Switch the pseudoclass Has*Arg functions from atoms to pseudoclass types.

r=dbaron

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 484182 [details] [diff] [review]
part 2.  Switch tree pseudoelements to using an nsAtomList, not an nsPseudoClassList, for their list of atoms.

The commit message should probably mention that you're switching storage of the arguments from mPseudoClassList to mClassList.  r=dbaron

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 484183 [details] [diff] [review]
part 3.  Get rid of the mAtom member of nsPseudoClassList.

>+void
>+nsCSSPseudoClasses::PseudoTypeToString(Type aType, nsAString& aString)
>+{
>+  NS_ABORT_IF_FALSE(aType < ePseudoClass_Count, "Unexpected type");
>+  (*CSSPseudoClasses_info[aType].mAtom)->ToString(aString);

You may also want to assert that aType >= 0.

r=dbaron

>+}
>+
> nsCSSPseudoClasses::Type
> nsCSSPseudoClasses::GetPseudoType(nsIAtom* aAtom)
> {
>   for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(CSSPseudoClasses_info); ++i) {
>     if (*CSSPseudoClasses_info[i].mAtom == aAtom) {
>       return Type(i);
>     }
>   }
>diff --git a/layout/style/nsCSSPseudoClasses.h b/layout/style/nsCSSPseudoClasses.h
>--- a/layout/style/nsCSSPseudoClasses.h
>+++ b/layout/style/nsCSSPseudoClasses.h
>@@ -67,11 +67,14 @@ public:
>   };
> 
>   static Type GetPseudoType(nsIAtom* aAtom);
>   static PRBool HasStringArg(Type aType);
>   static PRBool HasNthPairArg(Type aType);
>   static PRBool HasSelectorListArg(Type aType) {
>     return aType == ePseudoClass_any;
>   }
>+
>+  // Should only be used on types other than Count and NotPseudoClass
>+  static void PseudoTypeToString(Type aType, nsAString& aString);
> };
> 
> #endif /* nsCSSPseudoClasses_h___ */
>diff --git a/layout/style/nsCSSStyleRule.cpp b/layout/style/nsCSSStyleRule.cpp
>--- a/layout/style/nsCSSStyleRule.cpp
>+++ b/layout/style/nsCSSStyleRule.cpp
>@@ -131,88 +131,80 @@ nsAtomList::Clone(PRBool aDeep) const
> }
> 
> nsAtomList::~nsAtomList(void)
> {
>   MOZ_COUNT_DTOR(nsAtomList);
>   NS_CSS_DELETE_LIST_MEMBER(nsAtomList, this, mNext);
> }
> 
>-nsPseudoClassList::nsPseudoClassList(nsIAtom* aAtom,
>-                                     nsCSSPseudoClasses::Type aType)
>-  : mAtom(aAtom),
>-    mType(aType),
>+nsPseudoClassList::nsPseudoClassList(nsCSSPseudoClasses::Type aType)
>+  : mType(aType),
>     mNext(nsnull)
> {
>   NS_ASSERTION(!nsCSSPseudoClasses::HasStringArg(aType) &&
>                !nsCSSPseudoClasses::HasNthPairArg(aType),
>                "unexpected pseudo-class");
>   MOZ_COUNT_CTOR(nsPseudoClassList);
>   u.mMemory = nsnull;
> }
> 
>-nsPseudoClassList::nsPseudoClassList(nsIAtom* aAtom,
>-                                     nsCSSPseudoClasses::Type aType,
>+nsPseudoClassList::nsPseudoClassList(nsCSSPseudoClasses::Type aType,
>                                      const PRUnichar* aString)
>-  : mAtom(aAtom),
>-    mType(aType),
>+  : mType(aType),
>     mNext(nsnull)
> {
>   NS_ASSERTION(nsCSSPseudoClasses::HasStringArg(aType),
>                "unexpected pseudo-class");
>   NS_ASSERTION(aString, "string expected");
>   MOZ_COUNT_CTOR(nsPseudoClassList);
>   u.mString = NS_strdup(aString);
> }
> 
>-nsPseudoClassList::nsPseudoClassList(nsIAtom* aAtom,
>-                                     nsCSSPseudoClasses::Type aType,
>+nsPseudoClassList::nsPseudoClassList(nsCSSPseudoClasses::Type aType,
>                                      const PRInt32* aIntPair)
>-  : mAtom(aAtom),
>-    mType(aType),
>+  : mType(aType),
>     mNext(nsnull)
> {
>   NS_ASSERTION(nsCSSPseudoClasses::HasNthPairArg(aType),
>                "unexpected pseudo-class");
>   NS_ASSERTION(aIntPair, "integer pair expected");
>   MOZ_COUNT_CTOR(nsPseudoClassList);
>   u.mNumbers =
>     static_cast<PRInt32*>(nsMemory::Clone(aIntPair, sizeof(PRInt32) * 2));
> }
> 
> // adopts aSelectorList
>-nsPseudoClassList::nsPseudoClassList(nsIAtom* aAtom,
>-                                     nsCSSPseudoClasses::Type aType,
>+nsPseudoClassList::nsPseudoClassList(nsCSSPseudoClasses::Type aType,
>                                      nsCSSSelectorList* aSelectorList)
>-  : mAtom(aAtom),
>-    mType(aType),
>+  : mType(aType),
>     mNext(nsnull)
> {
>   NS_ASSERTION(nsCSSPseudoClasses::HasSelectorListArg(aType),
>                "unexpected pseudo-class");
>   NS_ASSERTION(aSelectorList, "selector list expected");
>   MOZ_COUNT_CTOR(nsPseudoClassList);
>   u.mSelectors = aSelectorList;
> }
> 
> nsPseudoClassList*
> nsPseudoClassList::Clone(PRBool aDeep) const
> {
>   nsPseudoClassList *result;
>   if (!u.mMemory) {
>-    result = new nsPseudoClassList(mAtom, mType);
>+    result = new nsPseudoClassList(mType);
>   } else if (nsCSSPseudoClasses::HasStringArg(mType)) {
>-    result = new nsPseudoClassList(mAtom, mType, u.mString);
>+    result = new nsPseudoClassList(mType, u.mString);
>   } else if (nsCSSPseudoClasses::HasNthPairArg(mType)) {
>-    result = new nsPseudoClassList(mAtom, mType, u.mNumbers);
>+    result = new nsPseudoClassList(mType, u.mNumbers);
>   } else {
>     NS_ASSERTION(nsCSSPseudoClasses::HasSelectorListArg(mType),
>                  "unexpected pseudo-class");
>     // This constructor adopts its selector list argument.
>-    result = new nsPseudoClassList(mAtom, mType, u.mSelectors->Clone());
>+    result = new nsPseudoClassList(mType, u.mSelectors->Clone());
>   }
> 
>   if (aDeep)
>     NS_CSS_CLONE_LIST_MEMBER(nsPseudoClassList, this, mNext, result,
>                              (PR_FALSE));
> 
>   return result;
> }
>@@ -419,40 +411,39 @@ void nsCSSSelector::AddClass(const nsStr
>     }
>     *list = new nsAtomList(aClass);
>   }
> }
> 
> void nsCSSSelector::AddPseudoClass(nsIAtom* aPseudoClass,
>                                    nsCSSPseudoClasses::Type aType)
> {
>-  AddPseudoClassInternal(new nsPseudoClassList(aPseudoClass, aType));
>+  AddPseudoClassInternal(new nsPseudoClassList(aType));
> }
> 
> void nsCSSSelector::AddPseudoClass(nsIAtom* aPseudoClass,
>                                    nsCSSPseudoClasses::Type aType,
>                                    const PRUnichar* aString)
> {
>-  AddPseudoClassInternal(new nsPseudoClassList(aPseudoClass, aType, aString));
>+  AddPseudoClassInternal(new nsPseudoClassList(aType, aString));
> }
> 
> void nsCSSSelector::AddPseudoClass(nsIAtom* aPseudoClass,
>                                    nsCSSPseudoClasses::Type aType,
>                                    const PRInt32* aIntPair)
> {
>-  AddPseudoClassInternal(new nsPseudoClassList(aPseudoClass, aType, aIntPair));
>+  AddPseudoClassInternal(new nsPseudoClassList(aType, aIntPair));
> }
> 
> void nsCSSSelector::AddPseudoClass(nsIAtom* aPseudoClass,
>                                    nsCSSPseudoClasses::Type aType,
>                                    nsCSSSelectorList* aSelectorList)
> {
>   // Take ownership of nsCSSSelectorList instead of copying.
>-  AddPseudoClassInternal(new nsPseudoClassList(aPseudoClass, aType,
>-                                               aSelectorList));
>+  AddPseudoClassInternal(new nsPseudoClassList(aType, aSelectorList));
> }
> 
> void nsCSSSelector::AddPseudoClassInternal(nsPseudoClassList *aPseudoClass)
> {
>   nsPseudoClassList** list = &mPseudoClassList;
>   while (nsnull != *list) {
>     list = &((*list)->mNext);
>   }
>@@ -765,30 +756,30 @@ nsCSSSelector::AppendToStringWithoutComb
>   if (isPseudoElement) {
> #ifdef MOZ_XUL
>     if (mPseudoClassList) {
>       NS_ABORT_IF_FALSE(nsCSSAnonBoxes::IsTreePseudoElement(mLowercaseTag),
>                         "must be tree pseudo-element");
>       aString.Append(PRUnichar('('));
>       for (nsPseudoClassList* list = mPseudoClassList; list;
>            list = list->mNext) {
>-        list->mAtom->ToString(temp);
>+        nsCSSPseudoClasses::PseudoTypeToString(list->mType, temp);
>         nsStyleUtil::AppendEscapedCSSIdent(temp, aString);
>         NS_ABORT_IF_FALSE(!list->u.mMemory, "data not expected");
>         aString.Append(PRUnichar(','));
>       }
>       // replace the final comma with a close-paren
>       aString.Replace(aString.Length() - 1, 1, PRUnichar(')'));
>     }
> #else
>     NS_ABORT_IF_FALSE(!mPseudoClassList, "unexpected pseudo-class list");
> #endif
>   } else {
>     for (nsPseudoClassList* list = mPseudoClassList; list; list = list->mNext) {
>-      list->mAtom->ToString(temp);
>+      nsCSSPseudoClasses::PseudoTypeToString(list->mType, temp);
>       // This should not be escaped since (a) the pseudo-class string
>       // has a ":" that can't be escaped and (b) all pseudo-classes at
>       // this point are known, and therefore we know they don't need
>       // escaping.
>       aString.Append(temp);
>       if (list->u.mMemory) {
>         aString.Append(PRUnichar('('));
>         if (nsCSSPseudoClasses::HasStringArg(list->mType)) {
>diff --git a/layout/style/nsICSSStyleRule.h b/layout/style/nsICSSStyleRule.h
>--- a/layout/style/nsICSSStyleRule.h
>+++ b/layout/style/nsICSSStyleRule.h
>@@ -79,29 +79,26 @@ private:
> 
>   // These are not supported and are not implemented! 
>   nsAtomList(const nsAtomList& aCopy);
>   nsAtomList& operator=(const nsAtomList& aCopy); 
> };
> 
> struct nsPseudoClassList {
> public:
>-  nsPseudoClassList(nsIAtom* aAtom, nsCSSPseudoClasses::Type aType);
>-  nsPseudoClassList(nsIAtom* aAtom, nsCSSPseudoClasses::Type aType,
>-                    const PRUnichar *aString);
>-  nsPseudoClassList(nsIAtom* aAtom, nsCSSPseudoClasses::Type aType,
>-                    const PRInt32 *aIntPair);
>-  nsPseudoClassList(nsIAtom* aAtom, nsCSSPseudoClasses::Type aType,
>+  nsPseudoClassList(nsCSSPseudoClasses::Type aType);
>+  nsPseudoClassList(nsCSSPseudoClasses::Type aType, const PRUnichar *aString);
>+  nsPseudoClassList(nsCSSPseudoClasses::Type aType, const PRInt32 *aIntPair);
>+  nsPseudoClassList(nsCSSPseudoClasses::Type aType,
>                     nsCSSSelectorList *aSelectorList /* takes ownership */);
>   ~nsPseudoClassList(void);
> 
>   /** Do a deep clone.  Should be used only on the first in the linked list. */
>   nsPseudoClassList* Clone() const { return Clone(PR_TRUE); }
> 
>-  nsCOMPtr<nsIAtom> mAtom;
>   union {
>     // For a given value of mType, we have either:
>     //   a. no value, which means mMemory is always null
>     //      (if none of the conditions for (b), (c), or (d) is true)
>     //   b. a string value, which means mString/mMemory is non-null
>     //      (if nsCSSPseudoClasses::HasStringArg(mType))
>     //   c. an integer pair value, which means mNumbers/mMemory is non-null
>     //      (if nsCSSPseudoClasses::HasNthPairArg(mType))

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 484184 [details] [diff] [review]
part 4.  Drop the nsIAtom arguments to AddPSeudoClass.

r=dbaron

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 484185 [details] [diff] [review]
part 5.  Get rid of some pseudoclass atoms in the CSS parser.

r=dbaron

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 484187 [details] [diff] [review]
part 6.  Make pseudoclass atoms private to nsCSSPseudoClasses.

r=dbaron

Comment 15

[Boris Zbarsky [:bzbarsky]]

> The commit message should probably mention that you're switching storage 

Done.

> You may also want to assert that aType >= 0.

Yes, done.

I'm figuring this is post-2.0 stuff, but let me know if you think I should ask for approval for some reason.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Fix serialization of tree selectors.

I missed this while doing part 2...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w of part 7

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 521256 [details] [diff] [review]
part 7.  Fix serialization of tree selectors.

Ah, so this should have been part of patch 2.

r=dbaron based on reading attachment 521257 [details] [diff] [review].

Comment 19

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/projects/birch/rev/1af999391d51
  http://hg.mozilla.org/projects/birch/rev/d15ce901326e
  http://hg.mozilla.org/projects/birch/rev/b89fb3d5ba02
  http://hg.mozilla.org/projects/birch/rev/64a403174648
  http://hg.mozilla.org/projects/birch/rev/df863a231d4d
  http://hg.mozilla.org/projects/birch/rev/d56f3c15e87a
  http://hg.mozilla.org/projects/birch/rev/497805f15c17

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/1af999391d51
https://hg.mozilla.org/mozilla-central/rev/d15ce901326e
https://hg.mozilla.org/mozilla-central/rev/b89fb3d5ba02
https://hg.mozilla.org/mozilla-central/rev/64a403174648
https://hg.mozilla.org/mozilla-central/rev/df863a231d4d
https://hg.mozilla.org/mozilla-central/rev/d56f3c15e87a
https://hg.mozilla.org/mozilla-central/rev/497805f15c17