243723 - deCOMtaminate nsIViewManager

Status: NEW

(Core :: Web Painting, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

Some methods on this interface (eg GetWidgetForView()) should really not be
using COM-like signatures (I'm picking on GetWidgetForView() because it and
especially its callers actually showed up in a profile of some DHTML stuff I was
looking at).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note profile is in bug 243726

Comment 2

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

Actually what I want to do is to move a lot of view manager methods to nsIView
and deCOMtaiminate them at the same time. It's happening slowly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So is that the plan for GetWidgetForView()?  If so, I'll put it on my "to
do as soon as I pass my topic" list... ;)

Comment 4

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

Actually we can just remove GetWidgetForView and have callers call
nsIView::GetNearestWidget instead.

Comment 5

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

Attachment: tiny cleanup related to the above

cleanup to recognize that (unless we have to back it out) we always have a root
view now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm... Is the aPoint-twiddling done by GetNearestWidget() going to be made up
for by the lack of AddRef/Release?  Would it make more sense to have an
nsIFrame-like API (with GetNearestWidget being like GetClosestView and
GetOffsetFromWidget being like GetOffsetFromView)?

Comment 7

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

I think if you just pass nsnull as the point parameter, you won't notice the
overhead.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Do that.

So I couldn't sleep.....

On that testcase, with this patch, overall time is not changed much
(GetWidgetForView was not a huge part of it, all things considered), but
instead of having:

84	176 nsViewManager::GetWidgetForView(nsIView*, nsIWidget**)
	 60 nsWindow::AddRef()
	 32 nsWidget::AddRef()

We now have:

93	 93 nsIView::GetNearestWidget(nsPoint*)

So the time in the function itself increased by 12% or so (probably plus minus
5% ;) ), but not having to addref those widgets/windows sure helps.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148637 [details] [diff] [review]
tiny cleanup related to the above

r+sr=bzbarsky on this btw, once the rootview patch relands.

Comment 10

[Markus Hübner]

Is this ready for checkin?

Comment 11

[Boris Zbarsky [:bzbarsky]]

The first patch in this bug depends on bug 233441 (per comments).  The second
patch was checked in on May 17, 2004 (as a trivial check of the relevant CVS
logs would have told you).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148640 [details] [diff] [review]
Do that.

This is checked in.

Comment 13

[Bernd]

I just filed 276015 for cleaining GetOffsetFromView and all its callers

Comment 14

[Zack Weinberg (:zwol)]

Should this bug still be open?  AFAIK the current plan is to eliminate the view manager altogether.