SPM: Fix flaky AppWindowInteractiveTest.TestFullscreen

The test expects the bounds is synchronously set when the app
window contents is ready to be committed. This is not always
the case under single process mash because the window bounds
does not change synchronously when toggling fullscreen like in
the classic mode. The CL presets window bounds when fullscreen
state changes so that the bounds change happens synchronously.

Bug: 926007
Change-Id: If99e5946df1f9804c21202f075cbb1bf258f4bce
Reviewed-on: https://chromium-review.googlesource.com/c/1444431
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629172}

ash/public/cpp/BUILD.gn

@@ -235,6 +235,7 @@ source_set("unit_tests") {
 }
 
 source_set("test_support") {
+  testonly = true
   sources = [
     "immersive/immersive_fullscreen_controller_test_api.cc",
     "immersive/immersive_fullscreen_controller_test_api.h",
@@ -247,6 +248,7 @@ source_set("test_support") {
     "//base",
     "//services/service_manager/public/cpp",
     "//ui/aura",
+    "//ui/aura:test_support",
     "//ui/gfx",
     "//ui/keyboard:mojom",
     "//ui/views",

ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.cc

@@ -6,11 +6,44 @@
 
 #include "ash/public/cpp/immersive/immersive_fullscreen_controller.h"
 #include "ash/public/cpp/immersive/immersive_fullscreen_controller_delegate.h"
+#include "base/run_loop.h"
 #include "ui/aura/env.h"
+#include "ui/aura/event_injector.h"
 #include "ui/aura/window.h"
+#include "ui/events/base_event_utils.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/widget/widget.h"
 
+namespace {
+
+// Update mouse location for |window|. For local window, update its aura::Env
+// directly. For remote window, update aura::Env by injecting a mouse move
+// event. EventInjector is used so that the Window Service side code under mash
+// sees the updated mouse location as well.
+void UpdateMouseLocation(aura::Window* window,
+                         const gfx::Point& screen_location,
+                         bool wait) {
+  if (window->env()->mode() == aura::Env::Mode::LOCAL) {
+    window->env()->SetLastMouseLocation(screen_location);
+    return;
+  }
+
+  ui::MouseEvent event(ui::ET_MOUSE_MOVED, screen_location, screen_location,
+                       ui::EventTimeForNow(), ui::EF_NONE, 0);
+  if (!wait) {
+    aura::EventInjector().Inject(window->GetHost(), &event);
+    return;
+  }
+
+  // Ensure the mouse event goes through when |wait| is set.
+  aura::EventInjector event_injector;
+  base::RunLoop run_loop;
+  event_injector.Inject(window->GetHost(), &event, run_loop.QuitClosure());
+  run_loop.Run();
+}
+
+}  // namespace
+
 namespace ash {
 
 ImmersiveFullscreenControllerTestApi::ImmersiveFullscreenControllerTestApi(
@@ -20,7 +53,8 @@ ImmersiveFullscreenControllerTestApi::ImmersiveFullscreenControllerTestApi(
 ImmersiveFullscreenControllerTestApi::~ImmersiveFullscreenControllerTestApi() =
     default;
 
-void ImmersiveFullscreenControllerTestApi::SetupForTest() {
+void ImmersiveFullscreenControllerTestApi::SetupForTest(
+    bool wait_for_mouse_event) {
   immersive_fullscreen_controller_->animations_disabled_for_test_ = true;
 
   // Move the mouse off of the top-of-window views so that it does not keep the
@@ -34,10 +68,9 @@ void ImmersiveFullscreenControllerTestApi::SetupForTest() {
       bottommost_in_screen = bounds_in_screen[i].bottom();
   }
   gfx::Point cursor_pos(0, bottommost_in_screen + 10);
-  immersive_fullscreen_controller_->widget()
-      ->GetNativeView()
-      ->env()
-      ->SetLastMouseLocation(cursor_pos);
+  UpdateMouseLocation(
+      immersive_fullscreen_controller_->widget()->GetNativeView(), cursor_pos,
+      wait_for_mouse_event);
   immersive_fullscreen_controller_->UpdateLocatedEventRevealedLock();
 }
 

ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.h

@@ -30,8 +30,11 @@ class ImmersiveFullscreenControllerTestApi {
   };
 
   // Disables animations and moves the mouse so that it is not over the
-  // top-of-window views for the sake of testing.
-  void SetupForTest();
+  // top-of-window views for the sake of testing. |wait_for_mouse_move| should
+  // normally be true to wait for the generated mouse events to go through under
+  // mash. It is provided for tests that call SetupForTest under the scope of
+  // TestMockTimeTaskRunner::ScopedContext that does not allow RunLoop::Run().
+  void SetupForTest(bool wait_for_mouse_event = true);
 
   bool IsTopEdgeHoverTimerRunning() const;
 

chrome/browser/apps/platform_apps/app_window_interactive_uitest.cc

@@ -15,7 +15,6 @@
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
-#include "ui/base/ui_base_features.h"
 
 #if defined(OS_MACOSX)
 #include "base/mac/mac_util.h"
@@ -458,9 +457,6 @@ IN_PROC_BROWSER_TEST_F(AppWindowInteractiveTest, TestCreateHidden) {
 #define MAYBE_TestFullscreen TestFullscreen
 #endif
 IN_PROC_BROWSER_TEST_F(AppWindowInteractiveTest, MAYBE_TestFullscreen) {
-  // Flaky on CrOS + IsUsingWindowService. https://crbug.com/926007
-  if (features::IsUsingWindowService())
-    return;
   ASSERT_TRUE(RunAppWindowInteractiveTest("testFullscreen")) << message_;
 }
 

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

@@ -82,7 +82,6 @@
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/ws/public/mojom/window_tree_constants.mojom.h"
 #include "ui/aura/client/aura_constants.h"
-#include "ui/aura/event_injector.h"
 #include "ui/aura/test/env_test_helper.h"
 #include "ui/aura/test/mus/change_completion_waiter.h"
 #include "ui/base/class_property.h"
@@ -267,15 +266,6 @@ class ImmersiveModeTester : public ImmersiveModeController::Observer {
   DISALLOW_COPY_AND_ASSIGN(ImmersiveModeTester);
 };
 
-// Update mouse location of aura::Env by injecting a mouse move event.
-// EventInjector is used so that the Window Service side code under mash sees
-// the updated mouse location as well.
-void UpdateMouseLocation(aura::Window* window, const gfx::Point& location) {
-  ui::MouseEvent event(ui::ET_MOUSE_MOVED, location, location,
-                       ui::EventTimeForNow(), ui::EF_NONE, 0);
-  aura::EventInjector().Inject(window->GetHost(), &event);
-}
-
 }  // namespace
 
 using views::Widget;
@@ -448,10 +438,6 @@ class ImmersiveModeBrowserViewTest
   void PreRunTestOnMainThread() override {
     InProcessBrowserTest::PreRunTestOnMainThread();
 
-    // Move mouse cursor beyond immersive UI to avoid affecting tests.
-    UpdateMouseLocation(browser()->window()->GetNativeWindow(),
-                        gfx::Point(0, 100));
-
     BrowserView::SetDisableRevealerDelayForTesting(true);
 
     ash::ImmersiveFullscreenControllerTestApi(

chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc

@@ -63,11 +63,11 @@ class ImmersiveModeControllerAshHostedAppBrowserTest
 
   GURL GetAppUrl() { return https_server_.GetURL("/simple.html"); }
 
-  void LaunchAppBrowser(bool await_url_load = true) {
+  void LaunchAppBrowser(bool wait = true) {
     ui_test_utils::UrlLoadObserver url_observer(
         GetAppUrl(), content::NotificationService::AllSources());
     browser_ = ExtensionBrowserTest::LaunchAppBrowser(app_);
-    if (await_url_load) {
+    if (wait) {
       // Wait for the URL to load so that the location bar end-state stabilizes.
       url_observer.Wait();
     }
@@ -77,7 +77,7 @@ class ImmersiveModeControllerAshHostedAppBrowserTest
     // which triggers an animation.
     ash::ImmersiveFullscreenControllerTestApi(
         static_cast<ImmersiveModeControllerAsh*>(controller_)->controller())
-        .SetupForTest();
+        .SetupForTest(/*wait_for_mouse_event=*/wait);
 
     browser_->window()->Show();
   }

ui/aura/event_injector.cc

@@ -4,6 +4,9 @@
 
 #include "ui/aura/event_injector.h"
 
+#include <utility>
+
+#include "base/bind.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/ws/public/mojom/constants.mojom.h"
 #include "ui/aura/env.h"
@@ -15,19 +18,41 @@
 
 namespace aura {
 
+namespace {
+
+void RunCallback(base::OnceClosure callback, bool processed) {
+  if (!callback)
+    return;
+
+  std::move(callback).Run();
+}
+
+}  // namespace
+
 EventInjector::EventInjector() {}
 
-EventInjector::~EventInjector() {}
+EventInjector::~EventInjector() {
+  // |event_injector_| should not be waiting for responses. Otherwise, the
+  // pending callback would not happen because the mojo channel is closed.
+  DCHECK(!has_pending_callback_ || !event_injector_.IsExpectingResponse());
+}
 
 ui::EventDispatchDetails EventInjector::Inject(WindowTreeHost* host,
-                                               ui::Event* event) {
+                                               ui::Event* event,
+                                               base::OnceClosure callback) {
   DCHECK(host);
   Env* env = host->window()->env();
   DCHECK(env);
   DCHECK(event);
 
-  if (env->mode() == Env::Mode::LOCAL)
-    return host->event_sink()->OnEventFromSource(event);
+  if (env->mode() == Env::Mode::LOCAL) {
+    ui::EventDispatchDetails details =
+        host->event_sink()->OnEventFromSource(event);
+    RunCallback(std::move(callback), /*processed=*/true);
+    return details;
+  }
+
+  has_pending_callback_ |= !callback.is_null();
 
   if (event->IsLocatedEvent()) {
     // The ui-service expects events coming in to have a location matching the
@@ -42,8 +67,9 @@ ui::EventDispatchDetails EventInjector::Inject(WindowTreeHost* host,
     env->window_tree_client_->connector()->BindInterface(
         ws::mojom::kServiceName, &event_injector_);
   }
-  event_injector_->InjectEventNoAck(host->GetDisplayId(),
-                                    ui::Event::Clone(*event));
+  event_injector_->InjectEvent(
+      host->GetDisplayId(), ui::Event::Clone(*event),
+      base::BindOnce(&RunCallback, std::move(callback)));
   return ui::EventDispatchDetails();
 }
 

ui/aura/event_injector.h

@@ -5,6 +5,7 @@
 #ifndef UI_AURA_EVENT_INJECTOR_H_
 #define UI_AURA_EVENT_INJECTOR_H_
 
+#include "base/callback.h"
 #include "services/ws/public/mojom/event_injector.mojom.h"
 #include "ui/aura/aura_export.h"
 
@@ -25,10 +26,18 @@ class AURA_EXPORT EventInjector {
   EventInjector();
   ~EventInjector();
 
-  ui::EventDispatchDetails Inject(WindowTreeHost* host, ui::Event* event);
+  // Inject |event| to |host|. |callback| is optional that gets invoked after
+  // the event is processed by |host|. In LOCAL mode,  |callback| is invoked
+  // synchronously. In MUS mode, it is invoked after a response is received
+  // from window-server (via mojo).
+  ui::EventDispatchDetails Inject(
+      WindowTreeHost* host,
+      ui::Event* event,
+      base::OnceClosure callback = base::OnceClosure());
 
  private:
   ws::mojom::EventInjectorPtr event_injector_;
+  bool has_pending_callback_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(EventInjector);
 };

ui/views/mus/desktop_window_tree_host_mus.cc

@@ -848,7 +848,23 @@ void DesktopWindowTreeHostMus::SetFullscreen(bool fullscreen) {
   if (IsFullscreen() == fullscreen)
     return;  // Nothing to do.
 
+  // Retrieve restore bounds before leaving fullscreen.
+  gfx::Rect restore_bounds;
+  if (!fullscreen)
+    restore_bounds = GetRestoredBounds();
+
+  // Change the fullscreen state.
   wm::SetWindowFullscreen(window(), fullscreen);
+
+  // Preset bounds with heuristic size to provide synchronous bounds change
+  // after the switch to/from fullscreen.
+  if (fullscreen) {
+    window()->SetProperty(aura::client::kRestoreBoundsKey,
+                          new gfx::Rect(GetWindowBoundsInScreen()));
+    SetBoundsInDIP(GetDisplay().bounds());
+  } else {
+    SetBoundsInDIP(restore_bounds);
+  }
 }
 
 bool DesktopWindowTreeHostMus::IsFullscreen() const {

ui/views/mus/desktop_window_tree_host_mus.h

@@ -8,14 +8,14 @@
 #include <memory>
 #include <set>
 
-#include "base/scoped_observer.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "ui/aura/mus/focus_synchronizer_observer.h"
 #include "ui/aura/mus/window_tree_host_mus.h"
 #include "ui/aura/window_observer.h"
 #include "ui/views/mus/mus_client_observer.h"
-#include "ui/views/view_observer.h"
 #include "ui/views/mus/mus_export.h"
+#include "ui/views/view_observer.h"
 #include "ui/views/widget/desktop_aura/desktop_window_tree_host.h"
 #include "ui/views/widget/widget.h"
 

ui/views/mus/desktop_window_tree_host_mus_unittest.cc

@@ -430,6 +430,37 @@ TEST_F(DesktopWindowTreeHostMusTest, CreateFullscreenWidget) {
   }
 }
 
+TEST_F(DesktopWindowTreeHostMusTest, SynchronousBoundsWhenTogglingFullscreen) {
+  const gfx::Rect display_bounds =
+      display::Screen::GetScreen()->GetPrimaryDisplay().bounds();
+
+  const Widget::InitParams::Type kWidgetTypes[] = {
+      Widget::InitParams::TYPE_WINDOW,
+      Widget::InitParams::TYPE_WINDOW_FRAMELESS,
+  };
+
+  for (auto widget_type : kWidgetTypes) {
+    Widget widget;
+    Widget::InitParams params = CreateParams(widget_type);
+    params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    widget.Init(params);
+
+    const gfx::Rect restore_bounds = widget.GetWindowBoundsInScreen();
+
+    // Entering fullscreen synchronously set show state and bounds.
+    widget.SetFullscreen(true);
+    EXPECT_TRUE(widget.IsFullscreen())
+        << "Enter fullscreen failed for type=" << widget_type;
+    EXPECT_EQ(display_bounds, widget.GetWindowBoundsInScreen());
+
+    // Leaving fullscreen synchronously set show state and bounds.
+    widget.SetFullscreen(false);
+    EXPECT_FALSE(widget.IsFullscreen())
+        << "Leave fullscreen failed for type=" << widget_type;
+    EXPECT_EQ(restore_bounds, widget.GetWindowBoundsInScreen());
+  }
+}
+
 TEST_F(DesktopWindowTreeHostMusTest, ClientWindowHasContent) {
   // Opaque window has content.
   {