If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

remove nsIAtoms from nsCSSPseudoClassList

RESOLVED FIXED in mozilla5

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dbaron, Assigned: bz)

Tracking

Trunk
mozilla5
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

12.79 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.54 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.19 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.43 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.78 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.39 KB, patch
Details | Diff | Splinter Review
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.
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).
I have patches for this; will attach on Monday.
Created attachment 484181 [details] [diff] [review]
Switch the pseudoclass Has*Arg functions from atoms to pseudoclass types.
Created attachment 484182 [details] [diff] [review]
part 2.  Switch tree pseudoelements to using an nsAtomList, not an nsPseudoClassList, for their list of atoms.
Attachment #484181 - Flags: review?(dbaron)
Created attachment 484183 [details] [diff] [review]
part 3.  Get rid of the mAtom member of nsPseudoClassList.
Attachment #484182 - Flags: review?(dbaron)
Created attachment 484184 [details] [diff] [review]
part 4.  Drop the nsIAtom arguments to AddPSeudoClass.
Attachment #484183 - Flags: review?(dbaron)
Created attachment 484185 [details] [diff] [review]
part 5.  Get rid of some pseudoclass atoms in the CSS parser.
Attachment #484184 - Flags: review?(dbaron)
Attachment #484185 - Flags: review?(dbaron)
Created attachment 484187 [details] [diff] [review]
part 6.  Make pseudoclass atoms private to nsCSSPseudoClasses.
Attachment #484187 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]
Comment on attachment 484181 [details] [diff] [review]
Switch the pseudoclass Has*Arg functions from atoms to pseudoclass types.

r=dbaron
Attachment #484181 - Flags: review?(dbaron) → review+
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
Attachment #484182 - Flags: review?(dbaron) → review+
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))
Attachment #484183 - Flags: review?(dbaron) → review+
Comment on attachment 484184 [details] [diff] [review]
part 4.  Drop the nsIAtom arguments to AddPSeudoClass.

r=dbaron
Attachment #484184 - Flags: review?(dbaron) → review+
Comment on attachment 484185 [details] [diff] [review]
part 5.  Get rid of some pseudoclass atoms in the CSS parser.

r=dbaron
Attachment #484185 - Flags: review?(dbaron) → review+
Comment on attachment 484187 [details] [diff] [review]
part 6.  Make pseudoclass atoms private to nsCSSPseudoClasses.

r=dbaron
Attachment #484187 - Flags: review?(dbaron) → review+
> 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.
Whiteboard: [need review] → [need gk2 ship]
Created attachment 521256 [details] [diff] [review]
part 7.  Fix serialization of tree selectors.

I missed this while doing part 2...
Attachment #521256 - Flags: review?(dbaron)
Created attachment 521257 [details] [diff] [review]
diff -w of part 7
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].
Attachment #521256 - Flags: review?(dbaron) → review+
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
Flags: in-testsuite-
Whiteboard: [need gk2 ship] → fixed-in-birch
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-birch
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.