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?

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

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.

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.

Sure, fine with me.

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... ;)

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

Comment on attachment 148954 [details] [diff] [review] diff -w, for review only seems ok to me

why are GetNextSibling etc not nonvirtual and inline?

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...

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

Alright then. Coming right up. ;)

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

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 :-)

> 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 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 ;-)

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. ;)

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

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

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

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 on attachment 153271 [details] [diff] [review] fix DEBUG-only leak regression r+sr=bzbarsky

Comment on attachment 153271 [details] [diff] [review] fix DEBUG-only leak regression Checked in to trunk, 2004-07-15 12:58 -0700.