[MacViews] Frameless app windows: make content view cover title bar.

[MacViews] Frameless app windows: make content view cover title bar.

AppKit resizes content views when the window resizes by calling
[NSView setFrameSize:], but does not take into account
[NSWindow contentRectForFrameRect:]. We intercept the former and call the
latter. This is similar to the method used by FullSizeContentView.

This CL also adds NativeWidgetMacFramelessNSWindow which overrides
contentRect <-> frameRect conversion.

The frameless NSWindow is used in app windows by overriding
NativeWidgetMac. This is similar to AppWindowDesktopNativeWidgetAuraWin
which provides a specialized WindowTreeHost. BrowserFrameMac (another
NativeWidgetMac subclass) will do the same to draw custom themed frames.

BUG=459877
Committed: https://crrev.com/9b683fa2526a4f26f27f4aa136b92748323aa5b4
Cr-Commit-Position: refs/heads/master@{#321251}

[jackhou1, 2015-03-10 16:42:15]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-03-13 23:32:55]

tapted, PTAL

This is isolated to app windows now. For browser windows, BrowserFrameMac can
also override CreateNSWindow.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:252: // When this view is added to a
window, AppKit calls setFrameSize before it is
Overriding viewDidMoveToWindow is one option. I like it because it puts the code
right next to setFrameSize below. We could instead override setContentView in
NativeWidgetMacFramelessNSWindow, or call setFrameSize at the end of
BridgedNativeWidget::SetRootView(). WDYT?

[tapted, 2015-03-18 00:06:39]

I like it! lots of nits mainly.

Can we also add to the CL description justifying the approach of overriding
views::NativeWidgetMac? That was really the only thing I was uneasy about when I
first saw it, but there's good precedent for doing that for browser frames
already.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:226:
[ns_window setFrame:NSMakeRect(50, 50, 200, 200) display:YES];
can you add a check that ensures this wasn't simply a no-op?

I think we can safely rely on the size being non-flaky. So, e.g., check the
frame size equals the expected default size for the app window api before, and
check after against the 200x200 constant

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:229: }
I think there are some other things this CL fixes which are easy to check here.
E.g.

// Windows created with NSBorderlessWindowMask by default don't have shadow, but
// packaged apps should always have one.
EXPECT_TRUE([ns_window hasShadow]);


