See bug 40988 comment 55. In short, IsViewInserted() is one of the things that can make insertion of abs pos objects O(N^2). It seems like this function should be able to just check a bit on the view (and we would need to maintain that bit, of course).

Sure, this is a great idea.

Actually my current thinking is that we should make it so views are *always* inserted. A view would be inserted into the tree in nsIView::Init. Instead of exposing InsertChild/RemoveChild, we'd expose a Reparent operation.

Sounds reasonable. I can't really think of a reason to have an unparented view lying about....

This patch takes a big step in the right direction by making a view manager always have a root view. SetRootView is no longer needed or supported. We deCOMtaminate GetRootView accordingly. I also remove GetWindowDimensions since no-one uses it. The behaviour of view destruction had to change a bit; previously, tearing down the viewport frame would destroy the view manager's root view; now we refrain from destroying the root view and leave that to the view manager destructor.

The next step after this is to change the API for non-root views. Instead of the current "create view, initialize it, insert it" behaviour, we'll have a single method nsIView::CreateChild which creates a child view, initializes it with the provided parameters, inserts it under the current view, and returns the new child. Also, view reparenting will change from "remove view, insert view" to a new nsIView::SetParent method.

Why is the code in nsContentSink.cpp not needed anymore?

It only does something if the root view QI's to an nsIScrollableView, and that never happens. That was true in the old code but it's much clearer in the new code...

Comment on attachment 147197 [details] [diff] [review] fix up semantics for root view It might be better to have nsIView::mRootView be an nsIView* instead of nsView* so you don't have to make the nsView name public. You could just put the NS_STATIC_CAST in a different RootView function. I find the nsIView::Destroy business a little weird. What's the problem, exactly? That a frame destructor calls that? Or, if it's just a safety thing, could you add an assertion?

(In reply to comment #8) > (From update of attachment 147197 [details] [diff] [review]) > It might be better to have nsIView::mRootView be an nsIView* instead of > nsView* so you don't have to make the nsView name public. You could > just put the NS_STATIC_CAST in a different RootView function. Done > I find the nsIView::Destroy business a little weird. What's the problem, > exactly? That a frame destructor calls that? Right. We don't want to tear down the root view when the viewport frame (which has the root view as its view) goes away. I'll add a better comment.

I checked in that patch last night.

This might have caused bug 242843

Er... is that the right bug number, roc?

er, bug 242823

Perhaps bug 243343 as well. roc mentioned there that he backed this out.

Can this one be marked fixed?

No, it was backed out.

Why does this depend on bug 337801 (view removal)? If I understand it correctly, then IsViewInserted() will not even exist after 337801. Was it the intention to block 337801 instead?

Because it was decided that this won't get done on it's own; the performance problem it's filed on won't go away until views are removed.