[MacViews] Fix behavior of non-resizable windows in fullscreen.

[MacViews] Fix behavior of non-resizable windows in fullscreen.

They should take up the entire screen, and the fullscreen button must
be enabled to allow the user to leave.

This also factors out NSWindowFullscreenNotificationWaiter.

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

[jackhou1, 2015-04-24 07:03:00]

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

[jackhou1, 2015-04-24 07:03:01]

tapted, what do you think?

https://codereview.chromium.org/1109493002/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/1109493002/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:305:
EXPECT_FALSE([ns_window collectionBehavior] &
This line fails. Probably need to wait for the animation to finish.

[tapted, 2015-05-06 06:20:42]

https://codereview.chromium.org/1109493002/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/1109493002/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:305:
EXPECT_FALSE([ns_window collectionBehavior] &
On 2015/04/24 07:03:01, jackhou1 (OOO until May 11) wrote:
> This line fails. Probably need to wait for the animation to finish.

bridged_native_widget_interactive_uitest.mm has some stuff for this.

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:436:
gfx::SetNSWindowCanFullscreen(window_, true);
Why always `true`? (this function is a toggle).

i.e. when coming out of fullscreen, what hides the button?

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:531: bool shows_fullscreen_controls =
target_fullscreen_state_ ||
What will call OnSizeConstraintsChanged() when target_fullscreen_state_ changes?

Maybe call OnSizeConstraintsChanged() unconditionally from
OnFullscreenTransitionComplete(), and update the comment in
ToggleDesiredFullscreenState to mention that the flag will be cleared once the
animation completes

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/views_nswind...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/views_nswind...
ui/views/cocoa/views_nswindow_delegate.mm:124:
willUseFullScreenContentSize:(NSSize)proposedSize {
Can you explain this change?

And, e.g., could we instead make non-resizable windows resizable while they are
fullscreen?
and why can't non-resizable windows occupy only their fixed size while in a
fullscreen desktop? (i.e. is this just to preserve the current behaviour of
packaged apps?)

[jackhou1, 2015-05-13 04:27:53]

https://codereview.chromium.org/1109493002/diff/1/chrome/browser/ui/cocoa/app...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/1109493002/diff/1/chrome/browser/ui/cocoa/app...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:305:
EXPECT_FALSE([ns_window collectionBehavior] &
On 2015/05/06 06:20:41, tapted wrote:
> On 2015/04/24 07:03:01, jackhou1 (OOO until May 11) wrote:
> > This line fails. Probably need to wait for the animation to finish.
> 
> bridged_native_widget_interactive_uitest.mm has some stuff for this.

Done.

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:436:
gfx::SetNSWindowCanFullscreen(window_, true);
On 2015/05/06 06:20:41, tapted wrote:
> Why always `true`? (this function is a toggle).
> 
> i.e. when coming out of fullscreen, what hides the button?

Changed to only set the collection behavior if not already fullscreen. It also
removes size constraints now.

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget.mm:531: bool shows_fullscreen_controls =
target_fullscreen_state_ ||
On 2015/05/06 06:20:41, tapted wrote:
> What will call OnSizeConstraintsChanged() when target_fullscreen_state_
changes?

There's a RootView::Layout after leaving fullscreen:

2   Chromium Framework                  0x0000000113502ab0
views::BridgedNativeWidget::OnSizeConstraintsChanged() + 208
3   Chromium Framework                  0x000000011360a9a2
views::NativeWidgetMac::OnSizeConstraintsChanged() + 50
4   Chromium Framework                  0x000000011360a902
views::NativeWidgetMac::OnRootViewLayout() + 50
5   Chromium Framework                  0x00000001136194d6
views::Widget::OnRootViewLayout() + 54
6   Chromium Framework                  0x000000011360ee7a
views::internal::RootView::Layout() + 58
7   Chromium Framework                  0x00000001135eb718
views::View::BoundsChanged(gfx::Rect const&) + 776
8   Chromium Framework                  0x00000001135eb325
views::View::SetBoundsRect(gfx::Rect const&) + 261
9   Chromium Framework                  0x00000001135eb1f0
views::View::SetBounds(int, int, int, int) + 144
10  Chromium Framework                  0x00000001135eb897
views::View::SetSize(gfx::Size const&) + 103
11  Chromium Framework                  0x00000001134f9d85 -[BridgedContentView
setFrameSize:] + 501
12  AppKit                              0x00007fff876ec090 -[NSWindow
_oldPlaceWindow:] + 1627
13  AppKit                              0x00007fff876eb3aa -[NSWindow
_setFrameCommon:display:stashSize:] + 2527
14  AppKit                              0x00007fff87e7df1c
-[_NSWindowFullScreenTransition setTransitionedWindowFrame:] + 84
15  AppKit                              0x00007fff878a2152
-[_NSWindowFullScreenTransition exitFullScreenTransitionForWindow:animated:] +
799
16  AppKit                              0x00007fff878a1d53 -[NSWindow
_resignFullScreenWindowAndExitFullScreen:] + 103

> Maybe call OnSizeConstraintsChanged() unconditionally from
> OnFullscreenTransitionComplete(), and update the comment in
> ToggleDesiredFullscreenState to mention that the flag will be cleared once the
> animation completes

Done.

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/views_nswind...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/1109493002/diff/1/ui/views/cocoa/views_nswind...
ui/views/cocoa/views_nswindow_delegate.mm:124:
willUseFullScreenContentSize:(NSSize)proposedSize {
On 2015/05/06 06:20:41, tapted wrote:
> Can you explain this change?
> 
> And, e.g., could we instead make non-resizable windows resizable while they
are
> fullscreen?

Yes (and doing the same with size constraints). But changing the resizable mask
causes OnSizeConstraintsChanged to be recursively called:

2   Chromium Framework                  0x0000000111734e9d
gfx::ApplyNSWindowSizeConstraints(NSWindow*, gfx::Size const&, gfx::Size const&,
bool, bool) + 429
3   Chromium Framework                  0x00000001198af6bf
views::BridgedNativeWidget::OnSizeConstraintsChanged() + 127
4   Chromium Framework                  0x00000001199b89a2
views::NativeWidgetMac::OnSizeConstraintsChanged() + 50
5   Chromium Framework                  0x00000001199b8902
views::NativeWidgetMac::OnRootViewLayout() + 50
6   Chromium Framework                  0x00000001199c74d6
views::Widget::OnRootViewLayout() + 54
7   Chromium Framework                  0x00000001199bce7a
views::internal::RootView::Layout() + 58
8   Chromium Framework                  0x0000000119999718
views::View::BoundsChanged(gfx::Rect const&) + 776
9   Chromium Framework                  0x0000000119999325
views::View::SetBoundsRect(gfx::Rect const&) + 261
10  Chromium Framework                  0x00000001199991f0
views::View::SetBounds(int, int, int, int) + 144
11  Chromium Framework                  0x0000000119999897
views::View::SetSize(gfx::Size const&) + 103
12  Chromium Framework                  0x00000001198a8085 -[BridgedContentView
setFrameSize:] + 501
13  AppKit                              0x00007fff876ec090 -[NSWindow
_oldPlaceWindow:] + 1627
14  AppKit                              0x00007fff876eb3aa -[NSWindow
_setFrameCommon:display:stashSize:] + 2527
15  AppKit                              0x00007fff878484cc __25-[NSWindow
setStyleMask:]_block_invoke + 2142
16  AppKit                              0x00007fff876f36b3
NSPerformWithScreenUpdatesDisabled + 65
17  AppKit                              0x00007fff87847c20 -[NSWindow
setStyleMask:] + 222

So to prevent it being changed back, I made
BridgedNativeWidget::ToggleDesiredFullscreenState set target_fullscreen_state_.
WDYT?

> and why can't non-resizable windows occupy only their fixed size while in a
> fullscreen desktop? (i.e. is this just to preserve the current behaviour of
> packaged apps?)

Yup, should this be different for non-app windows?

[tapted, 2015-05-13 06:12:42]

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (left):

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:377: DCHECK_NE(target_fullscreen_state,
target_fullscreen_state_);
I think we can keep this.... see below

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:392: OnSizeConstraintsChanged();
This should only happen if we're staying in this state -- i.e. within the `if`
block just below, before the early return.

Otherwise, we just undo this step in ToggleDesiredFullscreenState


(and with my other suggestions, the other check is redundant. i.e. you could do
just (below)

  if (target_fullscreen_state_ == actual_fullscreen_state) {
    // Ensure constraints are re-applied when completing a transition.
    OnSizeContraintsChanged();
    return;
  }

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:446: target_fullscreen_state_ = true;
it feels fragile to update target_fullscreen_state_ here as well as
OnFullscreenTransitionStart -- OnFullscreenTransitionStart/Complete is the
source of truth.

Also, I don't think it's nice to check the current styleMask -- Cocoa updates it
at weird times (I think the current code guarantees that if
target_fullscreen_state_ is false, we are trying to enter fullscreen, but I
don't think we need to check).

Here (instead of the if), I'd suggest just doing something like

  // Clear any constraints and enable fullscreen before starting any transition
  // (they will be re-applied when a transition out of fullscreen completes).
  ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), true, true);


But, I think this will leave the problem that changing the style mask will
trigger a call to setFrame -> OnRootViewLayout -> OnSizeConstraintsChanged -- I
think we need to avoid that, but in a more obvious way.

- is the frame size actually changing? (can we early exit in [BridgedContentView
setFrameSize:]. There's an early exit in View::SetBoundsRect when needs_layout
is false -- why isn't that coming into play?

Can we make the setStyleMask in ApplyNSWindowSizeConstraints a no-op?
e.g., here, do

  /* style_mask &= ~NSResizableWindowMask; */
  ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), true, true);

That's still hacky (we don't want that useless layout call before going
fullscreen), so would need a good comment.


Otherwise.... I'd be looking to nerf the hook in [BridgedContentView
setFrameSize:]. E.g.

// To make a non-user-resizable window properly fullscreen, it must have have
// NSResizableWindowMask set. Changing this flag triggers a resize, but it won't
// be the resize that takes the window into or out of fullscreen. To avoid a
// useless layout, temporarily suppress frame change propagations, and wait for
// the one that is triggered from [somewhere].
[bridged_view_ setIgnoreFrameChanges:YES];
ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), true, true);
[bridged_view_ setIgnoreFrameChanges:NO];



