49175 - View event processing needs to be made fast, robust and moved into the view manager

Status: RESOLVED DUPLICATE of bug 96832

(Core :: Web Painting, defect, P3)

Description

[alla]

While profiling to fix bug 39573 i found some slow code in
nsView::HandleEvent(). It basically looped numChildren times calling
GetChild(i), and each GetChild looped i times to find the child.

I've written a patch that fixes this and two more like it. I'll attach it. It
was a major helper in getting performance up on bug 39573.

Comment 1

[alla]

Attachment: Converts 3 O(n^2) functions in nsView to O(n)

Comment 2

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

The paint patch should be unnecessary. As far as I know, nsViewManager2 always 
uses NS_VIEW_FLAG_JUST_PAINT, so in fact all the code in nsView relating to 
painting of child views is dead code. I've actually removed that code from my 
tree and put in an assertion in case it would ever be used, and everything works 
just fine... This code should just be removed.

The other loop changes look good.

If Kevin doesn't want this bug, he can assign it to me, and if I get review and 
approval I can check the code in.

Comment 3

[Kevin McCluskey (gone)]

Robert is correct, nsViewManager2 always passes NS_VIEW_FLAG_JUST_PAINT. The 
other logic which walks the view tree pre-dates the use of a display list. Its 
been left around simply for debugging.

Added perf keyword.

The other part of the patch looks good.

Comment 4

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

Adding patch keyword.

Can we get rid of the unused painting code? It confuses the reader, and at this 
point it's basically just wrong.

Comment 5

[Kevin McCluskey (gone)]

Sure. I no longer use it for debugging purposes. 

Comment 6

[Kevin McCluskey (gone)]

Re-assigning to Robert. 

Comment 7

[alla]

There are more of these, in nsScrollPortView.cpp and nsScrollingView.cpp. I'm
attaching patches for these too.

Comment 8

[alla]

Attachment: Fixes for O(n^2) loops in nsScrollPortView.cpp and nsScrollingView.cpp

Comment 9

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

Attachment: Complete patch, incorporating nsView paint code trimming and Alex's patches

Comment 10

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

OK, can I have review and approval for patch 13142 please?

Comment 11

[Brendan Eich [:brendan]]

Hoping kmcclusk can give an r=, or find someone who will.

/be

Comment 12

[Kevin McCluskey (gone)]

I reviewed the changes and they look good.

r=[e-mail]

Comment 13

[Brendan Eich [:brendan]]

Great work, a=[e-mail].

/be

Comment 14

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

Fix checked in. Thanks Alex, Brendan, Kevin!

Comment 15

[Daniel Bratell]

alex mentions in bug 39753 that this bug's fix has been reverted. If that's the 
case shouldn't this bug be reopened?

Comment 16

[gabriel]

That should be bug 39573.

Comment 17

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

Reopening. To truly fix this bug is actually quite difficult, and probably won't 
happen soon.

My summary from bug 50335 discussion:
View event handling must be able to deal with the deletion of any arbitrary view 
during each event handler call. The best way to achieve this seems to be to move 
the event handling logic out of nsView::HandleEvent and up into nsViewManager. I 
have a patch in my tree that does this and stops the crashes but does not 
resolve all the problems, namely:
-- Some event processing can be dropped if a view is destroyed during event 
handling, even if the content is still around and would normally receive the 
event (could cause VERY RARE bugs)
-- Event processing needs to be reworked to handle Z-order properly, using a 
display-list-like mechanism.

A general "event handling in views" rewrite is needed. I will produce one, but 
maybe not soon :-).

Comment 18

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 19

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

*** This bug has been marked as a duplicate of 96832 ***