113562 - nsIContent needs HasAttr method

Status: RESOLVED FIXED

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

Description

[Joe Hewitt (gone)]

I caused a 1% slowdown when I landed my tooltip changes, primarily because I am
calling GetAttr every time a XUL frame is constructed.  In this case, I don't
need the actual attribute value, I only need to know if it exists.  So, we
should really implement an efficient HasAttr method on nsIContent so I can make
this discovery without wasting time copying the attribute value string.  I'd bet
there are plenty of other places in content and layout where HasAttr could be used.

Also, the DOM HasAttribute method is wastefully calling GetAttr, so I will
switch that over to HasAttr.

Patch on the way....

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This has certainly annoyed me in the past, and I suspect there are a bunch of
other places that could benefit.  In fact, constructing an nsAutoString is
expensive enough that the pattern (given |nsIContent* content|):

if (content->HasAttr(foo)) {
  nsAutoString val;
  content->GetAttr(foo, val);
  /* do stuff */
}

may likely be faster than

nsAutoString val;
if (NS_CONTENT_ATTR_NOT_THERE != content->GetAttr(foo, val)) {
  /* do stuff */
}

(or what I meant if you fixed all my typos :-)

Comment 2

[Johnny Stenback (:jst)]

Yeah, this makes alot of sense to me too, we might even want to go a step
further and have a IndexOfAttr() and a GetAttrByIndex() which would make code
that checks if an attr exists and then asks for the value be even faster, but
that puts new constraints on the attribute code in the implementations and
brings up issues with ordering attributes n' so on, so a simple HasAttr() for
now is fine with me.

Comment 3

[David Hyatt]

We should have a DOM method, hasAttributeValue(attr, val) that returns a
boolean, whether or not the attribute matches.  With such a method, we could
also fix up all our JavaScript and XBL to avoid the getAttribute(attr) == val
pattern.

I could add such a DOM method to nsIDOMXULElement, unless we feel such a method
would be warranted for all elements.

Comment 4

[David Hyatt]

You might want a boolean for case-insensitivity as a third argument.

Comment 5

[Johnny Stenback (:jst)]

Sounds like something for nsIDOMXULElement if we choose to add something like that.

Comment 6

[timeless]

hrm, would it be better named IsAttrValue(...)?

Comment 7

[David Hyatt]

I think we shoudl be consistent with the DOM APIs... hasAttribute/hasAttr,
getAttribute/getAttr, setAttribute/setAttr, etc.

Comment 8

[Joe Hewitt (gone)]

Attachment: patch for HasAttr

This patch adds HasAttr to nsIContent and implements it in all the appropriate
places.  It also replaces the wasteful call to GetAttr inside of HasAttribute
with a call to HasAttr for both xul and generic elements.

Comment 9

[jag (Peter Annema)]

Comment on attachment 60912 [details] [diff] [review]
patch for HasAttr

>Index: mozilla/content/base/src/nsGenericDOMDataNode.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericDOMDataNode.h,v
>retrieving revision 3.49
>diff -u -r3.49 nsGenericDOMDataNode.h
>--- mozilla/content/base/src/nsGenericDOMDataNode.h	16 Oct 2001 05:30:42 -0000	3.49
>+++ mozilla/content/base/src/nsGenericDOMDataNode.h	8 Dec 2001 05:52:37 -0000
>@@ -515,6 +519,10 @@
>   NS_IMETHOD GetAttr(PRInt32 aNameSpaceID, nsIAtom *aAttribute,            \
>                      nsIAtom*& aPrefix, nsAWritableString& aResult) const {    \
>     return _g.GetAttribute(aNameSpaceID, aAttribute, aPrefix, aResult);    \
>+  }                                                                        \
>+  NS_IMETHOD HasAttr(PRInt32 aNameSpaceID, nsIAtom *aAttribute,       \

Nit: line up the \

>Index: mozilla/content/base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.198
>diff -u -r3.198 nsGenericElement.cpp
>--- mozilla/content/base/src/nsGenericElement.cpp	21 Nov 2001 09:20:52 -0000	3.198
>+++ mozilla/content/base/src/nsGenericElement.cpp	8 Dec 2001 05:52:37 -0000
>Index: mozilla/content/html/content/src/nsAttributeContent.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsAttributeContent.cpp,v
>retrieving revision 1.40
>diff -u -r1.40 nsAttributeContent.cpp
>--- mozilla/content/html/content/src/nsAttributeContent.cpp	16 Oct 2001 05:30:49 -0000	1.40
>+++ mozilla/content/html/content/src/nsAttributeContent.cpp	8 Dec 2001 05:52:38 -0000
>@@ -146,6 +146,7 @@
>   NS_IMETHOD UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute, PRBool aNotify) { return NS_OK; }
>   NS_IMETHOD GetAttr(PRInt32 aNameSpaceID, nsIAtom *aAttribute, nsAWritableString& aResult) const {return NS_CONTENT_ATTR_NOT_THERE; }
>   NS_IMETHOD GetAttr(PRInt32 aNameSpaceID, nsIAtom *aAttribute, nsIAtom*& aPrefix, nsAWritableString& aResult) const {return NS_CONTENT_ATTR_NOT_THERE; }
>+  NS_IMETHOD HasAttr(PRInt32 aNameSpaceID, nsIAtom *aAttribute, PRBool* aResult) const { return NS_OK; }

You probably should set *aResult to PR_FALSE

Fix these two comments and you'll have r=jag

Comment 10

[Blake Ross]

Comment on attachment 60912 [details] [diff] [review]
patch for HasAttr

sr=blake

Comment 11

[Blake Ross]

And, er,

+  return HasAttr(nsid, name, aReturn);
+  
   return NS_OK;

Comment 12

[jag (Peter Annema)]

Er, yeah. That.

Comment 13

[Johnny Stenback (:jst)]

sr=jst with the signature of HasAttr() changed to *return* a boolean in stead of
nsresult, as discussed with hewitt.

Comment 14

[Joe Hewitt (gone)]

fix checked in for HasAttr - I'll open a separate bug for HasAttrValue