307600 - Implement nsIContent::AttrHasValue

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We have a lot of places that GetAttr just to do comparisons on the value.  These
comparisons could be optimized if they were happening inside nsAttrValue in a
lot of cases (with fallback to exactly the speed we have right now).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

I'll start converting callers in followup bugs once we're happy with the API.

In my testing, if the caller's type (string/atom) matches the callee's type for
the attr this is about 3 times faster than a GetAttr/compare, while if the
types don't match it's still about 30-40% faster.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 195348 [details] [diff] [review]
Like so

   virtual PRBool HasAttr(PRInt32 aNameSpaceID, nsIAtom* aName) const = 0;
+  virtual PRBool AttrHasValue(PRInt32 aNameSpaceID,
+			       nsIAtom* aName,
+			       const nsAString& aValue,
+			       PRBool aCaseSensitive) const
+  virtual PRBool AttrHasValue(PRInt32 aNameSpaceID, nsIAtom* aName,
+			       nsIAtom* aValue, PRBool aCaseSensitive) const

I was sort of thinking about naming these HasAttrWithValue(), but I'm not sure
now which is better.

r+sr=jst either way.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So... People suggested AttrValueIs or AttrIs.  Shaver also objected to having
PR_TRUE/PR_FALSE for the case and said separate methods would be better, but I
couldn't think of any reasonably short names for the case-insensitive one.

Using an enum instead of true/false was also suggested, but I really don't want
to include nsIContent in nsAttrValue.h, and declaring two separate enums that
just happen to share the same values seems fishy to me.

jst?  Thoughts?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for checkin

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch for XTF

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Even compiling

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually, XULElement needs some special stuff too...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.  I'll file followup bugs on other places that can use these methods.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and it looks like this checkin improved Tdhtml by 5-6%.