Closed Bug 113562 Opened 18 years ago Closed 18 years ago

nsIContent needs HasAttr method

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla0.9.7

People

(Reporter: hewitt, Assigned: hewitt)

References

Details

Attachments

(1 file)

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....
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
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 :-)
Component: DOM Content Models → DOM Other
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.
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.
Summary: nsIContent needs a HasAttr method → nsIContent needs HasAttr and HasAttrValue methods
Blocks: 113580
You might want a boolean for case-insensitivity as a third argument.
Sounds like something for nsIDOMXULElement if we choose to add something like that.
hrm, would it be better named IsAttrValue(...)?
I think we shoudl be consistent with the DOM APIs... hasAttribute/hasAttr,
getAttribute/getAttr, setAttribute/setAttr, etc.
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 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
Attachment #60912 - Flags: review+
Comment on attachment 60912 [details] [diff] [review]
patch for HasAttr

sr=blake
Attachment #60912 - Flags: superreview+
And, er,

+  return HasAttr(nsid, name, aReturn);
+  
   return NS_OK;
Er, yeah. That.
sr=jst with the signature of HasAttr() changed to *return* a boolean in stead of
nsresult, as discussed with hewitt.
fix checked in for HasAttr - I'll open a separate bug for HasAttrValue
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: nsIContent needs HasAttr and HasAttrValue methods → nsIContent needs HasAttr method
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.