OS X toolbar background doesn't have a good gradient

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Håkan Waara, Assigned: mstange)

Tracking

Trunk
mozilla1.9.1b1
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(8 attachments, 9 obsolete attachments)

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
Håkan Waara
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
Created attachment 325196 [details]
Thunderbird toolbar background [wrong]

Compare the two attachments.
(Reporter)

Comment 1

9 years ago
Created attachment 325197 [details]
Any other app (e.g. Mail) [right]
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
(Reporter)

Comment 3

9 years ago
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?
(Assignee)

Comment 5

9 years ago
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)

Updated

9 years ago
Assignee: joshmoz → mstange
(Assignee)

Comment 6

9 years ago
I'm working on a cool patch that doesn't need additional attributes.
Status: NEW → ASSIGNED
(Assignee)

Comment 7

9 years ago
Created attachment 330971 [details]
Screenshot of Addons window with upcoming patch
(Assignee)

Comment 8

9 years ago
Created attachment 331121 [details] [diff] [review]
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).
Attachment #331121 - Flags: review?(joshmoz)
(Assignee)

Comment 9

9 years ago
Created attachment 331124 [details] [diff] [review]
part 2 (toolkit): Use the new -moz-appearance and remove obsolete styles

I think some things are getting simpler with this patch.
Attachment #331124 - Flags: review?(mconnor)
(Assignee)

Comment 10

9 years ago
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.
(Assignee)

Comment 11

9 years ago
Created attachment 331300 [details] [diff] [review]
part 1 (widget) v0.2: remove something unnecessary
Attachment #331121 - Attachment is obsolete: true
Attachment #331300 - Flags: review?(joshmoz)
Attachment #331121 - Flags: review?(joshmoz)
(Assignee)

Comment 12

9 years ago
Created attachment 331301 [details] [diff] [review]
part 2 (toolkit) v0.2: remove even more

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)

Updated

9 years ago
Flags: wanted1.9.1?
(Assignee)

Comment 13

9 years ago
Created attachment 332238 [details] [diff] [review]
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.
Attachment #331300 - Attachment is obsolete: true
Attachment #332238 - Flags: superreview?(roc)
Attachment #332238 - Flags: review?(hwaara)
Attachment #331300 - Flags: review?(joshmoz)
(Assignee)

Comment 14

9 years ago
Created attachment 332240 [details] [diff] [review]
part 2 (toolkit / browser) v0.3: use -moz-mac-unified-toolbar
Attachment #331301 - Attachment is obsolete: true
Attachment #332240 - Flags: review?
Attachment #331301 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Attachment #332240 - Flags: review? → review?(mconnor)

Updated

9 years ago
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?
(Assignee)

Comment 16

9 years ago
(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?
(Assignee)

Comment 18

9 years ago
(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

9 years ago
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?
(Reporter)

Comment 21

9 years ago
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.
(Assignee)

Comment 23

9 years ago
Created attachment 332342 [details] [diff] [review]
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.
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)
(Reporter)

Comment 24

9 years ago
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? :-)
(Assignee)

Comment 25

9 years ago
Created attachment 332386 [details] [diff] [review]
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. :-(
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)
(Reporter)

Updated

9 years ago
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+
(Assignee)

Comment 27

9 years ago
Created attachment 332408 [details] [diff] [review]
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.
Attachment #332240 - Attachment is obsolete: true
Attachment #332408 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Blocks: 449500
(Assignee)

Updated

9 years ago
Depends on: 449832
(Assignee)

Comment 28

9 years ago
Created attachment 333267 [details] [diff] [review]
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.
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+

Updated

9 years ago
Blocks: 450261

Comment 30

9 years ago
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])? 
(Assignee)

Comment 31

9 years ago
I think it's ready to land. :)
(Assignee)

Comment 32

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

Comment 33

9 years ago
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 34

9 years ago
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
(Assignee)

Comment 35

9 years ago
Created attachment 335619 [details] [diff] [review]
updated patch [checked in]

This should apply.
Attachment #333267 - Attachment is obsolete: true

Comment 36

9 years ago
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 37

9 years ago
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)

Comment 38

9 years ago
Resolving as fixed, since both attachments are checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1

Updated

9 years ago
Flags: wanted1.9.1?

Comment 39

9 years ago
I backed this out due to a (about) 13% twinopen perf hit on the Leopard talos boxen.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

9 years ago
Attachment #332408 - Attachment description: part 2 (toolkit / browser) v0.4: Keep the binding (checked in) → part 2 (toolkit / browser) v0.4: Keep the binding

Updated

9 years ago
Attachment #335619 - Attachment description: updated patch (checked in) → updated patch
(Assignee)

Comment 40

9 years ago
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.
(Assignee)

Comment 41

9 years ago
Created attachment 336858 [details] [diff] [review]
toolkit speedup: don't use the generic selector
Attachment #336858 - Flags: review?(mconnor)
(Assignee)

Comment 42

9 years ago
Created attachment 336859 [details] [diff] [review]
widget speedup: cache the unified toolbar
Attachment #336859 - Flags: superreview?(roc)
Attachment #336859 - Flags: review?(hwaara)
(Reporter)

Comment 43

9 years ago
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.
(There's also https://wiki.mozilla.org/StandaloneTalos)
(Assignee)

Updated

9 years ago
Attachment #336858 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Attachment #336859 - Flags: superreview?(roc)
Attachment #336859 - Flags: review?(hwaara)
(Assignee)

Comment 46

9 years ago
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.
(Assignee)

Comment 47

9 years ago
Created attachment 337140 [details] [diff] [review]
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.
Attachment #337140 - Flags: superreview?(roc)
Attachment #337140 - Flags: review?(hwaara)
(Reporter)

Comment 48

9 years ago
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+
(Assignee)

Comment 49

9 years ago
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]
(Assignee)

Comment 50

9 years ago
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]
(Assignee)

Comment 51

9 years ago
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]
(Assignee)

Comment 52

9 years ago
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.
(Assignee)

Comment 53

9 years ago
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
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 54

9 years ago
I filed bug 455539 for the perf oddness.
Depends on: 455539
Depends on: 455705

Updated

9 years ago
Depends on: 460899
(Assignee)

Updated

8 years ago
Depends on: 507845
(Assignee)

Updated

8 years ago
Depends on: 507945
You need to log in before you can comment on or make changes to this bug.