Report ARC status asynchronously via DeviceStatusCollector

chrome/browser/chromeos/policy/device_status_collector.cc

Index: chrome/browser/chromeos/policy/device_status_collector.cc
diff --git a/chrome/browser/chromeos/policy/device_status_collector.cc b/chrome/browser/chromeos/policy/device_status_collector.cc
index 616aa251950bd8636c01e7e18d2226da5ecdb9ab..0c28bbcba1bcd20bf023b0ff78322c059286c1f9 100644
--- a/chrome/browser/chromeos/policy/device_status_collector.cc
+++ b/chrome/browser/chromeos/policy/device_status_collector.cc
@@ -48,6 +48,8 @@
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/system/statistics_provider.h"
+#include "components/arc/arc_bridge_service.h"
+#include "components/arc/common/enterprise_reporting.mojom.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -338,6 +340,30 @@ class GetStatusState : public base::RefCountedThreadSafe<GetStatusState> {
         base::Bind(&GetStatusState::OnCPUTempInfoReceived, this));
   }
 
+  bool SampleAndroidInfo(
+      const policy::DeviceStatusCollector::AndroidStatusFetcher&
+          android_status_fetcher) {
+    if (android_status_fetcher.is_null()) {

[ljusten (tachyonic), 2016/10/04 08:56:45]

A more elegant approach would be to make android_status_fetcher asynchronous,
i.e. have it take a response callback, so that it matches instance->GetStatus,
and make the default implementation of android_status_fetcher (if no fetcher is
passed into the constructor of DeviceStatusCollector) to be the arc code below.
The  PostTaskAndReplyWithResult and the OnAndroidInfoPairReceived could be moved
to the unit test this way. It would make this class very clean and move details
like the pair<string, string> method to the test.

[phweiss, 2016/10/05 15:34:22]

Done.

+      arc::ArcBridgeService *arc_service = arc::ArcBridgeService::Get();
+      if (arc_service == nullptr) return false;
+      arc::InstanceHolder<arc::mojom::EnterpriseReportingInstance>
+          *instance_holder = arc_service->enterprise_reporting();
+      if (instance_holder == nullptr) return false;
+      arc::mojom::EnterpriseReportingInstance *instance =
+          instance_holder->GetInstanceForMethod("GetStatus", 1);
+      if (instance == nullptr) return false;
+      instance->GetStatus(
+          base::Bind(&GetStatusState::OnAndroidInfoReceived, this));
+      return true;
+    } else {
+      base::PostTaskAndReplyWithResult(
+          content::BrowserThread::GetBlockingPool(), FROM_HERE,
+          android_status_fetcher,
+          base::Bind(&GetStatusState::OnAndroidInfoPairReceived, this));
+      return true;
+    }
+  }
+
  private:
   friend class RefCountedThreadSafe<GetStatusState>;
 
@@ -366,6 +392,18 @@ class GetStatusState : public base::RefCountedThreadSafe<GetStatusState> {
       *device_status_->add_cpu_temp_info() = info;
   }
 
+  void OnAndroidInfoPairReceived(std::pair<mojo::String, mojo::String> status) {

[ljusten (tachyonic), 2016/10/04 08:56:45]

Nit: Is it possible to change the signature of mojo::String to const
mojo::String& to prevent copies?

[phweiss, 2016/10/05 15:34:22]

No, mojo can connect C++ and Java, so their interfaces do not have such things.

+    OnAndroidInfoReceived(status.first, status.second);
+  }
+
+  void OnAndroidInfoReceived(mojo::String status,
+                             mojo::String droid_guard_info) {
+    em::AndroidStatus *android_status =
+        session_status_->mutable_android_status();
+    android_status->set_status_payload(status);
+    android_status->set_droid_guard_info(droid_guard_info);
+  }
+
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;
   policy::DeviceStatusCollector::StatusCallback response_;
   std::unique_ptr<em::DeviceStatusReportRequest> device_status_ =
@@ -379,7 +417,8 @@ DeviceStatusCollector::DeviceStatusCollector(
     chromeos::system::StatisticsProvider* provider,
     const VolumeInfoFetcher& volume_info_fetcher,
     const CPUStatisticsFetcher& cpu_statistics_fetcher,
-    const CPUTempFetcher& cpu_temp_fetcher)
+    const CPUTempFetcher& cpu_temp_fetcher,
+    const AndroidStatusFetcher& android_status_fetcher)
     : max_stored_past_activity_days_(kMaxStoredPastActivityDays),
       max_stored_future_activity_days_(kMaxStoredFutureActivityDays),
       local_state_(local_state),
@@ -387,6 +426,7 @@ DeviceStatusCollector::DeviceStatusCollector(
       volume_info_fetcher_(volume_info_fetcher),
       cpu_statistics_fetcher_(cpu_statistics_fetcher),
       cpu_temp_fetcher_(cpu_temp_fetcher),
+      android_status_fetcher_(android_status_fetcher),
       statistics_provider_(provider),
       cros_settings_(chromeos::CrosSettings::Get()),
       task_runner_(nullptr),
@@ -515,6 +555,9 @@ void DeviceStatusCollector::UpdateReportingSettings() {
     report_session_status_ = true;
   }
 
+  // TODO(phweiss): Create policy to control this, and turn it on by default.
+  report_android_status_ = false;
+
   if (!report_hardware_status_) {
     ClearCachedResourceUsage();
   } else if (!already_reporting_hardware_status) {
@@ -1085,8 +1128,11 @@ void DeviceStatusCollector::GetSessionStatus(
   em::SessionStatusReportRequest* status = state->session_status();
   bool anything_reported = false;
 
-  if (report_session_status_)
+  if (report_session_status_) {
     anything_reported |= GetAccountStatus(status);
+    if (report_android_status_)
+      anything_reported |= GetAndroidStatus(status, state);
+  }
 
   // Wipe pointer if we didn't actually add any data.
   if (!anything_reported)
@@ -1117,6 +1163,12 @@ bool DeviceStatusCollector::GetAccountStatus(
   return true;
 }
 
+bool DeviceStatusCollector::GetAndroidStatus(
+    em::SessionStatusReportRequest* status,
+    scoped_refptr<GetStatusState> state) {
+  return state->SampleAndroidInfo(android_status_fetcher_);
+}
+
 std::string DeviceStatusCollector::GetAppVersion(
     const std::string& kiosk_app_id) {
   Profile* const profile =