(anything else?)

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.h (right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:10: class
AppWindowNativeWidgetMac : public views::NativeWidgetMac {
needs a class comment

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:12:
AppWindowNativeWidgetMac(views::Widget* widget);
explicit

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:17: NSWindow*
CreateNSWindow(views::Widget::InitParams params) override;
pass |params| by const reference? Also the return type should be
gfx::NativeWindow since this (and native_widget_mac.h) should look like C++-only
headers (i.e. no obj-C allowed)

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:21: if
(!params.remove_standard_frame)
comment about this? (i.e. what native app window window types will have this
false? - DCHECK?)

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:24: NSUInteger
style_mask = NSTexturedBackgroundWindowMask | NSTitledWindowMask |
comment why NSTexturedBackgroundWindowMask is needed (i.e. just to allow moving
the window by clicking anywhere on it, when that's enabled?)

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:31: defer:YES];
Returns from functions are typically autoreleased -- I think that's a better
approach.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm (right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:8: #import
"chrome/browser/ui/views/apps/app_window_native_widget_mac.h"
nit: import->include

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:21:
init_params->native_widget = new AppWindowNativeWidgetMac(widget);
DCHECK(!init_params->native_widget);

i.e. ensure ChromeViewsDelegate hasn't already put an object on this that will
leak.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:252: // When this view is added to a
window, AppKit calls setFrameSize before it is
On 2015/03/13 23:32:55, jackhou1 wrote:
> Overriding viewDidMoveToWindow is one option. I like it because it puts the
code
> right next to setFrameSize below. We could instead override setContentView in
> NativeWidgetMacFramelessNSWindow, or call setFrameSize at the end of
> BridgedNativeWidget::SetRootView(). WDYT?

Yeah I like this.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:261: // -[NSWindow
contentRectForFrameRect].
This needs a comment like

The following ensures that the contentView for a frameless window can extend
over the titlebar of the new window containing it, since AppKit requires a
titlebar to give frameless windows correct shadows and rounded corners.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.h:19: // each other.
add (e.g.) ", even for windows that do not use NSBorderlessWindowMask. This
allows an NSWindow
to be frameless without attaining undesired side-effects of
NSBorderlessWindowMask.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.h:21: @interface
NativeWidgetMacFramelessNSWindow : NativeWidgetMacNSWindow
This is only used in chrome/browser/ui/views/apps/ so it should be in a separate
file there.

(If we also use it for browser windows in future we can move it to, e.g.,
chrome/browser/ui/views/mac or something)

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.h (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.h:122: // Called by InitNativeWidget.
Add comment that the return value will be autoreleased.

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.h:123: virtual NSWindow*
CreateNSWindow(Widget::InitParams params);
return gfx::NativeWindow, take const-reference

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:88: base::scoped_nsobject<NSWindow>
window(CreateNSWindow(params));
once autoreleased, add retain

[jackhou1, 2015-03-18 03:44:40]

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:226:
[ns_window setFrame:NSMakeRect(50, 50, 200, 200) display:YES];
On 2015/03/18 00:06:38, tapted wrote:
> can you add a check that ensures this wasn't simply a no-op?
> 
> I think we can safely rely on the size being non-flaky. So, e.g., check the
> frame size equals the expected default size for the app window api before, and
> check after against the 200x200 constant

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:229: }
On 2015/03/18 00:06:38, tapted wrote:
> I think there are some other things this CL fixes which are easy to check
here.
> E.g.
> 
> // Windows created with NSBorderlessWindowMask by default don't have shadow,
but
> // packaged apps should always have one.
> EXPECT_TRUE([ns_window hasShadow]);
> 
> 
> (anything else?)

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.h (right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:10: class
AppWindowNativeWidgetMac : public views::NativeWidgetMac {
On 2015/03/18 00:06:38, tapted wrote:
> needs a class comment

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:12:
AppWindowNativeWidgetMac(views::Widget* widget);
On 2015/03/18 00:06:38, tapted wrote:
> explicit

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.h:17: NSWindow*
CreateNSWindow(views::Widget::InitParams params) override;
On 2015/03/18 00:06:38, tapted wrote:
> pass |params| by const reference? Also the return type should be
> gfx::NativeWindow since this (and native_widget_mac.h) should look like
C++-only
> headers (i.e. no obj-C allowed)

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:21: if
(!params.remove_standard_frame)
On 2015/03/18 00:06:39, tapted wrote:
> comment about this? (i.e. what native app window window types will have this
> false? - DCHECK?)

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:24: NSUInteger
style_mask = NSTexturedBackgroundWindowMask | NSTitledWindowMask |
On 2015/03/18 00:06:38, tapted wrote:
> comment why NSTexturedBackgroundWindowMask is needed (i.e. just to allow
moving
> the window by clicking anywhere on it, when that's enabled?)

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:31: defer:YES];
On 2015/03/18 00:06:39, tapted wrote:
> Returns from functions are typically autoreleased -- I think that's a better
> approach.

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm (right):

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:8: #import
"chrome/browser/ui/views/apps/app_window_native_widget_mac.h"
On 2015/03/18 00:06:39, tapted wrote:
> nit: import->include

Done.

https://codereview.chromium.org/987323002/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:21:
init_params->native_widget = new AppWindowNativeWidgetMac(widget);
On 2015/03/18 00:06:39, tapted wrote:
> DCHECK(!init_params->native_widget);
> 
> i.e. ensure ChromeViewsDelegate hasn't already put an object on this that will
> leak.

Done.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.mm (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.mm:261: // -[NSWindow
contentRectForFrameRect].
On 2015/03/18 00:06:39, tapted wrote:
> This needs a comment like
> 
> The following ensures that the contentView for a frameless window can extend
> over the titlebar of the new window containing it, since AppKit requires a
> titlebar to give frameless windows correct shadows and rounded corners.

Done.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/native_w...
File ui/views/cocoa/native_widget_mac_nswindow.h (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.h:19: // each other.
On 2015/03/18 00:06:39, tapted wrote:
> add (e.g.) ", even for windows that do not use NSBorderlessWindowMask. This
> allows an NSWindow
> to be frameless without attaining undesired side-effects of
> NSBorderlessWindowMask.

Done.

https://codereview.chromium.org/987323002/diff/140001/ui/views/cocoa/native_w...
ui/views/cocoa/native_widget_mac_nswindow.h:21: @interface
NativeWidgetMacFramelessNSWindow : NativeWidgetMacNSWindow
On 2015/03/18 00:06:39, tapted wrote:
> This is only used in chrome/browser/ui/views/apps/ so it should be in a
separate
> file there.
> 
> (If we also use it for browser windows in future we can move it to, e.g.,
> chrome/browser/ui/views/mac or something)

Done.

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.h (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.h:122: // Called by InitNativeWidget.
On 2015/03/18 00:06:39, tapted wrote:
> Add comment that the return value will be autoreleased.

Done.

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.h:123: virtual NSWindow*
CreateNSWindow(Widget::InitParams params);
On 2015/03/18 00:06:39, tapted wrote:
> return gfx::NativeWindow, take const-reference

Done.

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/987323002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:88: base::scoped_nsobject<NSWindow>
window(CreateNSWindow(params));
On 2015/03/18 00:06:39, tapted wrote:
> once autoreleased, add retain

Done.

[tapted, 2015-03-18 03:51:59]

lgtm with the following

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h
(right):

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h:8: #import
<Cocoa/Cocoa.h>
I guess this can be removed too

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h:11: #include
"ui/views/views_export.h"
remove

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h:17:
VIEWS_EXPORT
remove

https://codereview.chromium.org/987323002/diff/180001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/987323002/diff/180001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:527: defer:YES];
autorelease

[tapted, 2015-03-18 03:52:47]

+ mac-views-reviews :)

[jackhou1, 2015-03-18 06:14:17]

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h
(right):

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h:8: #import
<Cocoa/Cocoa.h>
On 2015/03/18 03:51:58, tapted wrote:
> I guess this can be removed too

Done.

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h:11: #include
"ui/views/views_export.h"
On 2015/03/18 03:51:58, tapted wrote:
> remove

Done.

https://codereview.chromium.org/987323002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_widget_mac_frameless_nswindow.h:17:
VIEWS_EXPORT
On 2015/03/18 03:51:58, tapted wrote:
> remove

Done.

https://codereview.chromium.org/987323002/diff/180001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/987323002/diff/180001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:527: defer:YES];
On 2015/03/18 03:51:58, tapted wrote:
> autorelease

Done.

[Andre, 2015-03-18 20:57:24]

andresantoso@chromium.org changed reviewers:
+ andresantoso@chromium.org

[Andre, 2015-03-18 20:57:25]

I want to use NativeWidgetMacFramelessNSWindow for the browser window
as well. Can we move it out of apps/ subdirectory?

[jackhou1, 2015-03-18 21:39:51]

On 2015/03/18 20:57:25, Andre wrote:
> I want to use NativeWidgetMacFramelessNSWindow for the browser window
> as well. Can we move it out of apps/ subdirectory?

Yeah, I mentioned that to Trent. He said he'd prefer to put it in apps/ for now
and move it when it's being used by BrowserFrameMac.

[Andre, 2015-03-18 23:18:12]

On 2015/03/18 21:39:51, jackhou1 wrote:
> On 2015/03/18 20:57:25, Andre wrote:
> > I want to use NativeWidgetMacFramelessNSWindow for the browser window
> > as well. Can we move it out of apps/ subdirectory?
> 
> Yeah, I mentioned that to Trent. He said he'd prefer to put it in apps/ for
now
> and move it when it's being used by BrowserFrameMac.

I was hoping to use it soon though, see http://crrev.com/1015103003.

[jackhou1, 2015-03-18 23:47:26]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-03-18 23:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/987323002/#ps200001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-03-18 23:49:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987323002/200001

[tapted, 2015-03-19 00:28:20]

On 19 Mar 2015 10:18 am, <andresantoso@chromium.org> wrote:
>
> On 2015/03/18 21:39:51, jackhou1 wrote:
>>
>> On 2015/03/18 20:57:25, Andre wrote:
>> > I want to use NativeWidgetMacFramelessNSWindow for the browser window
>> > as well. Can we move it out of apps/ subdirectory?
>
>
>> Yeah, I mentioned that to Trent. He said he'd prefer to put it in apps/
for
>
> now
>>
>> and move it when it's being used by BrowserFrameMac.
>
>
> I was hoping to use it soon though, see http://crrev.com/1015103003.
>
> https://codereview.chromium.org/987323002/

There's no problem moving it later. It's just unusual to plan for that in
an orthogonal CL. Plans can often change in review

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-03-19 00:32:00]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-03-19 00:33:33]

Patchset 11 (id:??) landed as
https://crrev.com/9b683fa2526a4f26f27f4aa136b92748323aa5b4
Cr-Commit-Position: refs/heads/master@{#321251}

[Andre, 2015-03-19 01:45:17]

On 2015/03/19 00:28:20, tapted wrote:
> On 19 Mar 2015 10:18 am, <mailto:andresantoso@chromium.org> wrote:
> >
> > On 2015/03/18 21:39:51, jackhou1 wrote:
> >>
> >> On 2015/03/18 20:57:25, Andre wrote:
> >> > I want to use NativeWidgetMacFramelessNSWindow for the browser window
> >> > as well. Can we move it out of apps/ subdirectory?
> >
> >
> >> Yeah, I mentioned that to Trent. He said he'd prefer to put it in apps/
> for
> >
> > now
> >>
> >> and move it when it's being used by BrowserFrameMac.
> >
> >
> > I was hoping to use it soon though, see http://crrev.com/1015103003.
> >
> > https://codereview.chromium.org/987323002/
> 
> There's no problem moving it later. It's just unusual to plan for that in
> an orthogonal CL. Plans can often change in review
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

SGTM.