Currently, if a widget has been set to draw in the titlebar (using nsIWidget::SetDrawsInTitlebar()), code in nsChildView::WillPaint() causes the entire titlebar to be redrawn every time any part of the browser window is drawn! This currently doesn't make much difference on trunk (mozilla-central), where we rarely draw in the titlebar. But it causes a very large and visible performance hit on the UX branch, where we draw in the titlebar by default. People have seen this performance hit on the UX branch and blamed it on not (yet) having a fix for bug 676241. Not so. I have a patch for this bug, which I'll post shortly.

Here's my patch. As I mentioned above, it currently doesn't make much difference on the trunk. But try it on the UX branch, and you'll be amazed! (As best I can tell it's not possible to do tryserver builds on the UX branch. Please tell me if I'm wrong.) This patch makes a big difference (on the UX branch) whether or not Preferences : Advanced : General : Use hardware acceleration is on. But the difference is bigger if it's on. Some of my performance tests are subjective. For example window resizing feels a lot more responsive (with my patch, on the UX branch) than without it. Which is a bit surprising, since the titlebar does actually need to be redrawn while the window is being resized. But apparently even when resizing the window, without my patch the titlebar is redrawn far more often than necessary. A somewhat more objective test is to run an animation or HTML5 video (like the one at http://www.mozilla.org/en-US/firefox/20.0/whatsnew/) while running "top" in a Terminal window. You need to let the video run a while for top to "settle down". Once it does, you'll find that it uses about twice as much CPU without my patch as with it. (The titlebar should never be redrawn while the video is playing, as long as you don't resize the window. But without my patch *every paint anywhere in the window* causes it to be redrawn completely.) (So far all my tests have been done with non-opt builds. Later I'll try them with opt builds.)

(Following up comment #1) I should mention that in my tests, with my patch the UX branch is about as fast as mozilla-central.

This bug should block Australis' initial iteration on the trunk. (Though I don't know how to indicate that -- I don't know which bug to make this one block.) But, as far as I'm concerned, bug 676241 should no longer block it.

Comment on attachment 736929 [details] [diff] [review] Fix Oops, this doesn't work with multiple tabs (on the UX branch) -- not everything that should be updated does get updated. So I'll need to continue work on this next week. I'm confident, though, that it just requires minor tweaking.

Here's a patch that fixes the problems I reported in comment #4. It took me a little longer than I'd hoped to find it. But what I came up with seems very straightforward, and should be problem free. The reason for the missing updates is that the rectangles invalidated by -[ToolbarWindow setTitlebarNeedsDisplayInRect:] were getting cleaned up (by calls to -[NSView drawRect:] on the frame view) before nsChildView::WillPaint() got called to take advantage of their presence. (I don't know why invalidating parts of the frame view doesn't cause the same parts to be invalidated in our ChildView. But figuring that out is something for the longer term.)

Here's a UX-branch build I made with this patch. It's an opt build, with a build config as close as I can manage to that of regular mozilla-central nightlies. https://people.mozilla.com/~stmichaud/bmo/firefox-23.0a1.en-US.mac-ux-bugzilla861317.dmg Please try it out! As it turns out, the current UX branch no longer draws in the titlebar, or puts tabs there. See bug 625989 comment #72 and following. My test build was made from UX branch code as of this Monday morning (2013-04-15), when it still drew in the titlebar.

Note that drawing in the titlebar causes weirdness on OS X 10.6 -- the titlebar proper (the top 22 pixels) is the wrong color. But this happens with or without my patch, and presumably no longer happens on the current UX branch (which no longer has tabs in the titlebar).

First I must say I am very pleased with this. Great job finding this little (but significant) problem. I tried out your build and performance has dramatically increased! The swipe animations, which were before very laggy, are now working flawlessly. Resizing windows is also much faster. However, I must unfortunately point you towards a few little ui bugs from this. I can not be sure this is your fault, perhaps the day your UX build was from had this problem, but it looks to me like this is related to not updating enough. I will upload an image that shows and describes the errors.

Image showing and describing the graphical bug.

Thanks for your report, Josiah. But you really do need to describe in detail how to reproduce your glitches. The screenshots help, but aren't enough by themselves. I can reproduce the "separator being cut off" glitch: Just keep pressing Cmd-t until there are too many tabs to display in the tab bar. I can't (yet) reproduce the "left overflow button not displayed properly" glitch. The "separator being cut off" glitch (at least) does seem to have been caused by my patch -- it doesn't happen with the exact same codebase without my patch. Interestingly, it also happens whether or not HiDPI mode is on, and whether or not hardware acceleration is on. So there must be some updates that I'm still missing. I'll look for a quick fix. And if I find one I'll post it here. But if the fix will take longer, I think we should just land this patch and open followup bugs.

The reason I didn't give a way to reproduce is them because they reproduce every time, though I forgot that not everyone has enough tabs to start the overflow mode. As for the arrow bug, it happens every time as well. Just overflow the tabs, and click and hold the buttons so that the tabs keep sliding over. The bug happens every time from my experience. Also note that it is not just the left arrow, but the right as well. I have hardware acceleration on if it matters. Sorry for not clarifying this earlier. The arrow glitch I think might be essentially the same as the tab separator one. It looks like it is not drawing the part of the image that is in the titlebar. By fixing the first you probably will fix the second. But yes, these are minors bugs, and definitely don't stop landing them because of this.

> and click and hold the buttons so that the tabs keep sliding over This additional information helps, but still isn't enough. I suspect it matters which page is displayed in the visible tab when you do this. Which page did you test with?

(In reply to Steven Michaud from comment #12) > > and click and hold the buttons so that the tabs keep sliding over > > This additional information helps, but still isn't enough. I suspect it > matters which page is displayed in the visible tab when you do this. > > Which page did you test with? Apparently I was wrong about that. Click and hold the button twice. (Click and hold for half a second, release, and try again) the bug happens every time after that. It looks like it works the very first onClick, but after that it happens repeatedly. The tab page does not seem to influence it in any way. I tried the UX Start page, Bugzilla pages, stackexchange pages, all of them caused the same glitch. This is very easy to reproduce, and I am surprised this is not so on your end. Only other relevant detail I can think of right now is the tab count, which for me is 16.

Found another bug. Hopefully you can reproduce this one. Start with a window that does not completely fill the screen. Then hit the resize button. The titlebar area should become corrupted-looking. After it is done resizing it will look normal. Resizing back down does not cause the issue. Tab count = 16 Does not matter what site.

Thanks again. I can't reproduce this one, either. But I do see temporary glitches as the window is "maximizing" (which I don't see without my patch). And (as I mentioned earlier) I can reproduce your first glitch (the separator being cut off). So I've got enough to work with. Let me see if I can find a quick fix. If I can, I'll post the patch and a build, and ask you to test it/them.

I thought of one possible quick fix, but it doesn't work. Furthermore, if you take a close look at the "separator being cut off" glitch as it happens, it seems to be caused by trying to invalidate a region that's a few pixels too low, but which would be in exactly the right position if the tabs hadn't been moved up in the titlebar. So it looks like the "separator being cut off" bug (at least) wasn't *caused* by my patch, but was uncovered by it. (Without my patch, the titlebar is updated so often and so indiscriminately that invalidation bugs get papered over.) Once this patch lands, I'll open a new bug on the "separator being cut off" glitch, which seems to be the most easily described of all of these glitches. With luck fixing that will fix the rest of them.

Sounds like a fine plan to me. Thanks Stephen!

Comment on attachment 738704 [details] [diff] [review] Fix rev1 Review of attachment 738704 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsChildView.mm @@ +2671,5 @@ > + // Check the parts of the frame view occupied by the titlebar to see if all > + // or part of it is "dirty" (needs to be redrawn). This is effectively the > + // top 22 pixels of the frame view, and is not the same thing as the > + // "unified toolbar". mView is the same size as the frame view and covers > + // it. mView has a flipped coordinate system, but the frame view doesn't. excellent.

Comment on attachment 738704 [details] [diff] [review] Fix rev1 Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e75ff759c5c1