Closed Bug 439354 Opened 13 years ago Closed 13 years ago

OS X toolbar background doesn't have a good gradient

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: hwaara, Assigned: mstange)

References

()

Details

Attachments

(8 files, 9 obsolete files)

14.25 KB, image/png
Details
10.76 KB, image/png
Details
154.59 KB, image/png
Details
9.44 KB, patch
Details | Diff | Splinter Review
34.91 KB, patch
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
7.74 KB, patch
Details | Diff | Splinter Review
1.85 KB, patch
hwaara
: review+
Details | Diff | Splinter Review
Compare the two attachments.
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.
Assignee: nobody → joshmoz
Component: Mail Window Front End → Widget: Cocoa
Product: Thunderbird → Core
QA Contact: front-end → cocoa
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?
Summary: Toolbar background doesn't look right → OS X toolbar background doesn't have a good gradient
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?
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.
Assignee: joshmoz → mstange
I'm working on a cool patch that doesn't need additional attributes.
Status: NEW → ASSIGNED
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).
Attachment #331121 - Flags: review?(joshmoz)
I think some things are getting simpler with this patch.
Attachment #331124 - Flags: review?(mconnor)
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.
Attachment #331121 - Attachment is obsolete: true
Attachment #331300 - Flags: review?(joshmoz)
Attachment #331121 - Flags: review?(joshmoz)
I missed browser.css's #nav-bar styling in the last patch.
Attachment #331124 - Attachment is obsolete: true
Attachment #331301 - Flags: review?(mconnor)
Attachment #331124 - Flags: review?(mconnor)
Flags: wanted1.9.1?
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.
Attachment #331300 - Attachment is obsolete: true
Attachment #332238 - Flags: superreview?(roc)
Attachment #332238 - Flags: review?(hwaara)
Attachment #331300 - Flags: review?(joshmoz)
Attachment #331301 - Attachment is obsolete: true
Attachment #332240 - Flags: review?
Attachment #331301 - Flags: review?(mconnor)
Attachment #332240 - Flags: review? → review?(mconnor)
Attachment #332240 - Flags: review?(mconnor) → review+
(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?
(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.
(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?
(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...
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).
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?
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.
That could have all kinds of unexpected side effects, I'm reluctant to go there.
(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.
Attachment #332238 - Attachment is obsolete: true
Attachment #332342 - Flags: superreview?(roc)
Attachment #332342 - Flags: review?(hwaara)
Attachment #332238 - Flags: superreview?(roc)
Attachment #332238 - Flags: review?(hwaara)
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? :-)
(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. :-(
Attachment #332342 - Attachment is obsolete: true
Attachment #332386 - Flags: superreview?(roc)
Attachment #332386 - Flags: review?(hwaara)
Attachment #332342 - Flags: superreview?(roc)
Attachment #332342 - Flags: review?(hwaara)
Attachment #332386 - Flags: review?(hwaara) → review+
(In reply to comment #23)
> To me this doesn't look like Cocoa forgets about the dirty rects.

Great. Thanks!
Attachment #332386 - Flags: superreview?(roc) → superreview+
Dave Townsend prefers keeping the binding in order not to break extensions. The rest of the patch hasn't changed.
Attachment #332240 - Attachment is obsolete: true
Attachment #332408 - Flags: review?(mconnor)
Blocks: 449500
Depends on: 449832
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.
Attachment #332386 - Attachment is obsolete: true
Attachment #333267 - Flags: superreview?(roc)
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.
Attachment #333267 - Flags: superreview?(roc) → superreview+
Blocks: 450261
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])? 
I think it's ready to land. :)
I have to take that back; since bug 432131 was backed out, landing this would lead to ugly repainting problems on Tiger.
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.
Attachment #332408 - Flags: review?(mconnor)
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
This should apply.
Attachment #333267 - Attachment is obsolete: true
Comment on attachment 335619 [details] [diff] [review]
updated patch [checked in]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/b54001303198
Attachment #335619 - Attachment description: updated patch → updated patch (checked in)
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
Attachment #332408 - Attachment description: part 2 (toolkit / browser) v0.4: Keep the binding → part 2 (toolkit / browser) v0.4: Keep the binding (checked in)
Resolving as fixed, since both attachments are checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Flags: wanted1.9.1?
I backed this out due to a (about) 13% twinopen perf hit on the Leopard talos boxen.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #332408 - Attachment description: part 2 (toolkit / browser) v0.4: Keep the binding (checked in) → part 2 (toolkit / browser) v0.4: Keep the binding
Attachment #335619 - Attachment description: updated patch (checked in) → updated patch
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.
Attachment #336859 - Flags: superreview?(roc)
Attachment #336859 - Flags: review?(hwaara)
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.
Yes, the tryserver does perf tests. The results can be a bit noisy, though.
Attachment #336858 - Flags: review?(mconnor)
Attachment #336859 - Flags: superreview?(roc)
Attachment #336859 - Flags: review?(hwaara)
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.
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.
Attachment #337140 - Flags: superreview?(roc)
Attachment #337140 - Flags: review?(hwaara)
Comment on attachment 337140 [details] [diff] [review]
make isPaintingSuppressed faster [checked in]

This patch was a lot simpler than the unified toolbar caching. :-)
Attachment #337140 - Flags: review?(hwaara) → review+
Attachment #337140 - Flags: superreview?(roc) → superreview+
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.
Attachment #335619 - Attachment description: updated patch → updated patch [checked in]
Comment on attachment 337140 [details] [diff] [review]
make isPaintingSuppressed faster [checked in]

pushed: http://hg.mozilla.org/mozilla-central/rev/e4b5cf8d76e6
Attachment #337140 - Attachment description: make isPaintingSuppressed faster → make isPaintingSuppressed faster [checked in]
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
Attachment #332408 - Attachment description: part 2 (toolkit / browser) v0.4: Keep the binding → part 2 (toolkit / browser) v0.4: Keep the binding [checked in]
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.
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
I filed bug 455539 for the perf oddness.
Depends on: 455539
Depends on: 455705
Depends on: 460899
Depends on: 507845
Depends on: 507945
You need to log in before you can comment on or make changes to this bug.