439354 - OS X toolbar background doesn't have a good gradient

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Håkan Waara]

Attachment: Thunderbird toolbar background [wrong]

Compare the two attachments.

Comment 1

[Håkan Waara]

Attachment: Any other app (e.g. Mail) [right]

Comment 2

[Phil Ringnalda (:philor)]

Near as I can tell, this is the issue discussed in bug 303110 comment 28 and bug 303110 comment 29 - because our gradient is really a single-color titlebar and then a gradient on the toolbar below it, it doesn't look as smooth as the native gradient. It's more obvious by default in Thunderbird, because we have large icons with text, and Firefox doesn't even have large icons. Side-by-side comparing Thunderbird with small icons and no text with Firefox, so the toolbars are the same height, I don't see anything Thunderbird-specific.

Comment 3

[Håkan Waara]

This is one of the items that was Daring Fireball'd yesterday apparently. See the attached URL. 

Maybe this is something to take a look at for Firefox 3.0.1, or 3.next?

Comment 4

[Colin Barrett [:cbarrett]]

The issue was that adding properties to XUL for the titlebar gradient.

Perhaps you want to add start and end color properties for a gradient?

Comment 5

[Markus Stange [:mstange]]

I've got a different idea - we could add a "unifiedtoolbarheight" attribute to the XUL window elements and let widget/cocoa construct the correct gradient.
Then we'd add a -moz-appearance: unified-toolbar so that we don't even need gradient images (or gradient SVGs) any more.
This solution would always draw a correct gradient, even when changing the toolbar size or collapsing it.

Comment 6

[Markus Stange [:mstange]]

I'm working on a cool patch that doesn't need additional attributes.

Comment 7

[Markus Stange [:mstange]]

Attachment: Screenshot of Addons window with upcoming patch

Comment 8

[Markus Stange [:mstange]]

Attachment: part 1 (widget) v0.1: Add -moz-appearance: toolbar-primary, add rendering and notifiy the window about the primary toolbar's height so that it can draw the correct gradient

A small part of this patch depends on the patch for bug 446463.

In this patch I'm adding -moz-appearance: titlebar-primary for the unified toolbar. When the toolbar is rendered, the containing window is automatically notified about the toolbar's height. That way it can scale the gradient in the titlebar accordingly.

I'm moving some things around in nsCocoaWindow.h/.mm - the colors and the shading callback function move into nsCocoaWindow.h so that nsNativeThemeCocoa.mm can reuse it and the titlebarHeight moves from TitlebarAndBackgroundColor to ToolbarWindow.

I'm sorry for the titlebar invalidation chaos but I haven't found a different way of doing it.

With the patch, even on Tiger all titlebars are always drawn with the dark Leopard grey (takes care of bug 435337).

Comment 9

[Markus Stange [:mstange]]

Attachment: part 2 (toolkit): Use the new -moz-appearance and remove obsolete styles

I think some things are getting simpler with this patch.

Comment 10

[Markus Stange [:mstange]]

I should note that this approach has one problem: when the bookmarks toolbar is hidden, there's a slight mismatch between the colors of the toolbar and the selected tab. I'm going to fix this in a follow-up bug - when color management is turned on, theme images are subject to color correction but native toolbar rendering is not, so it's not exactly trivial.
When that's fixed, bug 403169 (or similar bugs) will finally be history.

Comment 11

[Markus Stange [:mstange]]

Attachment: part 1 (widget) v0.2: remove something unnecessary

Comment 12

[Markus Stange [:mstange]]

Attachment: part 2 (toolkit) v0.2: remove even more

I missed browser.css's #nav-bar styling in the last patch.

Comment 13

[Markus Stange [:mstange]]

Attachment: part 1 (widget) v0.3: Rename the -moz-appearance to "-moz-mac-unified-toolbar", improve titlebar redraw handling

Håkan, could you review this? Josh doesn't have time for these things for some weeks but we should get it in sooner rather than later so that there's enough time for adapting the theme.

Comment 14

[Markus Stange [:mstange]]

Attachment: part 2 (toolkit / browser) v0.3: use -moz-mac-unified-toolbar

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> Created an attachment (id=332240) [details]
> part 2 (toolkit / browser) v0.3: use -moz-mac-unified-toolbar

