MacViews: NativeWidget -> Widget notifications: fullscreen, activation.

MacViews: NativeWidget -> Widget notifications: fullscreen, activation.

Fullscreen is asynchronous on Mac, so NativeWidgetMac needs to
coordinate with the ViewsNSWindowDelegate to behave in the manner views
code expects. Part of this is maintaining the restored bounds, which
Cocoa doesn't provide access to.

Activation also goes through the ViewsNSWindowDelegate. It's simpler but
needs an interactive_ui_test, which is added. Other NSWindowDelegate
messages are deferred to a follow-up.

Maximize is "implemented" as a no-op on Mac, since it's
indistinguishable from a large window (whereas views code typically
expects maximize to remove window borders).

This CL gets the following views_unittests passing:
  WidgetTest.GetRestoredBounds
  WidgetTest.ExitFullscreenRestoreState
  WidgetTest.FullscreenFrameLayout

Many Cocoa-native tests are also added to ensure user-initiated
operations are correctly observed.

BUG=403679, 378134
Committed: https://crrev.com/50d05e112bdee6085dc461eba5435eb82ec60a46
Cr-Commit-Position: refs/heads/master@{#299263}

[tapted, 2014-10-03 05:33:54]

Patchset #5 (id:80001) has been deleted

[tapted, 2014-10-03 05:34:00]

Patchset #3 (id:40001) has been deleted

[tapted, 2014-10-03 10:39:10]

Patchset #7 (id:160001) has been deleted

[Andre, 2014-10-05 02:54:17]

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

[Andre, 2014-10-05 02:54:18]

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

https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:77: object:nil];
I think this should be object:window instead object:nil so that we only get
notifications for that window.

https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:89: [defaultCenter
removeObserver:self name:nil object:nil];
[defaultCenter removeObserver:self] is shorter and equivalent.

https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:114: return;
isEqualTo -> isEqual, but don't need this check if addObserver is changed above.

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

https://codereview.chromium.org/388453002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:26: // Override canBecome{Key,Main}Window
to always return YES, otherwise Windows
Update comment, {Key,Main} -> Key.

[Andre, 2014-10-05 18:14:21]

LGTM
I agree we shouldn't need the maximize stuff.

[tapted, 2014-10-07 07:22:31]

Patchset #9 (id:220001) has been deleted

[tapted, 2014-10-07 11:21:12]

Hi Andre - PTAL. I think this is ready to go. I've pruned it back to just the
stuff with test coverage so far.

Visibility notifications turned into a real pain, so I've deferred them to a
separate CL: crrev.com/630363003 (summary:  windowDidExpose: is only sent for
nonretained windows. BrowserWindowCocoa uses a combination of
applicationDidHide: and windowDidMiniaturize:, but that leaves gaps, so I'm
exploring a general solution in crrev.com/630363003 ).

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

https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:77: object:nil];
On 2014/10/05 02:54:18, Andre wrote:
> I think this should be object:window instead object:nil so that we only get
> notifications for that window.

Done.

https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:89: [defaultCenter
removeObserver:self name:nil object:nil];
On 2014/10/05 02:54:18, Andre wrote:
> [defaultCenter removeObserver:self] is shorter and equivalent.

Done.

https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:114: return;
On 2014/10/05 02:54:18, Andre wrote:
> isEqualTo -> isEqual, but don't need this check if addObserver is changed
above.

Done.

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

https://codereview.chromium.org/388453002/diff/140001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:26: // Override canBecome{Key,Main}Window
to always return YES, otherwise Windows
On 2014/10/05 02:54:18, Andre wrote:
> Update comment, {Key,Main} -> Key.

oops - I didn't mean to delete this :). I was playing around with
acceptsFirstResponder and transient children/menus, but we need to move away
from Cocoa's childWindow implementation anyway, since it has some intractable
quirks, like forcing child windows to be visible. So, that's a change for
another day.

[Andre, 2014-10-08 17:22:48]

