[CrOS PhoneHub] Refactor PhoneStatusView to use delegate.

The new Delegate class is called to open the connected device
settings UI.

This refactor is allows:
  * logging the current screen when the button is pressed
  * a unit test case for pressing the settings button

BUG=1138137,1106937

Change-Id: I71af2ec6c00813536c15f684fdb8459f4671cada
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2501072Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Tim Song <tengs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821352}

ash/system/phonehub/phone_hub_tray.cc

@@ -6,13 +6,14 @@
 
 #include "ash/accessibility/accessibility_controller_impl.h"
 #include "ash/focus_cycler.h"
+#include "ash/public/cpp/system_tray_client.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/style/ash_color_provider.h"
+#include "ash/system/model/system_tray_model.h"
 #include "ash/system/phonehub/phone_hub_content_view.h"
-#include "ash/system/phonehub/phone_status_view.h"
 #include "ash/system/phonehub/quick_actions_view.h"
 #include "ash/system/phonehub/task_continuation_view.h"
 #include "ash/system/tray/system_menu_button.h"
@@ -169,7 +170,7 @@ void PhoneHubTray::ShowBubble(bool show_by_click) {
   // We will always have this phone status view on top of the bubble view
   // to display any available phone status and the settings icon.
   std::unique_ptr<views::View> phone_status =
-      ui_controller_->CreateStatusHeaderView();
+      ui_controller_->CreateStatusHeaderView(this);
   if (phone_status) {
     phone_status->SetPaintToLayer();
     phone_status->layer()->SetFillsBoundsOpaquely(false);
@@ -206,6 +207,15 @@ const char* PhoneHubTray::GetClassName() const {
   return "PhoneHubTray";
 }
 
+bool PhoneHubTray::CanOpenConnectedDeviceSettings() {
+  return TrayPopupUtils::CanOpenWebUISettings();
+}
+
+void PhoneHubTray::OpenConnectedDevicesSettings() {
+  DCHECK(CanOpenConnectedDeviceSettings());
+  Shell::Get()->system_tray_model()->client()->ShowConnectedDevicesSettings();
+}
+
 void PhoneHubTray::CloseBubble() {
   if (content_view_)
     content_view_->OnBubbleClose();

ash/system/phonehub/phone_hub_tray.h

@@ -8,6 +8,7 @@
 #include "ash/ash_export.h"
 #include "ash/system/phonehub/phone_hub_content_view.h"
 #include "ash/system/phonehub/phone_hub_ui_controller.h"
+#include "ash/system/phonehub/phone_status_view.h"
 #include "ash/system/tray/tray_background_view.h"
 #include "base/scoped_observer.h"
 
@@ -29,6 +30,7 @@ class TrayBubbleWrapper;
 // This class represents the Phone Hub tray button in the status area and
 // controls the bubble that is shown when the tray button is clicked.
 class ASH_EXPORT PhoneHubTray : public TrayBackgroundView,
+                                public PhoneStatusView::Delegate,
                                 public PhoneHubUiController::Observer {
  public:
   explicit PhoneHubTray(Shelf* shelf);
@@ -53,6 +55,10 @@ class ASH_EXPORT PhoneHubTray : public TrayBackgroundView,
   TrayBubbleView* GetBubbleView() override;
   const char* GetClassName() const override;
 
+  // PhoneStatusView::Delegate:
+  bool CanOpenConnectedDeviceSettings() override;
+  void OpenConnectedDevicesSettings() override;
+
   views::View* content_view_for_testing() { return content_view_; }
 
   PhoneHubUiController* ui_controller_for_testing() {

ash/system/phonehub/phone_hub_ui_controller.cc

@@ -12,7 +12,6 @@
 #include "ash/system/phonehub/onboarding_view.h"
 #include "ash/system/phonehub/phone_connected_view.h"
 #include "ash/system/phonehub/phone_hub_content_view.h"
-#include "ash/system/phonehub/phone_status_view.h"
 #include "base/logging.h"
 #include "chromeos/components/phonehub/phone_hub_manager.h"
 
@@ -42,10 +41,12 @@ void PhoneHubUiController::SetPhoneHubManager(
   UpdateUiState(GetUiStateFromPhoneHubManager());
 }
 
-std::unique_ptr<views::View> PhoneHubUiController::CreateStatusHeaderView() {
+std::unique_ptr<views::View> PhoneHubUiController::CreateStatusHeaderView(
+    PhoneStatusView::Delegate* delegate) {
   if (!phone_hub_manager_)
     return nullptr;
-  return std::make_unique<PhoneStatusView>(phone_hub_manager_->GetPhoneModel());
+  return std::make_unique<PhoneStatusView>(phone_hub_manager_->GetPhoneModel(),
+                                           delegate);
 }
 
 std::unique_ptr<PhoneHubContentView> PhoneHubUiController::CreateContentView(

ash/system/phonehub/phone_hub_ui_controller.h

@@ -7,6 +7,7 @@
 
 #include "ash/ash_export.h"
 #include "ash/system/phonehub/phone_hub_content_view.h"
+#include "ash/system/phonehub/phone_status_view.h"
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
 #include "chromeos/components/phonehub/feature_status_provider.h"
@@ -67,7 +68,8 @@ class ASH_EXPORT PhoneHubUiController
       TrayBubbleView* bubble_view);
 
   // Creates the header view displaying the phone status.
-  std::unique_ptr<views::View> CreateStatusHeaderView();
+  std::unique_ptr<views::View> CreateStatusHeaderView(
+      PhoneStatusView::Delegate* delegate);
 
   // Observer functions.
   void AddObserver(Observer* observer);

ash/system/phonehub/phone_status_view.cc

@@ -6,13 +6,10 @@
 
 #include "ash/public/cpp/network_icon_image_source.h"
 #include "ash/public/cpp/shelf_config.h"
-#include "ash/public/cpp/system_tray_client.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/root_window_controller.h"
-#include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/style/ash_color_provider.h"
-#include "ash/system/model/system_tray_model.h"
 #include "ash/system/phonehub/phone_hub_tray.h"
 #include "ash/system/phonehub/phone_hub_view_ids.h"
 #include "ash/system/status_area_widget.h"
@@ -62,9 +59,11 @@ int GetSignalStrengthAsInt(PhoneStatusModel::SignalStrength signal_strength) {
 
 }  // namespace
 
-PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model)
+PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model,
+                                 Delegate* delegate)
     : TriView(kTitleContainerSpacing),
       phone_model_(phone_model),
+      delegate_(delegate),
       phone_name_label_(new views::Label),
       signal_icon_(new views::ImageView),
       mobile_provider_label_(new views::Label),
@@ -112,8 +111,9 @@ PhoneStatusView::PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model)
                                            IDS_ASH_STATUS_TRAY_SETTINGS);
   AddView(TriView::Container::END, settings_button_);
 
-  separator->SetVisible(TrayPopupUtils::CanOpenWebUISettings());
-  settings_button_->SetVisible(TrayPopupUtils::CanOpenWebUISettings());
+  DCHECK(delegate_);
+  separator->SetVisible(delegate_->CanOpenConnectedDeviceSettings());
+  settings_button_->SetVisible(delegate_->CanOpenConnectedDeviceSettings());
 
   Update();
 }
@@ -125,8 +125,8 @@ PhoneStatusView::~PhoneStatusView() {
 void PhoneStatusView::ButtonPressed(views::Button* sender,
                                     const ui::Event& event) {
   DCHECK_EQ(settings_button_, sender);
-  DCHECK(TrayPopupUtils::CanOpenWebUISettings());
-  Shell::Get()->system_tray_model()->client()->ShowConnectedDevicesSettings();
+  DCHECK(delegate_);
+  delegate_->OpenConnectedDevicesSettings();
 }
 
 void PhoneStatusView::OnModelChanged() {

ash/system/phonehub/phone_status_view.h

@@ -26,7 +26,14 @@ class ASH_EXPORT PhoneStatusView
       public views::ButtonListener,
       public chromeos::phonehub::PhoneModel::Observer {
  public:
-  explicit PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model);
+  class Delegate {
+   public:
+    virtual bool CanOpenConnectedDeviceSettings() = 0;
+    virtual void OpenConnectedDevicesSettings() = 0;
+  };
+
+  PhoneStatusView(chromeos::phonehub::PhoneModel* phone_model,
+                  Delegate* delegate);
   ~PhoneStatusView() override;
   PhoneStatusView(PhoneStatusView&) = delete;
   PhoneStatusView operator=(PhoneStatusView&) = delete;
@@ -40,6 +47,7 @@ class ASH_EXPORT PhoneStatusView
  private:
   FRIEND_TEST_ALL_PREFIXES(PhoneStatusViewTest, MobileProviderVisibility);
   FRIEND_TEST_ALL_PREFIXES(PhoneStatusViewTest, PhoneStatusLabelsContent);
+  FRIEND_TEST_ALL_PREFIXES(PhoneStatusViewTest, ClickOnSettings);
 
   // Update the labels and icons in the view to display current phone status.
   void Update();
@@ -54,6 +62,7 @@ class ASH_EXPORT PhoneStatusView
   void ConfigureTriViewContainer(TriView::Container container);
 
   chromeos::phonehub::PhoneModel* phone_model_ = nullptr;
+  Delegate* delegate_ = nullptr;
 
   // Owned by views hierarchy.
   views::Label* phone_name_label_ = nullptr;

ash/system/phonehub/phone_status_view_unittest.cc

@@ -17,7 +17,14 @@ namespace ash {
 
 using PhoneStatusModel = chromeos::phonehub::PhoneStatusModel;
 
-class PhoneStatusViewTest : public AshTestBase {
+class DummyEvent : public ui::Event {
+ public:
+  DummyEvent() : Event(ui::ET_UNKNOWN, base::TimeTicks(), 0) {}
+  ~DummyEvent() override = default;
+};
+
+class PhoneStatusViewTest : public AshTestBase,
+                            public PhoneStatusView::Delegate {
  public:
   PhoneStatusViewTest() = default;
   ~PhoneStatusViewTest() override = default;
@@ -27,22 +34,29 @@ class PhoneStatusViewTest : public AshTestBase {
     feature_list_.InitAndEnableFeature(chromeos::features::kPhoneHub);
     AshTestBase::SetUp();
 
-    phone_status_view_ = std::make_unique<PhoneStatusView>(&phone_model_);
+    status_view_ = std::make_unique<PhoneStatusView>(&phone_model_, this);
   }
 
   void TearDown() override {
-    phone_status_view_.reset();
+    status_view_.reset();
     AshTestBase::TearDown();
   }
 
- protected:
-  PhoneStatusView* status_view() { return phone_status_view_.get(); }
-  chromeos::phonehub::MutablePhoneModel* phone_model() { return &phone_model_; }
+  // PhoneStatusView::Delegate:
+  bool CanOpenConnectedDeviceSettings() override {
+    return can_open_connected_device_settings_;
+  }
+
+  void OpenConnectedDevicesSettings() override {
+    connected_device_settings_opened_ = true;
+  }
 
- private:
-  std::unique_ptr<PhoneStatusView> phone_status_view_;
+ protected:
+  std::unique_ptr<PhoneStatusView> status_view_;
   chromeos::phonehub::MutablePhoneModel phone_model_;
   base::test::ScopedFeatureList feature_list_;
+  bool can_open_connected_device_settings_ = false;
+  bool connected_device_settings_opened_ = false;
 };
 
 TEST_F(PhoneStatusViewTest, MobileProviderVisibility) {
@@ -54,25 +68,25 @@ TEST_F(PhoneStatusViewTest, MobileProviderVisibility) {
       PhoneStatusModel(PhoneStatusModel::MobileStatus::kNoSim, metadata,
                        PhoneStatusModel::ChargingState::kNotCharging,
                        PhoneStatusModel::BatterySaverState::kOff, 0);
-  phone_model()->SetPhoneStatusModel(phone_status);
+  phone_model_.SetPhoneStatusModel(phone_status);
   // Mobile provider should not be shown when there is no sim.
-  EXPECT_FALSE(status_view()->mobile_provider_label_->GetVisible());
+  EXPECT_FALSE(status_view_->mobile_provider_label_->GetVisible());
 
   phone_status =
       PhoneStatusModel(PhoneStatusModel::MobileStatus::kSimButNoReception,
                        metadata, PhoneStatusModel::ChargingState::kNotCharging,
                        PhoneStatusModel::BatterySaverState::kOff, 0);
-  phone_model()->SetPhoneStatusModel(phone_status);
+  phone_model_.SetPhoneStatusModel(phone_status);
   // Mobile provider should not be shown when there is no connection.
-  EXPECT_FALSE(status_view()->mobile_provider_label_->GetVisible());
+  EXPECT_FALSE(status_view_->mobile_provider_label_->GetVisible());
 
   phone_status =
       PhoneStatusModel(PhoneStatusModel::MobileStatus::kSimWithReception,
                        metadata, PhoneStatusModel::ChargingState::kNotCharging,
                        PhoneStatusModel::BatterySaverState::kOff, 0);
-  phone_model()->SetPhoneStatusModel(phone_status);
+  phone_model_.SetPhoneStatusModel(phone_status);
   // Mobile provider should be shown when there is a connection.
-  EXPECT_TRUE(status_view()->mobile_provider_label_->GetVisible());
+  EXPECT_TRUE(status_view_->mobile_provider_label_->GetVisible());
 }
 
 TEST_F(PhoneStatusViewTest, PhoneStatusLabelsContent) {
@@ -80,7 +94,7 @@ TEST_F(PhoneStatusViewTest, PhoneStatusLabelsContent) {
   base::string16 expected_provider_text = base::UTF8ToUTF16("Test Provider");
   base::string16 expected_battery_text = base::UTF8ToUTF16("10%");
 
-  phone_model()->SetPhoneName(expected_name_text);
+  phone_model_.SetPhoneName(expected_name_text);
 
   PhoneStatusModel::MobileConnectionMetadata metadata = {
       .signal_strength = PhoneStatusModel::SignalStrength::kZeroBars,
@@ -90,40 +104,54 @@ TEST_F(PhoneStatusViewTest, PhoneStatusLabelsContent) {
       PhoneStatusModel(PhoneStatusModel::MobileStatus::kSimWithReception,
                        metadata, PhoneStatusModel::ChargingState::kNotCharging,
                        PhoneStatusModel::BatterySaverState::kOff, 10);
-  phone_model()->SetPhoneStatusModel(phone_status);
+  phone_model_.SetPhoneStatusModel(phone_status);
 
   // All labels should display phone's status and information.
-  EXPECT_EQ(expected_name_text, status_view()->phone_name_label_->GetText());
+  EXPECT_EQ(expected_name_text, status_view_->phone_name_label_->GetText());
   EXPECT_EQ(expected_provider_text,
-            status_view()->mobile_provider_label_->GetText());
-  EXPECT_EQ(expected_battery_text, status_view()->battery_label_->GetText());
+            status_view_->mobile_provider_label_->GetText());
+  EXPECT_EQ(expected_battery_text, status_view_->battery_label_->GetText());
 
   expected_name_text = base::UTF8ToUTF16("New Phone Name");
   expected_provider_text = base::UTF8ToUTF16("New Provider");
   expected_battery_text = base::UTF8ToUTF16("20%");
 
-  phone_model()->SetPhoneName(expected_name_text);
+  phone_model_.SetPhoneName(expected_name_text);
   metadata.mobile_provider = expected_provider_text;
   phone_status =
       PhoneStatusModel(PhoneStatusModel::MobileStatus::kSimWithReception,
                        metadata, PhoneStatusModel::ChargingState::kNotCharging,
                        PhoneStatusModel::BatterySaverState::kOff, 20);
-  phone_model()->SetPhoneStatusModel(phone_status);
+  phone_model_.SetPhoneStatusModel(phone_status);
 
   // Changes in the model should be reflected in the labels.
-  EXPECT_EQ(expected_name_text, status_view()->phone_name_label_->GetText());
+  EXPECT_EQ(expected_name_text, status_view_->phone_name_label_->GetText());
   EXPECT_EQ(expected_provider_text,
-            status_view()->mobile_provider_label_->GetText());
-  EXPECT_EQ(expected_battery_text, status_view()->battery_label_->GetText());
+            status_view_->mobile_provider_label_->GetText());
+  EXPECT_EQ(expected_battery_text, status_view_->battery_label_->GetText());
 
   // Simulate phone disconnected with a null |PhoneStatusModel| returned.
-  phone_model()->SetPhoneStatusModel(base::nullopt);
+  phone_model_.SetPhoneStatusModel(base::nullopt);
 
   // Existing phone status will be cleared to reflect the model change.
-  EXPECT_TRUE(status_view()->mobile_provider_label_->GetText().empty());
-  EXPECT_TRUE(status_view()->battery_label_->GetText().empty());
-  EXPECT_TRUE(status_view()->battery_icon_->GetImage().isNull());
-  EXPECT_TRUE(status_view()->signal_icon_->GetImage().isNull());
+  EXPECT_TRUE(status_view_->mobile_provider_label_->GetText().empty());
+  EXPECT_TRUE(status_view_->battery_label_->GetText().empty());
+  EXPECT_TRUE(status_view_->battery_icon_->GetImage().isNull());
+  EXPECT_TRUE(status_view_->signal_icon_->GetImage().isNull());
+}
+
+TEST_F(PhoneStatusViewTest, ClickOnSettings) {
+  // The settings button is not visible if we can't open the settings.
+  EXPECT_FALSE(status_view_->settings_button_->GetVisible());
+
+  // The settings button is visible if we can open settings.
+  can_open_connected_device_settings_ = true;
+  status_view_ = std::make_unique<PhoneStatusView>(&phone_model_, this);
+  EXPECT_TRUE(status_view_->settings_button_->GetVisible());
+
+  // Click on the settings button.
+  status_view_->ButtonPressed(status_view_->settings_button_, DummyEvent());
+  EXPECT_TRUE(connected_device_settings_opened_);
 }
 
 }  // namespace ash