Open
Bug 243723
Opened 21 years ago
Updated 2 years ago
deCOMtaminate nsIViewManager
Categories
(Core :: Web Painting, defect, P5)
Tracking
()
NEW
People
(Reporter: bzbarsky, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Comment 1•21 years ago
|
||
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.
Reporter | ||
Comment 3•21 years ago
|
||
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.
Reporter | ||
Comment 6•21 years ago
|
||
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.
Reporter | ||
Comment 8•21 years ago
|
||
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.
Reporter | ||
Updated•21 years ago
|
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+
Reporter | ||
Comment 9•21 years ago
|
||
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+
Priority: -- → P1
Comment 10•20 years ago
|
||
Is this ready for checkin?
Reporter | ||
Comment 11•20 years ago
|
||
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
Reporter | ||
Comment 12•20 years ago
|
||
Comment on attachment 148640 [details] [diff] [review]
Do that.
This is checked in.
Attachment #148640 -
Attachment is obsolete: true
Comment 13•20 years ago
|
||
I just filed 276015 for cleaining GetOffsetFromView and all its callers
Updated•15 years ago
|
QA Contact: ian → layout.view-rendering
Comment 14•15 years ago
|
||
Should this bug still be open? AFAIK the current plan is to eliminate the view manager altogether.
Assignee: roc → nobody
Assignee | ||
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
Updated•6 years ago
|
Priority: P1 → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•