Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
916833005:
[Mac] Use a custom view to implement colored app windows. (Closed)
Description[Mac] Use a custom view to implement colored app windows.
When building with >=10.9 SDK, swizzling drawRect on the window no
longer works. See https://codereview.chromium.org/611453004
This changes NativeAppWindowCocoa to put an extra view in the title bar
that draws the color.
BUG=457991
Committed: https://crrev.com/adebcb60c6ae805b8e35640b06fa57ed37480da6
Cr-Commit-Position: refs/heads/master@{#317534}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address comments #Patch Set 3 : Sync and rebase #Patch Set 4 : Refactor to be simpler. #Patch Set 5 : Sync and rebase #
Total comments: 12
Patch Set 6 : Fix a couple of nits. #
Total comments: 2
Patch Set 7 : Address comments #
Messages
Total messages: 15 (3 generated)
jackhou@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:130: @interface NativeAppWindowController (Private) (Private) -> () https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:148: - (void)windowDidBecomeMain:(NSNotification*)notification { the FooMain methods should be able to be removed -- we only care about key status (app windows can't have non-main child windows, which is the only time we'd have to worry about this) https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:292: NSView* windowView = [[self contentView] superview]; Mavericks has (officially) banned the practice of manipulating the window's contentview superview. https://developer.apple.com/library/mac/releasenotes/AppKit/RN-AppKit/#10_10W... Should we try using NSTitlebarAccessoryViewController?
https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:130: @interface NativeAppWindowController (Private) On 2015/02/12 05:56:04, tapted wrote: > (Private) -> () Done. https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:148: - (void)windowDidBecomeMain:(NSNotification*)notification { On 2015/02/12 05:56:04, tapted wrote: > the FooMain methods should be able to be removed -- we only care about key > status (app windows can't have non-main child windows, which is the only time > we'd have to worry about this) Done. https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:292: NSView* windowView = [[self contentView] superview]; On 2015/02/12 05:56:04, tapted wrote: > Mavericks has (officially) banned the practice of manipulating the window's > contentview superview. > > https://developer.apple.com/library/mac/releasenotes/AppKit/RN-AppKit/#10_10W... > > Should we try using NSTitlebarAccessoryViewController? Yes, and looking around forums it seems like adding to contentview.superview may cause rendering bugs in 10.10. That API is only available on 10.10 though, so we'd still need the current method for <=10.9.
https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:292: NSView* windowView = [[self contentView] superview]; On 2015/02/13 00:04:33, jackhou1 wrote: > On 2015/02/12 05:56:04, tapted wrote: > > Mavericks has (officially) banned the practice of manipulating the window's > > contentview superview. > > > > > https://developer.apple.com/library/mac/releasenotes/AppKit/RN-AppKit/#10_10W... > > > > Should we try using NSTitlebarAccessoryViewController? > > Yes, and looking around forums it seems like adding to contentview.superview may > cause rendering bugs in 10.10. > That API is only available on 10.10 though, so we'd still need the current > method for <=10.9. Tested this out on 10.10 linking with both 10.6 and 10.9 SDKs, both work. Behavior is the same as it is currently. I think we should commit this CL first. I looked into NSTitlebarAccessoryViewController. It doesn't do quite what we want, and there might need to be more hackery to get the behavior right. I'll upload another CL to show what it looks like.
On 2015/02/18 04:35:09, jackhou1 wrote: > https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... > File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): > > https://codereview.chromium.org/916833005/diff/1/chrome/browser/ui/cocoa/apps... > chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:292: NSView* windowView > = [[self contentView] superview]; > On 2015/02/13 00:04:33, jackhou1 wrote: > > On 2015/02/12 05:56:04, tapted wrote: > > > Mavericks has (officially) banned the practice of manipulating the window's > > > contentview superview. > > > > > > > > > https://developer.apple.com/library/mac/releasenotes/AppKit/RN-AppKit/#10_10W... > > > > > > Should we try using NSTitlebarAccessoryViewController? > > > > Yes, and looking around forums it seems like adding to contentview.superview > may > > cause rendering bugs in 10.10. > > That API is only available on 10.10 though, so we'd still need the current > > method for <=10.9. > > Tested this out on 10.10 linking with both 10.6 and 10.9 SDKs, both work. > Behavior is the same as it is currently. I think we should commit this CL first. > > I looked into NSTitlebarAccessoryViewController. It doesn't do quite what we > want, and there might need to be more hackery to get the behavior right. I'll > upload another CL to show what it looks like. Other CL here: https://codereview.chromium.org/928303003/
lgtm with some nits I'm still a little nervous, but I guess it's worth a try. I'm curious what's actually happening in the fullscreen case, (but I guess it's drawn, and then just the webcontents painted over it?) https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h (right): https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:47: @interface TitlebarBackgroundView : NSView { forward declare this? i.e. just `@class TitlebarBackgroundView;` https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:53: inactiveColor:(NSColor*)inactiveColor; nit: minimum 4 space indent (colons are allowed to misalign in this case) https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:233: inactiveColor:(NSColor*)inactiveColor { nit: 4 space indent https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:336: NSView* window_view = [[window contentView] superview]; nit: window_view -> frame_view ? https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:336: NSView* window_view = [[window contentView] superview]; can you add a comment here? Something to the effect of ~AppKit only officially supports adding subviews to the window's contentView and not its superview (an NSNextStepFrame). The 10.10 SDK offers ... but it doesn't support .... https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:338: initWithFrame:NSMakeRect(0, since it's autoresized, can you just pass 0 for the width? https://codereview.chromium.org/916833005/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:53: const CGFloat kTitlebarBackgroundViewPaintHeight = 60.0; nit: comment for this (i.e. why 60 -- does it just need to be bigger than titlebar height + radius)
New patchsets have been uploaded after l-g-t-m from tapted@chromium.org
https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h (right): https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:47: @interface TitlebarBackgroundView : NSView { On 2015/02/19 03:11:02, tapted wrote: > forward declare this? i.e. just `@class TitlebarBackgroundView;` Done. https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:53: inactiveColor:(NSColor*)inactiveColor; On 2015/02/19 03:11:02, tapted wrote: > nit: minimum 4 space indent (colons are allowed to misalign in this case) Done. https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:233: inactiveColor:(NSColor*)inactiveColor { On 2015/02/19 03:11:02, tapted wrote: > nit: 4 space indent Done. https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:336: NSView* window_view = [[window contentView] superview]; On 2015/02/19 03:11:02, tapted wrote: > nit: window_view -> frame_view ? Done. https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:336: NSView* window_view = [[window contentView] superview]; On 2015/02/19 03:11:02, tapted wrote: > can you add a comment here? Something to the effect of ~AppKit only officially > supports adding subviews to the window's contentView and not its superview (an > NSNextStepFrame). The 10.10 SDK offers ... but it doesn't support .... Done. https://codereview.chromium.org/916833005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:338: initWithFrame:NSMakeRect(0, On 2015/02/19 03:11:02, tapted wrote: > since it's autoresized, can you just pass 0 for the width? Doesn't seem to work. https://codereview.chromium.org/916833005/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right): https://codereview.chromium.org/916833005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:53: const CGFloat kTitlebarBackgroundViewPaintHeight = 60.0; On 2015/02/19 03:11:02, tapted wrote: > nit: comment for this (i.e. why 60 -- does it just need to be bigger than > titlebar height + radius) Done.
> I'm curious what's actually happening in the fullscreen case, (but I guess it's > drawn, and then just the webcontents painted over it?) Yeah, I would guess so too. drawRect seems to be getting called when going into fullscreen. I think something else, maybe the menu bar, draws the title bar that slides down from the top.
On 2015/02/19 04:34:57, jackhou1 wrote: > > I'm curious what's actually happening in the fullscreen case, (but I guess > it's > > drawn, and then just the webcontents painted over it?) > > Yeah, I would guess so too. drawRect seems to be getting called when going into > fullscreen. I think something else, maybe the menu bar, draws the title bar that > slides down from the top. thanks for checking! slgtm . I guess a transparent window might go funky, but that's denied for non-frameless apps, and not implemented on mac anyway.
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/916833005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/adebcb60c6ae805b8e35640b06fa57ed37480da6 Cr-Commit-Position: refs/heads/master@{#317534} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
