Closed
Bug 208004
Opened 22 years ago
Closed 22 years ago
deCOMtaminate nsIFrame::GetView and SetView
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [patch])
Attachments
(1 file)
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
This is split out of bug 190735. This contains roughly the GetView / SetView /
HasView API changes in the patch there, plus all the changes needed to call
them. (I think the main difference is that I implemented the virtual function
|GetViewExternal| on nsIFrame.)
I didn't touch GetParentWithView / GetOffsetFromView yet, but I did move
nsFrame::GetClosestViewForFrame up to nsIFrame, and called it
nsIFrame::GetClosestView. And I used it in a lot more places, since it's a
common pattern (including replacing a 20+ line version in
nsTypedSelection::DoAutoScroll). I also added
nsIFrame::AreAncestorViewsVisible, which is a pattern used in three places
(twice in the ESM, once in DocShell). I'm not crazy about having it on
nsIFrame, but I'm not sure what else to do with it.
Assignee | ||
Comment 1•22 years ago
|
||
Assignee | ||
Comment 2•22 years ago
|
||
There's one thing I did in this patch that I might need to undo -- there were a
bunch of places that went through a good bit of trouble to get a view manager
from a view and it seemed much easier to get it from the pres shell, so I
changed it. But I've realized that the view hierarchy can extend across view
manager boundaries, so I probably need to change some of those things back,
although not necessarily all of them.
Wouldn't it be better to do this after we can remove the aPresContext parameter?
e.g., by adding an mPresContext field to nsFrame/nsIFrame?
Assignee | ||
Comment 4•22 years ago
|
||
I don't see how the order of those two matters.
if we do this first then we'll have to revisit all the GetView()/SetView() sites
again to remove the aPresContext parameter.
Assignee | ||
Comment 6•22 years ago
|
||
And if we do that first we'll have to revisit all the sites to change the rest
of the syntax. Unless you want to combine the two patches and make something
that's impossible to review, that is...
No, I was thinking that first we just provide a GetPresContext() method on
nsIFrame, implemented with an mPresContext member for now until we can move to a
single presentation model (at which time we'll be able to just do
mContent->GetDocument()->GetPresContext()).
Then we can remove aPresContext parameters method-by-method.
Assignee | ||
Comment 8•22 years ago
|
||
Method by method removal of the aPresContext parameters would be a lot more work
than removal all at once since many of the methods need it only to pass to
something else that takes an aPresContext parameter.
Assignee | ||
Comment 9•22 years ago
|
||
(However, it would be good to remove |aPresContext| parameters from methods on
nsFrameManager and similar first, since they can certainly have mPresContext
member variables without bloat problems. So I could do that...)
Assignee | ||
Updated•22 years ago
|
Whiteboard: [patch]
Assignee | ||
Comment 10•22 years ago
|
||
So I don't think there's much chance of the |aPresContext| removal happening
before this bitrots completely. Do you think this is worth checking in? (I
think so.)
Yeah, we should check this in
Comment on attachment 124752 [details] [diff] [review]
patch
I checked your GetViewManager changes; they all look fine, nowhere where you
converted to use the preshell was there any chance that you'd wandered out of
the current document.
I did not look at all the code, just the GetViewManager changes and the code in
nsIFrame.h, nsFrame.h and nsFrame.cpp.
It all looks great, r+sr=roc
Much of this code will get even better with the nsIView accessor changes in the
patch that I submitted last night :-)
Attachment #124752 -
Flags: superreview+
Attachment #124752 -
Flags: review+
Assignee | ||
Comment 13•22 years ago
|
||
Fix checked in to trunk, 2003-06-19 16:42/43/44 -0700.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•