[mash] Auto-position browser windows (just like in classic Ash)

The ash::kWindowPositionManagedTypeKey was inverted in Mash compared
to classic Ash (see BrowserFrameAsh::SetWindowAutoManaged()).

This fixes BrowserTestParam.TabbedOrAppBrowserWindowAutoManagementTest,
largely by updating that test
- to deal with the asynchronicity of mash
- to use Widget and BrowserWindow interfaces instead of NativeWindow
  when dealing with bounds (more closely matches production code)
- to appropriately test handling of initial widget bounds

Change-Id: I68914c360908973a27d432248316ff12243ae932
Reviewed-on: https://chromium-review.googlesource.com/1033489
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555641}

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

@@ -4,15 +4,50 @@
 
 #include "chrome/browser/ui/views/frame/browser_frame_ash.h"
 
+#include "chrome/browser/chromeos/ash_config.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/test/base/in_process_browser_test.h"
-#include "ui/aura/window.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace {
 
+// Waits for the given widget's bounds to change to the given rect.
+class WidgetBoundsWatcher : public views::WidgetObserver {
+ public:
+  WidgetBoundsWatcher(views::Widget* widget, const gfx::Rect& target_bounds)
+      : widget_(widget), target_bounds_(target_bounds) {}
+
+  ~WidgetBoundsWatcher() override {}
+
+  void Wait() {
+    // Unconditionally wait for the widget bounds to update, even if the bounds
+    // already match |target_bounds_|. Otherwise multiple values for
+    // |target_bounds_| will pass the test (either the current or re-positioned
+    // bounds would be accepted).
+    widget_->AddObserver(this);
+    run_loop_.Run();
+  }
+
+ private:
+  void OnWidgetBoundsChanged(views::Widget* widget,
+                             const gfx::Rect& new_bounds) override {
+    if (new_bounds == target_bounds_) {
+      run_loop_.Quit();
+      widget_->RemoveObserver(this);
+    }
+  }
+
+  views::Widget* widget_;
+  const gfx::Rect target_bounds_;
+  base::RunLoop run_loop_;
+  DISALLOW_COPY_AND_ASSIGN(WidgetBoundsWatcher);
+};
+
 class BrowserTestParam : public InProcessBrowserTest,
                          public testing::WithParamInterface<bool> {
  public:
@@ -35,43 +70,58 @@ IN_PROC_BROWSER_TEST_P(BrowserTestParam,
   browser()->window()->Close();
 
   // Open a new browser window (app or tabbed depending on a parameter).
-  bool test_app = CreateV1App();
+  bool is_test_app = CreateV1App();
   Browser::CreateParams params =
-      test_app ? Browser::CreateParams::CreateForApp(
-                     "test_browser_app", true /* trusted_source */, gfx::Rect(),
-                     browser()->profile(), true)
-               : Browser::CreateParams(browser()->profile(), true);
-  params.initial_show_state = ui::SHOW_STATE_DEFAULT;
-  Browser* browser = new Browser(params);
-  gfx::NativeWindow window = browser->window()->GetNativeWindow();
+      is_test_app ? Browser::CreateParams::CreateForApp(
+                        "test_browser_app", true /* trusted_source */,
+                        gfx::Rect(), browser()->profile(), true)
+                  : Browser::CreateParams(browser()->profile(), true);
   gfx::Rect original_bounds(gfx::Rect(150, 250, 400, 100));
-  window->SetBounds(original_bounds);
-  window->Show();
+  params.initial_show_state = ui::SHOW_STATE_NORMAL;
+  params.initial_bounds = original_bounds;
+  Browser* browser = new Browser(params);
+  browser->window()->Show();
 
-  // For tabbed browser window, it will be centered to work area by auto window
-  // mangement logic; for app browser window, it will remain the given bounds.
-  gfx::Rect work_area = display::Screen::GetScreen()
-                            ->GetDisplayNearestPoint(window->bounds().origin())
-                            .work_area();
-  gfx::Rect tabbed_expected_bounds = work_area;
-  tabbed_expected_bounds.ClampToCenteredSize(original_bounds.size());
-  tabbed_expected_bounds.set_y(original_bounds.y());
-  if (test_app)
-    EXPECT_EQ(original_bounds, window->bounds());
-  else
-    EXPECT_EQ(tabbed_expected_bounds, window->bounds());
+  if (chromeos::GetAshConfig() == ash::Config::MASH) {
+    WidgetBoundsWatcher watch(
+        BrowserView::GetBrowserViewForBrowser(browser)->GetWidget(),
+        original_bounds);
+    watch.Wait();
+  }
+
+  // The bounds passed via |initial_bounds| should be respected regardless of
+  // the window type.
+  EXPECT_EQ(original_bounds, browser->window()->GetBounds());
 
   // Close the browser and re-create the browser window with the same app name.
+  // Don't provide initial bounds. The bounds should have been saved, but for
+  // tabbed windows, the position should be auto-managed.
   browser->window()->Close();
+  params.initial_bounds = gfx::Rect();
   browser = new Browser(params);
   browser->window()->Show();
 
-  // The newly created browser window should remain the same bounds for each.
-  window = browser->window()->GetNativeWindow();
-  if (test_app)
-    EXPECT_EQ(original_bounds, window->bounds());
-  else
-    EXPECT_EQ(tabbed_expected_bounds, window->bounds());
+  // For tabbed browser window, it will be centered to work area by auto window
+  // management logic; for app browser window, it will remain the given bounds.
+  gfx::Rect expectation = original_bounds;
+  if (!is_test_app) {
+    expectation =
+        display::Screen::GetScreen()
+            ->GetDisplayNearestPoint(browser->window()->GetBounds().origin())
+            .work_area();
+    expectation.ClampToCenteredSize(original_bounds.size());
+    expectation.set_y(original_bounds.y());
+  }
+
+  if (chromeos::GetAshConfig() == ash::Config::MASH) {
+    WidgetBoundsWatcher watch(
+        BrowserView::GetBrowserViewForBrowser(browser)->GetWidget(),
+        expectation);
+    watch.Wait();
+  }
+
+  EXPECT_EQ(expectation, browser->window()->GetBounds())
+      << (is_test_app ? "for app window" : "for tabbed browser window");
 }
 
 INSTANTIATE_TEST_CASE_P(BrowserTestTabbedOrApp,

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

@@ -66,9 +66,13 @@ views::Widget::InitParams BrowserFrameMus::GetWidgetParams() {
   properties[ui::mojom::WindowManager::kShelfItemType_Property] =
       mojo::ConvertTo<std::vector<uint8_t>>(
           static_cast<int64_t>(ash::TYPE_BROWSER_SHORTCUT));
+
+  // TODO(estade): to match classic Ash, this property should be toggled to true
+  // for non-popups after the window is initially shown.
   properties[ash::mojom::kWindowPositionManaged_Property] =
-      mojo::ConvertTo<std::vector<uint8_t>>(
-          static_cast<int64_t>(browser->is_type_popup()));
+      mojo::ConvertTo<std::vector<uint8_t>>(static_cast<int64_t>(
+          !browser->bounds_overridden() && !browser->is_session_restore() &&
+          !browser->is_type_popup()));
   properties[ash::mojom::kCanConsumeSystemKeys_Property] =
       mojo::ConvertTo<std::vector<uint8_t>>(
           static_cast<int64_t>(browser->is_app()));

testing/buildbot/filters/mash.browser_tests.filter

@@ -46,9 +46,6 @@
 -HostedAppNonClientFrameViewAshTest.*
 -ImmersiveModeControllerAshHostedAppBrowserTest.*
 
-# Incorrect window bounds. Probably WindowSizerAsh problem.
--BrowserTestTabbedOrApp/BrowserTestParam.*
-
 # ash::Shell access from ChromeViewsDelegate::CreateDefaultNonClientFrameView()
 # e.g. from chromeos::CaptivePortalWindowProxy::Show().
 # See https://crbug.com/838974