OR!

If we can do (here)

// Ensure a setStyleMask call doesn't trigger a useless layout. [more words]
bool can_resize = style_mask() & NSResizableWindowMask;
ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), can_resize,
true);

and combine that with the 

willUseFullScreenContentSize:

trick in views_nswindow_delegate.mm

then maybe that's a nice way to do it.

But I'd bring back willUseFullScreenContentSize with a comment like ~"Without
this, windows with the NSResizableWindowMask cleared from their styleMask will
only enter fullscreen at their
current size rather than resizing to fill the screen."

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:538: if (target_fullscreen_state_) {
I think just (at the start, with an appropriate comment):

if (target_fullscreen_state_ || in_fullscreen_transition)
  return;

[jackhou1, 2015-05-13 06:53:10]

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (left):

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:377: DCHECK_NE(target_fullscreen_state,
target_fullscreen_state_);
On 2015/05/13 06:12:42, tapted wrote:
> I think we can keep this.... see below

Done.

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:392: OnSizeConstraintsChanged();
On 2015/05/13 06:12:42, tapted wrote:
> This should only happen if we're staying in this state -- i.e. within the `if`
> block just below, before the early return.
> 
> Otherwise, we just undo this step in ToggleDesiredFullscreenState
> 
> 
> (and with my other suggestions, the other check is redundant. i.e. you could
do
> just (below)
> 
>   if (target_fullscreen_state_ == actual_fullscreen_state) {
>     // Ensure constraints are re-applied when completing a transition.
>     OnSizeContraintsChanged();
>     return;
>   }
> 

Done.

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:446: target_fullscreen_state_ = true;
On 2015/05/13 06:12:42, tapted wrote:
> it feels fragile to update target_fullscreen_state_ here as well as
> OnFullscreenTransitionStart -- OnFullscreenTransitionStart/Complete is the
> source of truth.
> 
> Also, I don't think it's nice to check the current styleMask -- Cocoa updates
it
> at weird times (I think the current code guarantees that if
> target_fullscreen_state_ is false, we are trying to enter fullscreen, but I
> don't think we need to check).
> 
> Here (instead of the if), I'd suggest just doing something like
> 
>   // Clear any constraints and enable fullscreen before starting any
transition
>   // (they will be re-applied when a transition out of fullscreen completes).
>   ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), true, true);
> 
> 
> But, I think this will leave the problem that changing the style mask will
> trigger a call to setFrame -> OnRootViewLayout -> OnSizeConstraintsChanged --
I
> think we need to avoid that, but in a more obvious way.
> 
> - is the frame size actually changing? (can we early exit in
[BridgedContentView
> setFrameSize:]. There's an early exit in View::SetBoundsRect when needs_layout
> is false -- why isn't that coming into play?
> 
> Can we make the setStyleMask in ApplyNSWindowSizeConstraints a no-op?
> e.g., here, do
> 
>   /* style_mask &= ~NSResizableWindowMask; */
>   ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), true, true);
> 
> That's still hacky (we don't want that useless layout call before going
> fullscreen), so would need a good comment.
> 
> 
> Otherwise.... I'd be looking to nerf the hook in [BridgedContentView
> setFrameSize:]. E.g.
> 
> // To make a non-user-resizable window properly fullscreen, it must have have
> // NSResizableWindowMask set. Changing this flag triggers a resize, but it
won't
> // be the resize that takes the window into or out of fullscreen. To avoid a
> // useless layout, temporarily suppress frame change propagations, and wait
for
> // the one that is triggered from [somewhere].
> [bridged_view_ setIgnoreFrameChanges:YES];
> ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), true, true);
> [bridged_view_ setIgnoreFrameChanges:NO];
> 
> 
> 
> OR!
> 
> If we can do (here)
> 
> // Ensure a setStyleMask call doesn't trigger a useless layout. [more words]
> bool can_resize = style_mask() & NSResizableWindowMask;
> ApplyNSWindowSizeConstraints(window_, gfx::Size(), gfx::Size(), can_resize,
> true);
> 
> and combine that with the 
> 
> willUseFullScreenContentSize:
> 
> trick in views_nswindow_delegate.mm
> 
> then maybe that's a nice way to do it.
> 
> But I'd bring back willUseFullScreenContentSize with a comment like ~"Without
> this, windows with the NSResizableWindowMask cleared from their styleMask will
> only enter fullscreen at their
> current size rather than resizing to fill the screen."