On 2014/10/07 11:21:12, tapted wrote:
> Hi Andre - PTAL. I think this is ready to go. I've pruned it back to just the
> stuff with test coverage so far.
> 
> Visibility notifications turned into a real pain, so I've deferred them to a
> separate CL: crrev.com/630363003 (summary:  windowDidExpose: is only sent for
> nonretained windows. BrowserWindowCocoa uses a combination of
> applicationDidHide: and windowDidMiniaturize:, but that leaves gaps, so I'm
> exploring a general solution in crrev.com/630363003 ).
> 
>
https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
> File ui/views/cocoa/bridged_native_widget_unittest.mm (right):
> 
>
https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
> ui/views/cocoa/bridged_native_widget_unittest.mm:77: object:nil];
> On 2014/10/05 02:54:18, Andre wrote:
> > I think this should be object:window instead object:nil so that we only get
> > notifications for that window.
> 
> Done.
> 
>
https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
> ui/views/cocoa/bridged_native_widget_unittest.mm:89: [defaultCenter
> removeObserver:self name:nil object:nil];
> On 2014/10/05 02:54:18, Andre wrote:
> > [defaultCenter removeObserver:self] is shorter and equivalent.
> 
> Done.
> 
>
https://codereview.chromium.org/388453002/diff/140001/ui/views/cocoa/bridged_...
> ui/views/cocoa/bridged_native_widget_unittest.mm:114: return;
> On 2014/10/05 02:54:18, Andre wrote:
> > isEqualTo -> isEqual, but don't need this check if addObserver is changed
> above.
> 
> Done.
> 
>
https://codereview.chromium.org/388453002/diff/140001/ui/views/widget/native_...
> File ui/views/widget/native_widget_mac.mm (right):
> 
>
https://codereview.chromium.org/388453002/diff/140001/ui/views/widget/native_...
> ui/views/widget/native_widget_mac.mm:26: // Override canBecome{Key,Main}Window
> to always return YES, otherwise Windows
> On 2014/10/05 02:54:18, Andre wrote:
> > Update comment, {Key,Main} -> Key.
> 
> oops - I didn't mean to delete this :). I was playing around with
> acceptsFirstResponder and transient children/menus, but we need to move away
> from Cocoa's childWindow implementation anyway, since it has some intractable
> quirks, like forcing child windows to be visible. So, that's a change for
> another day.

LGTM

[tapted, 2014-10-09 06:58:08]

tapted@chromium.org changed reviewers:
+ rsesek@chromium.org, sky@chromium.org

[tapted, 2014-10-09 06:58:09]

Hi sky, rsesek please take a look for OWNERS

