Consider using a bit for IsViewInserted()

NEW
Unassigned

Status

()

Core
Layout: View Rendering
P2
normal
14 years ago
6 years ago

People

(Reporter: bz, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {perf})

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

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).
Keywords: perf

Updated

14 years ago
Blocks: 203448
Sure, this is a great idea.
Priority: -- → P2
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....
Created attachment 147197 [details] [diff] [review]
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.
Attachment #147197 - Flags: superreview?(dbaron)
Attachment #147197 - Flags: review?(dbaron)
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?
Attachment #147197 - Flags: superreview?(dbaron)
Attachment #147197 - Flags: superreview+
Attachment #147197 - Flags: review?(dbaron)
Attachment #147197 - Flags: review+
(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.
Er... is that the right bug number, roc?
Perhaps bug 243343 as well.  roc mentioned there that he backed this out.

Comment 15

13 years ago
Can this one be marked fixed?
QA Contact: ian → layout.view-rendering
Assignee: roc → nobody

Comment 17

6 years ago
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.
You need to log in before you can comment on or make changes to this bug.