cros: Make SystemTrayClient browser tests work under mash

The eliminates non-public ash dependencies from the tests.

* Convert the tests to use the mojo system tray test API
* Clean up the test api, always use the same SystemTray*, remove some
  unnecessary RunLoop spins
* Put the tests on the mojo FYI bot. I'll move them to the main
  waterfall when I'm confident they are stable.

Bug: 775121
Test: browser_tests --mash --gtest_filter="SystemTrayClient*"
Change-Id: Id790ca77aa179db499c45825a1466e18cc9f78d7
Reviewed-on: https://chromium-review.googlesource.com/723604Reviewed-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@{#509596}

ash/ash_view_ids.h

@@ -12,15 +12,23 @@ enum ViewID {
   VIEW_ID_NONE = 0,
 
   // Ash IDs start above the range used in Chrome (c/b/ui/view_ids.h).
-  VIEW_ID_MEDIA_TRAY_VIEW = 10000,
-  VIEW_ID_USER_VIEW_MEDIA_INDICATOR,
-  VIEW_ID_BLUETOOTH_DEFAULT_VIEW,
+  VIEW_ID_ASH_START = 10000,
+
   VIEW_ID_ACCESSIBILITY_TRAY_ITEM,
-  VIEW_ID_TRAY_ENTERPRISE,
+  VIEW_ID_BLUETOOTH_DEFAULT_VIEW,
+  // System tray network submenu item for extension-controlled networks.
   VIEW_ID_EXTENSION_CONTROLLED_WIFI,
-
-  // View ID that is used to mark sticky header rows in a scroll view.
+  VIEW_ID_MEDIA_TRAY_VIEW,
+  // Sticky header rows in a scroll view.
   VIEW_ID_STICKY_HEADER,
+  // System tray menu item for "device is managed by example.com".
+  VIEW_ID_TRAY_ENTERPRISE,
+  // System tray up-arrow icon that shows an update is available.
+  VIEW_ID_TRAY_UPDATE_ICON,
+  // System tray menu item label for updates (e.g. "Restart to update").
+  VIEW_ID_TRAY_UPDATE_MENU_LABEL,
+  VIEW_ID_USER_VIEW_MEDIA_INDICATOR,
+  // Keep alphabetized.
 };
 
 }  // namespace ash

ash/public/interfaces/system_tray_test_api.mojom