sky: ui/views
rsesek: base/mac (or anything if you're curious :).

(for additional context, other NativeWidget -> Widget notifications are in a
follow-up: crrev/630363003 which split off from here after window-visibility
notifications turned out to be a bit of a doozie on Cocoa).

Thanks!

[Robert Sesek, 2014-10-09 14:09:41]

https://codereview.chromium.org/388453002/diff/240001/base/mac/sdk_forward_de...
File base/mac/sdk_forward_declarations.mm (right):

https://codereview.chromium.org/388453002/diff/240001/base/mac/sdk_forward_de...
base/mac/sdk_forward_declarations.mm:17: NSString* const
NSWindowDidEnterFullScreenNotification =
This and the constant below are also defined in
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm.

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:55: // Called by the NSWindowDelegate
when a fullscreen operation begins or fails.
What does the parameter mean? And why "begins or fails"?

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:56: void
OnTargetFullscreenStateChanged(bool target_fullscreen_state);
'Target' doesn't read quite right here, I think. Maybe
"OnFullscreenTransitionStart" to match the complement below?

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:94: NSNotificationCenter*
defaultCenter = [NSNotificationCenter defaultCenter];
Can probably do without this local.

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:110: -
(void)maybeQuit:(NSNotification*)notification changedArg:(int*)changedArg {
|notification| is unused here.

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:28:
parent_->OnTargetFullscreenStateChanged(false);
I'm a little surprised this doesn't go through OnFullscreenTransitionComplete
instead.

[sky, 2014-10-09 17:46:25]

LGTM (assuming you can get rsesek happy). Also, how about sending me a patch
that adds you as an OWNER for the appropriate set of cocoa files in views?

[tapted, 2014-10-10 11:57:10]

Thanks Robert - PTAL.

On 2014/10/09 17:46:25, sky wrote:
> Also, how about sending me a patch
> that adds you as an OWNER for the appropriate set of cocoa files in views?

Will do :). I expect in the bring-up there will be a bunch of cross-platform
tests I want to fix/add too, but I can direct some to sadrul when he's back.

https://codereview.chromium.org/388453002/diff/240001/base/mac/sdk_forward_de...
File base/mac/sdk_forward_declarations.mm (right):

https://codereview.chromium.org/388453002/diff/240001/base/mac/sdk_forward_de...
base/mac/sdk_forward_declarations.mm:17: NSString* const
NSWindowDidEnterFullScreenNotification =
On 2014/10/09 14:09:41, Robert Sesek wrote:
> This and the constant below are also defined in
> chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm.

ooh - good catch. I think that would have broken a non-component build building
browser_tests. Removed the dupe in native_app_window_cocoa_browsertest.mm . (and
it crossed my mind there could be a used-in-tests-only version of this file, but
the linker should be dropping anything like that from release builds)

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:55: // Called by the NSWindowDelegate
when a fullscreen operation begins or fails.
On 2014/10/09 14:09:41, Robert Sesek wrote:
> What does the parameter mean? And why "begins or fails"?

Updated the comment (and `fail` deferred to a follow-up - see below)

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:56: void
OnTargetFullscreenStateChanged(bool target_fullscreen_state);
On 2014/10/09 14:09:41, Robert Sesek wrote:
> 'Target' doesn't read quite right here, I think. Maybe
> "OnFullscreenTransitionStart" to match the complement below?

Done.

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:94: NSNotificationCenter*
defaultCenter = [NSNotificationCenter defaultCenter];
On 2014/10/09 14:09:41, Robert Sesek wrote:
> Can probably do without this local.

Done.

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget_unittest.mm:110: -
(void)maybeQuit:(NSNotification*)notification changedArg:(int*)changedArg {
On 2014/10/09 14:09:41, Robert Sesek wrote:
> |notification| is unused here.

renamed to 

- (void)maybeQuitForChangedArg:(int*)changedArg;

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:28:
parent_->OnTargetFullscreenStateChanged(false);
On 2014/10/09 14:09:41, Robert Sesek wrote:
> I'm a little surprised this doesn't go through OnFullscreenTransitionComplete
> instead.

You're right. I forgot to update these two after getting the state variables
sorted out. I think I'd like to land these with some tests, since I'm not yet
sure of the circumstances that Cocoa might send these notifications. But I can
set up a test that posts notifications to simulate it.

And it's likely I'll need another method on BridgedNativeWidget so that it
doesn't get stuck in a loop retrying.

So changed these to NOTREACHED() + todo.  Still need get fullscreen happy on
SnowLeopard anyway :/

[Robert Sesek, 2014-10-10 15:09:43]

LGTM

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/388453002/diff/240001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:28:
parent_->OnTargetFullscreenStateChanged(false);
On 2014/10/10 11:57:10, tapted wrote:
> So changed these to NOTREACHED() + todo.  Still need get fullscreen happy on
> SnowLeopard anyway :/

I'm not sure SL is worth the effort.

[tapted, 2014-10-12 22:51:29]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-10-12 22:52:45]

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

[commit-bot: I haz the power, 2014-10-12 23:31:46]

Committed patchset #10 (id:270001)

[commit-bot: I haz the power, 2014-10-12 23:32:33]

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