Closed Bug 507845 Opened 10 years ago Closed 10 years ago

Replace the hack to redraw the titlebar with something more sensible

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .4-fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: verified1.9.1, verified1.9.2, Whiteboard: [tb3needed])

Attachments

(1 file)

Attached patch v1Splinter Review
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.
Attachment #392114 - Flags: review?(joshmoz)
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.
Blocks: 458127
(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? :/
The patch applies cleanly on top of the patch in bug 269410, by the way.
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.
I've asked roc and he says that we can't test what's actually being repainted.
Too bad.
Attachment #392114 - Flags: review?(joshmoz) → review+
http://hg.mozilla.org/mozilla-central/rev/cdf768b8b45a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [tb3needs]
Markus, has this backed enough to request 1.9.1 approval?
Attachment #392114 - Flags: approval1.9.1.3?
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)
Attachment #392114 - Flags: approval1.9.1.3? → approval1.9.1.4?
Whiteboard: [tb3needs] → [tb3needs][awaiting branch approval]
Comment on attachment 392114 [details] [diff] [review]
v1

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #392114 - Flags: approval1.9.1.4? → approval1.9.1.4+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ec0fdb90ae45
Whiteboard: [tb3needs][awaiting branch approval] → [tb3needs]
Whiteboard: [tb3needs] → [tb3needed]
verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a2pre) Gecko/20090911 Namoroka/3.6a2pre
Keywords: verified1.9.2
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?
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?
Thanks, Markus. Following those steps shows the problem in 3.5.3, but not in 3.5.4pre. Adding the "verified1.9.1" keyword.
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.