Reland: Let ash provide window show state if client has SHOW_STATE_DEFAULT

For lock screen app windows the window manager needs to specify the
initial show state of the window. Ensure the client (chrome) does not
overwrite the aura window show-state property in ash if the client
window has ui::SHOW_STATE_DEFAULT.

Fixes LockScreenNoteTakingTest.Launch pass under SingleProcessMash

The reland changes FirstAppRunToastManager to use a WidgetObserver
to watch for app window bounds changes. Mash tears down aura::Windows
similarly to aura on MS Windows and Linux. The aura::Window for
the app window can be destroyed before AppWindowRegistry notifies
StateController and FirstAppRunToastManager that the app has been
closed, so it's not safe for FirstAppRunToastManager::Reset() to
manipulate the aura::Window to remove the ScopedObserver. See bug.

TBR=sky@chromium.org
TBR=tsepez@chromium.org

Bug: 899055
Test: added to views_mus_unittests
Change-Id: I886dbe45fe53708beb4441c1d9da2587a2352997
Reviewed-on: https://chromium-review.googlesource.com/c/1312972Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605110}

chrome/browser/chromeos/lock_screen_apps/first_app_run_toast_manager.cc

@@ -30,11 +30,42 @@ constexpr int kToastDialogVerticalOffset = 20;
 
 }  // namespace
 
+// Observes the note taking app widget so bounds changes can update the toast
+// position.
+class FirstAppRunToastManager::AppWidgetObserver
+    : public views::WidgetObserver {
+ public:
+  AppWidgetObserver(FirstAppRunToastManager* manager, views::Widget* widget)
+      : manager_(manager), widget_(widget) {
+    widget_->AddObserver(this);
+  }
+
+  ~AppWidgetObserver() override {
+    // This is a no-op of the observer was previously removed.
+    widget_->RemoveObserver(this);
+  }
+
+  // views::WidgetObserver:
+  void OnWidgetBoundsChanged(views::Widget* widget,
+                             const gfx::Rect& new_bounds) override {
+    manager_->AdjustToastWidgetBounds();
+  }
+
+  void OnWidgetDestroying(views::Widget* widget) override {
+    widget_->RemoveObserver(this);
+  }
+
+ private:
+  FirstAppRunToastManager* manager_;
+  views::Widget* widget_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppWidgetObserver);
+};
+
 FirstAppRunToastManager::FirstAppRunToastManager(Profile* profile)
     : profile_(profile),
       toast_widget_observer_(this),
       app_window_observer_(this),
