243724 - [FIX]deCOMtaminate nsIWidget::GetChildren

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Looking at this DHTML profile, I see us spending nearly 1.5% of the total time
in nsBaseWidget::GetChildren.  I don't see any reason this function can't just
return an nsCOMArray<nsIWidget>*, really....  Alternately, we could switch to a
GetChildCount()/GetChildAt() api, which can be made fast.

Thoughts on which is preferable?

Comment 1

[Christopher Blizzard (:blizzard)]

I prefer the latter, myself.  Mostly from a style standpoint.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would actually prefer storing the children in a linked list and providing
GetFirstChild() and GetNextSibling(). Insertion and removal need to be fast, but
indexed access does not.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm...  I guess we always iterate over widgets in a linear order, don't we?

Chris, that sound OK to you?  If so, I'll hack this up sometime soon.

Comment 4

[Christopher Blizzard (:blizzard)]

Sure, fine with me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, looks like Mac code iterates over widgets in reverse order in
nsWindow::FindWidgetHit (widget/src/mac/nsWindow.cpp).  So I'll just go and do
something evil with PRCList here... ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er, or not.  It's simpler to just do a straight doubly-linked list and not
bother with evil casting.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w, for review only

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: diff for checkin

Comment 9

[Christopher Blizzard (:blizzard)]

Comment on attachment 148954 [details] [diff] [review]
diff -w, for review only

seems ok to me

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

why are GetNextSibling etc not nonvirtual and inline?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Mostly because I didn't want to add members to nsIWidget.  If you think adding
such members is fine, I can certainly push them up there...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see a problem with that. I think we've already broken all compilers that
don't handle this case well...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Alright then.  Coming right up.  ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to that comment

The only differences from the first patch are in nsBaseWidget.{h,cpp} and
nsIWidget.h

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 149006 [details] [diff] [review]
Patch updated to that comment

+    nsIWidget* GetFirstChild(void) const {

Don't use (void). Just () is sufficient in C++.

+    for (nsIWidget* kid = mFirstChild; kid; ) {
+	 nsIWidget* next = kid->GetNextSibling();
+	 kid->Destroy();
+	 kid = next;
     }

could be just
while (mFirstChild) {
  mFirstChild->Destroy();
}
right?

 + parent->RemoveChild(this);

I think you should hold a strong ref to this during this method, just to be on
the safe side. Currently we seem to be assuming that the caller holds a strong
ref.

I'd give you sr+ but you can wait for blizzard if you want :-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Don't use (void). Just () is sufficient in C++.

OK.

> could be just
> while (mFirstChild) {

That assumes they _will_ remove themselves.  Is that a good assumption?  I guess
that's done by nsBaseWidget, so yes...

If you prefer, I can do it that way.

> I think you should hold a strong ref to this during this method

You're right, since the caller's ref goes away when we remove.  Will do.

If you want to sr, go for it.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 149006 [details] [diff] [review]
Patch updated to that comment

sr+ with those changes.

Hmm, technically I'm not a widget peer or owner. But marshalling all the owners
would be hellish. I won't tell if you won't ;-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

I've left the Destroy() thing as-is because I actually can't tell whether the
gtk2 nsWindow::Destroy ever calls nsBaseWidget::Destroy and the code as written
works no matter whether it does....

I think blizzard being OK with the basic idea is good enough owner-wise.  ;)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked in with some minor edits to fix mac orange (changing an assert to a
null-check, basically).

Comment 20

[timeless]

I checked in a fix for some xlib fallout (visible on boffo) it would have 
broken speedracer had it not been awol.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Doh.  I hate changing platform-specific code...  Thanks for catching that, Josh!

Comment 22

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

Attachment: fix DEBUG-only leak regression

Comment 23

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

Comment on attachment 153271 [details] [diff] [review]
fix DEBUG-only leak regression

(I filed bug 251527 on making nsIWidget::GetParent return
already_AddRefed<nsIWidget>.)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153271 [details] [diff] [review]
fix DEBUG-only leak regression

r+sr=bzbarsky

Comment 25

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

Comment on attachment 153271 [details] [diff] [review]
fix DEBUG-only leak regression

Checked in to trunk, 2004-07-15 12:58 -0700.