By bringing back the willUseFullScreenContentSize, we no longer need to set the
style mask or size constraints here. So this just enables fullscreen collection
behavior.

https://codereview.chromium.org/1109493002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:538: if (target_fullscreen_state_) {
On 2015/05/13 06:12:42, tapted wrote:
> I think just (at the start, with an appropriate comment):
> 
> if (target_fullscreen_state_ || in_fullscreen_transition)
>   return;

Done.

[tapted, 2015-05-13 07:26:35]

lgtm with gn changes

https://codereview.chromium.org/1109493002/diff/80001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/1109493002/diff/80001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:673: 'test/nswindow_fullscreen_notification_waiter.mm',
You'll need a .gn change for this too. It's weird there -- I think you'll need a
different condition (not build_ime).

ui/gfx/BUILD.gn has a similar problem with ui_cocoa_test_helper.h/mm .. that
might be a better place. I  think I'm leadning towards ui/base, but will defer
to owners..

https://codereview.chromium.org/1109493002/diff/80001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/1109493002/diff/80001/ui/views/views.gyp#newc...
ui/views/views.gyp:908: '../base/ui_base.gyp:ui_base_test_support',
also needs a .gn change

[jackhou1, 2015-05-14 04:37:44]

Patchset #6 (id:100001) has been deleted