-      native_app_window_observer_(this),
       weak_ptr_factory_(this) {}
 
 FirstAppRunToastManager::~FirstAppRunToastManager() {
@@ -59,7 +90,10 @@ void FirstAppRunToastManager::RunForAppWindow(
   }
 
   app_window_ = app_window;
-  native_app_window_observer_.Add(app_window_->GetNativeWindow());
+  views::Widget* app_widget =
+      views::Widget::GetWidgetForNativeWindow(app_window_->GetNativeWindow());
+  DCHECK(app_widget);
+  app_widget_observer_ = std::make_unique<AppWidgetObserver>(this, app_widget);
 
   if (app_window_->GetNativeWindow()->HasFocus()) {
     CreateAndShowToastDialog();
@@ -70,7 +104,7 @@ void FirstAppRunToastManager::RunForAppWindow(
 }
 
 void FirstAppRunToastManager::Reset() {
-  native_app_window_observer_.RemoveAll();
+  app_widget_observer_.reset();
   app_window_observer_.RemoveAll();
   toast_widget_observer_.RemoveAll();
 
@@ -101,15 +135,6 @@ void FirstAppRunToastManager::OnAppWindowActivated(
   }
 }
 
-void FirstAppRunToastManager::OnWindowBoundsChanged(
-    aura::Window* window,
-    const gfx::Rect& old_bounds,
-    const gfx::Rect& new_bounds,
-    ui::PropertyChangeReason reason) {
-  if (toast_widget_ && window == app_window_->GetNativeWindow())
-    AdjustToastWidgetBounds();
-}
-
 void FirstAppRunToastManager::CreateAndShowToastDialog() {
   auto* toast_dialog = new ToastDialogView(
       base::UTF8ToUTF16(app_window_->GetExtension()->short_name()),
@@ -132,7 +157,9 @@ void FirstAppRunToastManager::ToastDialogDismissed() {
 }
 
 void FirstAppRunToastManager::AdjustToastWidgetBounds() {
-  DCHECK(toast_widget_);
+  if (!toast_widget_)
+    return;
+
   DCHECK(app_window_);
 
   const gfx::Rect app_window_bounds =

chrome/browser/chromeos/lock_screen_apps/first_app_run_toast_manager.h

@@ -28,7 +28,6 @@ namespace lock_screen_apps {
 // dialog informing the user about the app that's been launched from the lock
 // screen.
 class FirstAppRunToastManager : public extensions::AppWindowRegistry::Observer,
-                                public aura::WindowObserver,
                                 public views::WidgetObserver {
  public:
   explicit FirstAppRunToastManager(Profile* profile);
@@ -48,12 +47,6 @@ class FirstAppRunToastManager : public extensions::AppWindowRegistry::Observer,
   // views::WidgetObserver:
   void OnWidgetDestroyed(views::Widget* widget) override;
 
-  // aura::WindowObserver:
-  void OnWindowBoundsChanged(aura::Window* window,
-                             const gfx::Rect& old_bounds,
-                             const gfx::Rect& new_bounds,
-                             ui::PropertyChangeReason reason) override;
-
   // extensions::AppWindowRegistry::Observer:
   void OnAppWindowActivated(extensions::AppWindow* app_window) override;
 
@@ -88,8 +81,9 @@ class FirstAppRunToastManager : public extensions::AppWindowRegistry::Observer,
   ScopedObserver<extensions::AppWindowRegistry,
                  extensions::AppWindowRegistry::Observer>
       app_window_observer_;
-  ScopedObserver<aura::Window, aura::WindowObserver>
-      native_app_window_observer_;
+
+  class AppWidgetObserver;
+  std::unique_ptr<AppWidgetObserver> app_widget_observer_;
 
   base::WeakPtrFactory<FirstAppRunToastManager> weak_ptr_factory_;
 

chrome/browser/chromeos/lock_screen_apps/note_taking_browsertest.cc

@@ -17,10 +17,13 @@
 #include "chromeos/chromeos_switches.h"
 #include "components/prefs/pref_service.h"
 #include "components/session_manager/core/session_manager.h"
+#include "extensions/browser/app_window/app_window.h"
+#include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/common/api/app_runtime.h"
 #include "extensions/common/switches.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
+#include "ui/aura/test/mus/change_completion_waiter.h"
 
 namespace {
 
@@ -146,6 +149,20 @@ class LockScreenNoteTakingTest : public extensions::ExtensionBrowserTest {
       return false;
     }
 
+    // By this point the app window is created on the lock screen. Ensure the
+    // asynchronous window maximize from ash completes.
+    aura::test::WaitForAllChangesToComplete();
+    extensions::AppWindow* app_window =
+        lock_screen_apps::StateController::Get()->note_app_window_for_test();
+    if (!app_window) {
+      *error = "No app window";
+      return false;
+    }
+    if (!app_window->GetBaseWindow()->IsMaximized()) {
+      *error = "App window not maximized";
+      return false;
+    }
+
     // Close the app window created by the API test.
     ready_to_close.Reply("close");
 

chrome/browser/chromeos/lock_screen_apps/state_controller.h

@@ -108,6 +108,7 @@ class StateController : public ash::mojom::TrayActionClient,
   // |SetPrimaryProfile|
   void SetLockScreenLockScreenProfileCreatorForTesting(
       std::unique_ptr<LockScreenProfileCreator> profile_creator);
+  extensions::AppWindow* note_app_window_for_test() { return note_app_window_; }
 
   // Initializes mojo bindings for the StateController - it creates binding to
   // ash's tray action interface and sets this object as the interface's client.

chrome/test/data/extensions/lock_screen_apps/app_launch/test.js

@@ -11,7 +11,6 @@ chrome.test.runTests([
   function hasAccessToCurrentWindow() {
     chrome.test.assertTrue(!!chrome.app.window.current);
     chrome.test.assertTrue(!!chrome.app.window.current());
-    chrome.test.assertTrue(chrome.app.window.current().isMaximized());
     chrome.test.succeed();
   },
 

services/ws/test_ws/test_window_service.cc

@@ -16,6 +16,7 @@
 #include "services/ws/test_host_event_dispatcher.h"
 #include "services/ws/test_ws/test_gpu_interface_provider.h"
 #include "services/ws/window_service.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/env.h"
 #include "ui/aura/mus/property_utils.h"
 #include "ui/compositor/test/context_factories_for_test.h"
@@ -74,6 +75,11 @@ std::unique_ptr<aura::Window> TestWindowService::NewTopLevel(
     property_converter->SetPropertyFromTransportValue(
         top_level.get(), property.first, &property.second);
   }
+  if (maximize_next_window_) {
+    top_level->SetProperty(aura::client::kShowStateKey,
+                           ui::SHOW_STATE_MAXIMIZED);
+    maximize_next_window_ = false;
+  }
   return top_level;
 }
 
@@ -164,6 +170,11 @@ void TestWindowService::OnGpuServiceInitialized() {
     std::move(pending_create_service_).Run();
 }
 
+void TestWindowService::MaximizeNextWindow(MaximizeNextWindowCallback cb) {
+  maximize_next_window_ = true;
+  std::move(cb).Run();
+}
+
 void TestWindowService::Shutdown(
     test_ws::mojom::TestWs::ShutdownCallback callback) {
   // WindowService depends upon Screen, which is owned by AuraTestHelper.

services/ws/test_ws/test_window_service.h

@@ -91,6 +91,7 @@ class TestWindowService : public service_manager::Service,
   void OnGpuServiceInitialized() override;
 
   // test_ws::mojom::TestWs:
+  void MaximizeNextWindow(MaximizeNextWindowCallback cb) override;
   void Shutdown(test_ws::mojom::TestWs::ShutdownCallback callback) override;
 
   void BindServiceFactory(
@@ -127,6 +128,7 @@ class TestWindowService : public service_manager::Service,
 
   bool started_ = false;
   bool ui_service_created_ = false;
+  bool maximize_next_window_ = false;
 
   base::OnceClosure pending_create_service_;
 

services/ws/test_ws/test_ws.mojom

@@ -8,6 +8,10 @@ const string kServiceName = "test_ws";
 
 // Implemented by TestWindowService.
 interface TestWs {
+  // Requests that the window service create the next top-level window with
+  // show state maximized.
+  MaximizeNextWindow() => ();
+
   // Used when caller needs to explicitly shutdown the window service hosted
   // in test_ws. Callback is provided so that caller can resume its shutdown
   // sequence.

testing/buildbot/filters/chromeos.mash.fyi.browser_tests.filter

@@ -126,9 +126,6 @@
 -LauncherPlatformAppBrowserTest.PanelAttentionStatus
 -LauncherPlatformAppBrowserTest.PanelItemClickBehavior
 
-# JS failure: hasAccessToCurrentWindow: FAIL (no message)
--LockScreenNoteTakingTest.*
-
 # Missing magnification manager and also RefCounted check failed:
 # CalledOnValidSequence() from SchedulerWorkerDelegate::OnMainExit
 -LoginScreenDefaultPolicyInSessionBrowsertest.*

testing/buildbot/filters/chromeos.single_process_mash.browser_tests.filter

@@ -31,9 +31,6 @@
 # Viz hit testing not supported with Mash https://crbug.com/879308
 -PDFExtensionHitTestTest.MouseLeave/1
 
-# https://crbug.com/899055
--LockScreenNoteTakingTest.*
-
 # Flaky. https://crbug.com/888188.
 -ChromeSessionManagerTest.LoginExistingUsers
 -ChromeSessionManagerTest.PRE_LoginExistingUsers

ui/base/class_property.cc

@@ -67,4 +67,4 @@ std::set<const void*> PropertyHandler::GetAllPropertyKeys() const {
   return keys;
 }
 
-} // namespace ui
\ No newline at end of file
+}  // namespace ui

ui/views/mus/BUILD.gn

@@ -169,6 +169,7 @@ test("views_mus_unittests") {
     "//cc",
     "//net",
     "//services/ws/public/mojom",
+    "//services/ws/test_ws:mojom",
     "//skia",
     "//testing/gtest",
     "//third_party/icu",

ui/views/mus/desktop_window_tree_host_mus.cc

@@ -416,7 +416,10 @@ void DesktopWindowTreeHostMus::Init(const Widget::InitParams& params) {
   content_window()->SetTransparent(translucent);
   window()->SetTransparent(translucent);
 
-  window()->SetProperty(aura::client::kShowStateKey, params.show_state);
+  // The window manager may provide the initial show state, for example for
+  // Chrome OS lock screen windows. https://crbug.com/899055
+  if (params.show_state != ui::SHOW_STATE_DEFAULT)
+    window()->SetProperty(aura::client::kShowStateKey, params.show_state);
 
   if (!params.bounds.IsEmpty())
     SetBoundsInDIP(params.bounds);

ui/views/mus/desktop_window_tree_host_mus_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
+#include "services/ws/test_ws/test_ws.mojom.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/cursor_client.h"
 #include "ui/aura/client/focus_client.h"
@@ -102,6 +103,29 @@ class ExpectsNullCursorClientDuringTearDown : public aura::WindowObserver {
   DISALLOW_COPY_AND_ASSIGN(ExpectsNullCursorClientDuringTearDown);
 };
 
+// Tests that the window service can set the initial show state for a window.
+// https://crbug.com/899055
+TEST_F(DesktopWindowTreeHostMusTest, ShowStateFromWindowService) {
+  // Configure the window service to maximize the next top-level window.
+  test_ws::mojom::TestWsPtr test_ws_ptr;
+  MusClient::Get()->window_tree_client()->connector()->BindInterface(
+      test_ws::mojom::kServiceName, &test_ws_ptr);
+  test_ws::mojom::TestWsAsyncWaiter wait_for(test_ws_ptr.get());
+  wait_for.MaximizeNextWindow();
+
+  // Create a widget with the default show state.
+  Widget widget;
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.bounds = gfx::Rect(0, 0, 100, 100);
+  EXPECT_EQ(ui::SHOW_STATE_DEFAULT, params.show_state);
+  widget.Init(params);
+  aura::test::WaitForAllChangesToComplete();
+
+  // Window service provided the show state.
+  EXPECT_TRUE(widget.IsMaximized());
+}
+
 TEST_F(DesktopWindowTreeHostMusTest, Visibility) {
   std::unique_ptr<Widget> widget(CreateWidget());
   EXPECT_FALSE(widget->IsVisible());