Is there an issue here with removing the unified window bindings that might break extensions or applications created that use them?

Comment 16

[Markus Stange [:mstange]]

(In reply to comment #15)
> Is there an issue here with removing the unified window bindings that might
> break extensions or applications created that use them?

Yes, extensions / applications would have to be updated. Is this something we can afford to do or should I keep the binding?

Themes won't be affected because they bring their own binding.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #16)
> (In reply to comment #15)
> > Is there an issue here with removing the unified window bindings that might
> > break extensions or applications created that use them?
> 
> Yes, extensions / applications would have to be updated. Is this something we
> can afford to do or should I keep the binding?

I was hoping there might be a way to make the current binding just work but it looks like with the changes as they are now that isn't really easy to do. 

> Themes won't be affected because they bring their own binding.

Except that that binding will no longer be effective because you have removed the titlebarcolor attributes I think.

Regardless of whether extensions/apps have to make changes this patch does cause a regression in what capabilities are available to colour the titlebar (i.e. they can't do anything except opt for the leopard unified colour where before they could choose any fixed colour they wanted).

Given that this is an OSX only feature maybe that is ok, and I certainly prefer the new approach. If we do this we mst be careful to document exactly what changes people need to make. I don't suppose there is a way to specify gradient colours using -moz-title-gradient style rules or something?

Comment 18

[Markus Stange [:mstange]]

(In reply to comment #17)
> I was hoping there might be a way to make the current binding just work

There is - just keep it.

> > Themes won't be affected because they bring their own binding.
> 
> Except that that binding will no longer be effective because you have removed
> the titlebarcolor attributes I think.

I didn't remove the titlebarcolor functionality. The GrApple theme still works. I only removed the usage of the titlebarcolor attributes in the default theme because for the default appearance they're not necessary anymore. The functionality is still there.

> If we do this we mst be careful to document exactly what
> changes people need to make.

Of course.

> I don't suppose there is a way to specify gradient
> colours using -moz-title-gradient style rules or something?

Using a CSS property instead of presentational attributes would certainly be nice. Unfortunately I don't know how to do that and I don't know if it's worth it.

We could introduce *gradientstart and *gradientend attributes like Colin suggested in comment 4 but I don't really want to make things more complex...

Comment 19

[Stefan [:stefanh]]

Despite the problems with future extensions etc, I think Markus patch is the right approach here. If the patch gets in, it'll also be possible to fix the statusbar/sidebar in a much saner way (since you can access info about the state of the window from nsNativeThemeCocoa).

Comment 20

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

Looks good.

+// When used to draw the titlebar:
+//   *aIn == 0 at the bottom of the titlebar, *aIn == 1 at the top
+// When used to draw the toolbar:
+//   *aIn == 0 at the top of the toolbar, *aIn == 1 at the bottom

Why do these have to be inconsistent?

+    // We can't just use a static because the height can vary by window, and we don't
+    // want to recalculate this every time we draw. A member is the best solution.
+    mTitlebarHeight = frameRect.size.height - [self contentRectForFrameRect:frameRect].size.height;

We should probably just recalculate that every time, it's not slow. But you're just moving the code so it's OK.

+// [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?

Comment 21

[Håkan Waara]

So this stuff is getting really complicated. I don't really understand all that is going on to draw the titlebar/gradient stuff. 

I agree with roc that the full redraw of the window is something we really want to avoid. 

Right now everything below the titlebar is custom drawn. With the titlebarcolor/gradient stuff, our intent is basically that we also want to draw the titlebar ourselves as well. There is (obviously) no support for this, which makes things awful. 

So I had another idea: Would all this (and other related hacks) become simpler if the ToolbarWindow was a borderless window where we draw the titlebar by ourselves? The downside is that we need to draw the close buttons, the pill button etc. ourselves, but I'm not sure how the current situation is any better. It might be worth an experiment at least? I have a hunch that it could simplify these things.

Comment 22

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

That could have all kinds of unexpected side effects, I'm reluctant to go there.

Comment 23

[Markus Stange [:mstange]]

Attachment: part 1 (widget) v0.4: Address review comments

(In reply to comment #20)
> +// When used to draw the titlebar:
> +//   *aIn == 0 at the bottom of the titlebar, *aIn == 1 at the top
> +// When used to draw the toolbar:
> +//   *aIn == 0 at the top of the toolbar, *aIn == 1 at the bottom
> 
> Why do these have to be inconsistent?

Good idea to make them consistent... I just had to flip the gradient start and end points in patternDraw.

> 
> +    // We can't just use a static because the height can vary by window, and
> we don't
> +    // want to recalculate this every time we draw. A member is the best
> solution.
> +    mTitlebarHeight = frameRect.size.height - [self
> contentRectForFrameRect:frameRect].size.height;
> 
> We should probably just recalculate that every time, it's not slow.

Done.

> +// [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?

I don't think [self display] changes anything about the needsDisplay markers. It just calls drawRect synchronously and unconditionally.
To make sure, I added some printfs and this is what I get when I collapse the toolbar of the Page Info window:

 ~~ setNeedsDisplay
 ~~ setNeedsDisplay
 ~~ setNeedsDisplay
 ~~ setNeedsDisplayInRect: (0, 0, 515, 368)
 ~~ setNeedsDisplayInRect: (0, 0, 535, 23)
 ~~ setNeedsDisplayInRect: (0, 22, 11, 368)
 ~~ setNeedsDisplayInRect: (525, 22, 10, 368)
 ~~ setNeedsDisplayInRect: (0, 389, 535, 11)
 !! drawRect: (0, 0, 535, 399)
 >>>>> SUPPRESS PAINTING 
 !! drawRect: (0, 0, 535, 399) -- canceled
 !! drawRect: (0, 0, 515, 367) -- canceled
 <<<<< UNSUPPRESS PAINTING 
 !! drawRect: (0, 0, 515, 367)

To me this doesn't look like Cocoa forgets about the dirty rects.

(In reply to comment #21)
> So this stuff is getting really complicated.

Well, it certainly makes the CSS less complicated.

> I don't really understand all that
> is going on to draw the titlebar/gradient stuff.

What exactly is it that you don't understand?

> I agree with roc that the full redraw of the window is something we really 
> want to avoid.

Well, we do avoid it - using the isPaintingSuppressed guard.

> So I had another idea: Would all this (and other related hacks) become simpler
> if the ToolbarWindow was a borderless window where we draw the titlebar by
> ourselves?

I don't think so. I don't want a half-baked implementation, and there's a lot we can't do yet that would be necessary to implement it properly... just an example: how would you implement dragging a window by the titlebar while a sheet is open? Or lighting the buttons up on mouseover while the window is in the background?

> It might be worth an experiment at least?

Maybe, but now is probably the wrong time for it.

Comment 24

[Håkan Waara]

Comment on attachment 332342 [details] [diff] [review]
part 1 (widget) v0.4: Address review comments

Ok, with the painting suppression I guess it's not that bad. Sorry for missing it. 

By complicated, I mean we have a lot of code (already) to deal with all this, and am basically just interested if there's any opportunity for us to simplify it on the widget side. This patch makes it much simpler in CSS land, which is awesome.

Since you seem to have a good overview of it by now, would you like to document how our toolbar/titlebar drawing works, including our NSColor subclass etc, somewhere near the implementation? That would help a lot.

Here are some more code-specific comments:

>+static BOOL IsPaintingSuppressed(nsIWidget* aWidget)
>+{
>+  // Get the top-level ChildView.
>+  nsIWidget* topLevelWidget;
>+  while (aWidget) {
>+    topLevelWidget = aWidget;
>+    aWidget = aWidget->GetParent();
>+  }

Can nsChildView::GetTopLevelWidget be used here?

> // NSWindow subclass for handling windows with toolbars.
> @interface ToolbarWindow : NSWindow
> {
>   TitlebarAndBackgroundColor *mColor;
>+  float mUnifiedToolbarHeight;
>+  BOOL mSuppressPainting;
> }
> - (void)setTitlebarColor:(NSColor*)aColor forActiveWindow:(BOOL)aActive;
>+- (void)setUnifiedToolbarHeight:(float)aToolbarHeight;
> - (NSColor*)activeTitlebarColor;
> - (NSColor*)inactiveTitlebarColor;
>+- (float)unifiedToolbarHeight;
>+- (float)titlebarHeight;
>+- (void)redrawTitlebar;
>+- (BOOL)isPaintingSuppressed;
> // This method is also available on NSWindows (via a category), and is the 
> // preferred way to check the background color of a window.
> - (NSColor*)windowBackgroundColor;
> @end

Which methods here are not used by others? For example, redrawTitlebar is not, so it can live in a private category. Please create a ToolbarWindow(Private) category in the .mm file and put methods that are not used by other classes there.


>+static NSWindow* NativeWindowForFrame(nsIFrame* aFrame, int* aLevelsUp = NULL)
>+{
>+  if (!aFrame)
>+    return nil;  
>+
>+  nsIWidget* widget = aFrame->GetWindow();
>+  if (!widget)
>+    return nil;
>+
>+  // get the top-level widget
>+  nsIWidget* topLevelWidget;
>+  if (aLevelsUp)
>+    *aLevelsUp = -1;
>+  while (widget) {
>+    topLevelWidget = widget;
>+    widget = widget->GetParent();
>+    if (aLevelsUp)
>+      ++*aLevelsUp;
>+  }
>+
>+  return (NSWindow*)topLevelWidget->GetNativeData(NS_NATIVE_WINDOW);
>+}

Maybe this could somehow also make use of a GetTopLevelWidget? If it can't access it, maybe it's time for such a utility function in nsCocoaUtils?

>+void
>+nsNativeThemeCocoa::DrawUnifiedToolbar(CGContextRef cgContext, const HIRect& inBoxRect,
>+                                       nsIFrame *aFrame)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>+
>+  float titlebarHeight = 0;
>+  int levelsUp = 0;
>+  NSWindow* win = NativeWindowForFrame(aFrame, &levelsUp);
>+
>+  // If the toolbar is directly below the titlebar in the top level view of a ToolbarWindow
>+  if ([win isKindOfClass:[ToolbarWindow class]] && levelsUp == 0 &&
>+      inBoxRect.origin.y <= 0) {
>+    // Consider the titlebar height when calculating the gradient.
>+    titlebarHeight = [(ToolbarWindow*)win titlebarHeight];
>+    // Notify the window about the toolbar's height so that it can draw the
>+    // correct gradient in the titlebar.
>+    [(ToolbarWindow*)win setUnifiedToolbarHeight:inBoxRect.size.height];
>+  }
>+  
>+  BOOL isMain = win ? [win isMainWindow] : YES;
>+
>+  // Draw the gradient
>+  UnifiedGradientInfo info = { titlebarHeight, inBoxRect.size.height, isMain, NO };
>+  struct CGFunctionCallbacks callbacks = { 0, unifiedShading, NULL };
>+  CGFunctionRef function = CGFunctionCreate(&info, 1,  NULL, 4, NULL, &callbacks);
>+  float srcY = inBoxRect.origin.y;
>+  float dstY = srcY + inBoxRect.size.height - 1;
>+  CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
>+  CGShadingRef shading = CGShadingCreateAxial(colorSpace,
>+                                              CGPointMake(0, srcY),
>+                                              CGPointMake(0, dstY), function,
>+                                              NO, NO);
>+  CGColorSpaceRelease(colorSpace);
>+  CGFunctionRelease(function);
>+  CGContextClipToRect(cgContext, inBoxRect);
>+  CGContextDrawShading(cgContext, shading);
>+  CGShadingRelease(shading);

There's no Objective-C interface for most of this stuff in Core Graphics, right? :-)

Comment 25

[Markus Stange [:mstange]]

Attachment: part 1 (widget) v0.5: Address review comments

(In reply to comment #24)
> Since you seem to have a good overview of it by now, would you like to document
> how our toolbar/titlebar drawing works, including our NSColor subclass etc,
> somewhere near the implementation? That would help a lot.

Done.

> Can nsChildView::GetTopLevelWidget be used here?
> [...]
> Maybe this could somehow also make use of a GetTopLevelWidget? If it can't
> access it, maybe it's time for such a utility function in nsCocoaUtils?

I added a GetTopLevelWidget method to nsIWidget and removed the nsChildView implementation.

> 
> > // NSWindow subclass for handling windows with toolbars.
> > @interface ToolbarWindow : NSWindow
> > {
> >   TitlebarAndBackgroundColor *mColor;
> >+  float mUnifiedToolbarHeight;
> >+  BOOL mSuppressPainting;
> > }
> > - (void)setTitlebarColor:(NSColor*)aColor forActiveWindow:(BOOL)aActive;
> >+- (void)setUnifiedToolbarHeight:(float)aToolbarHeight;
> > - (NSColor*)activeTitlebarColor;
> > - (NSColor*)inactiveTitlebarColor;
> >+- (float)unifiedToolbarHeight;
> >+- (float)titlebarHeight;
> >+- (void)redrawTitlebar;
> >+- (BOOL)isPaintingSuppressed;
> > // This method is also available on NSWindows (via a category), and is the 
> > // preferred way to check the background color of a window.
> > - (NSColor*)windowBackgroundColor;
> > @end
> 
> Which methods here are not used by others? For example, redrawTitlebar is not,
> so it can live in a private category. Please create a ToolbarWindow(Private)
> category in the .mm file and put methods that are not used by other classes
> there.

Done. redrawTitlebar seems to be the only method that's only used by ToolbarWindow - and the (in)activeTitlebarColor methods weren't used at all, so I removed them.

> There's no Objective-C interface for most of this stuff in Core Graphics,
> right? :-)

http://blog.oofn.net/2006/01/15/gradients-in-cocoa/ says there isn't any. :-(

Comment 26

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

(In reply to comment #23)
> To me this doesn't look like Cocoa forgets about the dirty rects.

Great. Thanks!

Comment 27

[Markus Stange [:mstange]]

Attachment: part 2 (toolkit / browser) v0.4: Keep the binding [checked in]

Dave Townsend prefers keeping the binding in order not to break extensions. The rest of the patch hasn't changed.

Comment 28

[Markus Stange [:mstange]]

Attachment: part 1 (widget) v0.51: update to trunk, add widget border.

I updated the patch so that it applies cleanly.

And I made one change: I specify 1px bottom border in GetWidgetBorder because it makes sense.

Comment 29

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

Comment on attachment 333267 [details] [diff] [review]
part 1 (widget) v0.51: update to trunk, add widget border.

it helps to use -p in your patches so each hunk has a function name in it.

Comment 30

[Stefan [:stefanh]]

Markus, is attachment #333267 [details] [diff] [review] ready to land or does it need any more review? mconnor, are you happy with the new toolkit+browser version (attachment #332408 [details] [diff] [review])? 

Comment 31

[Markus Stange [:mstange]]

I think it's ready to land. :)

Comment 32

[Markus Stange [:mstange]]

I have to take that back; since bug 432131 was backed out, landing this would lead to ugly repainting problems on Tiger.

Comment 33

[Markus Stange [:mstange]]

Comment on attachment 332408 [details] [diff] [review]
part 2 (toolkit / browser) v0.4: Keep the binding [checked in]

mconnor said that this doesn't need another review.

Comment 34

[Stefan [:stefanh]]

Comment on attachment 333267 [details] [diff] [review]
part 1 (widget) v0.51: update to trunk, add widget border.

Hmm, this patch has bitrotted a bit

Comment 35

[Markus Stange [:mstange]]

Attachment: updated patch [checked in]

This should apply.

Comment 36

[Stefan [:stefanh]]

Comment on attachment 335619 [details] [diff] [review]
updated patch [checked in]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/b54001303198

Comment 37

[Stefan [:stefanh]]

Comment on attachment 332408 [details] [diff] [review]
part 2 (toolkit / browser) v0.4: Keep the binding [checked in]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/a2709097171f

Comment 38

[Stefan [:stefanh]]

Resolving as fixed, since both attachments are checked in.

Comment 39

[Stefan [:stefanh]]

I backed this out due to a (about) 13% twinopen perf hit on the Leopard talos boxen.

Comment 40

[Markus Stange [:mstange]]

Over the last days I did some profiling. This is what I found out:
 - Gecko's background image tiling seems to be extremely fast.
 - Redrawing the titlebar has no noticable cost.
 - The main performance hits are:
    (1) the generic selector in toolbar.css and
    (2) drawing the toolbar gradient.

I have patches for both (1) and (2), but they won't get rid of the performance hit completely. However, I've had ideas for other possible twinopen optimizations and filed bug 453540 for them.

Comment 41

[Markus Stange [:mstange]]

Attachment: toolkit speedup: don't use the generic selector

Comment 42

[Markus Stange [:mstange]]

Attachment: widget speedup: cache the unified toolbar

Comment 43

[Håkan Waara]

I can't review it at this very moment, but a general question: do you have any data of how much / whether this helps performance? 

I *think* the try server can perform perf tests these days.

Comment 44

[Justin Dolske [:Dolske]]

Yes, the tryserver does perf tests. The results can be a bit noisy, though.

Comment 45

[Justin Dolske [:Dolske]]

(There's also https://wiki.mozilla.org/StandaloneTalos)

Comment 46

[Markus Stange [:mstange]]

Thanks for insisting on the data, Håkan. I had fooled myself into believing the results of the StandaloneTalos runs that I did over the past days, and it looks like I was on the wrong track.

Today I did another StandaloneTalos session: I prepared 5 builds with the different patches applied, restarted my machine for every build, and ran every build 20 x 20 cycles.
The results are extremely noisy, but two things are striking: 
1. My "optimization" patches don't help at all. :(
   (previously it looked like they did)
2. The biggest regression follows the widget-only patch (attachment 335619 [details] [diff] [review])
   - so in a build where the new -moz-appearance isn't even in use.

That makes me think that I should have a serious look at the IsPaintingSuppressed function.

(In reply to comment #44)
> Yes, the tryserver does perf tests. The results can be a bit noisy, though.

Unfortunately it looks like it doesn't run Twinopen / Txul, which is what's affected here.

Comment 47

[Markus Stange [:mstange]]

Attachment: make isPaintingSuppressed faster [checked in]

Aha! This seems to be it.

In my StandaloneTalos run the unpatched twinopen average was 150ms. With attachment 335619 [details] [diff] [review] applied I got 160ms.
This patch brings it back down to 150ms.

Comment 48

[Håkan Waara]

Comment on attachment 337140 [details] [diff] [review]
make isPaintingSuppressed faster [checked in]

This patch was a lot simpler than the unified toolbar caching. :-)

Comment 49

[Markus Stange [:mstange]]

Comment on attachment 335619 [details] [diff] [review]
updated patch [checked in]

pushed: http://hg.mozilla.org/mozilla-central/rev/519540dd7880

I'm now waiting for the Talos numbers before I push the other patches.

Comment 50

[Markus Stange [:mstange]]

Comment on attachment 337140 [details] [diff] [review]
make isPaintingSuppressed faster [checked in]

pushed: http://hg.mozilla.org/mozilla-central/rev/e4b5cf8d76e6

Comment 51

[Markus Stange [:mstange]]

Comment on attachment 332408 [details] [diff] [review]
part 2 (toolkit / browser) v0.4: Keep the binding [checked in]

pushed: http://hg.mozilla.org/mozilla-central/rev/1a50eaa93b36

Comment 52

[Markus Stange [:mstange]]

Quick summary of the perf numbers:
 - The widget patches didn't change anything.
 - After the toolkit / browser patch was checked in, the numbers went up again,
   4% on 10.4 and 10% on 10.5.

I pushed http://hg.mozilla.org/mozilla-central/rev/ff469575e3ee to bring back the background styles of #nav-bar in browser.css. This change obviously doesn't make any sense: background styles shouldn't matter when -moz-appearance is set. However, in my local StandaloneTalos run, this change was exactly what cured the perf regression.
If the Talos machines feel the same way about that change, we'll have to look for a bug.

Comment 53

[Markus Stange [:mstange]]

Victory!

It looks like the 10.5 machines were very happy about that change:
http://graphs.mozilla.org/graph.html#show=794384,794371,794398,1431186&sel=1221514542,1221578618

It looks good on 10.4, too, although not as good as on 10.5:
http://graphs.mozilla.org/graph.html#show=394866,394876,394892,911651&sel=1221372392,1221578682

So at the moment it looks like the regression is down to 0% on 10.5 and 2% on 10.4. Therefore I'm declaring this bug as fixed.

Comment 54

[Markus Stange [:mstange]]

I filed bug 455539 for the perf oddness.