Closed Bug 233636 Opened 21 years ago Closed 20 years ago

GetClasses should have a better signature


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: sicking, Assigned: dbaron)




(1 file, 2 obsolete files)

It would be nice with a signature on nsIStyledContent didn't force us to copy
all the atoms into a new array. Unfortunatly that will force us to be consistent
with how we store the parsed classlist.

This should be the case once XUL and SVG is converted over to the new
attributecode though. However the classlist will then be stored either as an
atom or as an nsCOMArray<nsIAtom> so we'll will have to deal with that somehow.
It looks like the only place the return value is used is as input to

Perhaps we should just make this return a const nsAttrValue* pointing to the
internal attr value?  The EnumerateAllRules code can do the little bit with
figuring out whether the value is an nsIAtom* or an nsCOMArray<nsIAtom>*....
Comment on attachment 141224 [details] [diff] [review]
Per conversation with sicking, this may be ok

The goal here is that when the nsHTMLValue thing dies the callers don't have to
Attachment #141224 - Flags: superreview?(dbaron)
Attachment #141224 - Flags: review?(bugmail)
Ack, i just realized that this might not be doable in SVG. In SVG the .className
is an nsIDOMSVGAnimatedString which means that internally SVG might want to
store the class as an nsISVGValue rather then a nsIAtom or nsCOMArray<nsIAtom>.

However if the nsISVGValue internally stores the value as an nsIAtom and/or
nsCOMArray<nsIAtom> then we could still return a struct like

{ nsIAtom*, nsCOMArray<nsIAtom>* }

Though on the other hand, then the nsISVGValue might as well store an
nsAttrValue internally.

Alex, how does svg currently store the classlist, and how do you think you'll
store it in the future?
Attachment #141224 - Flags: superreview?(dbaron) → superreview+
hrm, i could have sworn i added another comment here. Reposting as much as I can

I talked with Alex about how svg should store the classname(s) in order to
properly implement HasClass/GetClasses. Svg does indeed need to store the
classname as an nsISVGValue which will contain the parsed atoms.

However it could probably store the value however we want inside that
nsISVGValue. So an nsAttrValue would most likly work fine, in which case the
current patch is fine.

Alex let me know if that sounds ok with you. Otherwise we would have to make
GetClasses return a struct as said in my previous comment.
Attached patch Merged to tip (obsolete) — Splinter Review
Attachment #141224 - Attachment is obsolete: true
Attachment #141224 - Flags: review?(bugmail)
Comment on attachment 143205 [details] [diff] [review]
Merged to tip

So sicking, could we make a decision here?  Is this ok or do we want to go with
the struct?
Attachment #143205 - Flags: review?(bugmail)
I'd still like to get alex input on this...
Sorry guys, I forgot all about this bug...
I talked to sicking and we agreed that the patch it is ok with SVG.
We'll need to expose an nsIDOMSVGAnimatedString interface for the class
attribute in SVG somehow, but that should be possible by statically or
dynamically wrapping the nsAttrValue with an appropriate SVG-specific class.
Comment on attachment 143205 [details] [diff] [review]
Merged to tip

>Index: content/base/src/nsAttrValue.h
>+  // Returns the atom at aIndex (0-based).  Do not call this with
>+  // aIndex >= GetAtomCount().
>+  nsIAtom* GetAtomAt(PRInt32 aIndex) const;

This should probably be called |AtomAt| since it should never return nsnull.
(though admittedly i've been very poor with following this convention in this

>Index: content/base/src/nsAttrValue.cpp
>+nsAttrValue::GetAtomAt(PRInt32 aIndex) const
>+  NS_PRECONDITION(aIndex >= 0, "Index must not be negative");
>+  NS_PRECONDITION(GetAtomCount() > 0, "GetAtomAt() called on an nsAttrValue with no atoms");
>+  NS_PRECONDITION(GetAtomCount() > aIndex, "aIndex out of range");

The middle precondition isn't strictly needed since if GetAtomCount() == 0 one
of the other preconditions will fail. Do as you want.

>+  if (Type() == eAtom) {
>+    return GetAtomValue();
>+  }

It'd be faster to do |BaseType() == eAtomBase| in that test.

With that, r=sicking
Attachment #143205 - Flags: review?(bugmail) → review+
Attachment #143205 - Attachment is obsolete: true
Comment on attachment 143426 [details] [diff] [review]
Updated to review comments

Attachment #143426 - Flags: review+
Checked in.
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.