Add API to EligibleHostDevicesProvider to retrieve active host devices

Bug: 923594
Change-Id: I1d2bfdb8b7067a7bf524e1e99cc90e2bda3429a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1876735
Commit-Queue: Max Li <themaxli@chromium.org>
Reviewed-by: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709977}

chromeos/services/multidevice_setup/BUILD.gn

@@ -56,6 +56,7 @@ static_library("multidevice_setup") {
     "//base",
     "//chromeos/components/multidevice",
     "//chromeos/components/multidevice/logging",
+    "//chromeos/constants:constants",
     "//chromeos/services/device_sync/proto:util",
     "//chromeos/services/device_sync/public/cpp",
     "//chromeos/services/device_sync/public/mojom",
@@ -142,6 +143,7 @@ source_set("unit_tests") {
     "//base/test:test_support",
     "//chromeos/components/multidevice",
     "//chromeos/components/multidevice:test_support",
+    "//chromeos/constants:constants",
     "//chromeos/services/device_sync/public/cpp:test_support",
     "//chromeos/services/multidevice_setup/public/cpp:oobe_completion_tracker",
     "//chromeos/services/multidevice_setup/public/cpp:prefs",

chromeos/services/multidevice_setup/eligible_host_devices_provider.h

@@ -22,6 +22,13 @@ class EligibleHostDevicesProvider {
   // BETTER_TOGETHER_HOST feature.
   virtual multidevice::RemoteDeviceRefList GetEligibleHostDevices() const = 0;
 
+  // Returns all eligible host devices sorted by the last time they were used
+  // as determined by the server.
+  // TODO(themaxli): This should return a new type (instead of a list of
+  // RemoteDeviceRef) which includes OnlineStatus for display in the UI
+  virtual multidevice::RemoteDeviceRefList GetEligibleActiveHostDevices()
+      const = 0;
+
  protected:
   EligibleHostDevicesProvider() = default;
 

chromeos/services/multidevice_setup/eligible_host_devices_provider_impl.cc

@@ -4,10 +4,12 @@
 
 #include "chromeos/services/multidevice_setup/eligible_host_devices_provider_impl.h"
 
+#include "base/feature_list.h"
 #include "base/memory/ptr_util.h"
 #include "base/no_destructor.h"
 #include "chromeos/components/multidevice/software_feature.h"
 #include "chromeos/components/multidevice/software_feature_state.h"
+#include "chromeos/constants/chromeos_features.h"
 
 namespace chromeos {
 
@@ -58,6 +60,11 @@ EligibleHostDevicesProviderImpl::GetEligibleHostDevices() const {
   return eligible_devices_from_last_sync_;
 }
 
+multidevice::RemoteDeviceRefList
+EligibleHostDevicesProviderImpl::GetEligibleActiveHostDevices() const {
+  return eligible_active_devices_from_last_sync_;
+}
+
 void EligibleHostDevicesProviderImpl::OnNewDevicesSynced() {
   UpdateEligibleDevicesSet();
 }
@@ -73,6 +80,90 @@ void EligibleHostDevicesProviderImpl::UpdateEligibleDevicesSet() {
       eligible_devices_from_last_sync_.push_back(remote_device);
     }
   }
+
+  if (base::FeatureList::IsEnabled(
+          features::kCryptAuthV2DeviceActivityStatus)) {
+    eligible_active_devices_from_last_sync_.clear();
+    eligible_active_devices_from_last_sync_ = eligible_devices_from_last_sync_;
+
+    device_sync_client_->GetDevicesActivityStatus(
+        base::Bind(&EligibleHostDevicesProviderImpl::OnGetDevicesActivityStatus,
+                   base::Unretained(this)));
+  }
+}
+
+void EligibleHostDevicesProviderImpl::OnGetDevicesActivityStatus(
+    device_sync::mojom::NetworkRequestResult network_result,
+    base::Optional<std::vector<device_sync::mojom::DeviceActivityStatusPtr>>
+        devices_activity_status_optional) {
+  if (network_result != device_sync::mojom::NetworkRequestResult::kSuccess ||
+      !devices_activity_status_optional) {
+    return;
+  }
+
+  base::flat_map<std::string, device_sync::mojom::DeviceActivityStatusPtr>
+      id_to_activity_status_map;
+  for (device_sync::mojom::DeviceActivityStatusPtr& device_activity_status_ptr :
+       *devices_activity_status_optional) {
+    id_to_activity_status_map.insert({device_activity_status_ptr->device_id,
+                                      std::move(device_activity_status_ptr)});
+  }
+
+  // Sort the list preferring online devices, then last activity time, then
+  // last update time.
+  std::sort(
+      eligible_active_devices_from_last_sync_.begin(),
+      eligible_active_devices_from_last_sync_.end(),
+      [&id_to_activity_status_map](const auto& first_device,
+                                   const auto& second_device) {
+        // This is actually wrong; we should be using the instance ID here and
+        // not the public key since that's the ID DeviceActivityStatus uses.
+        // TODO(themaxli): update this when instance ID is available on
+        // RemoteDeviceRef.
+        auto it1 = id_to_activity_status_map.find(first_device.public_key());
+        auto it2 = id_to_activity_status_map.find(second_device.public_key());
+        if (it1 == id_to_activity_status_map.end() &&
+            it2 == id_to_activity_status_map.end()) {
+          return first_device.last_update_time_millis() >
+                 second_device.last_update_time_millis();
+        }
+
+        if (it1 == id_to_activity_status_map.end()) {
+          return false;
+        }
+
+        if (it2 == id_to_activity_status_map.end()) {
+          return true;
+        }
+
+        const device_sync::mojom::DeviceActivityStatusPtr&
+            first_activity_status = std::get<1>(*it1);
+        const device_sync::mojom::DeviceActivityStatusPtr&
+            second_activity_status = std::get<1>(*it2);
+
+        if (first_activity_status->connectivity_status ==
+                cryptauthv2::ConnectivityStatus::ONLINE &&
+            second_activity_status->connectivity_status !=
+                cryptauthv2::ConnectivityStatus::ONLINE) {
+          return true;
+        }
+
+        if (second_activity_status->connectivity_status ==
+                cryptauthv2::ConnectivityStatus::ONLINE &&
+            first_activity_status->connectivity_status !=
+                cryptauthv2::ConnectivityStatus::ONLINE) {
+          return false;
+        }
+
+        if (first_activity_status->last_activity_time !=
+            second_activity_status->last_activity_time) {
+          return first_activity_status->last_activity_time >
+                 second_activity_status->last_activity_time;
+        }
+
+        return first_device.last_update_time_millis() >
+               second_device.last_update_time_millis();
+      });
 }
 
 }  // namespace multidevice_setup

chromeos/services/multidevice_setup/eligible_host_devices_provider_impl.h

@@ -40,15 +40,22 @@ class EligibleHostDevicesProviderImpl
 
   // EligibleHostDevicesProvider:
   multidevice::RemoteDeviceRefList GetEligibleHostDevices() const override;
+  multidevice::RemoteDeviceRefList GetEligibleActiveHostDevices()
+      const override;
 
   // device_sync::DeviceSyncClient::Observer:
   void OnNewDevicesSynced() override;
 
   void UpdateEligibleDevicesSet();
 
+  void OnGetDevicesActivityStatus(
+      device_sync::mojom::NetworkRequestResult,
+      base::Optional<std::vector<device_sync::mojom::DeviceActivityStatusPtr>>);
+
   device_sync::DeviceSyncClient* device_sync_client_;
 
   multidevice::RemoteDeviceRefList eligible_devices_from_last_sync_;
+  multidevice::RemoteDeviceRefList eligible_active_devices_from_last_sync_;
 
   DISALLOW_COPY_AND_ASSIGN(EligibleHostDevicesProviderImpl);
 };

