[Mac] Update fullscreen state when window is fullscreened natively.

[Mac] Update fullscreen state when window is fullscreened natively.

This calls OnNativeWindowChanged when the window enters or exits
fullscreen. It also redundantly calls AppWindow::OSFullscreen and
AppWindow::Restore to get the AppWindow to update it's internal state.

BUG=372301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270904

[tapted, 2014-05-13 01:14:47]

Following on from our discussion - I think the extra state is OK, and likely
necessary to correctly handle changes while the fullscreen animation is still
progressing. Maybe we can simplify it orthogonally, but we should try to add a
regression test.

Can you adapt something like
AcceleratorCommandsPlatformAppFullscreenBrowserTest.ToggleFullscreen for Mac?

Have a look at void CocoaTest::Init() in ui/gfx/test/ui_cocoa_test_helper.mm if
you need to disable the resize animation.

[jackhou1, 2014-05-15 06:15:03]

On 2014/05/13 01:14:47, tapted wrote:
> Following on from our discussion - I think the extra state is OK, and likely
> necessary to correctly handle changes while the fullscreen animation is still
> progressing. Maybe we can simplify it orthogonally, but we should try to add a
> regression test.
> 
> Can you adapt something like
> AcceleratorCommandsPlatformAppFullscreenBrowserTest.ToggleFullscreen for Mac?
> 
> Have a look at void CocoaTest::Init() in ui/gfx/test/ui_cocoa_test_helper.mm
if
> you need to disable the resize animation.

Unfortunately, the ui_cocoa_test_helper stuff doesn't make the fullscreen
transition synchronous. I implemented the test by waiting for notifications.

PTAL

[tapted, 2014-05-15 06:39:20]

You'll probably still need the notification, but can you use the ~"disable
window resize" stuff in ui/gfx/test/ui_cocoa_test_helper.mm to make the test
execute faster?

Also I think most of the bots will have their deployment target still set as
snow leopard (even though it runs on lion/etc.), so the ifdef probably needs to
be smaller for the test to run on bots.

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

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:129: 
#endif here

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:169:
SetUpAppWithWindows(1);
here do

// Only test fullscreen for 10.7 and above.
if (base::mac::IsOSSnowLeopard())
  return;

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:184:
EXPECT_TRUE([ns_window styleMask] & NSFullScreenWindowMask);
I think it's worth moving `IsFullscreen` out of the anonymous namespace in
app_window.cc, and putting it as a private member function, then making this
test a friend, and doing some extra checks on AppWindow::IsFullscreen

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:189:
app_window->Restore();
does window->IsFullscreenOrPending() change immediately, either side of this
line? (i.e. without waiting for the notification?). It should!

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:198:
[watcher waitForNotification];
likewise window->IsFullscreenOrPending() should be false before this.

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:206:
[watcher waitForNotification];
(but this one is native, so window->IsFullscreenOrPending() isn't interesting
until after the notification)

[jackhou1, 2014-05-15 07:15:28]

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

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:129: 
On 2014/05/15 06:39:20, tapted wrote:
> #endif here

Done.

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:169:
SetUpAppWithWindows(1);
On 2014/05/15 06:39:20, tapted wrote:
> here do
> 
> // Only test fullscreen for 10.7 and above.
> if (base::mac::IsOSSnowLeopard())
>   return;

Done.

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:184:
EXPECT_TRUE([ns_window styleMask] & NSFullScreenWindowMask);
On 2014/05/15 06:39:20, tapted wrote:
> I think it's worth moving `IsFullscreen` out of the anonymous namespace in
> app_window.cc, and putting it as a private member function, then making this
> test a friend, and doing some extra checks on AppWindow::IsFullscreen

Added AppWindow::fullscreen_types_for_test like app_window_contents_for_test.

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:189:
app_window->Restore();
On 2014/05/15 06:39:20, tapted wrote:
> does window->IsFullscreenOrPending() change immediately, either side of this
> line? (i.e. without waiting for the notification?). It should!

Done.

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:198:
[watcher waitForNotification];
On 2014/05/15 06:39:20, tapted wrote:
> likewise window->IsFullscreenOrPending() should be false before this.

I take it you mean true? The comment on IsFullscreenOrPending says "whether the
window is fullscreen or about to enter fullscreen".

[tapted, 2014-05-15 07:44:40]

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

https://codereview.chromium.org/280483004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:198:
[watcher waitForNotification];
On 2014/05/15 07:15:29, jackhou1 wrote:
> On 2014/05/15 06:39:20, tapted wrote:
> > likewise window->IsFullscreenOrPending() should be false before this.
> 
> I take it you mean true? The comment on IsFullscreenOrPending says "whether
the
> window is fullscreen or about to enter fullscreen".

oops - yep.

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

https://codereview.chromium.org/280483004/diff/30001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:181:
EXPECT_FALSE(app_window->fullscreen_types_for_test());
nit: EXPECT_EQ(0, ..  (it will give a nicer log message if it ever fails)

[jackhou1, 2014-05-15 08:05:03]

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

https://codereview.chromium.org/280483004/diff/30001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa_browsertest.mm:181:
EXPECT_FALSE(app_window->fullscreen_types_for_test());
On 2014/05/15 07:44:40, tapted wrote:
> nit: EXPECT_EQ(0, ..  (it will give a nicer log message if it ever fails)

Done.

[tapted, 2014-05-15 08:06:28]

sweet! - lgtm

[jackhou1, 2014-05-15 08:07:16]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-05-15 08:08:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/280483004/50001

[commit-bot: I haz the power, 2014-05-15 11:32:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-15 14:00:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 14:00:47]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[tapted, 2014-05-16 00:27:07]

On 2014/05/15 14:00:47, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

Retrying for jack. The flake: http://crbug.com/351500

[tapted, 2014-05-16 00:27:13]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-05-16 00:27:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/280483004/50001

[commit-bot: I haz the power, 2014-05-16 02:29:38]

Change committed as 270904