Closed Bug 453858 Opened 12 years ago Closed 12 years ago

[FIX]Improve nsArraySH::GetItemAt (bites dromaeo)

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: perf)

Attachments

(1 file)

Right now nsArraySH::GetItemAt returns an nsIDOMNode, but the caller just wants an nsISupports and the callees all have an nsINode.

Ideally we'd use an interface other than nsIDOMNodeList here.

This pops up on some of the dromaeo tests.
Attachment #337102 - Flags: superreview?(jst)
Attachment #337102 - Flags: review?(jst)
Note to self:  Needs a missing include of nsINodeList in nsDOMClassInfo.cpp.
Nevermind comment 1: nsContentList.h includes it.
Comment on attachment 337102 [details] [diff] [review]
Somewhat like this, perhaps

nsresult
nsArraySH::GetItemAt(nsISupports *aNative, PRUint32 aIndex,
                     nsISupports **aResult)

I wonder if we could get away with returning a weak reference to the item here to avoid hitting addref/release one extra time? New bug on that if it seems feasable?

r+sr=jst
Attachment #337102 - Flags: superreview?(jst)
Attachment #337102 - Flags: superreview+
Attachment #337102 - Flags: review?(jst)
Attachment #337102 - Flags: review+
> I wonder if we could get away with returning a weak reference to the item here

Some of the overrides of this make it a little difficult, when I looked.  :(
Pushed changeset d544d5580393.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
And pushed changeset 5bb75cf5681a to fix orange.
And changeset 921f13d47435.
And changeset c8473a063eb0.
And changeset 075d7eb53180.  :(
But now it's stuck, at least!
Comment on attachment 337102 [details] [diff] [review]
Somewhat like this, perhaps

>+  if (aIndex < mElements.Length()) {
>+    mElements[aIndex];
>+  }
I didn't look to see what got checked in but this looks wrong.
Yeah, it caused orange on tinderbox; one of the followup checkins fixed it.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.