Reland "Add additional states to FindMyDeviceController"

This is a reland of 3792c739

Reason for reland: This is safe to reland as the original revert was
due a merge conflict with crrev/2428735 which has been reverted. This
reland will land first.

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}

Bug: 1106937
Change-Id: Id8400f68f1980dc8288d7c1b2545810721844249
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2444410
Commit-Queue: Jimmy Gong <jimmyxgong@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813053}

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

@@ -247,12 +247,12 @@ void MultidevicePhoneHubHandler::OnDndStateChanged() {
 }
 
 void MultidevicePhoneHubHandler::OnPhoneRingingStateChanged() {
-  // 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;
+  phonehub::FindMyDeviceController::Status ringing_status =
+      fake_phone_hub_manager_->fake_find_my_device_controller()
+          ->GetPhoneRingingStatus();
+
   FireWebUIListener("find-my-device-status-changed",
-                    base::Value(status_as_int));
+                    base::Value(static_cast<int>(ringing_status)));
 }
 
 void MultidevicePhoneHubHandler::OnTetherStatusChanged() {

chromeos/components/phonehub/fake_find_my_device_controller.cc

@@ -11,22 +11,33 @@ FakeFindMyDeviceController::FakeFindMyDeviceController() = default;
 
 FakeFindMyDeviceController::~FakeFindMyDeviceController() = default;
 
-bool FakeFindMyDeviceController::IsPhoneRinging() const {
-  return is_phone_ringing_;
+void FakeFindMyDeviceController::SetPhoneRingingState(
+    Status phone_ringing_status) {
+  if (phone_ringing_status_ == phone_ringing_status)
+    return;
+  phone_ringing_status_ = phone_ringing_status;
+  NotifyPhoneRingingStateChanged();
 }
 
 void FakeFindMyDeviceController::SetIsPhoneRingingInternal(
     bool is_phone_ringing) {
-  if (is_phone_ringing_ == is_phone_ringing)
+  Status phone_ringing_status =
+      is_phone_ringing ? Status::kRingingOn : Status::kRingingOff;
+
+  if (phone_ringing_status_ == Status::kRingingNotAvailable)
     return;
 
-  is_phone_ringing_ = is_phone_ringing;
-  NotifyPhoneRingingStateChanged();
+  SetPhoneRingingState(phone_ringing_status);
 }
 
 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,13 +15,15 @@ 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:
-  bool is_phone_ringing_ = false;
+  Status phone_ringing_status_ = Status::kRingingOff;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/find_my_device_controller.h

@@ -22,20 +22,31 @@ 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,26 +5,70 @@
 #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() = default;
+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);
+}
 
-bool FindMyDeviceControllerImpl::IsPhoneRinging() const {
-  return is_phone_ringing_;
+FindMyDeviceControllerImpl::~FindMyDeviceControllerImpl() {
+  do_not_disturb_controller_->RemoveObserver(this);
 }
 
 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,24 +5,42 @@
 #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 {
 
-// TODO(https://crbug.com/1106937): Add real implementation.
-class FindMyDeviceControllerImpl : public FindMyDeviceController {
+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 {
  public:
-  FindMyDeviceControllerImpl();
+  FindMyDeviceControllerImpl(DoNotDisturbController* do_not_disturb_controller,
+                             MessageSender* message_sender);
   ~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,6 +6,9 @@
 
 #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 {
@@ -39,25 +42,101 @@ class FindMyDeviceControllerImplTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    controller_ = std::make_unique<FindMyDeviceControllerImpl>();
+    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_->AddObserver(&fake_observer_);
   }
 
   void TearDown() override { controller_->RemoveObserver(&fake_observer_); }
 
-  bool IsPhoneRinging() const { return controller_->IsPhoneRinging(); }
+  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);
+  }
 
   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_;
 };
 
-// TODO(https://crbug.com/1106937): Remove this test once we have real
-// functionality to test.
-TEST_F(FindMyDeviceControllerImplTest, Initialize) {
-  EXPECT_FALSE(IsPhoneRinging());
+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());
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/phone_hub_manager_impl.cc

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

chromeos/components/phonehub/phone_status_processor_unittest.cc

@@ -137,7 +137,8 @@ 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_TRUE(fake_find_my_device_controller_->IsPhoneRinging());
+  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
+            fake_find_my_device_controller_->GetPhoneRingingStatus());
   EXPECT_TRUE(fake_notification_access_manager_->HasAccessBeenGranted());
 
   base::Optional<PhoneStatusModel> phone_status_model =
@@ -198,7 +199,8 @@ 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_TRUE(fake_find_my_device_controller_->IsPhoneRinging());
+  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
+            fake_find_my_device_controller_->GetPhoneRingingStatus());
   EXPECT_TRUE(fake_notification_access_manager_->HasAccessBeenGranted());
 
   base::Optional<PhoneStatusModel> phone_status_model =
@@ -221,7 +223,8 @@ 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_TRUE(fake_find_my_device_controller_->IsPhoneRinging());
+  EXPECT_EQ(FindMyDeviceController::Status::kRingingOn,
+            fake_find_my_device_controller_->GetPhoneRingingStatus());
   EXPECT_TRUE(fake_notification_access_manager_->HasAccessBeenGranted());
 
   phone_status_model = mutable_phone_model_->phone_status_model();