208004 - deCOMtaminate nsIFrame::GetView and SetView

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

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

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?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't see how the order of those two matters.

Comment 5

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

if we do this first then we'll have to revisit all the GetView()/SetView() sites
again to remove the aPresContext parameter.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 7

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

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

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

Yeah, we should check this in

Comment 12

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

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-06-19 16:42/43/44 -0700.