Revert "[CrOS PhoneHub] Disable Locate Phone button when Silence Phone is on."

This reverts commit 55f62f27.

Reason for revert: causes QuickActionsViewTest.LocatePhoneToggle failures 
https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/20941

Original change's description:
> [CrOS PhoneHub] Disable Locate Phone button when Silence Phone is on.
>
> - Add an observer for state change in Silence Phone so that Locate Phone
> can be disable when Silence Phone is on.
> - Refactor InitQuickActionsItems() since Locate Phone and Silence Phone
> is no longer independent.
>
> BUG=1106937,1126208
>
> Change-Id: I6f015e868d6d00df6a40447de62f3efd0b2f2ad0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2480662
> Reviewed-by: Tim Song <tengs@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Commit-Queue: Andre Le <leandre@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#819161}

TBR=khorimoto@chromium.org,tengs@chromium.org,leandre@chromium.org

Change-Id: Ibcaef1859c90cdfd1b4fb4ff1bf686e28601787f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1106937
Bug: 1126208
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2489304Reviewed-by: Anatoliy Potapchuk <apotapchuk@chromium.org>
Commit-Queue: Anatoliy Potapchuk <apotapchuk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819303}

ash/BUILD.gn

@@ -2165,7 +2165,6 @@ test("ash_unittests") {
     "system/phonehub/phone_hub_ui_controller_unittest.cc",
     "system/phonehub/phone_status_view_unittest.cc",
     "system/phonehub/quick_actions_view_unittest.cc",
-    "system/phonehub/silence_phone_quick_action_controller_unittest.cc",
     "system/phonehub/task_continuation_view_unittest.cc",
     "system/power/backlights_forced_off_setter_unittest.cc",
     "system/power/peripheral_battery_notifier_unittest.cc",

ash/system/phonehub/locate_phone_quick_action_controller.cc

@@ -7,7 +7,6 @@
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/phonehub/quick_action_item.h"
-#include "ash/system/phonehub/silence_phone_quick_action_controller.h"
 #include "base/timer/timer.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -25,19 +24,14 @@ constexpr base::TimeDelta kWaitForRequestTimeout =
 using Status = chromeos::phonehub::FindMyDeviceController::Status;
 
 LocatePhoneQuickActionController::LocatePhoneQuickActionController(
-    chromeos::phonehub::FindMyDeviceController* find_my_device_controller,
-    SilencePhoneQuickActionController* silence_phone_controller)
-    : find_my_device_controller_(find_my_device_controller),
-      silence_phone_controller_(silence_phone_controller) {
+    chromeos::phonehub::FindMyDeviceController* find_my_device_controller)
+    : find_my_device_controller_(find_my_device_controller) {
   DCHECK(find_my_device_controller_);
-  DCHECK(silence_phone_controller_);
   find_my_device_controller_->AddObserver(this);
-  silence_phone_controller_->AddObserver(this);
 }
 
 LocatePhoneQuickActionController::~LocatePhoneQuickActionController() {
   find_my_device_controller_->RemoveObserver(this);
-  silence_phone_controller_->RemoveObserver(this);
 }
 
 QuickActionItem* LocatePhoneQuickActionController::CreateItem() {
@@ -61,35 +55,20 @@ void LocatePhoneQuickActionController::OnButtonPressed(bool is_now_enabled) {
   find_my_device_controller_->RequestNewPhoneRingingState(!is_now_enabled);
 }
 
-void LocatePhoneQuickActionController::OnSilencePhoneItemStateChanged() {
-  is_silence_enabled_ = silence_phone_controller_->IsItemEnabled();
-  UpdateState();
-}
-
 void LocatePhoneQuickActionController::OnPhoneRingingStateChanged() {
-  UpdateState();
-}
-
-void LocatePhoneQuickActionController::UpdateState() {
-  // Disable Locate Phone if Silence Phone is on, otherwise change accordingly
-  // based on status from FindMyDeviceController.
-  if (is_silence_enabled_) {
-    state_ = ActionState::kNotAvailable;
-  } else {
-    switch (find_my_device_controller_->GetPhoneRingingStatus()) {
-      case Status::kRingingOff:
-        state_ = ActionState::kOff;
-        break;
-      case Status::kRingingOn:
-        state_ = ActionState::kOn;
-        break;
-      case Status::kRingingNotAvailable:
-        state_ = ActionState::kNotAvailable;
-        break;
-    }
+  switch (find_my_device_controller_->GetPhoneRingingStatus()) {
+    case Status::kRingingOff:
+      state_ = ActionState::kOff;
+      break;
+    case Status::kRingingOn:
+      state_ = ActionState::kOn;
+      break;
+    case Status::kRingingNotAvailable:
+      item_->SetEnabled(false);
+      return;
   }
-
   SetItemState(state_);
+  item_->SetEnabled(true);
 
   // If |requested_state_| correctly resembles the current state, reset it and
   // the timer.
@@ -104,9 +83,6 @@ void LocatePhoneQuickActionController::SetItemState(ActionState state) {
   int state_text_id;
   int sub_label_text;
   switch (state) {
-    case ActionState::kNotAvailable:
-      item_->SetEnabled(false);
-      return;
     case ActionState::kOff:
       icon_enabled = false;
       state_text_id = IDS_ASH_PHONE_HUB_QUICK_ACTIONS_DISABLED_STATE_TOOLTIP;
@@ -119,7 +95,6 @@ void LocatePhoneQuickActionController::SetItemState(ActionState state) {
       break;
   }
 
-  item_->SetEnabled(true);
   item_->SetToggled(icon_enabled);
   item_->SetSubLabel(l10n_util::GetStringUTF16(sub_label_text));
   base::string16 tooltip_state =

ash/system/phonehub/locate_phone_quick_action_controller.h

@@ -5,7 +5,7 @@
 #ifndef ASH_SYSTEM_PHONEHUB_LOCATE_PHONE_QUICK_ACTION_CONTROLLER_H_
 #define ASH_SYSTEM_PHONEHUB_LOCATE_PHONE_QUICK_ACTION_CONTROLLER_H_
 
-#include "ash/system/phonehub/silence_phone_quick_action_controller.h"
+#include "ash/system/phonehub/quick_action_controller_base.h"
 #include "chromeos/components/phonehub/find_my_device_controller.h"
 
 namespace base {
@@ -17,12 +17,10 @@ namespace ash {
 // Controller of a quick action item that toggles Locate phone mode.
 class LocatePhoneQuickActionController
     : public QuickActionControllerBase,
-      public SilencePhoneQuickActionController::Observer,
       public chromeos::phonehub::FindMyDeviceController::Observer {
  public:
-  LocatePhoneQuickActionController(
-      chromeos::phonehub::FindMyDeviceController* find_my_device_controller,
-      SilencePhoneQuickActionController* silence_phone_controller);
+  explicit LocatePhoneQuickActionController(
+      chromeos::phonehub::FindMyDeviceController* find_my_device_controller);
   ~LocatePhoneQuickActionController() override;
   LocatePhoneQuickActionController(LocatePhoneQuickActionController&) = delete;
   LocatePhoneQuickActionController operator=(
@@ -32,20 +30,13 @@ class LocatePhoneQuickActionController
   QuickActionItem* CreateItem() override;
   void OnButtonPressed(bool is_now_enabled) override;
 
-  // SilencePhoneQuickActionController::Observer:
-  void OnSilencePhoneItemStateChanged() override;
-
   // chromeos::phonehub::FindMyDeviceController::Observer:
   void OnPhoneRingingStateChanged() override;
 
  private:
   // All the possible states that the locate phone button can be viewed. Each
   // state has a corresponding icon, labels and tooltip view.
-  enum class ActionState { kNotAvailable, kOff, kOn };
-
-  // Compute and update the state of the item according to Silence Phone item
-  // and FindMyDeviceController.
-  void UpdateState();
+  enum class ActionState { kOff, kOn };
 
   // Set the item (including icon, label and tooltips) to a certain state.
   void SetItemState(ActionState state);
@@ -56,15 +47,11 @@ class LocatePhoneQuickActionController
 
   chromeos::phonehub::FindMyDeviceController* find_my_device_controller_ =
       nullptr;
-  SilencePhoneQuickActionController* silence_phone_controller_ = nullptr;
   QuickActionItem* item_ = nullptr;
 
   // Keep track the current state of the item.
   ActionState state_;
 
-  // Keep track the state of Silence Phone item.
-  bool is_silence_enabled_;
-
   // State that user requests when clicking the button.
   base::Optional<ActionState> requested_state_;
 

ash/system/phonehub/quick_actions_view.cc

@@ -38,25 +38,20 @@ QuickActionsView::QuickActionsView(
 QuickActionsView::~QuickActionsView() = default;
 
 void QuickActionsView::InitQuickActionItems() {
-  auto enable_hotspot_controller =
-      std::make_unique<EnableHotspotQuickActionController>(
-          phone_hub_manager_->GetTetherController());
-  enable_hotspot_ = AddChildView(enable_hotspot_controller->CreateItem());
-  quick_action_controllers_.push_back(std::move(enable_hotspot_controller));
-
-  auto silence_phone_controller =
-      std::make_unique<SilencePhoneQuickActionController>(
-          phone_hub_manager_->GetDoNotDisturbController());
-  silence_phone_ = AddChildView(silence_phone_controller->CreateItem());
-
-  auto locate_phone_controller =
-      std::make_unique<LocatePhoneQuickActionController>(
-          phone_hub_manager_->GetFindMyDeviceController(),
-          silence_phone_controller.get());
-  locate_phone_ = AddChildView(locate_phone_controller->CreateItem());
-
-  quick_action_controllers_.push_back(std::move(silence_phone_controller));
-  quick_action_controllers_.push_back(std::move(locate_phone_controller));
+  enable_hotspot_ =
+      AddItem(std::make_unique<EnableHotspotQuickActionController>(
+          phone_hub_manager_->GetTetherController()));
+  silence_phone_ = AddItem(std::make_unique<SilencePhoneQuickActionController>(
+      phone_hub_manager_->GetDoNotDisturbController()));
+  locate_phone_ = AddItem(std::make_unique<LocatePhoneQuickActionController>(
+      phone_hub_manager_->GetFindMyDeviceController()));
+}
+
+QuickActionItem* QuickActionsView::AddItem(
+    std::unique_ptr<QuickActionControllerBase> controller) {
+  auto* item = AddChildView(controller->CreateItem());
+  quick_action_controllers_.push_back(std::move(controller));
+  return item;
 }
 
 }  // namespace ash

ash/system/phonehub/quick_actions_view.h

@@ -32,6 +32,10 @@ class ASH_EXPORT QuickActionsView : public views::View {
   // Add all the quick actions items to the view.
   void InitQuickActionItems();
 
+  // Helper function to add an item to the view given its controller.
+  QuickActionItem* AddItem(
+      std::unique_ptr<QuickActionControllerBase> controller);
+
   // Controllers of quick actions items. Owned by this.
   std::vector<std::unique_ptr<QuickActionControllerBase>>
       quick_action_controllers_;

ash/system/phonehub/quick_actions_view_unittest.cc

@@ -105,14 +105,10 @@ TEST_F(QuickActionsViewTest, SilencePhoneToggle) {
                                                              DummyEvent());
   EXPECT_TRUE(dnd_controller()->IsDndEnabled());
 
-  // Locate phone should be disabled when do not disturb is enabled.
-  EXPECT_FALSE(actions_view()->locate_phone_for_testing()->GetEnabled());
-
   // Togge again to disable.
   actions_view()->silence_phone_for_testing()->ButtonPressed(nullptr,
                                                              DummyEvent());
   EXPECT_FALSE(dnd_controller()->IsDndEnabled());
-  EXPECT_TRUE(actions_view()->locate_phone_for_testing()->GetEnabled());
 
   // Test the error state.
   dnd_controller()->SetShouldRequestFail(true);

ash/system/phonehub/silence_phone_quick_action_controller.cc

@@ -32,18 +32,6 @@ SilencePhoneQuickActionController::~SilencePhoneQuickActionController() {
   dnd_controller_->RemoveObserver(this);
 }
 
-void SilencePhoneQuickActionController::AddObserver(Observer* observer) {
-  observer_list_.AddObserver(observer);
-}
-
-void SilencePhoneQuickActionController::RemoveObserver(Observer* observer) {
-  observer_list_.RemoveObserver(observer);
-}
-
-bool SilencePhoneQuickActionController::IsItemEnabled() {
-  return item_->IsToggled();
-}
-
 QuickActionItem* SilencePhoneQuickActionController::CreateItem() {
   DCHECK(!item_);
   item_ = new QuickActionItem(this, IDS_ASH_PHONE_HUB_SILENCE_PHONE_TITLE,
@@ -102,9 +90,6 @@ void SilencePhoneQuickActionController::SetItemState(ActionState state) {
   item_->SetIconTooltip(
       l10n_util::GetStringFUTF16(IDS_ASH_PHONE_HUB_QUICK_ACTIONS_TOGGLE_TOOLTIP,
                                  item_->GetItemLabel(), tooltip_state));
-
-  for (auto& observer : observer_list_)
-    observer.OnSilencePhoneItemStateChanged();
 }
 
 void SilencePhoneQuickActionController::CheckRequestedState() {

ash/system/phonehub/silence_phone_quick_action_controller.h

@@ -6,8 +6,6 @@
 #define ASH_SYSTEM_PHONEHUB_SILENCE_PHONE_QUICK_ACTION_CONTROLLER_H_
 
 #include "ash/system/phonehub/quick_action_controller_base.h"
-#include "base/observer_list.h"
-#include "base/observer_list_types.h"
 #include "chromeos/components/phonehub/do_not_disturb_controller.h"
 
 namespace base {
@@ -17,18 +15,10 @@ class OneShotTimer;
 namespace ash {
 
 // Controller of a quick action item that toggles silence phone mode.
-class ASH_EXPORT SilencePhoneQuickActionController
+class SilencePhoneQuickActionController
     : public QuickActionControllerBase,
       public chromeos::phonehub::DoNotDisturbController::Observer {
  public:
-  class Observer : public base::CheckedObserver {
-   public:
-    ~Observer() override = default;
-
-    // Called when the state of the item has changed.
-    virtual void OnSilencePhoneItemStateChanged() = 0;
-  };
-
   explicit SilencePhoneQuickActionController(
       chromeos::phonehub::DoNotDisturbController* dnd_controller);
   ~SilencePhoneQuickActionController() override;
@@ -37,12 +27,6 @@ class ASH_EXPORT SilencePhoneQuickActionController
   SilencePhoneQuickActionController operator=(
       SilencePhoneQuickActionController&) = delete;
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
-
-  // Return true if the item is enabled/toggled.
-  bool IsItemEnabled();
-
   // QuickActionControllerBase:
   QuickActionItem* CreateItem() override;
   void OnButtonPressed(bool is_now_enabled) override;
@@ -75,9 +59,6 @@ class ASH_EXPORT SilencePhoneQuickActionController
   // if the requested state is similar to the current state after the button is
   // pressed for a certain time.
   std::unique_ptr<base::OneShotTimer> check_requested_state_timer_;
-
-  // Registered observers.
-  base::ObserverList<Observer> observer_list_;
 };
 
 }  // namespace ash

ash/system/phonehub/silence_phone_quick_action_controller_unittest.cc

-// Copyright 2020 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/system/phonehub/silence_phone_quick_action_controller.h"
-
-#include "ash/test/ash_test_base.h"
-#include "chromeos/components/phonehub/fake_do_not_disturb_controller.h"
-
-namespace ash {
-
-class SilencePhoneQuickActionControllerTest
-    : public AshTestBase,
-      public SilencePhoneQuickActionController::Observer {
- public:
-  SilencePhoneQuickActionControllerTest() = default;
-
-  ~SilencePhoneQuickActionControllerTest() override = default;
-
-  // AshTestBase:
-  void SetUp() override {
-    AshTestBase::SetUp();
-
-    dnd_controller_ =
-        std::make_unique<chromeos::phonehub::FakeDoNotDisturbController>();
-    controller_ = std::make_unique<SilencePhoneQuickActionController>(
-        dnd_controller_.get());
-
-    controller_->AddObserver(this);
-    controller_->CreateItem();
-  }
-
-  void TearDown() override {
-    controller_->RemoveObserver(this);
-    controller_.reset();
-    dnd_controller_.reset();
-    AshTestBase::TearDown();
-  }
-
- protected:
-  // SilencePhoneQuickActionController::Observer:
-  void OnSilencePhoneItemStateChanged() override { ++num_calls_; }
-
-  SilencePhoneQuickActionController* controller() { return controller_.get(); }
-
-  chromeos::phonehub::FakeDoNotDisturbController* dnd_controller() {
-    return dnd_controller_.get();
-  }
-
-  size_t GetNumObserverCalls() { return num_calls_; }
-
- private:
-  std::unique_ptr<SilencePhoneQuickActionController> controller_;
-  std::unique_ptr<chromeos::phonehub::FakeDoNotDisturbController>
-      dnd_controller_;
-  size_t num_calls_ = 0;
-};
-
-TEST_F(SilencePhoneQuickActionControllerTest, ItemStateChanged) {
-  // Set request to fail to avoid triggering state's changes by the model.
-  dnd_controller()->SetShouldRequestFail(true);
-
-  // Initially, there's one observer call during initiation.
-  EXPECT_EQ(1u, GetNumObserverCalls());
-
-  // Press the button to enabled state will trigger observer.
-  controller()->OnButtonPressed(false /* is_now_enabled */);
-  EXPECT_EQ(2u, GetNumObserverCalls());
-
-  // Item state changed to enabled.
-  EXPECT_TRUE(controller()->IsItemEnabled());
-
-  // Press the button to disabled state will trigger observer.
-  controller()->OnButtonPressed(true /* is_now_enabled */);
-  EXPECT_EQ(3u, GetNumObserverCalls());
-
-  // Item state changed to disabled.
-  EXPECT_FALSE(controller()->IsItemEnabled());
-
-  dnd_controller()->SetShouldRequestFail(false);
-}
-
-}  // namespace ash