233636 - GetClasses should have a better signature

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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>*....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Per conversation with sicking, this may be ok

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd still like to get alex input on this...

Comment 9

[Alex Fritze]

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 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 143426 [details] [diff] [review]
Updated to review comments

sweet

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in.