MacViews: Cater for the implied visibility change with -[NSWindow toggleFullScreen:]

Calling toggleFullScreen: implicitly makes the NSWindow visible. Failing
to cater for this causes tests to fail when layers are added.

AppKit updates window visibility quite late in the toggleFullScreen
flow. The best thing to do seems to be to just pre-empt the implied
visibility change and ensure the window is visible, before calling
toggleFullScreen:.

The fullscreen tests also get a lot flakier when layers enter the
picture. This is because tests are parallelised, and if two processes
simultaneously call toggleFullScreen:, then one of them fails. This CL
moves them to the macviews_interactive_ui_tests (which will soon be
absorbed into interactive_ui_tests).

BUG=424058
TBR=sky@chromium.org (gyp/gn)

Review URL: https://codereview.chromium.org/783043002

Cr-Commit-Position: refs/heads/master@{#307403}

5 files changed, 253 insertions, 187 deletions

diff --git a/ui/views/BUILD.gn b/ui/views/BUILD.gn
index 62aaff1..5896564 100644
--- a/ui/views/BUILD.gn
+++ b/ui/views/BUILD.gn
@@ -249,6 +249,7 @@ test("views_unittests") {
 if (is_mac) {
   test("macviews_interactive_ui_tests") {
     sources = [
+      "cocoa/bridged_native_widget_interactive_uitest.mm",
       "run_all_unittests.cc",
       "widget/native_widget_mac_interactive_uitest.mm",
     ]
diff --git a/ui/views/cocoa/bridged_native_widget.mm b/ui/views/cocoa/bridged_native_widget.mm
index 3cc7bb4..21aaaa9 100644
--- a/ui/views/cocoa/bridged_native_widget.mm
+++ b/ui/views/cocoa/bridged_native_widget.mm
@@ -180,6 +180,10 @@ void BridgedNativeWidget::OnWindowWillClose() {
 
 void BridgedNativeWidget::OnFullscreenTransitionStart(
     bool target_fullscreen_state) {
+  // Note: This can fail for fullscreen changes started externally, but a user
+  // shouldn't be able to do that if the window is invisible to begin with.
+  DCHECK(window_visible_);
+
   DCHECK_NE(target_fullscreen_state, target_fullscreen_state_);
   target_fullscreen_state_ = target_fullscreen_state;
   in_fullscreen_transition_ = true;
@@ -210,11 +214,6 @@ void BridgedNativeWidget::OnFullscreenTransitionComplete(
 }
 
 void BridgedNativeWidget::ToggleDesiredFullscreenState() {
-  if (base::mac::IsOSSnowLeopard()) {
-    NOTIMPLEMENTED();
-    return;  // TODO(tapted): Implement this for Snow Leopard.
-  }
-
   // If there is currently an animation into or out of fullscreen, then AppKit
   // emits the string "not in fullscreen state" to stdio and does nothing. For
   // this case, schedule a transition back into the desired state when the
@@ -224,6 +223,23 @@ void BridgedNativeWidget::ToggleDesiredFullscreenState() {
     return;
   }
 
+  // Going fullscreen implicitly makes the window visible. AppKit does this.
+  // That is, -[NSWindow isVisible] is always true after a call to -[NSWindow
+  // toggleFullScreen:]. Unfortunately, this change happens after AppKit calls
+  // -[NSWindowDelegate windowWillEnterFullScreen:], and AppKit doesn't send an
+  // orderWindow message. So intercepting the implicit change is hard.
+  // Luckily, to trigger externally, the window typically needs to be visible in
+  // the first place. So we can just ensure the window is visible here instead
+  // of relying on AppKit to do it, and not worry that OnVisibilityChanged()
+  // won't be called for externally triggered fullscreen requests.
+  if (!window_visible_)
+    SetVisibilityState(SHOW_INACTIVE);
+
+  if (base::mac::IsOSSnowLeopard()) {
+    NOTIMPLEMENTED();
+    return;  // TODO(tapted): Implement this for Snow Leopard.
+  }
+
   // Since fullscreen requests are ignored if the collection behavior does not
   // allow it, save the collection behavior and restore it after.
   NSWindowCollectionBehavior behavior = [window_ collectionBehavior];
diff --git a/ui/views/cocoa/bridged_native_widget_interactive_uitest.mm b/ui/views/cocoa/bridged_native_widget_interactive_uitest.mm
new file mode 100644
index 0000000..5911b35
--- /dev/null
+++ b/ui/views/cocoa/bridged_native_widget_interactive_uitest.mm
@@ -0,0 +1,230 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ui/views/cocoa/bridged_native_widget.h"
+
+#import <Cocoa/Cocoa.h>
+
+#import "base/mac/mac_util.h"
+#import "base/mac/sdk_forward_declarations.h"
+#include "base/run_loop.h"
+#include "ui/views/test/widget_test.h"
+
+@interface NativeWidgetMacNotificationWaiter : NSObject {
+ @private
+  scoped_ptr<base::RunLoop> runLoop_;
+  base::scoped_nsobject<NSWindow> window_;
+  int enterCount_;
+  int exitCount_;
+  int targetEnterCount_;
+  int targetExitCount_;
+}
+
+@property(readonly, nonatomic) int enterCount;
+@property(readonly, nonatomic) int exitCount;
+
+// Initialize for the given window and start tracking notifications.
+- (id)initWithWindow:(NSWindow*)window;
+
+// Keep spinning a run loop until the enter and exit counts match.
+- (void)waitForEnterCount:(int)enterCount exitCount:(int)exitCount;
+
+// private:
+// Exit the RunLoop if there is one and the counts being tracked match.
+- (void)maybeQuitForChangedArg:(int*)changedArg;
+
+- (void)onEnter:(NSNotification*)notification;
+- (void)onExit:(NSNotification*)notification;
+
+@end
+
+@implementation NativeWidgetMacNotificationWaiter
+
+@synthesize enterCount = enterCount_;
+@synthesize exitCount = exitCount_;
+
+- (id)initWithWindow:(NSWindow*)window {
+  if ((self = [super init])) {
+    window_.reset([window retain]);
+    NSNotificationCenter* defaultCenter = [NSNotificationCenter defaultCenter];
+    [defaultCenter addObserver:self
+                      selector:@selector(onEnter:)
+                          name:NSWindowDidEnterFullScreenNotification
+                        object:window];
+    [defaultCenter addObserver:self
+                      selector:@selector(onExit:)
+                          name:NSWindowDidExitFullScreenNotification
+                        object:window];
+  }
+  return self;
+}
+
+- (void)dealloc {
+  DCHECK(!runLoop_);
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
+- (void)waitForEnterCount:(int)enterCount exitCount:(int)exitCount {
+  if (enterCount_ >= enterCount && exitCount_ >= exitCount)
+    return;
+
+  targetEnterCount_ = enterCount;
+  targetExitCount_ = exitCount;
+  runLoop_.reset(new base::RunLoop);
+  runLoop_->Run();
+  runLoop_.reset();
+}
+
+- (void)maybeQuitForChangedArg:(int*)changedArg {
+  ++*changedArg;
+  if (!runLoop_)
+    return;
+
+  if (enterCount_ >= targetEnterCount_ && exitCount_ >= targetExitCount_)
+    runLoop_->Quit();
+}
+
+- (void)onEnter:(NSNotification*)notification {
+  [self maybeQuitForChangedArg:&enterCount_];
+}
+
+- (void)onExit:(NSNotification*)notification {
+  [self maybeQuitForChangedArg:&exitCount_];
+}
+
+@end
+
+namespace views {
+
+class BridgedNativeWidgetUITest : public test::WidgetTest {
+ public:
+  BridgedNativeWidgetUITest() {}
+
+  // testing::Test:
+  void SetUp() override {
+    WidgetTest::SetUp();
+    Widget::InitParams init_params =
+        CreateParams(Widget::InitParams::TYPE_WINDOW);
+    init_params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    widget_.reset(new Widget);
+    widget_->Init(init_params);
+  }
+
+  void TearDown() override {
+    // Ensures any compositor is removed before ViewsTestBase tears down the
+    // ContextFactory.
+    widget_.reset();
+    WidgetTest::TearDown();
+  }
+
+  NSWindow* test_window() {
+    return widget_->GetNativeWindow();
+  }
+
+ protected:
+  scoped_ptr<Widget> widget_;
+};
+
+// Tests for correct fullscreen tracking, regardless of whether it is initiated
+// by the Widget code or elsewhere (e.g. by the user).
+TEST_F(BridgedNativeWidgetUITest, FullscreenSynchronousState) {
+  EXPECT_FALSE(widget_->IsFullscreen());
+  if (base::mac::IsOSSnowLeopard())
+    return;
+
+  // Allow user-initiated fullscreen changes on the Window.
+  [test_window()
+      setCollectionBehavior:[test_window() collectionBehavior] |
+                            NSWindowCollectionBehaviorFullScreenPrimary];
+
+  base::scoped_nsobject<NativeWidgetMacNotificationWaiter> waiter(
+      [[NativeWidgetMacNotificationWaiter alloc] initWithWindow:test_window()]);
+  const gfx::Rect restored_bounds = widget_->GetRestoredBounds();
+
+  // First show the widget. A user shouldn't be able to initiate fullscreen
+  // unless the window is visible in the first place.
+  widget_->Show();
+
+  // Simulate a user-initiated fullscreen. Note trying to to this again before
+  // spinning a runloop will cause Cocoa to emit text to stdio and ignore it.
+  [test_window() toggleFullScreen:nil];
+  EXPECT_TRUE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  // Note there's now an animation running. While that's happening, toggling the
+  // state should work as expected, but do "nothing".
+  widget_->SetFullscreen(false);
+  EXPECT_FALSE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+  widget_->SetFullscreen(false);  // Same request - should no-op.
+  EXPECT_FALSE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  widget_->SetFullscreen(true);
+  EXPECT_TRUE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  // Always finish out of fullscreen. Otherwise there are 4 NSWindow objects
+  // that Cocoa creates which don't close themselves and will be seen by the Mac
+  // test harness on teardown. Note that the test harness will be waiting until
+  // all animations complete, since these temporary animation windows will not
+  // be removed from the window list until they do.
+  widget_->SetFullscreen(false);
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  // Now we must wait for the notifications. Since, if the widget is torn down,
+  // the NSWindowDelegate is removed, and the pending request to take out of
+  // fullscreen is lost. Since a message loop has not yet spun up in this test
+  // we can reliably say there will be one enter and one exit, despite all the
+  // toggling above.
+  [waiter waitForEnterCount:1 exitCount:1];
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+}
+
+// Test fullscreen without overlapping calls and without changing collection
+// behavior on the test window.
+TEST_F(BridgedNativeWidgetUITest, FullscreenEnterAndExit) {
+  base::scoped_nsobject<NativeWidgetMacNotificationWaiter> waiter(
+      [[NativeWidgetMacNotificationWaiter alloc] initWithWindow:test_window()]);
+
+  EXPECT_FALSE(widget_->IsFullscreen());
+  const gfx::Rect restored_bounds = widget_->GetRestoredBounds();
+  EXPECT_FALSE(restored_bounds.IsEmpty());
+
+  // Ensure this works without having to change collection behavior as for the
+  // test above. Also check that making a hidden widget fullscreen shows it.
+  EXPECT_FALSE(widget_->IsVisible());
+  widget_->SetFullscreen(true);
+  EXPECT_TRUE(widget_->IsVisible());
+  if (base::mac::IsOSSnowLeopard()) {
+    // On Snow Leopard, SetFullscreen() isn't implemented. But shouldn't crash.
+    EXPECT_FALSE(widget_->IsFullscreen());
+    return;
+  }
+
+  EXPECT_TRUE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  // Should be zero until the runloop spins.
+  EXPECT_EQ(0, [waiter enterCount]);
+  [waiter waitForEnterCount:1 exitCount:0];
+
+  // Verify it hasn't exceeded.
+  EXPECT_EQ(1, [waiter enterCount]);
+  EXPECT_EQ(0, [waiter exitCount]);
+  EXPECT_TRUE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  widget_->SetFullscreen(false);
+  EXPECT_FALSE(widget_->IsFullscreen());
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+
+  [waiter waitForEnterCount:1 exitCount:1];
+  EXPECT_EQ(1, [waiter enterCount]);
+  EXPECT_EQ(1, [waiter exitCount]);
+  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
+}
+
+}  // namespace views
diff --git a/ui/views/cocoa/bridged_native_widget_unittest.mm b/ui/views/cocoa/bridged_native_widget_unittest.mm
index 41a97f5..f3720d5 100644
--- a/ui/views/cocoa/bridged_native_widget_unittest.mm
+++ b/ui/views/cocoa/bridged_native_widget_unittest.mm
@@ -11,7 +11,6 @@
 #import "base/mac/sdk_forward_declarations.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
-#include "base/run_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #import "testing/gtest_mac.h"
@@ -44,91 +43,6 @@ NSRange EmptyRange() {
 
 }  // namespace
 
-@interface NativeWidgetMacNotificationWaiter : NSObject {
- @private
-  scoped_ptr<base::RunLoop> runLoop_;
-  base::scoped_nsobject<NSWindow> window_;
-  int enterCount_;
-  int exitCount_;
-  int targetEnterCount_;
-  int targetExitCount_;
-}
-
-@property(readonly, nonatomic) int enterCount;
-@property(readonly, nonatomic) int exitCount;
-
-// Initialize for the given window and start tracking notifications.
-- (id)initWithWindow:(NSWindow*)window;
-
-// Keep spinning a run loop until the enter and exit counts match.
-- (void)waitForEnterCount:(int)enterCount exitCount:(int)exitCount;
-
-// private:
-// Exit the RunLoop if there is one and the counts being tracked match.
-- (void)maybeQuitForChangedArg:(int*)changedArg;
-
-- (void)onEnter:(NSNotification*)notification;
-- (void)onExit:(NSNotification*)notification;
-
-@end
-
-@implementation NativeWidgetMacNotificationWaiter
-
-@synthesize enterCount = enterCount_;
-@synthesize exitCount = exitCount_;
-
-- (id)initWithWindow:(NSWindow*)window {
-  if ((self = [super init])) {
-    window_.reset([window retain]);
-    NSNotificationCenter* defaultCenter = [NSNotificationCenter defaultCenter];
-    [defaultCenter addObserver:self
-                      selector:@selector(onEnter:)
-                          name:NSWindowDidEnterFullScreenNotification
-                        object:window];
-    [defaultCenter addObserver:self
-                      selector:@selector(onExit:)
-                          name:NSWindowDidExitFullScreenNotification
-                        object:window];
-  }
-  return self;
-}
-
-- (void)dealloc {
-  DCHECK(!runLoop_);
-  [[NSNotificationCenter defaultCenter] removeObserver:self];
-  [super dealloc];
-}
-
-- (void)waitForEnterCount:(int)enterCount exitCount:(int)exitCount {
-  if (enterCount_ >= enterCount && exitCount_ >= exitCount)
-    return;
-
-  targetEnterCount_ = enterCount;
-  targetExitCount_ = exitCount;
-  runLoop_.reset(new base::RunLoop);
-  runLoop_->Run();
-  runLoop_.reset();
-}
-
-- (void)maybeQuitForChangedArg:(int*)changedArg {
-  ++*changedArg;
-  if (!runLoop_)
-    return;
-
-  if (enterCount_ >= targetEnterCount_ && exitCount_ >= targetExitCount_)
-    runLoop_->Quit();
-}
-
-- (void)onEnter:(NSNotification*)notification {
-  [self maybeQuitForChangedArg:&enterCount_];
-}
-
-- (void)onExit:(NSNotification*)notification {
-  [self maybeQuitForChangedArg:&exitCount_];
-}
-
-@end
-
 // Class to override -[NSWindow toggleFullScreen:] to a no-op. This simulates
 // NSWindow's behavior when attempting to toggle fullscreen state again, when
 // the last attempt failed but Cocoa has not yet sent
@@ -508,102 +422,6 @@ TEST_F(BridgedNativeWidgetTest, TextInput_DeleteForward) {
   EXPECT_EQ_RANGE(NSMakeRange(0, 0), [ns_view_ selectedRange]);
 }
 
-// Tests for correct fullscreen tracking, regardless of whether it is initiated
-// by the Widget code or elsewhere (e.g. by the user).
-TEST_F(BridgedNativeWidgetTest, FullscreenSynchronousState) {
-  EXPECT_FALSE(widget_->IsFullscreen());
-  if (base::mac::IsOSSnowLeopard())
-    return;
-
-  // Allow user-initiated fullscreen changes on the Window.
-  [test_window()
-      setCollectionBehavior:[test_window() collectionBehavior] |
-                            NSWindowCollectionBehaviorFullScreenPrimary];
-
-  base::scoped_nsobject<NativeWidgetMacNotificationWaiter> waiter(
-      [[NativeWidgetMacNotificationWaiter alloc] initWithWindow:test_window()]);
-  const gfx::Rect restored_bounds = widget_->GetRestoredBounds();
-
-  // Simulate a user-initiated fullscreen. Note trying to to this again before
-  // spinning a runloop will cause Cocoa to emit text to stdio and ignore it.
-  [test_window() toggleFullScreen:nil];
-  EXPECT_TRUE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  // Note there's now an animation running. While that's happening, toggling the
-  // state should work as expected, but do "nothing".
-  widget_->SetFullscreen(false);
-  EXPECT_FALSE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-  widget_->SetFullscreen(false);  // Same request - should no-op.
-  EXPECT_FALSE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  widget_->SetFullscreen(true);
-  EXPECT_TRUE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  // Always finish out of fullscreen. Otherwise there are 4 NSWindow objects
-  // that Cocoa creates which don't close themselves and will be seen by the Mac
-  // test harness on teardown. Note that the test harness will be waiting until
-  // all animations complete, since these temporary animation windows will not
-  // be removed from the window list until they do.
-  widget_->SetFullscreen(false);
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  // Now we must wait for the notifications. Since, if the widget is torn down,
-  // the NSWindowDelegate is removed, and the pending request to take out of
-  // fullscreen is lost. Since a message loop has not yet spun up in this test
-  // we can reliably say there will be one enter and one exit, despite all the
-  // toggling above.
-  base::MessageLoopForUI message_loop;
-  [waiter waitForEnterCount:1 exitCount:1];
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-}
-
-// Test fullscreen without overlapping calls and without changing collection
-// behavior on the test window.
-TEST_F(BridgedNativeWidgetTest, FullscreenEnterAndExit) {
-  base::MessageLoopForUI message_loop;
-  base::scoped_nsobject<NativeWidgetMacNotificationWaiter> waiter(
-      [[NativeWidgetMacNotificationWaiter alloc] initWithWindow:test_window()]);
-
-  EXPECT_FALSE(widget_->IsFullscreen());
-  const gfx::Rect restored_bounds = widget_->GetRestoredBounds();
-  EXPECT_FALSE(restored_bounds.IsEmpty());
-
-  // Ensure this works without having to change collection behavior as for the
-  // test above.
-  widget_->SetFullscreen(true);
-  if (base::mac::IsOSSnowLeopard()) {
-    // On Snow Leopard, SetFullscreen() isn't implemented. But shouldn't crash.
-    EXPECT_FALSE(widget_->IsFullscreen());
-    return;
-  }
-
-  EXPECT_TRUE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  // Should be zero until the runloop spins.
-  EXPECT_EQ(0, [waiter enterCount]);
-  [waiter waitForEnterCount:1 exitCount:0];
-
-  // Verify it hasn't exceeded.
-  EXPECT_EQ(1, [waiter enterCount]);
-  EXPECT_EQ(0, [waiter exitCount]);
-  EXPECT_TRUE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  widget_->SetFullscreen(false);
-  EXPECT_FALSE(widget_->IsFullscreen());
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-
-  [waiter waitForEnterCount:1 exitCount:1];
-  EXPECT_EQ(1, [waiter enterCount]);
-  EXPECT_EQ(1, [waiter exitCount]);
-  EXPECT_EQ(restored_bounds, widget_->GetRestoredBounds());
-}
-
 typedef BridgedNativeWidgetTestBase BridgedNativeWidgetSimulateFullscreenTest;
 
 // Simulate the notifications that AppKit would send out if a fullscreen
diff --git a/ui/views/views.gyp b/ui/views/views.gyp
index 8d32c84..7a2e13d 100644
--- a/ui/views/views.gyp
+++ b/ui/views/views.gyp
@@ -880,6 +880,7 @@
             'views_test_support',
           ],
           'sources': [
+            'cocoa/bridged_native_widget_interactive_uitest.mm',
             'run_all_unittests.cc',
             'widget/native_widget_mac_interactive_uitest.mm',
           ],