chromeos: Introduce ash::TestSystemTrayClient for ash_unittests

This allows some OpenUiDelegate classes to be eliminated from ash
system tray code. AshTestBase now provides a shared SystemTrayClient
implementation for tests.

Long ago we had a similar class "TestSystemTrayDelegate" that did
something similar.

Bug: 970889
Test: ash_unittests
Change-Id: I0035dccfe343df4ef8504cb0113684539eae2ef1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1649056
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667121}

ash/multi_device_setup/multi_device_notification_presenter.cc

@@ -56,18 +56,6 @@ MultiDeviceNotificationPresenter::GetNotificationDescriptionForLogging(
   NOTREACHED();
 }
 
-MultiDeviceNotificationPresenter::OpenUiDelegate::~OpenUiDelegate() = default;
-
-void MultiDeviceNotificationPresenter::OpenUiDelegate::
-    OpenMultiDeviceSetupUi() {
-  Shell::Get()->system_tray_model()->client()->ShowMultiDeviceSetup();
-}
-
-void MultiDeviceNotificationPresenter::OpenUiDelegate::
-    OpenConnectedDevicesSettings() {
-  Shell::Get()->system_tray_model()->client()->ShowConnectedDevicesSettings();
-}
-
 // static
 MultiDeviceNotificationPresenter::NotificationType
 MultiDeviceNotificationPresenter::GetMetricValueForNotification(
@@ -91,7 +79,6 @@ MultiDeviceNotificationPresenter::MultiDeviceNotificationPresenter(
     : message_center_(message_center),
       connector_(connector),
       binding_(this),
-      open_ui_delegate_(std::make_unique<OpenUiDelegate>()),
       weak_ptr_factory_(this) {
   DCHECK(message_center_);
   DCHECK(connector_);
@@ -191,14 +178,17 @@ void MultiDeviceNotificationPresenter::OnNotificationClicked(
                             kNotificationTypeMax);
   switch (notification_status_) {
     case Status::kNewUserNotificationVisible:
-      open_ui_delegate_->OpenMultiDeviceSetupUi();
+      Shell::Get()->system_tray_model()->client()->ShowMultiDeviceSetup();
       break;
     case Status::kExistingUserHostSwitchedNotificationVisible:
       // Clicks on the 'host switched' and 'Chromebook added' notifications have
       // the same effect, i.e. opening the Settings subpage.
       FALLTHROUGH;
     case Status::kExistingUserNewChromebookNotificationVisible:
-      open_ui_delegate_->OpenConnectedDevicesSettings();
+      Shell::Get()
+          ->system_tray_model()
+          ->client()
+          ->ShowConnectedDevicesSettings();
       break;
     case Status::kNoNotificationVisible:
       NOTREACHED();

ash/multi_device_setup/multi_device_notification_presenter.h

@@ -84,16 +84,6 @@ class ASH_EXPORT MultiDeviceNotificationPresenter
   // MultiDevice setup notification ID.
   static const char kNotificationId[];
 
-  // These methods are delegated to a nested class to make them easier to stub
-  // in unit tests. This way they can all be stubbed simultaneously by building
-  // a test delegate class deriving from OpenUiDelegate.
-  class OpenUiDelegate {
-   public:
-    virtual ~OpenUiDelegate();
-    virtual void OpenMultiDeviceSetupUi();
-    virtual void OpenConnectedDevicesSettings();
-  };
-
   // Represents each possible MultiDevice setup notification that the setup flow
   // can show with a "none" option for the general state with no notification
   // present.
@@ -141,7 +131,6 @@ class ASH_EXPORT MultiDeviceNotificationPresenter
   mojo::Binding<chromeos::multidevice_setup::mojom::AccountStatusChangeDelegate>
       binding_;
 
-  std::unique_ptr<OpenUiDelegate> open_ui_delegate_;
   base::WeakPtrFactory<MultiDeviceNotificationPresenter> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MultiDeviceNotificationPresenter);

ash/multi_device_setup/multi_device_notification_presenter_unittest.cc

@@ -8,10 +8,10 @@
 #include <memory>
 #include <utility>
 
+#include "ash/public/cpp/test/test_system_tray_client.h"
 #include "ash/session/test_session_controller_client.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/test/ash_test_base.h"
-#include "ash/test/ash_test_helper.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -94,43 +94,12 @@ class TestMessageCenter : public message_center::FakeMessageCenter {
 
 class MultiDeviceNotificationPresenterTest : public NoSessionAshTestBase {
  public:
-  class TestOpenUiDelegate
-      : public MultiDeviceNotificationPresenter::OpenUiDelegate {
-   public:
-    TestOpenUiDelegate() = default;
-    ~TestOpenUiDelegate() override = default;
-
-    int open_multi_device_setup_ui_count() const {
-      return open_multi_device_setup_ui_count_;
-    }
-
-    int open_connected_devices_settings_count() const {
-      return open_connected_devices_settings_count_;
-    }
-
-    // MultiDeviceNotificationPresenter::OpenUiDelegate:
-    void OpenMultiDeviceSetupUi() override {
-      ++open_multi_device_setup_ui_count_;
-    }
-
-    void OpenConnectedDevicesSettings() override {
-      ++open_connected_devices_settings_count_;
-    }
-
-   private:
-    int open_multi_device_setup_ui_count_ = 0;
-    int open_connected_devices_settings_count_ = 0;
-  };
-
- protected:
   MultiDeviceNotificationPresenterTest() = default;
 
   void SetUp() override {
     NoSessionAshTestBase::SetUp();
 
-    std::unique_ptr<TestOpenUiDelegate> test_open_ui_delegate =
-        std::make_unique<TestOpenUiDelegate>();
-    test_open_ui_delegate_ = test_open_ui_delegate.get();
+    test_system_tray_client_ = GetSystemTrayClient();
 
     service_manager::mojom::ConnectorRequest request;
     connector_ = service_manager::Connector::Create(&request);
@@ -150,8 +119,6 @@ class MultiDeviceNotificationPresenterTest : public NoSessionAshTestBase {
     notification_presenter_ =
         std::make_unique<MultiDeviceNotificationPresenter>(
             &test_message_center_, connector_.get());
-    notification_presenter_->open_ui_delegate_ =
-        std::move(test_open_ui_delegate);
   }
 
   void TearDown() override {
@@ -271,7 +238,7 @@ class MultiDeviceNotificationPresenterTest : public NoSessionAshTestBase {
   }
 
   base::HistogramTester histogram_tester_;
-  TestOpenUiDelegate* test_open_ui_delegate_;
+  TestSystemTrayClient* test_system_tray_client_;
   TestMessageCenter test_message_center_;
   std::unique_ptr<service_manager::Connector> connector_;
   std::unique_ptr<chromeos::multidevice_setup::FakeMultiDeviceSetup>
@@ -354,7 +321,7 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   notification_presenter_->RemoveMultiDeviceSetupNotification();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationClicked", 0);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -369,7 +336,7 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   ClickNotification();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 1);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 1);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationClicked", 1);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -384,7 +351,7 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   DismissNotification(true /* by_user */);
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationDismissed", 1);
 
   ShowNewUserNotification();
@@ -393,7 +360,7 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   DismissNotification(false /* by_user */);
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationDismissed", 1);
 }
 
@@ -406,7 +373,7 @@ TEST_F(MultiDeviceNotificationPresenterTest, TestNoLongerNewUserEvent) {
   TriggerNoLongerNewUserEvent();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationClicked", 0);
   AssertPotentialHostBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -421,7 +388,8 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   notification_presenter_->RemoveMultiDeviceSetupNotification();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_connected_devices_settings_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_connected_devices_settings_count(),
+            0);
   AssertHostSwitchedBucketCount("MultiDeviceSetup_NotificationClicked", 0);
   AssertHostSwitchedBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -436,7 +404,8 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   ClickNotification();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_connected_devices_settings_count(), 1);
+  EXPECT_EQ(test_system_tray_client_->show_connected_devices_settings_count(),
+            1);
   AssertHostSwitchedBucketCount("MultiDeviceSetup_NotificationClicked", 1);
   AssertHostSwitchedBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -451,7 +420,7 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   DismissNotification(true /* by_user */);
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertHostSwitchedBucketCount("MultiDeviceSetup_NotificationDismissed", 1);
 
   ShowExistingUserHostSwitchedNotification();
@@ -460,7 +429,7 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   DismissNotification(false /* by_user */);
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertHostSwitchedBucketCount("MultiDeviceSetup_NotificationDismissed", 1);
 }
 
@@ -475,7 +444,8 @@ TEST_F(
   notification_presenter_->RemoveMultiDeviceSetupNotification();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_connected_devices_settings_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_connected_devices_settings_count(),
+            0);
   AssertNewChromebookBucketCount("MultiDeviceSetup_NotificationClicked", 0);
   AssertNewChromebookBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -490,7 +460,8 @@ TEST_F(MultiDeviceNotificationPresenterTest,
   ClickNotification();
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_connected_devices_settings_count(), 1);
+  EXPECT_EQ(test_system_tray_client_->show_connected_devices_settings_count(),
+            1);
   AssertNewChromebookBucketCount("MultiDeviceSetup_NotificationClicked", 1);
   AssertNewChromebookBucketCount("MultiDeviceSetup_NotificationShown", 1);
 }
@@ -506,7 +477,7 @@ TEST_F(
   DismissNotification(true /* by_user */);
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertNewChromebookBucketCount("MultiDeviceSetup_NotificationDismissed", 1);
 
   ShowExistingUserNewChromebookNotification();
@@ -515,7 +486,7 @@ TEST_F(
   DismissNotification(false /* by_user */);
   VerifyNoNotificationIsVisible();
 
-  EXPECT_EQ(test_open_ui_delegate_->open_multi_device_setup_ui_count(), 0);
+  EXPECT_EQ(test_system_tray_client_->show_multi_device_setup_count(), 0);
   AssertNewChromebookBucketCount("MultiDeviceSetup_NotificationDismissed", 1);
 }
 

ash/public/cpp/BUILD.gn

@@ -297,6 +297,8 @@ source_set("test_support") {
     "test/test_keyboard_controller_observer.h",
     "test/test_new_window_delegate.cc",
     "test/test_new_window_delegate.h",
+    "test/test_system_tray_client.cc",
+    "test/test_system_tray_client.h",
   ]
 
   deps = [

ash/public/cpp/system_tray_client.h

@@ -12,8 +12,6 @@
 
 namespace ash {
 
-class SystemTrayClient;
-
 // Handles method calls delegated back to chrome from ash.
 class ASH_PUBLIC_EXPORT SystemTrayClient {
  public:

ash/public/cpp/test/test_system_tray_client.cc

@@ -0,0 +1,78 @@
+// Copyright 2019 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/public/cpp/test/test_system_tray_client.h"
+
+namespace ash {
+
+TestSystemTrayClient::TestSystemTrayClient() = default;
+
+TestSystemTrayClient::~TestSystemTrayClient() = default;
+
+void TestSystemTrayClient::ShowSettings() {}
+
+void TestSystemTrayClient::ShowBluetoothSettings() {
+  show_bluetooth_settings_count_++;
+}
+
+void TestSystemTrayClient::ShowBluetoothPairingDialog(
+    const std::string& address,
+    const base::string16& name_for_display,
+    bool paired,
+    bool connected) {}
+
+void TestSystemTrayClient::ShowDateSettings() {}
+
+void TestSystemTrayClient::ShowSetTimeDialog() {}
+
+void TestSystemTrayClient::ShowDisplaySettings() {}
+
+void TestSystemTrayClient::ShowPowerSettings() {}
+
+void TestSystemTrayClient::ShowChromeSlow() {}
+
+void TestSystemTrayClient::ShowIMESettings() {}
+
+void TestSystemTrayClient::ShowConnectedDevicesSettings() {
+  show_connected_devices_settings_count_++;
+}
+
+void TestSystemTrayClient::ShowAboutChromeOS() {}
+
+void TestSystemTrayClient::ShowHelp() {}
+
+void TestSystemTrayClient::ShowAccessibilityHelp() {}
+
+void TestSystemTrayClient::ShowAccessibilitySettings() {}
+
+void TestSystemTrayClient::ShowPaletteHelp() {}
+
+void TestSystemTrayClient::ShowPaletteSettings() {}
+
+void TestSystemTrayClient::ShowPublicAccountInfo() {}
+
+void TestSystemTrayClient::ShowEnterpriseInfo() {}
+
+void TestSystemTrayClient::ShowNetworkConfigure(const std::string& network_id) {
+}
+
+void TestSystemTrayClient::ShowNetworkCreate(const std::string& type) {}
+
+void TestSystemTrayClient::ShowThirdPartyVpnCreate(
+    const std::string& extension_id) {}
+
+void TestSystemTrayClient::ShowArcVpnCreate(const std::string& app_id) {}
+
+void TestSystemTrayClient::ShowNetworkSettings(const std::string& network_id) {}
+
+void TestSystemTrayClient::ShowMultiDeviceSetup() {
+  show_multi_device_setup_count_++;
+}
+
+void TestSystemTrayClient::RequestRestartForUpdate() {}
+
+void TestSystemTrayClient::SetLocaleAndExit(
+    const std::string& locale_iso_code) {}
+
+}  // namespace ash