@@ -15,6 +15,14 @@ enum TrayItem {
 
 // All methods operate on the system tray on the primary display.
 interface SystemTrayTestApi {
+  // Disables animations (e.g. the tray view icon slide-in).
+  DisableAnimations() => ();
+
+  // Returns true if the view exists and is visible in the system tray area
+  // at the bottom of the screen. Usually these views are icons, like the Wi-Fi
+  // or battery icon.
+  IsTrayViewVisible(int32 view_id) => (bool visible);
+
   // Shows the system tray bubble menu.
   ShowBubble() => ();
 
@@ -27,4 +35,11 @@ interface SystemTrayTestApi {
   // Returns the tooltip for a bubble view, or the empty string if the view
   // does not exist.
   GetBubbleViewTooltip(int32 view_id) => (mojo.common.mojom.String16 tooltip);
+
+  // Returns the label for a bubble view, or the empty string if the view does
+  // not exist. The view must be a views::Label.
+  GetBubbleLabelText(int32 view_id) => (mojo.common.mojom.String16 tooltip);
+
+  // Returns true if the clock is using 24 hour time.
+  Is24HourClock() => (bool is_24_hour);
 };

ash/system/tray/system_tray.h

@@ -45,9 +45,12 @@ enum BubbleCreationType {
   BUBBLE_USE_EXISTING,  // Uses any existing bubble, or creates a new one.
 };
 
+// For historical reasons, SystemTray is both a controller and a view. It
+// manages all the SystemTrayItem controllers, creates icon views that appear in
+// the tray, creates the bubble menu and fills the menu with items. It is also
+// the view that contains the icons in the tray.
 class ASH_EXPORT SystemTray : public TrayBackgroundView {
  public:
-
   explicit SystemTray(Shelf* shelf);
   ~SystemTray() override;
 

ash/system/tray/system_tray_test_api.cc

@@ -5,13 +5,17 @@
 #include "ash/system/tray/system_tray_test_api.h"
 
 #include "ash/shell.h"
+#include "ash/system/date/date_view.h"
+#include "ash/system/date/tray_system_info.h"
 #include "ash/system/enterprise/tray_enterprise.h"
 #include "ash/system/network/tray_network.h"
 #include "ash/system/tray/system_tray.h"
 #include "base/run_loop.h"
 #include "base/strings/string16.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
+#include "ui/compositor/scoped_animation_duration_scale_mode.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/views/controls/label.h"
 #include "ui/views/view.h"
 
 namespace ash {
@@ -46,16 +50,26 @@ void SystemTrayTestApi::BindRequest(mojom::SystemTrayTestApiRequest request) {
                           std::move(request));
 }
 
+void SystemTrayTestApi::DisableAnimations(DisableAnimationsCallback cb) {
+  disable_animations_ = std::make_unique<ui::ScopedAnimationDurationScaleMode>(
+      ui::ScopedAnimationDurationScaleMode::ZERO_DURATION);
+  std::move(cb).Run();
+}
+
+void SystemTrayTestApi::IsTrayViewVisible(int view_id,
+                                          IsTrayViewVisibleCallback cb) {
+  // Search for the view among the tray icons.
+  views::View* view = tray_->GetViewByID(view_id);
+  std::move(cb).Run(view && view->visible());
+}
+
 void SystemTrayTestApi::ShowBubble(ShowBubbleCallback cb) {
-  SystemTray* tray = Shell::Get()->GetPrimarySystemTray();
-  tray->ShowDefaultView(ash::BUBBLE_CREATE_NEW, false /* show_by_click */);
-  base::RunLoop().RunUntilIdle();  // May animate.
+  tray_->ShowDefaultView(ash::BUBBLE_CREATE_NEW, false /* show_by_click */);
   std::move(cb).Run();
 }
 
 void SystemTrayTestApi::ShowDetailedView(mojom::TrayItem item,
                                          ShowDetailedViewCallback cb) {
-  SystemTray* tray = Shell::Get()->GetPrimarySystemTray();
   SystemTrayItem* tray_item;
   switch (item) {
     case mojom::TrayItem::kEnterprise:
@@ -65,35 +79,42 @@ void SystemTrayTestApi::ShowDetailedView(mojom::TrayItem item,
       tray_item = tray_->tray_network_;
       break;
   }
-  tray->ShowDetailedView(tray_item, 0 /* delay */, BUBBLE_CREATE_NEW);
-  base::RunLoop().RunUntilIdle();  // May animate.
+  tray_->ShowDetailedView(tray_item, 0 /* delay */, BUBBLE_CREATE_NEW);
   std::move(cb).Run();
 }
 
 void SystemTrayTestApi::IsBubbleViewVisible(int view_id,
                                             IsBubbleViewVisibleCallback cb) {
-  SystemTray* tray = Shell::Get()->GetPrimarySystemTray();
-
-  // Find the view in the bubble (not the tray itself).
-  views::View* view =
-      tray->GetSystemBubble()->bubble_view()->GetViewByID(view_id);
+  views::View* view = GetBubbleView(view_id);
   std::move(cb).Run(view && view->visible());
 }
 
 void SystemTrayTestApi::GetBubbleViewTooltip(int view_id,
                                              GetBubbleViewTooltipCallback cb) {
-  SystemTray* tray = Shell::Get()->GetPrimarySystemTray();
-
-  // Find the view in the bubble (not the tray itself).
-  views::View* view =
-      tray->GetSystemBubble()->bubble_view()->GetViewByID(view_id);
-  if (!view) {
-    std::move(cb).Run(base::string16());
-    return;
-  }
   base::string16 tooltip;
-  view->GetTooltipText(gfx::Point(), &tooltip);
+  views::View* view = GetBubbleView(view_id);
+  if (view)
+    view->GetTooltipText(gfx::Point(), &tooltip);
   std::move(cb).Run(tooltip);
 }
 
+void SystemTrayTestApi::GetBubbleLabelText(int view_id,
+                                           GetBubbleLabelTextCallback cb) {
+  base::string16 text;
+  views::View* view = GetBubbleView(view_id);
+  if (view)
+    text = static_cast<views::Label*>(view)->text();
+  std::move(cb).Run(text);
+}
+
+void SystemTrayTestApi::Is24HourClock(Is24HourClockCallback cb) {
+  base::HourClockType type = tray_->tray_system_info_->GetTimeTrayForTesting()
+                                 ->GetHourTypeForTesting();
+  std::move(cb).Run(type == base::k24HourClock);
+}
+
+views::View* SystemTrayTestApi::GetBubbleView(int view_id) const {
+  return tray_->GetSystemBubble()->bubble_view()->GetViewByID(view_id);
+}
+
 }  // namespace ash

ash/system/tray/system_tray_test_api.h

@@ -5,10 +5,16 @@
 #ifndef ASH_SYSTEM_TRAY_SYSTEM_TRAY_TEST_API_H_
 #define ASH_SYSTEM_TRAY_SYSTEM_TRAY_TEST_API_H_
 
+#include <memory>
+
 #include "ash/public/interfaces/system_tray_test_api.mojom.h"
 #include "ash/system/tray/system_tray.h"
 #include "base/macros.h"
 
+namespace ui {
+class ScopedAnimationDurationScaleMode;
+}
+
 namespace ash {
 
 // Use by tests to access private state of SystemTray.
@@ -36,6 +42,8 @@ class SystemTrayTestApi : public mojom::SystemTrayTestApi {
   TrayTiles* tray_tiles() { return tray_->tray_tiles_; }
 
   // mojom::SystemTrayTestApi:
+  void DisableAnimations(DisableAnimationsCallback cb) override;
+  void IsTrayViewVisible(int view_id, IsTrayViewVisibleCallback cb) override;
   void ShowBubble(ShowBubbleCallback cb) override;
   void ShowDetailedView(mojom::TrayItem item,
                         ShowDetailedViewCallback cb) override;
@@ -43,9 +51,16 @@ class SystemTrayTestApi : public mojom::SystemTrayTestApi {
                            IsBubbleViewVisibleCallback cb) override;
   void GetBubbleViewTooltip(int view_id,
                             GetBubbleViewTooltipCallback cb) override;
+  void GetBubbleLabelText(int view_id, GetBubbleLabelTextCallback cb) override;
+  void Is24HourClock(Is24HourClockCallback cb) override;
 
  private:
+  // Returns a view in the bubble menu (not the tray itself). Returns null if
+  // not found.
+  views::View* GetBubbleView(int view_id) const;
+
   SystemTray* const tray_;
+  std::unique_ptr<ui::ScopedAnimationDurationScaleMode> disable_animations_;
 
   DISALLOW_COPY_AND_ASSIGN(SystemTrayTestApi);
 };

ash/system/update/tray_update.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/update/tray_update.h"
 
+#include "ash/ash_view_ids.h"
 #include "ash/metrics/user_metrics_action.h"
 #include "ash/metrics/user_metrics_recorder.h"
 #include "ash/public/interfaces/update.mojom.h"
@@ -84,6 +85,7 @@ class TrayUpdate::UpdateView : public ActionableView {
 
     base::string16 label_text;
     update_label_ = TrayPopupUtils::CreateDefaultLabel();
+    update_label_->set_id(VIEW_ID_TRAY_UPDATE_MENU_LABEL);
     if (owner->factory_reset_required_) {
       label_text = bundle.GetLocalizedString(
           IDS_ASH_STATUS_TRAY_RESTART_AND_POWERWASH_TO_UPDATE);
@@ -147,6 +149,12 @@ bool TrayUpdate::GetInitialVisibility() {
   return update_required_ || update_over_cellular_available_;
 }
 
+views::View* TrayUpdate::CreateTrayView(LoginStatus status) {
+  views::View* view = TrayImageItem::CreateTrayView(status);
+  view->set_id(VIEW_ID_TRAY_UPDATE_ICON);
+  return view;
+}
+
 views::View* TrayUpdate::CreateDefaultView(LoginStatus status) {
   if (update_required_ || update_over_cellular_available_) {
     update_view_ = new UpdateView(this);

ash/system/update/tray_update.h

@@ -55,6 +55,7 @@ class ASH_EXPORT TrayUpdate : public TrayImageItem {
 
   // Overridden from TrayImageItem.
   bool GetInitialVisibility() override;
+  views::View* CreateTrayView(LoginStatus status) override;
   views::View* CreateDefaultView(LoginStatus status) override;
   void OnDefaultViewDestroyed() override;
 

chrome/browser/ui/ash/system_tray_client_browsertest.cc

@@ -6,16 +6,7 @@
 
 #include "ash/ash_view_ids.h"
 #include "ash/public/interfaces/constants.mojom.h"
-#include "ash/root_window_controller.h"
-#include "ash/shell.h"
-#include "ash/system/date/date_view.h"
-#include "ash/system/date/system_info_default_view.h"
-#include "ash/system/date/tray_system_info.h"
-#include "ash/system/tray/label_tray_view.h"
-#include "ash/system/tray/system_tray.h"
-#include "ash/system/tray/system_tray_test_api.h"
-#include "ash/system/tray/tray_item_view.h"
-#include "ash/system/update/tray_update.h"
+#include "ash/public/interfaces/system_tray_test_api.mojom.h"
 #include "base/i18n/time_formatting.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/login/login_manager_test.h"
@@ -41,15 +32,6 @@
 using chromeos::ProfileHelper;
 using user_manager::UserManager;
 
-namespace {
-
-// TODO(jamescook): Add a test-only mojo API to get system tray details.
-ash::SystemTray* GetSystemTray() {
-  return ash::Shell::GetPrimaryRootWindowController()->GetSystemTray();
-}
-
-}  // namespace
-
 class SystemTrayClientTest : public InProcessBrowserTest {
  public:
   SystemTrayClientTest() = default;
@@ -65,8 +47,17 @@ class SystemTrayClientTest : public InProcessBrowserTest {
     InProcessBrowserTest::SetUp();
   }
 
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    // Connect to the ash test interface.
+    content::ServiceManagerConnection::GetForProcess()
+        ->GetConnector()
+        ->BindInterface(ash::mojom::kServiceName, &tray_test_api_);
+  }
+
  protected:
   chromeos::FakeUpdateEngineClient* fake_update_engine_client_ = nullptr;
+  ash::mojom::SystemTrayTestApiPtr tray_test_api_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SystemTrayClientTest);
@@ -74,17 +65,20 @@ class SystemTrayClientTest : public InProcessBrowserTest {
 
 // Test that a chrome update shows the update icon in the system menu.
 IN_PROC_BROWSER_TEST_F(SystemTrayClientTest, UpdateTrayIcon) {
-  ash::TrayUpdate* tray_update = GetSystemTray()->tray_update();
+  ash::mojom::SystemTrayTestApiAsyncWaiter wait_for(tray_test_api_.get());
 
   // When no update is pending, the icon isn't visible.
-  EXPECT_FALSE(tray_update->tray_view()->visible());
+  bool visible = false;
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_FALSE(visible);
 
   // Simulate an upgrade. This sends a mojo message to ash.
   UpgradeDetector::GetInstance()->NotifyUpgrade();
   content::RunAllPendingInMessageLoop();
 
   // Tray icon is now visible.
-  EXPECT_TRUE(tray_update->tray_view()->visible());
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_TRUE(visible);
 }
 
 // Tests that the update icon becomes visible after an update is detected
@@ -92,11 +86,13 @@ IN_PROC_BROWSER_TEST_F(SystemTrayClientTest, UpdateTrayIcon) {
 // after user's one time permission on the update is set successfully in Update
 // Engine.
 IN_PROC_BROWSER_TEST_F(SystemTrayClientTest, UpdateOverCellularTrayIcon) {
-  ash::TrayItemView::DisableAnimationsForTest();
-  ash::TrayUpdate* tray_update = GetSystemTray()->tray_update();
+  ash::mojom::SystemTrayTestApiAsyncWaiter wait_for(tray_test_api_.get());
+  wait_for.DisableAnimations();
 
   // When no update is pending, the icon isn't visible.
-  EXPECT_FALSE(tray_update->tray_view()->visible());
+  bool visible = false;
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_FALSE(visible);
 
   chromeos::UpdateEngineClient::Status status;
   status.status =
@@ -106,11 +102,12 @@ IN_PROC_BROWSER_TEST_F(SystemTrayClientTest, UpdateOverCellularTrayIcon) {
   content::RunAllPendingInMessageLoop();
 
   // When an update is available over cellular networks, the icon is visible.
-  EXPECT_TRUE(tray_update->tray_view()->visible());
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_TRUE(visible);
 
-  GetSystemTray()->ShowDefaultView(ash::BUBBLE_CREATE_NEW,
-                                   false /* show_by_click */);
-  base::string16 label = tray_update->GetLabelForTesting()->text();
+  wait_for.ShowBubble();
+  base::string16 label;
+  wait_for.GetBubbleLabelText(ash::VIEW_ID_TRAY_UPDATE_MENU_LABEL, &label);
   EXPECT_EQ("Click to view update details", base::UTF16ToUTF8(label));
 
   // Notifies that the user's one time permission on update over cellular
@@ -120,22 +117,26 @@ IN_PROC_BROWSER_TEST_F(SystemTrayClientTest, UpdateOverCellularTrayIcon) {
   content::RunAllPendingInMessageLoop();
 
   // When user permission is granted, the icon becomes invisible.
-  EXPECT_FALSE(tray_update->tray_view()->visible());
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_FALSE(visible);
 }
 
 // Test that a flash update causes the update UI to show in the system menu.
 IN_PROC_BROWSER_TEST_F(SystemTrayClientTest, FlashUpdateTrayIcon) {
-  ash::TrayUpdate* tray_update = GetSystemTray()->tray_update();
+  ash::mojom::SystemTrayTestApiAsyncWaiter wait_for(tray_test_api_.get());
 
   // When no update is pending, the icon isn't visible.
-  EXPECT_FALSE(tray_update->tray_view()->visible());
+  bool visible = false;
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_FALSE(visible);
 
   // Simulate a Flash update. This sends a mojo message to ash.
   SystemTrayClient::Get()->SetFlashUpdateAvailable();
   content::RunAllPendingInMessageLoop();
 
   // Tray icon is now visible.
-  EXPECT_TRUE(tray_update->tray_view()->visible());
+  wait_for.IsTrayViewVisible(ash::VIEW_ID_TRAY_UPDATE_ICON, &visible);
+  EXPECT_TRUE(visible);
 }
 
 using SystemTrayClientEnterpriseTest = policy::DevicePolicyCrosBrowserTest;
@@ -179,22 +180,6 @@ class SystemTrayClientClockTest : public chromeos::LoginManagerTest {
     content::RunAllPendingInMessageLoop();
   }
 
-  static ash::TraySystemInfo* GetTraySystemInfo() {
-    return ash::SystemTrayTestApi(ash::Shell::Get()->GetPrimarySystemTray())
-        .tray_system_info();
-  }
-
-  static base::HourClockType GetHourType() {
-    const ash::SystemInfoDefaultView* system_info_default_view =
-        GetTraySystemInfo()->GetDefaultViewForTesting();
-    return system_info_default_view->GetDateView()->GetHourTypeForTesting();
-  }
-
-  static void CreateDefaultView() {
-    GetTraySystemInfo()->CreateDefaultViewForTesting(
-        ash::LoginStatus::NOT_LOGGED_IN);
-  }
-
  protected:
   const AccountId account_id1_;
   const AccountId account_id2_;
@@ -212,21 +197,32 @@ IN_PROC_BROWSER_TEST_F(SystemTrayClientClockTest,
 
 // Test that clock type is taken from user profile for current active user.
 IN_PROC_BROWSER_TEST_F(SystemTrayClientClockTest, TestMultiProfile24HourClock) {
+  // Connect to ash.
+  ash::mojom::SystemTrayTestApiPtr tray_test_api;
+  content::ServiceManagerConnection::GetForProcess()
+      ->GetConnector()
+      ->BindInterface(ash::mojom::kServiceName, &tray_test_api);
+  ash::mojom::SystemTrayTestApiAsyncWaiter wait_for(tray_test_api.get());
+
+  // Login a user with a 24-hour clock.
   LoginUser(account_id1_.GetUserEmail());
   SetupUserProfile(account_id1_, true /* use_24_hour_clock */);
-  CreateDefaultView();
-  EXPECT_EQ(base::k24HourClock, GetHourType());
+  bool is_24_hour = false;
+  wait_for.Is24HourClock(&is_24_hour);
+  EXPECT_TRUE(is_24_hour);
 
+  // Add a user with a 12-hour clock.
   chromeos::UserAddingScreen::Get()->Start();
   content::RunAllPendingInMessageLoop();
   AddUser(account_id2_.GetUserEmail());
   SetupUserProfile(account_id2_, false /* use_24_hour_clock */);
-  CreateDefaultView();
-  EXPECT_EQ(base::k12HourClock, GetHourType());
+  wait_for.Is24HourClock(&is_24_hour);
+  EXPECT_FALSE(is_24_hour);
 
+  // Switch back to the user with the 24-hour clock.
   UserManager::Get()->SwitchActiveUser(account_id1_);
   // Allow clock setting to be sent to ash over mojo.
   content::RunAllPendingInMessageLoop();
-  CreateDefaultView();
-  EXPECT_EQ(base::k24HourClock, GetHourType());
+  wait_for.Is24HourClock(&is_24_hour);
+  EXPECT_TRUE(is_24_hour);
 }

testing/buildbot/filters/mojo.fyi.browser_tests.filter

@@ -121,3 +121,6 @@ PolicyCertVerifierTest.VerifyUntrustedCert
 PolicyCertVerifierTest.VerifyUsingAdditionalTrustAnchor
 ScriptContextTest.GetEffectiveDocumentURL
 SiteEngagementUiBrowserTest.Basic
+SystemTrayClientEnterpriseTest.*
+SystemTrayClientClockTest.*
+SystemTrayClientTest.*