GetClasses should have a better signature

RESOLVED FIXED

Status

()

RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: sicking, Assigned: dbaron)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
RuleHash::EnumerateAllRules

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>*....
Created attachment 141224 [details] [diff] [review]
Per conversation with sicking, this may be ok
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
care...
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?
hrm, i could have sworn i added another comment here. Reposting as much as I can
remember:

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.
Created attachment 143205 [details] [diff] [review]
Merged to tip
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...

Comment 9

15 years ago
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
class).

>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+
Created attachment 143426 [details] [diff] [review]
Updated to review comments
Attachment #143205 - Attachment is obsolete: true
Comment on attachment 143426 [details] [diff] [review]
Updated to review comments

sweet
Attachment #143426 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.