347208 - Cocoa widgets paint way too often

Status: RESOLVED INVALID

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

1. Open a trunk Cocoa widgets build of Firefox
2. Open Quartz Debug, turn on "Flash screen updates"
3. Load a web page. I used an lxr C++ source code page as my test page.

== Expected Results ==
Page should not be redrawing unnecessarily, should not draw more often than Carbon widgets do.

== Actual Results ==
After hitting enter in the URL bar to go to the lxr page, the entire content area redraws 2-3x as often as it does in a Carbon build.

Comment 1

[Simon Fraser [no longer active]]

Does this happen in Camino?

Comment 2

[Josh Aas]

This does not happen in Camino. Here is what happens with the following test:

1. Go to a web page that does not have any animations, like an lxr page.
2. Turn on update flashing with Quartz Debug.
3. Go to another site, like Apple.com.

In trunk Camino, the first time the entire content area repaints is to paint part of the new page. In trunk Firefox, the page repaints up to 5 times before it paints to draw the new page. I'm still having trouble figuring out where the extra paints are coming from...

Comment 3

[Josh Aas]

When I was looking through drawing stack traces I noticed that a lot of painting is happening with [NSWindow displayIfNeeded] on the stack before the child view redraws, which would explain why this happens in Firefox but not Camino. I do not see [NSApp updateWindows] on the stack, so this probably isn't coming from the appshell.

Comment 4

[Josh Aas]

Attachment: stack trace for unnecessary draw

This is a stack trace from an unnecessary draw - I think. It is hard to tell sometimes. This stack wouldn't happen in Camino.

The [NSWindow displayIfNeeded] is coming ultimately from CFRunloop observer, not the [NSApp updateWindows] call in our appshell.

Comment 5

[Simon Fraser [no longer active]]

Did you have any modal dialogs up for that stack, or is the new event handling stuff supposed to call itself re-entrantly? That stack looks fugly to me.

Comment 6

[Josh Aas]

There were no dialogs up or anything like that. Our stacks often look like that now, I don't know why.

Comment 7

[Mark Mentovai]

The new thread manager is supposed to run the entire app on an event under Cocoa, but a side effect is that sometimes, there'll be another event that gets processed before the one we want, and we may wind up with a relatively static base-of-stack that enters the event loop a few times.

Comment 8

[Stuart Parmenter]

Attachment: fix

this fixes it for me.  I'm unable to reproduce the stuff that was in bug 294415 which is part of the stuff this removes.  from my reading of the docs, display will always redraw everything immediately vs displayIfNeeded will only redraw the stuff that has been marked as dirty (what we actually want).  If we do need something similar to what was in 294415, i suggest that we keep a dirty region and at the end of drawRect we push a single message out telling us what we need to repaint.  I think this is enough though.

Comment 9

[Mark Mentovai]

Comment on attachment 238283 [details] [diff] [review]
fix

For the Invalidate method that takes a rect:

+    [mView displayIfNeeded];

That can be displayIfNeededInRect:

Comment 10

[Stuart Parmenter]

(In reply to comment #9)
> (From update of attachment 238283 [details] [diff] [review] [edit])
> For the Invalidate method that takes a rect:
> 
> +    [mView displayIfNeeded];
> 
> That can be displayIfNeededInRect:
> 

we don't want displayIfNeededInRect since we want a single paint for the areas that are currently marked dirty -- otherwise we end up with >1 paint if other areas are dirty.

Comment 11

[Stuart Parmenter]

checked in.

Comment 12

[Josh Aas]

Reopening because the extra drawing I was seeing is all still there and Tp didn't change at all after the checkin. I'm also concerned about losing the invalidate during a draw, which it looks to me like pav's fix would regress. Perhaps we should back out the patch until we understand what is going on better.

Stuart - do you see what I'm talking about when I say it draws the static page you're *leaving* about 5 times before you go to the new page?

Load the Firefox welcome page, then turn on Quartz Debug screen update flashing. Then go to apple.com and notice that the Firefox welcome page updates completely 5-6 times before updating to the new apple.com page. This doesn't happen in Carbon or embedded Cocoa (Camino).

Comment 13

[Simon Fraser [no longer active]]

Comment on attachment 238283 [details] [diff] [review]
fix

This patch negates some changes that I made to fix bug 294415. Did you check to see if that bug regressed?

Comment 14

[Stuart Parmenter]

(In reply to comment #13)
> (From update of attachment 238283 [details] [diff] [review] [edit])
> This patch negates some changes that I made to fix bug 294415. Did you check to
> see if that bug regressed?
> 

"I'm unable to reproduce the stuff that was in bug 294415 which is part of the stuff this removes." from comment 10...

Comment 15

[Stuart Parmenter]

(In reply to comment #12)
> Stuart - do you see what I'm talking about when I say it draws the static page
> you're *leaving* about 5 times before you go to the new page?
> 
> Load the Firefox welcome page, then turn on Quartz Debug screen update
> flashing. Then go to apple.com and notice that the Firefox welcome page updates
> completely 5-6 times before updating to the new apple.com page. This doesn't
> happen in Carbon or embedded Cocoa (Camino).
> 

The pages flash a lot less for me but still more than they did before.  Looking further.

Comment 16

[Stuart Parmenter]

OK, how about this: This whole bug is bogus.

According to people at Apple, way quartz debug paint flashing works is based on the union of the dirty area (the aRect passed in to the drawRect method on the child view).  The union while loading a page will be the throbber to the status bar.  This is the area you are seeing flashing.  If you turn off either the status bar or remove the throbber from the toolbar the large flashes will go away.

The area flashing is _not_ the area we are actually repainting in cocoa builds given that we go rect by rect inside the dirty region and paint those.  See the code here: http://lxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsChildView.mm#2513

In cocoa-cairo builds we are actually asking to repaint the whole area -- we should build an nsIRegion and stick it on the event for this case and let the view manager deal with it.  I have a patch for that, but it should live in another bug.

Comment 17

[Josh Aas]

Nice find!

Can you back out your patch? I don't want to risk regressing anything if there is no clear benefit from the change.

Comment 18

[Simon Fraser [no longer active]]

I think the real issue here is that now we have a window full of NSQuickDraw views, none of which is opaque, so that every draw has to back up to the root view.

I wonder if we could drop in some opaque NSViews to back certain NSQDViews?

Comment 19

[Josh Aas]

If that is actually the problem then we should just skip this whole thing and turn on Cairo-Cocoa. Problem gone.

Comment 20

[Stuart Parmenter]

(In reply to comment #17)
> Can you back out your patch? I don't want to risk regressing anything if there
> is no clear benefit from the change.
> 

My patch will avoid the extra repaints the patch for 294415 added that queue up all the invalidates while *any view is locked* -- not just the child window's view (or subviews).  As I'm unable to reproduce that bug with the current builds, I don't really see much reason to back it out.

Comment 21

[Josh Aas]

How about adding something like this then:

if ([NSView focusedView] == THIS_NATIVE_VIEW)
  SendDelayedInvalidate();

Comment 22

[Stuart Parmenter]

Can you actually reproduce the bug you're wanting to add this code back for?  If so, you want something more than just that -- you'd want to do what I suggested in comment 8.  Did anyone actually read comment 8 before replying to this bug?

Comment 23

[Stuart Parmenter]

i backed out this patch as it caused some problems like bug 352712. i'll look in to revamping the patch further when I have more time.