507845 - Replace the hack to redraw the titlebar with something more sensible

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Markus Stange [:mstange]]

Attachment: v1

Back in bug 439354 comment 20, Roc said:

> +// [self display] seems to be the only way to repaint a window's titlebar.
> +// The bad thing about it is that it repaints all the window's subviews as
> well.
> +// So we use a guard to prevent unnecessary redrawing.
> 
> Hmm. If there was a dirty area already set in the window, won't this mean that
> Cocoa forgets about the dirty area but we won't have repainted it? Perhaps this
> works because we only call it in the middle of already repainting the window,
> and there's no other code doing invalidation during repainting? I don't feel
> comfortable about this... Can we save and restore the dirty area or something
> like that?

I then did some tests and verified that [window display] in fact doesn't forget about the dirty rects... on 10.5. Only much later I realized that this isn't the case on 10.4, and that we indeed drop some invalidations there. This is most certainly the cause of bug 458127, and if you've got 10.4, you can also reproduce it like this in Firefox:

1. Open the Preferences window.
2. Go to the Security pane.
3. Close the Preferences window.
4. Open it again.

Now most of the window stays white until you move your mouse around.

I finally found out a better way to redraw the titlebar (call displayRect on the border view), so we can remove that hack.

Comment 1

[Markus Stange [:mstange]]

I could write an automated test for this, but I guess it doesn't make sense when we don't have any Tinderboxes that run 10.4.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #1)
> I could write an automated test for this, but I guess it doesn't make sense
> when we don't have any Tinderboxes that run 10.4.

Wasn't bug 455089 supposed to have fixed that? :/

Comment 3

[Markus Stange [:mstange]]

The patch applies cleanly on top of the patch in bug 269410, by the way.

Comment 4

[Markus Stange [:mstange]]

Hmm, maybe I can't write a test for this after all. I was assuming that the MozAfterPaint event fired after actual redraws, not only after invalidations. With this bug we're invalidating just fine, we're just not repainting properly... and it seems I can't detect that.

Comment 5

[Markus Stange [:mstange]]

I've asked roc and he says that we can't test what's actually being repainted.
Too bad.

Comment 6

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/cdf768b8b45a

Comment 7

[Mark Banner (:standard8)]

Markus, has this backed enough to request 1.9.1 approval?

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 392114 [details] [diff] [review]
v1

Yes, I think we should take this now.

Reward: Fix for 10.4-only repainting bug (empty window parts), removal of hacks
Risk: very low, understood code, two weeks bake time
Tests: untestable (filed bug 510970)

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 392114 [details] [diff] [review]
v1

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 10

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ec0fdb90ae45

Comment 11

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a2pre) Gecko/20090911 Namoroka/3.6a2pre

Comment 12

[juan becerra [:juanb]]

I could not reproduce the problem on 3.5.3, for example, nor on an earlier version, nor in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1.4pre) Gecko/20091001 Shiretoko/3.5.4pre

All of these behaved the same on 10.4. Verified?

Comment 13

[Markus Stange [:mstange]]

Another way of verifying it on 10.4 which should work on 3.5 is this:
 1. Install  and activate the GrApple Delicious theme from
    http://www.takebacktheweb.org/ and restart Firefox.
 2. Open this page.
 3. Click the toolbar collapse button in the top right corner of the Firefox
    window. Without this patch, you should see a painting error which you can
    move away by scrolling.

Does that work?

Comment 14

[juan becerra [:juanb]]

Thanks, Markus. Following those steps shows the problem in 3.5.3, but not in 3.5.4pre. Adding the "verified1.9.1" keyword.