Revert "Add additional states to FindMyDeviceController"

This reverts commit 3792c739.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 812962 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzM3OTJjNzM5M2NjNTExMzExNzhiMThiZTViMzFlOGU3ODUxMjI1ZWUM

Sample Failed Build: https://ci.chromium.org/b/8867601753821372336

Sample Failed Step: compile

Original change's description:
> Add additional states to FindMyDeviceController
> 
> Fully implements the FindMyDeviceControllerImpl class. This change
> also features a new state: kRingingNotAvailable.
> This state is only set if the DoNotDisturb is enabled.
> 
> Bug: 1106937
> Test: chromeos_components_unittests
> Change-Id: Ifeb4dd88ea19ad6949bad2a92d840f392d7ccd47
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438773
> Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#812962}


Change-Id: Ieb095886c387c2fbcb8c08aad64f10092098302b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1106937
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2444469
Cr-Commit-Position: refs/heads/master@{#812987}

chrome/browser/ui/webui/chromeos/multidevice_internals/multidevice_internals_phone_hub_handler.cc

@@ -247,12 +247,12 @@ void MultidevicePhoneHubHandler::OnDndStateChanged() {
 }
 
 void MultidevicePhoneHubHandler::OnPhoneRingingStateChanged() {
-  phonehub::FindMyDeviceController::Status ringing_status =
-      fake_phone_hub_manager_->fake_find_my_device_controller()
-          ->GetPhoneRingingStatus();
-
+  // TODO(jimmyxgong): Change to casting the enum TBA to int.
+  bool is_ringing = fake_phone_hub_manager_->fake_find_my_device_controller()
+                        ->IsPhoneRinging();
+  int status_as_int = is_ringing ? 2 : 1;
   FireWebUIListener("find-my-device-status-changed",
-                    base::Value(static_cast<int>(ringing_status)));
+                    base::Value(status_as_int));
 }
 
 void MultidevicePhoneHubHandler::OnTetherStatusChanged() {

chromeos/components/phonehub/fake_find_my_device_controller.cc

@@ -11,33 +11,22 @@ FakeFindMyDeviceController::FakeFindMyDeviceController() = default;
 
 FakeFindMyDeviceController::~FakeFindMyDeviceController() = default;
 
-void FakeFindMyDeviceController::SetPhoneRingingState(
-    Status phone_ringing_status) {
-  if (phone_ringing_status_ == phone_ringing_status)
-    return;
-  phone_ringing_status_ = phone_ringing_status;
-  NotifyPhoneRingingStateChanged();
+bool FakeFindMyDeviceController::IsPhoneRinging() const {
+  return is_phone_ringing_;
 }
 
 void FakeFindMyDeviceController::SetIsPhoneRingingInternal(
     bool is_phone_ringing) {
-  Status phone_ringing_status =
-      is_phone_ringing ? Status::kRingingOn : Status::kRingingOff;
-
-  if (phone_ringing_status_ == Status::kRingingNotAvailable)
+  if (is_phone_ringing_ == is_phone_ringing)
     return;
 
-  SetPhoneRingingState(phone_ringing_status);
+  is_phone_ringing_ = is_phone_ringing;
+  NotifyPhoneRingingStateChanged();
 }
 
 void FakeFindMyDeviceController::RequestNewPhoneRingingState(bool ringing) {
   SetIsPhoneRingingInternal(ringing);
 }
 
-FindMyDeviceController::Status
-FakeFindMyDeviceController::GetPhoneRingingStatus() {
-  return phone_ringing_status_;
-}
-
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/fake_find_my_device_controller.h

@@ -15,15 +15,13 @@ class FakeFindMyDeviceController : public FindMyDeviceController {
   FakeFindMyDeviceController();
   ~FakeFindMyDeviceController() override;
 
-  void SetPhoneRingingState(Status status);
-
   // FindMyDeviceController:
+  bool IsPhoneRinging() const override;
   void SetIsPhoneRingingInternal(bool is_phone_ringing) override;
   void RequestNewPhoneRingingState(bool ringing) override;
-  Status GetPhoneRingingStatus() override;
 
  private:
-  Status phone_ringing_status_ = Status::kRingingOff;
+  bool is_phone_ringing_ = false;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/find_my_device_controller.h

@@ -22,31 +22,20 @@ class FindMyDeviceController {
     virtual void OnPhoneRingingStateChanged() = 0;
   };
 
-  enum class Status {
-    // The connected phone is not currently ringing.
-    kRingingOff = 0,
-    // The connected phone is currently ringing.
-    kRingingOn = 1,
-    // Ringing is not available if the phone's DoNotDisturb mode is enabled.
-    // To re-enable ringing, DoNotDisturb mode must be disabled.
-    kRingingNotAvailable = 2,
-  };
-
   FindMyDeviceController(const FindMyDeviceController&) = delete;
   FindMyDeviceController& operator=(const FindMyDeviceController&) = delete;
   virtual ~FindMyDeviceController();
 
+  // Returns whether the phone is ringing as a result of Find My Device
+  // functionality. Note that this function does not return true if the phone is
+  // ringing for another reason (e.g., a normal phone call).
+  virtual bool IsPhoneRinging() const = 0;
+
   // Note: Ringing the phone via Find My Device is not a synchronous operation,
   // since it requires sending a message to the connected phone. Use the
   // observer interface to be notified of when the state changes.
   virtual void RequestNewPhoneRingingState(bool ringing) = 0;
 
-  // Returns the current ringing state of the connected phone. There are three
-  // possible states (on, off, disabled). The status is a result of Find My
-  // Device Functionality. Note that this function does not return true if the
-  // phone is ringing for another reason (e.g., a normal phone call)
-  virtual Status GetPhoneRingingStatus() = 0;
-
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 

chromeos/components/phonehub/find_my_device_controller_impl.cc

@@ -5,70 +5,26 @@
 #include "chromeos/components/phonehub/find_my_device_controller_impl.h"
 
 #include "chromeos/components/multidevice/logging/logging.h"
-#include "chromeos/components/phonehub/message_sender.h"
 
 namespace chromeos {
 namespace phonehub {
 
-FindMyDeviceControllerImpl::FindMyDeviceControllerImpl(
-    DoNotDisturbController* do_not_disturb_controller,
-    MessageSender* message_sender)
-    : do_not_disturb_controller_(do_not_disturb_controller),
-      message_sender_(message_sender) {
-  DCHECK(do_not_disturb_controller_);
-  DCHECK(message_sender_);
+FindMyDeviceControllerImpl::FindMyDeviceControllerImpl() = default;
 
-  do_not_disturb_controller_->AddObserver(this);
-}
+FindMyDeviceControllerImpl::~FindMyDeviceControllerImpl() = default;
 
-FindMyDeviceControllerImpl::~FindMyDeviceControllerImpl() {
-  do_not_disturb_controller_->RemoveObserver(this);
+bool FindMyDeviceControllerImpl::IsPhoneRinging() const {
+  return is_phone_ringing_;
 }
 
 void FindMyDeviceControllerImpl::SetIsPhoneRingingInternal(
     bool is_phone_ringing) {
   is_phone_ringing_ = is_phone_ringing;
-  UpdateStatus();
-}
-
-FindMyDeviceController::Status
-FindMyDeviceControllerImpl::GetPhoneRingingStatus() {
-  return phone_ringing_status_;
 }
 
 void FindMyDeviceControllerImpl::RequestNewPhoneRingingState(bool ringing) {
-  if (phone_ringing_status_ == Status::kRingingNotAvailable) {
-    PA_LOG(WARNING) << "Cannot request new ringing status because DoNotDisturb "
-                    << "mode is enabled.";
-    return;
-  }
-
   PA_LOG(INFO) << "Attempting to set Find My Device phone ring state; new "
                << "value: " << ringing;
-  message_sender_->SendRingDeviceRequest(ringing);
-}
-
-void FindMyDeviceControllerImpl::OnDndStateChanged() {
-  UpdateStatus();
-}
-
-FindMyDeviceController::Status FindMyDeviceControllerImpl::ComputeStatus()
-    const {
-  if (do_not_disturb_controller_->IsDndEnabled()) {
-    PA_LOG(WARNING) << "Cannot set ringing status because DoNotDisturb mode is "
-                    << "enabled.";
-    return Status::kRingingNotAvailable;
-  }
-  return is_phone_ringing_ ? Status::kRingingOn : Status::kRingingOff;
-}
-
-void FindMyDeviceControllerImpl::UpdateStatus() {
-  Status status = ComputeStatus();
-  if (phone_ringing_status_ == status)
-    return;
-
-  phone_ringing_status_ = status;
-  NotifyPhoneRingingStateChanged();
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/find_my_device_controller_impl.h

@@ -5,42 +5,24 @@
 #ifndef CHROMEOS_COMPONENTS_PHONEHUB_FIND_MY_DEVICE_CONTROLLER_IMPL_H_
 #define CHROMEOS_COMPONENTS_PHONEHUB_FIND_MY_DEVICE_CONTROLLER_IMPL_H_
 
-#include "chromeos/components/phonehub/do_not_disturb_controller.h"
 #include "chromeos/components/phonehub/find_my_device_controller.h"
 
 namespace chromeos {
 namespace phonehub {
 
-class MessageSender;
-
-// Responsible for sending and receiving updates in regards to the Find My
-// Device feature which involves ringing the user's remote phone.
-class FindMyDeviceControllerImpl : public FindMyDeviceController,
-                                   public DoNotDisturbController::Observer {
+// TODO(https://crbug.com/1106937): Add real implementation.
+class FindMyDeviceControllerImpl : public FindMyDeviceController {
  public:
-  FindMyDeviceControllerImpl(DoNotDisturbController* do_not_disturb_controller,
-                             MessageSender* message_sender);
+  FindMyDeviceControllerImpl();
   ~FindMyDeviceControllerImpl() override;
 
  private:
-  friend class FindMyDeviceControllerImplTest;
-
-  Status ComputeStatus() const;
-  void UpdateStatus();
-
   // FindMyDeviceController:
+  bool IsPhoneRinging() const override;
   void SetIsPhoneRingingInternal(bool is_phone_ringing) override;
   void RequestNewPhoneRingingState(bool ringing) override;
-  Status GetPhoneRingingStatus() override;
-
-  // DoNotDisturbController::Observer:
-  void OnDndStateChanged() override;
 
   bool is_phone_ringing_ = false;
-  Status phone_ringing_status_ = Status::kRingingOff;
-
-  DoNotDisturbController* do_not_disturb_controller_;
-  MessageSender* message_sender_;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/find_my_device_controller_impl_unittest.cc

@@ -6,9 +6,6 @@
 
 #include <memory>
 
-#include "chromeos/components/phonehub/fake_do_not_disturb_controller.h"
-#include "chromeos/components/phonehub/fake_message_sender.h"
-#include "chromeos/components/phonehub/find_my_device_controller.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -42,101 +39,25 @@ class FindMyDeviceControllerImplTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    fake_do_not_disturb_controller_ =
-        std::make_unique<FakeDoNotDisturbController>();
-    fake_message_sender_ = std::make_unique<FakeMessageSender>();
-    controller_ = std::make_unique<FindMyDeviceControllerImpl>(
-        fake_do_not_disturb_controller_.get(), fake_message_sender_.get());
+    controller_ = std::make_unique<FindMyDeviceControllerImpl>();
     controller_->AddObserver(&fake_observer_);
   }
 
   void TearDown() override { controller_->RemoveObserver(&fake_observer_); }
 
-  FindMyDeviceController::Status GetPhoneRingingStatus() const {
-    return controller_->GetPhoneRingingStatus();
-  }
-
-  void SetIsPhoneRingingInternal(bool is_phone_ringing) {
-    controller_->SetIsPhoneRingingInternal(is_phone_ringing);
-  }
-
-  void RequestNewPhoneRingingState(bool ringing) {
-    controller_->RequestNewPhoneRingingState(ringing);
-  }
+  bool IsPhoneRinging() const { return controller_->IsPhoneRinging(); }
 
   size_t GetNumObserverCalls() const { return fake_observer_.num_calls(); }
 
- protected:
-  std::unique_ptr<FakeDoNotDisturbController> fake_do_not_disturb_controller_;
-  std::unique_ptr<FakeMessageSender> fake_message_sender_;
-
  private:
-  std::unique_ptr<FindMyDeviceControllerImpl> controller_;
   FakeObserver fake_observer_;
+  std::unique_ptr<FindMyDeviceController> controller_;
 };
 
-TEST_F(FindMyDeviceControllerImplTest, RingingStateChanges) {
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingOff,
-            GetPhoneRingingStatus());
-
-  // Simulate flipping DoNotDisturb mode to enabled, this should set the
-  // FindMyPhone status to kRingingNotAvailable.
-  fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
-      /*is_dnd_enabled=*/true);
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingNotAvailable,
-            GetPhoneRingingStatus());
-  // Simulate initiating phone ringing when DoNotDisturb mode is enabled. This
-  // will not update the internal status.
-  SetIsPhoneRingingInternal(/*is_phone_ringing=*/true);
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingNotAvailable,
-            GetPhoneRingingStatus());
-  EXPECT_EQ(1u, GetNumObserverCalls());
-
-  // Flip DoNotDisturb back to disabled, expect status to reset back to its
-  // previous state.
-  fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
-      /*is_dnd_enabled=*/false);
-  // Since we previously recorded that the phone should be ringing during
-  // DoNotDisturb mode was enabled, we return to that state once DoNotDisturb
-  // is disabled.
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
-            GetPhoneRingingStatus());
-  EXPECT_EQ(2u, GetNumObserverCalls());
-
-  // Attempt to set ringing status with the same previous state. Expect that no
-  // observer calls were made.
-  SetIsPhoneRingingInternal(/*is_phone_ringing=*/true);
-  EXPECT_EQ(2u, GetNumObserverCalls());
-}
-
-TEST_F(FindMyDeviceControllerImplTest, RequestNewRingStatus) {
-  RequestNewPhoneRingingState(/*ringing=*/true);
-  EXPECT_EQ(1u, fake_message_sender_->GetRingDeviceRequestCallCount());
-  EXPECT_TRUE(fake_message_sender_->GetRecentRingDeviceRequest());
-
-  // Simulate flipping DoNotDisturb mode to enabled, this should set the
-  // FindMyPhone status to kRingingNotAvailable and not send any new messages.
-  fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
-      /*is_dnd_enabled=*/true);
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingNotAvailable,
-            GetPhoneRingingStatus());
-
-  RequestNewPhoneRingingState(/*ringing=*/false);
-  EXPECT_EQ(1u, fake_message_sender_->GetRingDeviceRequestCallCount());
-  // No new messages were sent, expect that the last request was still the
-  // previous "true" value.
-  EXPECT_TRUE(fake_message_sender_->GetRecentRingDeviceRequest());
-
-  // Flip DoNotDisturb mode to disabled, expect that messages are able to be
-  // sent again.
-  fake_do_not_disturb_controller_->SetDoNotDisturbStateInternal(
-      /*is_dnd_enabled=*/false);
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingOff,
-            GetPhoneRingingStatus());
-
-  RequestNewPhoneRingingState(/*ringing=*/false);
-  EXPECT_EQ(2u, fake_message_sender_->GetRingDeviceRequestCallCount());
-  EXPECT_FALSE(fake_message_sender_->GetRecentRingDeviceRequest());
+// TODO(https://crbug.com/1106937): Remove this test once we have real
+// functionality to test.
+TEST_F(FindMyDeviceControllerImplTest, Initialize) {
+  EXPECT_FALSE(IsPhoneRinging());
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -45,9 +45,8 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
       connection_scheduler_(std::make_unique<ConnectionSchedulerImpl>(
           connection_manager_.get(),
           feature_status_provider_.get())),
-      find_my_device_controller_(std::make_unique<FindMyDeviceControllerImpl>(
-          do_not_disturb_controller_.get(),
-          message_sender_.get())),
+      find_my_device_controller_(
+          std::make_unique<FindMyDeviceControllerImpl>()),
       notification_access_manager_(
           std::make_unique<NotificationAccessManagerImpl>(pref_service)),
       notification_manager_(std::make_unique<NotificationManagerImpl>()),

chromeos/components/phonehub/phone_status_processor_unittest.cc

@@ -137,8 +137,7 @@ TEST_F(PhoneStatusProcessorTest, PhoneStatusSnapshotUpdate) {
   EXPECT_EQ(base::UTF8ToUTF16(test_remote_device_.name()),
             *mutable_phone_model_->phone_name());
   EXPECT_TRUE(fake_do_not_disturb_controller_->IsDndEnabled());
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
-            fake_find_my_device_controller_->GetPhoneRingingStatus());
+  EXPECT_TRUE(fake_find_my_device_controller_->IsPhoneRinging());
   EXPECT_TRUE(fake_notification_access_manager_->HasAccessBeenGranted());
 
   base::Optional<PhoneStatusModel> phone_status_model =
@@ -199,8 +198,7 @@ TEST_F(PhoneStatusProcessorTest, PhoneStatusUpdate) {
   EXPECT_EQ(base::UTF8ToUTF16(test_remote_device_.name()),
             *mutable_phone_model_->phone_name());
   EXPECT_TRUE(fake_do_not_disturb_controller_->IsDndEnabled());
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
-            fake_find_my_device_controller_->GetPhoneRingingStatus());
+  EXPECT_TRUE(fake_find_my_device_controller_->IsPhoneRinging());
   EXPECT_TRUE(fake_notification_access_manager_->HasAccessBeenGranted());
 
   base::Optional<PhoneStatusModel> phone_status_model =
@@ -223,8 +221,7 @@ TEST_F(PhoneStatusProcessorTest, PhoneStatusUpdate) {
   EXPECT_EQ(base::UTF8ToUTF16(test_remote_device_.name()),
             *mutable_phone_model_->phone_name());
   EXPECT_TRUE(fake_do_not_disturb_controller_->IsDndEnabled());
-  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
-            fake_find_my_device_controller_->GetPhoneRingingStatus());
+  EXPECT_TRUE(fake_find_my_device_controller_->IsPhoneRinging());
   EXPECT_TRUE(fake_notification_access_manager_->HasAccessBeenGranted());
 
   phone_status_model = mutable_phone_model_->phone_status_model();