chromeos/services/multidevice_setup/eligible_host_devices_provider_impl_unittest.cc

@@ -9,11 +9,14 @@
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "chromeos/components/multidevice/remote_device_test_util.h"
 #include "chromeos/components/multidevice/software_feature.h"
 #include "chromeos/components/multidevice/software_feature_state.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/services/device_sync/proto/cryptauth_api.pb.h"
 #include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
+#include "chromeos/services/device_sync/public/mojom/device_sync.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chromeos {
@@ -27,7 +30,7 @@ const size_t kNumTestDevices = 5;
 }  // namespace
 
 class MultiDeviceSetupEligibleHostDevicesProviderImplTest
-    : public testing::Test {
+    : public testing::TestWithParam<bool> {
  protected:
   MultiDeviceSetupEligibleHostDevicesProviderImplTest()
       : test_devices_(
@@ -36,6 +39,11 @@ class MultiDeviceSetupEligibleHostDevicesProviderImplTest
 
   // testing::Test:
   void SetUp() override {
+    use_get_devices_activity_status_ = GetParam();
+    scoped_feature_list_.InitWithFeatureState(
+        chromeos::features::kCryptAuthV2DeviceActivityStatus,
+        use_get_devices_activity_status_);
+
     fake_device_sync_client_ =
         std::make_unique<device_sync::FakeDeviceSyncClient>();
     fake_device_sync_client_->set_synced_devices(test_devices_);
@@ -52,6 +60,33 @@ class MultiDeviceSetupEligibleHostDevicesProviderImplTest
 
   EligibleHostDevicesProvider* provider() { return provider_.get(); }
 
+  void SetBitsOnTestDevices() {
+    // Devices 0, 1, and 2 are supported.
+    GetMutableRemoteDevice(test_devices()[0])
+        ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
+        multidevice::SoftwareFeatureState::kSupported;
+    GetMutableRemoteDevice(test_devices()[1])
+        ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
+        multidevice::SoftwareFeatureState::kSupported;
+    GetMutableRemoteDevice(test_devices()[2])
+        ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
+        multidevice::SoftwareFeatureState::kSupported;
+
+    // Device 3 is enabled.
+    GetMutableRemoteDevice(test_devices()[3])
+        ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
+        multidevice::SoftwareFeatureState::kEnabled;
+
+    // Device 4 is not supported.
+    GetMutableRemoteDevice(test_devices()[4])
+        ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
+        multidevice::SoftwareFeatureState::kNotSupported;
+  }
+
+  bool use_get_devices_activity_status() {
+    return use_get_devices_activity_status_;
+  }
+
  private:
   multidevice::RemoteDeviceRefList test_devices_;
 
@@ -59,14 +94,18 @@ class MultiDeviceSetupEligibleHostDevicesProviderImplTest
 
   std::unique_ptr<EligibleHostDevicesProvider> provider_;
 
+  bool use_get_devices_activity_status_;
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(MultiDeviceSetupEligibleHostDevicesProviderImplTest);
 };
 
-TEST_F(MultiDeviceSetupEligibleHostDevicesProviderImplTest, Empty) {
+TEST_P(MultiDeviceSetupEligibleHostDevicesProviderImplTest, Empty) {
   EXPECT_TRUE(provider()->GetEligibleHostDevices().empty());
 }
 
-TEST_F(MultiDeviceSetupEligibleHostDevicesProviderImplTest, NoEligibleDevices) {
+TEST_P(MultiDeviceSetupEligibleHostDevicesProviderImplTest, NoEligibleDevices) {
   GetMutableRemoteDevice(test_devices()[0])
       ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
       multidevice::SoftwareFeatureState::kNotSupported;
@@ -82,28 +121,28 @@ TEST_F(MultiDeviceSetupEligibleHostDevicesProviderImplTest, NoEligibleDevices) {
   EXPECT_TRUE(provider()->GetEligibleHostDevices().empty());
 }
 
-TEST_F(MultiDeviceSetupEligibleHostDevicesProviderImplTest,
+TEST_P(MultiDeviceSetupEligibleHostDevicesProviderImplTest,
        SupportedAndEnabled) {
-  // Devices 0, 1, and 2 are supported.
-  GetMutableRemoteDevice(test_devices()[0])
-      ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
-      multidevice::SoftwareFeatureState::kSupported;
-  GetMutableRemoteDevice(test_devices()[1])
-      ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
-      multidevice::SoftwareFeatureState::kSupported;
-  GetMutableRemoteDevice(test_devices()[2])
-      ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
-      multidevice::SoftwareFeatureState::kSupported;
+  SetBitsOnTestDevices();
 
-  // Device 3 is enabled.
-  GetMutableRemoteDevice(test_devices()[3])
-      ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
-      multidevice::SoftwareFeatureState::kEnabled;
+  multidevice::RemoteDeviceRefList devices{test_devices()[0], test_devices()[1],
+                                           test_devices()[2], test_devices()[3],
+                                           test_devices()[4]};
+  fake_device_sync_client()->set_synced_devices(devices);
+  fake_device_sync_client()->NotifyNewDevicesSynced();
 
-  // Device 4 is not supported.
-  GetMutableRemoteDevice(test_devices()[4])
-      ->software_features[multidevice::SoftwareFeature::kBetterTogetherHost] =
-      multidevice::SoftwareFeatureState::kNotSupported;
+  base::flat_set<multidevice::RemoteDeviceRef> eligible_devices =
+      provider()->GetEligibleHostDevices();
+  EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[0]));
+  EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[1]));
+  EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[2]));
+  EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[3]));
+  EXPECT_FALSE(base::Contains(eligible_devices, test_devices()[4]));
+}
+
+TEST_P(MultiDeviceSetupEligibleHostDevicesProviderImplTest,
+       GetDevicesActivityStatus) {
+  SetBitsOnTestDevices();
 
   multidevice::RemoteDeviceRefList devices{test_devices()[0], test_devices()[1],
                                            test_devices()[2], test_devices()[3],
@@ -111,8 +150,77 @@ TEST_F(MultiDeviceSetupEligibleHostDevicesProviderImplTest,
   fake_device_sync_client()->set_synced_devices(devices);
   fake_device_sync_client()->NotifyNewDevicesSynced();
 
+  GetMutableRemoteDevice(test_devices()[0])->last_update_time_millis = 1;
+  GetMutableRemoteDevice(test_devices()[1])->last_update_time_millis = 25;
+  GetMutableRemoteDevice(test_devices()[2])->last_update_time_millis = 10;
+  GetMutableRemoteDevice(test_devices()[3])->last_update_time_millis = 100;
+  GetMutableRemoteDevice(test_devices()[4])->last_update_time_millis = 10000;
+
+  std::vector<device_sync::mojom::DeviceActivityStatusPtr>
+      device_activity_statuses;
+  device_activity_statuses.emplace_back(
+      device_sync::mojom::DeviceActivityStatus::New(
+          "publicKey0", base::Time::FromTimeT(50),
+          cryptauthv2::ConnectivityStatus::ONLINE));
+  device_activity_statuses.emplace_back(
+      device_sync::mojom::DeviceActivityStatus::New(
+          "publicKey1", base::Time::FromTimeT(100),
+          cryptauthv2::ConnectivityStatus::OFFLINE));
+  device_activity_statuses.emplace_back(
+      device_sync::mojom::DeviceActivityStatus::New(
+          "publicKey2", base::Time::FromTimeT(200),
+          cryptauthv2::ConnectivityStatus::ONLINE));
+  device_activity_statuses.emplace_back(
+      device_sync::mojom::DeviceActivityStatus::New(
+          "publicKey3", base::Time::FromTimeT(50),
+          cryptauthv2::ConnectivityStatus::ONLINE));
+  if (use_get_devices_activity_status()) {
+    fake_device_sync_client()->InvokePendingGetDevicesActivityStatusCallback(
+        device_sync::mojom::NetworkRequestResult::kSuccess,
+        std::move(device_activity_statuses));
+  }
+
+  if (use_get_devices_activity_status()) {
+    multidevice::RemoteDeviceRefList eligible_devices =
+        provider()->GetEligibleActiveHostDevices();
+    EXPECT_EQ(4u, eligible_devices.size());
+    EXPECT_EQ(test_devices()[2], eligible_devices[0]);
+    EXPECT_EQ(test_devices()[3], eligible_devices[1]);
+    EXPECT_EQ(test_devices()[0], eligible_devices[2]);
+    EXPECT_EQ(test_devices()[1], eligible_devices[3]);
+  } else {
     base::flat_set<multidevice::RemoteDeviceRef> eligible_devices =
         provider()->GetEligibleHostDevices();
+    EXPECT_EQ(4u, eligible_devices.size());
+    EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[0]));
+    EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[1]));
+    EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[2]));
+    EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[3]));
+  }
+}
+
+TEST_P(MultiDeviceSetupEligibleHostDevicesProviderImplTest,
+       GetDevicesActivityStatusFailedRequest) {
+  if (!use_get_devices_activity_status()) {
+    return;
+  }
+
+  SetBitsOnTestDevices();
+
+  multidevice::RemoteDeviceRefList devices{test_devices()[0], test_devices()[1],
+                                           test_devices()[2], test_devices()[3],
+                                           test_devices()[4]};
+  fake_device_sync_client()->set_synced_devices(devices);
+  fake_device_sync_client()->NotifyNewDevicesSynced();
+  fake_device_sync_client()->InvokePendingGetDevicesActivityStatusCallback(
+      device_sync::mojom::NetworkRequestResult::kInternalServerError,
+      base::nullopt);
+
+  multidevice::RemoteDeviceRefList eligible_active_devices =
+      provider()->GetEligibleActiveHostDevices();
+  multidevice::RemoteDeviceRefList eligible_devices =
+      provider()->GetEligibleHostDevices();
+  EXPECT_EQ(eligible_devices, eligible_active_devices);
   EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[0]));
   EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[1]));
   EXPECT_TRUE(base::Contains(eligible_devices, test_devices()[2]));
