[CrOS PhoneHub] Remove connecting state and add timer to find my device.

- Remove kConnecting and add timer to track requested state in find my
device controller.
- Edit FakeFindMyDeviceController to use for unit test.
- Remove a TODO in enable hotspot since unnecessary.

BUG=1106937,1126208

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

ash/system/phonehub/enable_hotspot_quick_action_controller.cc

@@ -39,8 +39,6 @@ QuickActionItem* EnableHotspotQuickActionController::CreateItem() {
 void EnableHotspotQuickActionController::OnButtonPressed(bool is_now_enabled) {
   is_now_enabled ? tether_controller_->Disconnect()
                  : tether_controller_->AttemptConnection();
-  // TODO(leandre): Add a timer to switch back to off state after connecting
-  // failed.
 }
 
 void EnableHotspotQuickActionController::OnTetherStatusChanged() {

ash/system/phonehub/locate_phone_quick_action_controller.cc

@@ -7,10 +7,20 @@
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/phonehub/quick_action_item.h"
+#include "base/timer/timer.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace ash {
 
+namespace {
+
+// Time to wait until we check the state of the phone to prevent showing wrong
+// state
+constexpr base::TimeDelta kWaitForRequestTimeout =
+    base::TimeDelta::FromSeconds(5);
+
+}  // namespace
+
 using Status = chromeos::phonehub::FindMyDeviceController::Status;
 
 LocatePhoneQuickActionController::LocatePhoneQuickActionController(
@@ -33,26 +43,42 @@ QuickActionItem* LocatePhoneQuickActionController::CreateItem() {
 }
 
 void LocatePhoneQuickActionController::OnButtonPressed(bool is_now_enabled) {
-  SetState(ActionState::kConnecting);
+  requested_state_ = is_now_enabled ? ActionState::kOff : ActionState::kOn;
+  SetItemState(requested_state_.value());
+
+  check_requested_state_timer_ = std::make_unique<base::OneShotTimer>();
+  check_requested_state_timer_->Start(
+      FROM_HERE, kWaitForRequestTimeout,
+      base::BindOnce(&LocatePhoneQuickActionController::CheckRequestedState,
+                     base::Unretained(this)));
+
   find_my_device_controller_->RequestNewPhoneRingingState(!is_now_enabled);
 }
 
 void LocatePhoneQuickActionController::OnPhoneRingingStateChanged() {
   switch (find_my_device_controller_->GetPhoneRingingStatus()) {
     case Status::kRingingOff:
-      SetState(ActionState::kOff);
+      state_ = ActionState::kOff;
       break;
     case Status::kRingingOn:
-      SetState(ActionState::kOn);
+      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.
+  if (state_ == requested_state_) {
+    check_requested_state_timer_.reset();
+    requested_state_.reset();
+  }
 }
 
-void LocatePhoneQuickActionController::SetState(ActionState state) {
+void LocatePhoneQuickActionController::SetItemState(ActionState state) {
   bool icon_enabled;
   int state_text_id;
   int sub_label_text;
@@ -62,11 +88,6 @@ void LocatePhoneQuickActionController::SetState(ActionState state) {
       state_text_id = IDS_ASH_PHONE_HUB_QUICK_ACTIONS_DISABLED_STATE_TOOLTIP;
       sub_label_text = IDS_ASH_PHONE_HUB_QUICK_ACTIONS_OFF_STATE;
       break;
-    case ActionState::kConnecting:
-      icon_enabled = true;
-      state_text_id = IDS_ASH_PHONE_HUB_QUICK_ACTIONS_CONNECTING_STATE_TOOLTIP;
-      sub_label_text = IDS_ASH_PHONE_HUB_QUICK_ACTIONS_CONNECTING_STATE;
-      break;
     case ActionState::kOn:
       icon_enabled = true;
       state_text_id = IDS_ASH_PHONE_HUB_QUICK_ACTIONS_ENABLED_STATE_TOOLTIP;
@@ -83,4 +104,14 @@ void LocatePhoneQuickActionController::SetState(ActionState state) {
                                  item_->GetItemLabel(), tooltip_state));
 }
 
+void LocatePhoneQuickActionController::CheckRequestedState() {
+  // If the current state is different from the requested state, it means that
+  // we fail to change the state, so switch back to the original one.
+  if (state_ != requested_state_)
+    SetItemState(state_);
+
+  check_requested_state_timer_.reset();
+  requested_state_.reset();
+}
+
 }  // namespace ash

ash/system/phonehub/locate_phone_quick_action_controller.h

@@ -8,6 +8,10 @@
 #include "ash/system/phonehub/quick_action_controller_base.h"
 #include "chromeos/components/phonehub/find_my_device_controller.h"
 
+namespace base {
+class OneShotTimer;
+}  // namespace base
+
 namespace ash {
 
 // Controller of a quick action item that toggles Locate phone mode.
@@ -32,14 +36,29 @@ class LocatePhoneQuickActionController
  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 { kOff, kConnecting, kOn };
+  enum class ActionState { kOff, kOn };
 
   // Set the item (including icon, label and tooltips) to a certain state.
-  void SetState(ActionState state);
+  void SetItemState(ActionState state);
+
+  // Check to see if the requested state is the same as the current state of the
+  // phone. Make changes to item's state if necessary.
+  void CheckRequestedState();
 
   chromeos::phonehub::FindMyDeviceController* find_my_device_controller_ =
       nullptr;
   QuickActionItem* item_ = nullptr;
+
+  // Keep track the current state of the item.
+  ActionState state_;
+
+  // State that user requests when clicking the button.
+  base::Optional<ActionState> requested_state_;
+
+  // Timer that fires to prevent showing wrong state in the item. It will check
+  // if the requested state is the same as the current state after the button is
+  // pressed for a certain time.
+  std::unique_ptr<base::OneShotTimer> check_requested_state_timer_;
 };
 
 }  // namespace ash

ash/system/phonehub/quick_actions_view_unittest.cc

@@ -143,6 +143,23 @@ TEST_F(QuickActionsViewTest, LocatePhoneToggle) {
                                                             DummyEvent());
   EXPECT_EQ(FindMyDeviceController::Status::kRingingOff,
             find_my_device_controller()->GetPhoneRingingStatus());
+
+  // Test the error state.
+  find_my_device_controller()->SetShouldRequestFail(true);
+  actions_view()->locate_phone_for_testing()->ButtonPressed(nullptr,
+                                                            DummyEvent());
+
+  // In error state, find my device is disabled but the button should still be
+  // on after being pressed.
+  EXPECT_EQ(FindMyDeviceController::Status::kRingingOff,
+            find_my_device_controller()->GetPhoneRingingStatus());
+  EXPECT_TRUE(actions_view()->locate_phone_for_testing()->IsToggled());
+
+  // After a certain time, the button should be corrected to be off.
+  task_environment()->FastForwardBy(kWaitForRequestTimeout);
+  EXPECT_FALSE(actions_view()->locate_phone_for_testing()->IsToggled());
+
+  find_my_device_controller()->SetShouldRequestFail(false);
 }
 
 }  // namespace ash

chromeos/components/phonehub/fake_find_my_device_controller.cc

@@ -31,6 +31,7 @@ void FakeFindMyDeviceController::SetIsPhoneRingingInternal(
 }
 
 void FakeFindMyDeviceController::RequestNewPhoneRingingState(bool ringing) {
+  if (!should_request_fail_)
     SetIsPhoneRingingInternal(ringing);
 }
 
@@ -39,5 +40,10 @@ FakeFindMyDeviceController::GetPhoneRingingStatus() {
   return phone_ringing_status_;
 }
 
+void FakeFindMyDeviceController::SetShouldRequestFail(
+    bool should_request_fail) {
+  should_request_fail_ = should_request_fail;
+}
+
 }  // namespace phonehub
 }  // namespace chromeos

chromeos/components/phonehub/fake_find_my_device_controller.h

@@ -22,8 +22,14 @@ class FakeFindMyDeviceController : public FindMyDeviceController {
   void RequestNewPhoneRingingState(bool ringing) override;
   Status GetPhoneRingingStatus() override;
 
+  void SetShouldRequestFail(bool is_request_fail);
+
  private:
   Status phone_ringing_status_ = Status::kRingingOff;
+
+  // Indicates if the connection to the phone is working correctly. If it is
+  // true, there is a problem and the phone cannot change its state.
+  bool should_request_fail_ = false;
 };
 
 }  // namespace phonehub