I think we can radically simplify the widget implementation of bug 865374: - Disabling window title drawing can be done by overriding _drawTitleBar: on the frame view. - Measuring the buttons doesn't need to happen inside the frame view class. I've moved the measuring code into a function in nsNativeThemeCocoa.mm. - Since overriding _drawTitleBar: can be done using swizzling, no dynamic subclass construction is necessary. - The fullscreen button measuring code was never used because it's called so early that the fullscreen button doesn't exist. In practice, only the default value was returned. - Small change: [self convertRect:[closeButton bounds] fromView:closeButton] is the same as [closeButton frame] because closeButton is a direct subview of self. The patch also uses NSMaxX(buttonBox) instead of buttonBox.size.width so that the left margin in front of the close button is included. Another thing that should be mentioned is the fact that returning a custom subclass from frameViewClassForStyleMask: changed the window's appearance: The highlight border and the window corner radius were suddenly using 10.4 style. Maybe this is caused by the frame view checking [self class] == NSThemeFrame during drawing (somewhere in drawing code we don't control)... in any case, this patch fixes the appearance to the normal state.

I just noticed that the argument to _drawTitleBar: should be a CGRect, not an NSRect. I'll fix that when checking in.

I'll get to this sometime next week. > The highlight border and the window corner radius were suddenly using 10.4 style. This is the most serious problem you found. Please give a detailed description of what you saw, and where you saw it (on which OS versions). Please also provide screenshots.

I just tried comparing today's UX nightly (which has the patch for bug 865374) with yesterday's (which doesn't). I tested on OS X 10.7.5. The only difference I can see is that the window corners are slightly more rounded.

Comment on attachment 744861 [details] [diff] [review] v1 I changed my mind about waiting until next week. Looking through your patch a few times, I decided it *is* better and simpler than mine, while preserving its full functionality (including the possibility of swizzling more NSFrameView (and subclasses') methods as we need them. > I just noticed that the argument to _drawTitleBar: should be a > CGRect, not an NSRect. Not so. Though it's only a matter of style. I assume you concluded this from looking at as class-dump of the AppKit framework. But these dumps never refer to "NSRect" -- only to "struct CGRect". And in any case NSRect and CGRect have exactly the same definition. As a matter of style, I think we should use NSRect in a purely AppKit context -- for example in the following: - (void)_drawTitleBar:(NSRect)aRect;

Comment on attachment 744861 [details] [diff] [review] v1 But wait a minute: I need to test exactly what _drawTitlebar: does, and what happens when we stop it being called. Please don't land this until I've commented again.

Comment on attachment 744861 [details] [diff] [review] v1 OK, I've now used an interpose library to test knocking out calls to -[NSGrayFrame _drawTitleBar:] and -[NSThemeFrame _drawTitleBar:] on all our supported platforms (back to SnowLeopard), in Firefox with different settings (like with and without hardware acceleration), and even (for good measure) in other browsers. In all cases it just stops the title cell from drawing -- which is what we want. And disassembling these methods on Lion, I find that both of them just call -[NSTitledFrame _drawTitleStringIn:withColor:]. So go ahead and land your patch, Markus. Of course you'll first need to update it to current UX branch.

(In reply to Steven Michaud from comment #4) > > I just noticed that the argument to _drawTitleBar: should be a > > CGRect, not an NSRect. > > Not so. Though it's only a matter of style. > > I assume you concluded this from looking at as class-dump of the > AppKit framework. But these dumps never refer to "NSRect" -- only to > "struct CGRect". I see. I agree that we should use NSRect then. (In reply to Steven Michaud from comment #6) > OK, I've now used an interpose library to test knocking out calls to > -[NSGrayFrame _drawTitleBar:] and -[NSThemeFrame _drawTitleBar:] on all our > supported platforms (back to SnowLeopard), in Firefox with different > settings (like with and without hardware acceleration), and even (for good > measure) in other browsers. Wow, thanks for that! > In all cases it just stops the title cell from drawing -- which is what we > want. > > And disassembling these methods on Lion, I find that both of them just call > -[NSTitledFrame _drawTitleStringIn:withColor:]. Good, that's what I had hoped (but not verified).

This is what I see on 10.8.3 in HiDPI mode. The differences are: corner radius, window button position, highlight line width/colors, HiDPI compatibility. "screenshot with bug" was with dynamic subclass creation in frameViewClassForStyleMask:, "screenshot without bug" without.

The window shadow is different, too.

Landed on UX: https://hg.mozilla.org/projects/ux/rev/355c24244f51

Landed on Jamun as https://hg.mozilla.org/projects/jamun/rev/355c24244f51