Reland: cros: Adapt ShelfBrowserTest to work with --mash

The original CL caused mash_browser_test flake because the test was not
waiting for in-flight window bounds/visibility changes to apply, so
there was a race between browser, window server and ash.

Original CL description:
* Add a mojo shelf test API
* Convert 2 tests to use it

Bug: 781925, 678687
Test: browser_tests --mash
Change-Id: Icfe23d4c8c3bcd84bff8dc46d2c9680cc656ce50
Reviewed-on: https://chromium-review.googlesource.com/755344Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514582}

TBR=tsepez@chromium.org

Change-Id: Icfe23d4c8c3bcd84bff8dc46d2c9680cc656ce50
Reviewed-on: https://chromium-review.googlesource.com/759034Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515028}

ash/BUILD.gn

@@ -1669,6 +1669,8 @@ static_library("test_support_common") {
     "shelf/overflow_button_test_api.h",
     "shelf/shelf_button_pressed_metric_tracker_test_api.cc",
     "shelf/shelf_button_pressed_metric_tracker_test_api.h",
+    "shelf/shelf_test_api.cc",
+    "shelf/shelf_test_api.h",
     "shelf/shelf_view_test_api.cc",
     "shelf/shelf_view_test_api.h",
     "shell/toplevel_window.cc",

ash/mojo_test_interface_factory.cc

@@ -6,7 +6,9 @@
 
 #include <utility>
 
+#include "ash/public/interfaces/shelf_test_api.mojom.h"
 #include "ash/public/interfaces/system_tray_test_api.mojom.h"
+#include "ash/shelf/shelf_test_api.h"
 #include "ash/system/tray/system_tray_test_api.h"
 #include "base/bind.h"
 #include "base/single_thread_task_runner.h"
@@ -15,8 +17,12 @@ namespace ash {
 namespace mojo_test_interface_factory {
 namespace {
 
-// This isn't strictly necessary, but exists to make threading and arguments
-// clearer.
+// These functions aren't strictly necessary, but exist to make threading and
+// arguments clearer.
+void BindShelfTestApiOnMainThread(mojom::ShelfTestApiRequest request) {
+  ShelfTestApi::BindRequest(std::move(request));
+}
+
 void BindSystemTrayTestApiOnMainThread(
     mojom::SystemTrayTestApiRequest request) {
   SystemTrayTestApi::BindRequest(std::move(request));
@@ -27,6 +33,8 @@ void BindSystemTrayTestApiOnMainThread(
 void RegisterInterfaces(
     service_manager::BinderRegistry* registry,
     scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner) {
+  registry->AddInterface(base::Bind(&BindShelfTestApiOnMainThread),
+                         main_thread_task_runner);
   registry->AddInterface(base::Bind(&BindSystemTrayTestApiOnMainThread),
                          main_thread_task_runner);
 }

ash/mus/manifest.json

@@ -26,12 +26,13 @@
           "ash::mojom::SystemTray",
           "ash::mojom::TabletModeController",
           "ash::mojom::TrayAction",
-	  "ash::mojom::VoiceInteractionController",
+          "ash::mojom::VoiceInteractionController",
           "ash::mojom::VpnList",
           "ash::mojom::WallpaperController"
         ],
         // Test-only interfaces.
         "test": [
+          "ash::mojom::ShelfTestApi",
           "ash::mojom::SystemTrayTestApi"
         ],
         // Only chrome is allowed to use this (required as dbus runs in Chrome).

ash/public/interfaces/BUILD.gn

@@ -68,6 +68,7 @@ mojom("interfaces_internal") {
 mojom("test_interfaces") {
   testonly = true
   sources = [
+    "shelf_test_api.mojom",
     "system_tray_test_api.mojom",
   ]
   deps = [

ash/public/interfaces/shelf_test_api.mojom

+// Copyright 2017 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.
+
+module ash.mojom;
+
+// All methods operate on the shelf on the primary display.
+interface ShelfTestApi {
+  // Returns true if the shelf is visible (e.g. not auto-hidden).
+  IsVisible() => (bool visible);
+
+  // Forces a visibility update and then runs the callback.
+  UpdateVisibility() => ();
+
+  // Returns true if a window is overlapping the shelf, which changes its
+  // appearance slightly.
+  HasOverlappingWindow() => (bool overlap);
+
+  // Returns true if the shelf alignment is BOTTOM_LOCKED, which is not exposed
+  // via prefs.
+  IsAlignmentBottomLocked() => (bool locked);
+};

ash/shelf/shelf_test_api.cc

+// Copyright 2017 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.
+
+#include "ash/shelf/shelf_test_api.h"
+
+#include "ash/root_window_controller.h"
+#include "ash/shelf/shelf.h"
+#include "ash/shelf/shelf_layout_manager.h"
+#include "ash/shell.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+
+namespace ash {
+
+ShelfTestApi::ShelfTestApi(Shelf* shelf) : shelf_(shelf) {}
+
+ShelfTestApi::~ShelfTestApi() = default;
+
+// static
+void ShelfTestApi::BindRequest(mojom::ShelfTestApiRequest request) {
+  Shelf* shelf = Shell::Get()->GetPrimaryRootWindowController()->shelf();
+  mojo::MakeStrongBinding(std::make_unique<ShelfTestApi>(shelf),
+                          std::move(request));
+}
+
+void ShelfTestApi::IsVisible(IsVisibleCallback cb) {
+  std::move(cb).Run(shelf_->shelf_layout_manager()->IsVisible());
+}
+
+void ShelfTestApi::UpdateVisibility(UpdateVisibilityCallback cb) {
+  shelf_->shelf_layout_manager()->UpdateVisibilityState();
+  std::move(cb).Run();
+}
+
+void ShelfTestApi::HasOverlappingWindow(HasOverlappingWindowCallback cb) {
+  std::move(cb).Run(shelf_->shelf_layout_manager()->window_overlaps_shelf());
+}
+
+void ShelfTestApi::IsAlignmentBottomLocked(IsAlignmentBottomLockedCallback cb) {
+  std::move(cb).Run(shelf_->alignment() == SHELF_ALIGNMENT_BOTTOM_LOCKED);
+}
+
+}  // namespace ash

ash/shelf/shelf_test_api.h

+// Copyright 2017 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.
+
+#ifndef ASH_SHELF_SHELF_TEST_API_H_
+#define ASH_SHELF_SHELF_TEST_API_H_
+
+#include "ash/public/interfaces/shelf_test_api.mojom.h"
+#include "base/macros.h"
+
+namespace ash {
+
+class Shelf;
+
+// Allows tests to access private state of the shelf.
+class ShelfTestApi : public mojom::ShelfTestApi {
+ public:
+  explicit ShelfTestApi(Shelf* shelf);
+  ~ShelfTestApi() override;
+
+  // Creates and binds an instance from a remote request (e.g. from chrome).
+  static void BindRequest(mojom::ShelfTestApiRequest request);
+
+  // mojom::ShelfTestApi:
+  void IsVisible(IsVisibleCallback cb) override;
+  void UpdateVisibility(UpdateVisibilityCallback cb) override;
+  void HasOverlappingWindow(HasOverlappingWindowCallback cb) override;
+  void IsAlignmentBottomLocked(IsAlignmentBottomLockedCallback cb) override;
+
+ private:
+  Shelf* shelf_;
+
+  DISALLOW_COPY_AND_ASSIGN(ShelfTestApi);
+};
+
+}  // namespace ash
+
+#endif  // ASH_SHELF_SHELF_TEST_API_H_

chrome/browser/ui/ash/shelf_browsertest.cc

@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/shelf/shelf.h"
-#include "ash/shelf/shelf_layout_manager.h"
+#include "ash/public/cpp/shelf_prefs.h"
+#include "ash/public/interfaces/constants.mojom.h"
+#include "ash/public/interfaces/shelf_test_api.mojom.h"
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
@@ -12,38 +13,85 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/chromeos_switches.h"
+#include "components/signin/core/account_id/account_id.h"
 #include "components/user_manager/user_names.h"
+#include "content/public/common/service_manager_connection.h"
+#include "services/service_manager/public/cpp/connector.h"
+#include "ui/aura/test/mus/change_completion_waiter.h"
+#include "ui/display/display.h"
+#include "ui/display/screen.h"
+#include "ui/views/mus/mus_client.h"
 
-using ShelfBrowserTest = InProcessBrowserTest;
+class ShelfBrowserTest : public InProcessBrowserTest {
+ public:
+  ShelfBrowserTest() = default;
+  ~ShelfBrowserTest() override = default;
+
+  // InProcessBrowserTest:
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    // Connect to the ash test interface for the shelf.
+    content::ServiceManagerConnection::GetForProcess()
+        ->GetConnector()
+        ->BindInterface(ash::mojom::kServiceName, &shelf_test_api_);
+  }
+
+ protected:
+  // Waits for in-flight changes to window bounds, visibility, etc. to complete.
+  // Both ui service and ash will see the changes before this returns.
+  void WaitForWindowChanges() {
+    // Only need to wait for mus and mash. Classic ash is synchronous.
+    if (!views::MusClient::Exists())
+      return;
+    aura::test::WaitForAllChangesToComplete(
+        views::MusClient::Get()->window_tree_client());
+  }
+
+  ash::mojom::ShelfTestApiPtr shelf_test_api_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ShelfBrowserTest);
+};
 
 // Confirm that a status bubble doesn't cause the shelf to darken.
 IN_PROC_BROWSER_TEST_F(ShelfBrowserTest, StatusBubble) {
-  ash::ShelfLayoutManager* shelf_layout_manager =
-      ash::Shelf::ForWindow(browser()->window()->GetNativeWindow())
-          ->shelf_layout_manager();
-  EXPECT_TRUE(shelf_layout_manager->IsVisible());
+  ash::mojom::ShelfTestApiAsyncWaiter shelf(shelf_test_api_.get());
+  bool shelf_visible = false;
+  shelf.IsVisible(&shelf_visible);
+  EXPECT_TRUE(shelf_visible);
 
   // Ensure that the browser abuts the shelf.
+  const int shelf_top =
+      display::Screen::GetScreen()->GetPrimaryDisplay().work_area().bottom();
   gfx::Rect bounds = browser()->window()->GetBounds();
-  bounds.set_height(shelf_layout_manager->GetIdealBounds().y() - bounds.y());
+  bounds.set_height(shelf_top - bounds.y());
   browser()->window()->SetBounds(bounds);
-  EXPECT_FALSE(shelf_layout_manager->window_overlaps_shelf());
+  WaitForWindowChanges();
+
+  // Browser does not overlap shelf.
+  bool has_overlapping_window = false;
+  shelf.HasOverlappingWindow(&has_overlapping_window);
+  EXPECT_FALSE(has_overlapping_window);
 
   // Show status, which may overlap the shelf by a pixel.
   browser()->window()->GetStatusBubble()->SetStatus(
       base::UTF8ToUTF16("Dummy Status Text"));
-  shelf_layout_manager->UpdateVisibilityState();
+  WaitForWindowChanges();
+  shelf.UpdateVisibility();
 
   // Ensure that status doesn't cause overlap.
-  EXPECT_FALSE(shelf_layout_manager->window_overlaps_shelf());
+  shelf.HasOverlappingWindow(&has_overlapping_window);
+  EXPECT_FALSE(has_overlapping_window);
 
   // Ensure that moving the browser slightly down does cause overlap.
   bounds.Offset(0, 1);
   browser()->window()->SetBounds(bounds);
-  EXPECT_TRUE(shelf_layout_manager->window_overlaps_shelf());
+  WaitForWindowChanges();
+  shelf.HasOverlappingWindow(&has_overlapping_window);
+  EXPECT_TRUE(has_overlapping_window);
 }
 
-class ShelfGuestSessionBrowserTest : public InProcessBrowserTest {
+class ShelfGuestSessionBrowserTest : public ShelfBrowserTest {
  protected:
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitch(chromeos::switches::kGuestSession);
@@ -58,7 +106,15 @@ class ShelfGuestSessionBrowserTest : public InProcessBrowserTest {
 // Tests that in guest session, shelf alignment could be initialized to bottom
 // aligned, instead of bottom locked (crbug.com/699661).
 IN_PROC_BROWSER_TEST_F(ShelfGuestSessionBrowserTest, ShelfAlignment) {
-  ash::Shelf* shelf =
-      ash::Shelf::ForWindow(browser()->window()->GetNativeWindow());
-  EXPECT_EQ(ash::SHELF_ALIGNMENT_BOTTOM, shelf->alignment());
+  // Check the alignment pref for the primary display.
+  ash::ShelfAlignment alignment = ash::GetShelfAlignmentPref(
+      browser()->profile()->GetPrefs(),
+      display::Screen::GetScreen()->GetPrimaryDisplay().id());
+  EXPECT_EQ(ash::SHELF_ALIGNMENT_BOTTOM, alignment);
+
+  // Check the locked state, which is not exposed via prefs.
+  ash::mojom::ShelfTestApiAsyncWaiter shelf(shelf_test_api_.get());
+  bool shelf_bottom_locked = false;
+  shelf.IsAlignmentBottomLocked(&shelf_bottom_locked);
+  EXPECT_FALSE(shelf_bottom_locked);
 }

testing/buildbot/filters/mash.browser_tests.filter

@@ -37,6 +37,8 @@ WebviewLoginTest.*
 AcceleratorCommandsFullscreenBrowserTest.*
 AcceleratorCommandsPlatformAppFullscreenBrowserTest.*
 ArcAppLauncherBrowserTest.*
+ShelfBrowserTest.*
+ShelfGuestSessionBrowserTest.*
 SoundsManagerTestImpl.*
 SystemTrayClientClockTest.*
 SystemTrayClientEnterpriseTest.*