[jackhou1, 2015-05-14 04:38:34]

https://codereview.chromium.org/1109493002/diff/80001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/1109493002/diff/80001/ui/base/ui_base.gyp#new...
ui/base/ui_base.gyp:673: 'test/nswindow_fullscreen_notification_waiter.mm',
On 2015/05/13 07:26:34, tapted wrote:
> You'll need a .gn change for this too. It's weird there -- I think you'll need
a
> different condition (not build_ime).
> 
> ui/gfx/BUILD.gn has a similar problem with ui_cocoa_test_helper.h/mm .. that
> might be a better place. I  think I'm leadning towards ui/base, but will defer
> to owners..

Done.

https://codereview.chromium.org/1109493002/diff/80001/ui/views/views.gyp
File ui/views/views.gyp (right):

https://codereview.chromium.org/1109493002/diff/80001/ui/views/views.gyp#newc...
ui/views/views.gyp:908: '../base/ui_base.gyp:ui_base_test_support',
On 2015/05/13 07:26:34, tapted wrote:
> also needs a .gn change

Done.

[jackhou1, 2015-05-14 04:46:29]

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

[jackhou1, 2015-05-14 04:46:30]

thakis, please review for OWNERS:

*.gyp
*.gn
ui/base/test/*

All related to factoring out NSWindowFullscreenNotificationWaiter.

[Nico, 2015-05-15 21:28:09]

Sorry, this fell through the cracks.

Could the thing in base/test be somewhere in ui/ instead? NSWindowFullscreen
feels more like ui/ than base/ to me. (Sorry about the bikeshed :-/)

[jackhou1, 2015-05-18 04:30:24]

On 2015/05/15 21:28:09, Nico wrote:
> Sorry, this fell through the cracks.
> 
> Could the thing in base/test be somewhere in ui/ instead? NSWindowFullscreen
> feels more like ui/ than base/ to me. (Sorry about the bikeshed :-/)

It's actually already in ui/base/test. But I initially misinterpreted this and
moved them from ui/base/test/ to ui/gfx/test/. See Patch Set 7 vs 9. 

tapted pointed out that ui/base/test is probably better as fewer things depend
on it, so I'll revert back to Patch Set 7 if you don't have a preference either
way.

[Nico, 2015-05-19 00:25:54]

On 2015/05/18 04:30:24, jackhou1 wrote:
> On 2015/05/15 21:28:09, Nico wrote:
> > Sorry, this fell through the cracks.
> > 
> > Could the thing in base/test be somewhere in ui/ instead? NSWindowFullscreen
> > feels more like ui/ than base/ to me. (Sorry about the bikeshed :-/)
> 
> It's actually already in ui/base/test. But I initially misinterpreted this and
> moved them from ui/base/test/ to ui/gfx/test/. See Patch Set 7 vs 9. 
> 
> tapted pointed out that ui/base/test is probably better as fewer things depend
> on it, so I'll revert back to Patch Set 7 if you don't have a preference
either
> way.

Jeez, sorry. Patch set 7 lgtm.

[jackhou1, 2015-05-20 05:59:57]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-05-20 05:59:57]

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/1109493002/#ps200001
(title: "Revert to Patch Set 7")

[commit-bot: I haz the power, 2015-05-20 06:00:09]

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

[commit-bot: I haz the power, 2015-05-20 06:47:33]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-05-20 06:48:21]

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