@@ -120,6 +228,10 @@ TEST_F(MultiDeviceSetupEligibleHostDevicesProviderImplTest,
   EXPECT_FALSE(base::Contains(eligible_devices, test_devices()[4]));
 }
 
+INSTANTIATE_TEST_SUITE_P(,
+                         MultiDeviceSetupEligibleHostDevicesProviderImplTest,
+                         testing::Bool());
+
 }  // namespace multidevice_setup
 
 }  // namespace chromeos

chromeos/services/multidevice_setup/fake_eligible_host_devices_provider.cc

@@ -17,6 +17,11 @@ FakeEligibleHostDevicesProvider::GetEligibleHostDevices() const {
   return eligible_host_devices_;
 }
 
+multidevice::RemoteDeviceRefList
+FakeEligibleHostDevicesProvider::GetEligibleActiveHostDevices() const {
+  return eligible_host_devices_;
+}
+
 }  // namespace multidevice_setup
 
 }  // namespace chromeos

chromeos/services/multidevice_setup/fake_eligible_host_devices_provider.h

@@ -28,6 +28,8 @@ class FakeEligibleHostDevicesProvider : public EligibleHostDevicesProvider {
  private:
   // EligibleHostDevicesProvider:
   multidevice::RemoteDeviceRefList GetEligibleHostDevices() const override;
+  multidevice::RemoteDeviceRefList GetEligibleActiveHostDevices()
+      const override;
 
   multidevice::RemoteDeviceRefList eligible_host_devices_;