[MacViews] Implement size constraints for app windows.

[MacViews] Implement size constraints for app windows.

This works the same way as NativeAppWindowCocoa by setting
properties on the NSWindow in OnSizeConstraintsChanged.

This also moves some code in common with NativeAppWindowCocoa
out to c/b/ui/cocoa/apps/nswindow_util.h. More code will be moved
there as they're implemented on MacViews, e.g.
CalculateDraggableRegions.

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

[jackhou1, 2015-03-20 04:33:31]

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

[jackhou1, 2015-03-20 04:33:32]

tapted, could you take a look?

[tapted, 2015-03-20 12:12:53]

https://codereview.chromium.org/1023083002/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:26: gfx::Size
minimum_size = delegate()->GetMinimumSize();
I think to be consistent with aura widgets, this can all move into
BridgedNativeWidget::SizeConstraintsChanged()

(e.g. consider that when Mac goes to Aura,
DesktopNativeWidgetAura::OnSizeConstraintsChanged will call
desktop_window_tree_host_->SizeConstraintsChanged();. On Windows, that then
calls HWNDMessageHandler::SCC, on Mac it will probably call
BridgedNativeWidget::SCC

https://codereview.chromium.org/1023083002/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:46:
setEnabled:shows_fullscreen_controls];
I wonder if this is all complicated enough to try to share it with
native_app_window_cocoa...

e.g.

ui/gfx/mac/nswindow_util.h

ApplySizeConstraints(NSWindow*, gfx::Size min_size, gfx::Size max_size,
can_resize, can_fullscreen).

https://codereview.chromium.org/1023083002/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac.mm:531: void
NativeWidgetMac::OnSizeConstraintsChanged() {
Aura widgets also call this towards the end of InitNativeWidget() -- is that
needed?

[tapted, 2015-03-25 18:50:23]

looking good - I think you're still going (e.g. nswindow_util.h needs some
comments), but here are some nits :)

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_util.h
File ui/gfx/mac/nswindow_util.h (right):

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:8: #import <Cocoa/Cocoa.h>
nit: forward declare @class NSWindow;

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:10: #include "ui/gfx/geometry/size.h"
nit: forward declare

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:18: gfx::Size min_size,
nit: const-reference

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
File ui/gfx/mac/nswindow_util.mm (right):

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.mm:8: #include "base/mac/sdk_forward_declarations.h"
nit: include->import all (3) of these.

[jackhou1, 2015-03-25 23:32:24]

Yeah, I still need to sort out this bug:
https://code.google.com/p/chromium/issues/detail?id=362039

If it manifests in the views implementation too, I'll probably need to override
AppWindowNativeWidgetMac::OnSizeConstraintsChanged.

Will ping this CL.

https://codereview.chromium.org/1023083002/diff/1/chrome/browser/ui/views/app...
File chrome/browser/ui/views/apps/app_window_native_widget_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:26: gfx::Size
minimum_size = delegate()->GetMinimumSize();
On 2015/03/20 12:12:53, tapted (travelling) wrote:
> I think to be consistent with aura widgets, this can all move into
> BridgedNativeWidget::SizeConstraintsChanged()
> 
> (e.g. consider that when Mac goes to Aura,
> DesktopNativeWidgetAura::OnSizeConstraintsChanged will call
> desktop_window_tree_host_->SizeConstraintsChanged();. On Windows, that then
> calls HWNDMessageHandler::SCC, on Mac it will probably call
> BridgedNativeWidget::SCC

Done.

https://codereview.chromium.org/1023083002/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_window_native_widget_mac.mm:46:
setEnabled:shows_fullscreen_controls];
On 2015/03/20 12:12:53, tapted (travelling) wrote:
> I wonder if this is all complicated enough to try to share it with
> native_app_window_cocoa...
> 
> e.g.
> 
> ui/gfx/mac/nswindow_util.h
> 
> ApplySizeConstraints(NSWindow*, gfx::Size min_size, gfx::Size max_size,
> can_resize, can_fullscreen).

Done.

