233441 - Consider using a bit for IsViewInserted()

Status: NEW

(Core :: Web Painting, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

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

Sure, this is a great idea.

Comment 2

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

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

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

Attachment: fix up semantics for root view

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.

Comment 5

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

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.

Comment 6

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

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

Comment 7

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

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 8

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

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?

Comment 9

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

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

Comment 10

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

I checked in that patch last night.

Comment 11

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

This might have caused bug 242843

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

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

er, bug 242823

Comment 14

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

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

Comment 15

[Markus Hübner]

Can this one be marked fixed?

Comment 16

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

No, it was backed out.

Comment 17

[Gábor Stefanik]

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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.