Open Bug 243723 Opened 16 years ago Updated 1 year ago

deCOMtaminate nsIViewManager

Categories

(Core :: Web Painting, defect, P5)

x86
Linux
defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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).
Blocks: 243726
Note profile is in bug 243726
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.
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... ;)
Actually we can just remove GetWidgetForView and have callers call
nsIView::GetNearestWidget instead.
cleanup to recognize that (unless we have to back it out) we always have a root
view now.
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)?
I think if you just pass nsnull as the point parameter, you won't notice the
overhead.
Attached patch Do that. (obsolete) — Splinter Review
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.
Attachment #148640 - Flags: superreview?(roc)
Attachment #148640 - Flags: review?(roc)
Attachment #148640 - Flags: superreview?(roc)
Attachment #148640 - Flags: superreview+
Attachment #148640 - Flags: review?(roc)
Attachment #148640 - Flags: review+
Comment on attachment 148637 [details] [diff] [review]
tiny cleanup related to the above

r+sr=bzbarsky on this btw, once the rootview patch relands.
Attachment #148637 - Flags: superreview+
Attachment #148637 - Flags: review+
Is this ready for checkin?
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).
Depends on: 233441
Comment on attachment 148640 [details] [diff] [review]
Do that.

This is checked in.
Attachment #148640 - Attachment is obsolete: true
I just filed 276015 for cleaining GetOffsetFromView and all its callers
QA Contact: ian → layout.view-rendering
Blocks: deCOM
Should this bug still be open?  AFAIK the current plan is to eliminate the view manager altogether.
Component: Layout: View Rendering → Layout: Web Painting
Priority: P1 → P5
You need to log in before you can comment on or make changes to this bug.