https://codereview.chromium.org/1023083002/diff/1/ui/views/widget/native_widg...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/1/ui/views/widget/native_widg...
ui/views/widget/native_widget_mac.mm:531: void
NativeWidgetMac::OnSizeConstraintsChanged() {
On 2015/03/20 12:12:53, tapted (travelling) wrote:
> Aura widgets also call this towards the end of InitNativeWidget() -- is that
> needed?

ChromeNativeAppWindowViews::InitializeDefaultWindow() calls
SetContentSizeConstraints(). But it might be needed for non-app-window widgets.
Added call in NativeWidgetMac::InitNativeWidget.

[jackhou1, 2015-03-26 03:50:46]

Looks like that bug doesn't happen on Views, updated the test.

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_util.h
File ui/gfx/mac/nswindow_util.h (right):

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:8: #import <Cocoa/Cocoa.h>
On 2015/03/25 18:50:23, tapted (travelling) wrote:
> nit: forward declare @class NSWindow;

Done.

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:10: #include "ui/gfx/geometry/size.h"
On 2015/03/25 18:50:23, tapted (travelling) wrote:
> nit: forward declare

Done.

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:18: gfx::Size min_size,
On 2015/03/25 18:50:23, tapted (travelling) wrote:
> nit: const-reference

Done.

Is it a rule to always pass gfx::SmallThing by const-ref? I would have thought
it'd be better to copy, do we ever do that in Chrome?

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
File ui/gfx/mac/nswindow_util.mm (right):

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.mm:8: #include "base/mac/sdk_forward_declarations.h"
On 2015/03/25 18:50:23, tapted (travelling) wrote:
> nit: include->import all (3) of these.

Done.

[tapted, 2015-03-30 06:33:38]

Sorry again for the delay.. I think I started with an initial draft comment, and
then the codereview UI bumped it to a different section of `My Issues` :/. but
I'm  finally approaching inbox-zero again..

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_util.h
File ui/gfx/mac/nswindow_util.h (right):

https://codereview.chromium.org/1023083002/diff/40001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:18: gfx::Size min_size,
On 2015/03/26 03:50:46, jackhou1 wrote:
> On 2015/03/25 18:50:23, tapted (travelling) wrote:
> > nit: const-reference
> 
> Done.
> 
> Is it a rule to always pass gfx::SmallThing by const-ref? I would have thought
> it'd be better to copy, do we ever do that in Chrome?

`git grep '(.*gfx::Size' -- '*.h'` has mostly const-reference

Unless it's smaller than a pointer, I don't think there are compelling upsides
to requiring the copy constructor call. And there are other downsides to
copy-constructing, like being unable to "passthrough" an argument to another
function without #including its header.

There are some other arguments around pointer-aliasing and codegen opportunities
when you actually want to modify the argument in your function, but they apply
mostly to inline functions AFAIK.

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_frame_view_mac.h (right):

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.h:12: class
NativeFrameViewMac : public views::NativeFrameView {
needs a class comment. It should explain why this is needed (and why just for
app windows).

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.h:14: static const char
kViewClassName[];
Is this needed for anything? I think it's more common to leave this off for new
view types until it's required to analyse something

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_frame_view_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.mm:22: gfx::Rect
NativeFrameViewMac::GetWindowBoundsForClientBounds(
I'm not sure exactly what this is solving

the right way to do this might be something in
AppWindowFrameView::GetWindowBoundsForClientBounds() .. obviously we can't just
#ifdef in the code below, but there is also a global function
views::GetWindowBoundsForClientBounds(..) which is used on Windows (provided by
ui/views/win/hwnd_util.h) -- mac could provide something like that.

Alternatively, maybe NativeFrameViewMac really belongs in ui/views/widget/cocoa
and NativeWidgetMac should provide it... 

Let's chat!

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_util.h
File ui/gfx/mac/nswindow_util.h (right):

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:15: void SetFullScreenCollectionBehavior(NSWindow*
window, bool allow_fullscreen);
can this and SetResizableStyleMask move to the .mm in an anonymous namespace?

(if not, I'd go for names that are closer to the comment, like
SetNSWindowCanFullscreen)

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:23: void ApplySizeConstraints(NSWindow* window,
this should be in namespace gfx (missed this before  - sorry)

Also you'll need GFX_EXPORT (this will fix the mac_debug compile error)

And the name... it's maybe OK. But it might also make sense to put `NSWindow` in
the function names so they sound less generic / less likely to collide with
other global functions. E.g. ApplyNSWindowSizeConstraints.

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
File ui/gfx/mac/nswindow_util.mm (right):

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.mm:14: const int kUnboundedSize = 0;
since `const` has internal linkage implicitly, this can be removed from the
anonymous namespace (I'm pretty sure Nico prefers this, and he will probably be
the OWNER reviewer :). But since I suggested an anonymous namespace for the
functions below, in another comment, I'd maybe just keep it for now and see what
nico thinks

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.mm:50: if (base::mac::IsOSSnowLeopard())
Maybe a comment why this is snow leopard only (i.e. Apple stopped putting resize
indicators on the UI? - but is there a reason why it's not just set for all OSes
(would it break stuff?)

[jackhou1, 2015-03-31 00:50:43]

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_frame_view_mac.h (right):

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.h:12: class
NativeFrameViewMac : public views::NativeFrameView {
On 2015/03/30 06:33:37, tapted wrote:
> needs a class comment. It should explain why this is needed (and why just for
> app windows).
> 

Done.

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.h:14: static const char
kViewClassName[];
On 2015/03/30 06:33:37, tapted wrote:
> Is this needed for anything? I think it's more common to leave this off for
new
> view types until it's required to analyse something

Done.

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_frame_view_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.mm:22: gfx::Rect
NativeFrameViewMac::GetWindowBoundsForClientBounds(
On 2015/03/30 06:33:37, tapted wrote:
> I'm not sure exactly what this is solving
> 
> the right way to do this might be something in
> AppWindowFrameView::GetWindowBoundsForClientBounds() .. obviously we can't
just
> #ifdef in the code below, but there is also a global function
> views::GetWindowBoundsForClientBounds(..) which is used on Windows (provided
by
> ui/views/win/hwnd_util.h) -- mac could provide something like that.
> 
> Alternatively, maybe NativeFrameViewMac really belongs in
ui/views/widget/cocoa
> and NativeWidgetMac should provide it... 
> 
> Let's chat!

AppWindow needs something to provide the right inset between the outer (window)
and inner (content) bounds. NativeFrameViewMac is analogous to
GlassAppWindowFrameViewWin.

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_util.h
File ui/gfx/mac/nswindow_util.h (right):

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:15: void SetFullScreenCollectionBehavior(NSWindow*
window, bool allow_fullscreen);
On 2015/03/30 06:33:37, tapted wrote:
> can this and SetResizableStyleMask move to the .mm in an anonymous namespace?
> 
> (if not, I'd go for names that are closer to the comment, like
> SetNSWindowCanFullscreen)

Done.

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.h:23: void ApplySizeConstraints(NSWindow* window,
On 2015/03/30 06:33:37, tapted wrote:
> this should be in namespace gfx (missed this before  - sorry)
> 
> Also you'll need GFX_EXPORT (this will fix the mac_debug compile error)
> 
> And the name... it's maybe OK. But it might also make sense to put `NSWindow`
in
> the function names so they sound less generic / less likely to collide with
> other global functions. E.g. ApplyNSWindowSizeConstraints.

Done.

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
File ui/gfx/mac/nswindow_util.mm (right):

https://codereview.chromium.org/1023083002/diff/80001/ui/gfx/mac/nswindow_uti...
ui/gfx/mac/nswindow_util.mm:50: if (base::mac::IsOSSnowLeopard())
On 2015/03/30 06:33:37, tapted wrote:
> Maybe a comment why this is snow leopard only (i.e. Apple stopped putting
resize
> indicators on the UI? - but is there a reason why it's not just set for all
OSes
> (would it break stuff?)

Done.

[tapted, 2015-03-31 01:06:30]

lgtm % nit and NativeFrameViewMac renamed

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_frame_view_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.mm:22: gfx::Rect
NativeFrameViewMac::GetWindowBoundsForClientBounds(
On 2015/03/31 00:50:42, jackhou1 wrote:
> On 2015/03/30 06:33:37, tapted wrote:
> > I'm not sure exactly what this is solving
> > 
> > the right way to do this might be something in
> > AppWindowFrameView::GetWindowBoundsForClientBounds() .. obviously we can't
> just
> > #ifdef in the code below, but there is also a global function
> > views::GetWindowBoundsForClientBounds(..) which is used on Windows (provided
> by
> > ui/views/win/hwnd_util.h) -- mac could provide something like that.
> > 
> > Alternatively, maybe NativeFrameViewMac really belongs in
> ui/views/widget/cocoa
> > and NativeWidgetMac should provide it... 
> > 
> > Let's chat!
> 
> AppWindow needs something to provide the right inset between the outer
(window)
> and inner (content) bounds. NativeFrameViewMac is analogous to
> GlassAppWindowFrameViewWin.

OK I think we should call it NativeAppWindowFrameViewMac then.

https://codereview.chromium.org/1023083002/diff/100001/ui/gfx/mac/nswindow_ut...
File ui/gfx/mac/nswindow_util.mm (right):

https://codereview.chromium.org/1023083002/diff/100001/ui/gfx/mac/nswindow_ut...
ui/gfx/mac/nswindow_util.mm:56: // only shown for apps that have unbounded size.
nit: apps->windows

[jackhou1, 2015-03-31 01:59:48]

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_frame_view_mac.mm (right):

https://codereview.chromium.org/1023083002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_frame_view_mac.mm:22: gfx::Rect
NativeFrameViewMac::GetWindowBoundsForClientBounds(
On 2015/03/31 01:06:30, tapted wrote:
> On 2015/03/31 00:50:42, jackhou1 wrote:
> > On 2015/03/30 06:33:37, tapted wrote:
> > > I'm not sure exactly what this is solving
> > > 
> > > the right way to do this might be something in
> > > AppWindowFrameView::GetWindowBoundsForClientBounds() .. obviously we can't
> > just
> > > #ifdef in the code below, but there is also a global function
> > > views::GetWindowBoundsForClientBounds(..) which is used on Windows
(provided
> > by
> > > ui/views/win/hwnd_util.h) -- mac could provide something like that.
> > > 
> > > Alternatively, maybe NativeFrameViewMac really belongs in
> > ui/views/widget/cocoa
> > > and NativeWidgetMac should provide it... 
> > > 
> > > Let's chat!
> > 
> > AppWindow needs something to provide the right inset between the outer
> (window)
> > and inner (content) bounds. NativeFrameViewMac is analogous to
> > GlassAppWindowFrameViewWin.
> 
> OK I think we should call it NativeAppWindowFrameViewMac then.

Done.

https://codereview.chromium.org/1023083002/diff/100001/ui/gfx/mac/nswindow_ut...
File ui/gfx/mac/nswindow_util.mm (right):

https://codereview.chromium.org/1023083002/diff/100001/ui/gfx/mac/nswindow_ut...
ui/gfx/mac/nswindow_util.mm:56: // only shown for apps that have unbounded size.
On 2015/03/31 01:06:30, tapted wrote:
> nit: apps->windows

Done.

[jackhou1, 2015-03-31 02:00:09]

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

[jackhou1, 2015-03-31 02:00:09]

thakis, please review for OWNERS in:

ui/gfx/mac/

[Nico, 2015-03-31 16:46:14]

We try not to do "_util" files as they tend to end up as dumping grounds. (…I
can't find the thread about this atm, but there was one.) Can you think of a
better, more focused name?

[jackhou1, 2015-04-01 04:59:10]

On 2015/03/31 16:46:14, Nico wrote:
> We try not to do "_util" files as they tend to end up as dumping grounds. (…I
> can't find the thread about this atm, but there was one.) Can you think of a
> better, more focused name?

Changed to nswindow_frame_controls.

[Nico, 2015-04-01 18:30:47]

ui/ lgtm, thanks!

[jackhou1, 2015-04-01 23:26:29]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-04-01 23:26:29]

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/1023083002/#ps160001
(title: "Sync and rebase")

[commit-bot: I haz the power, 2015-04-01 23:27:17]

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

[commit-bot: I haz the power, 2015-04-02 00:21:57]

Committed patchset #9 (id:160001)

[jackhou1, 2015-04-02 00:35:01]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1051953003/ by jackhou@chromium.org.

The reason for reverting is: Broke GN build:
http://build.chromium.org/p/chromium.mac/builders/Mac%20GN/builds/6200.

[jackhou1, 2015-04-02 03:23:44]

On 2015/04/02 00:35:01, jackhou1 wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/1051953003/ by mailto:jackhou@chromium.org.
> 
> The reason for reverting is: Broke GN build:
> http://build.chromium.org/p/chromium.mac/builders/Mac%20GN/builds/6200.

GN change is a straight-forward mirror of gfx.gyp. Passes on
mac_chromium_gn_rel. Recommitting.

[jackhou1, 2015-04-02 03:23:53]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-04-02 03:23:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1023083002/#ps180001
(title: "Update ui/gfx/BUILD.gn")

[commit-bot: I haz the power, 2015-04-02 03:24:22]

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

[commit-bot: I haz the power, 2015-04-02 04:49:37]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-03 20:20:16]

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

[commit-bot: I haz the power, 2015-04-03 20:22:47]

Patchset 10 (id:??) landed as
https://crrev.com/b563202b63c248fa7f3605f802d7649ae7d48562
Cr-Commit-Position: refs/heads/master@{#323429}