Updated DeviceStatusReportRequest to contain new monitoring data.

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 1af72bb4ff1d0f944267bc615fd73172d3fbb414..25d31d5f2eebf2ea3b6f53f2393e1cfcdabea007 100644
--- a/chrome/browser/chromeos/policy/device_status_collector.cc
+++ b/chrome/browser/chromeos/policy/device_status_collector.cc
@@ -23,6 +23,7 @@
 #include "chrome/common/pref_names.h"
 #include "chromeos/network/device_state.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/system/statistics_provider.h"
@@ -94,6 +95,7 @@ DeviceStatusCollector::DeviceStatusCollector(
       report_location_(false),
       report_network_interfaces_(false),
       report_users_(false),
+      report_hardware_status_(false),
       weak_factory_(this) {
   if (location_update_requester)
     location_update_requester_ = *location_update_requester;
@@ -120,6 +122,8 @@ DeviceStatusCollector::DeviceStatusCollector(
       chromeos::kReportDeviceNetworkInterfaces, callback);
   users_subscription_ = cros_settings_->AddSettingsObserver(
       chromeos::kReportDeviceUsers, callback);
+  hardware_status_subscription_ = cros_settings_->AddSettingsObserver(
+      chromeos::kReportDeviceHardwareStatus, callback);
 
   // The last known location is persisted in local state. This makes location
   // information available immediately upon startup and avoids the need to
@@ -206,6 +210,10 @@ void DeviceStatusCollector::UpdateReportingSettings() {
       chromeos::kReportDeviceUsers, &report_users_)) {
     report_users_ = true;
   }
+  if (!cros_settings_->GetBoolean(

[pneubeck (no reviews), 2014/12/09 03:00:09]

IMO it would help a bit to drop a comment in front of these repeated 'if
(!cros_settings_->Get...)' that we report everything by default (if not
explicitly disabled).

DeviceLocation falls out of this pattern because it's not released and could be
moved to a separate block/be removed.

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

Done.

+      chromeos::kReportDeviceHardwareStatus, &report_hardware_status_)) {
+    report_hardware_status_ = true;
+  }
 
   if (report_location_) {
     ScheduleGeolocationUpdateRequest();
@@ -395,9 +403,29 @@ void DeviceStatusCollector::GetNetworkInterfaces(
     { shill::kTypeCellular,  em::NetworkInterface::TYPE_CELLULAR,  },
   };
 
+  // Maps flimflam device connection status to proto enum constants.

[pneubeck (no reviews), 2014/12/09 03:00:10]

we shouldn't use the name flimflam of the previous incarnation of the network
manager anymore.
flimflam -> shill

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

Done.

+  static const struct {
+    const char* type_string;

[pneubeck (no reviews), 2014/12/09 03:00:09]

type_string -> state_string

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

Done.

+    em::NetworkState::ConnectionState state_constant;
+  } kConnectionStateMap[] = {
+    { shill::kStateIdle,              em::NetworkState::IDLE },
+    { shill::kStateCarrier,           em::NetworkState::CARRIER },
+    { shill::kStateAssociation,       em::NetworkState::ASSOCIATION },
+    { shill::kStateConfiguration,     em::NetworkState::CONFIGURATION },
+    { shill::kStateReady,             em::NetworkState::READY },
+    { shill::kStatePortal,            em::NetworkState::PORTAL },
+    { shill::kStateOffline,           em::NetworkState::OFFLINE },
+    { shill::kStateOnline,            em::NetworkState::ONLINE },
+    { shill::kStateDisconnect,        em::NetworkState::DISCONNECT },
+    { shill::kStateFailure,           em::NetworkState::FAILURE },
+    { shill::kStateActivationFailure,
+        em::NetworkState::ACTIVATION_FAILURE },
+  };
+
   chromeos::NetworkStateHandler::DeviceStateList device_list;
-  chromeos::NetworkHandler::Get()->network_state_handler()->GetDeviceList(
-      &device_list);
+  chromeos::NetworkStateHandler* network_state_handler =
+      chromeos::NetworkHandler::Get()->network_state_handler();
+  network_state_handler->GetDeviceList(&device_list);
 
   chromeos::NetworkStateHandler::DeviceStateList::const_iterator device;
   for (device = device_list.begin(); device != device_list.end(); ++device) {
@@ -421,6 +449,38 @@ void DeviceStatusCollector::GetNetworkInterfaces(
       interface->set_meid((*device)->meid());
     if (!(*device)->imei().empty())
       interface->set_imei((*device)->imei());
+    if (!(*device)->path().empty())
+      interface->set_device_path((*device)->path());
+  }
+
+  // Walk the various networks and store their state in the status report.
+  chromeos::NetworkStateHandler::NetworkStateList state_list;
+  network_state_handler->GetNetworkListByType(
+      chromeos::NetworkTypePattern::Default(),
+      false,  // configured_only

[pneubeck (no reviews), 2014/12/09 03:00:09]

just verify with your intent here:
this will list _all_ networks. Which means any network that is configured or
that is visible, i.e. also visible but not configured networks.

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

Hmmm. It's probably better to be a little tighter and only report configured
networks. Changed this, good catch.

+      false,  // visible_only,
+      0,      // no limit to number of results
+      &state_list);
+
+  chromeos::NetworkStateHandler::NetworkStateList::const_iterator state;
+  for (state = state_list.begin(); state != state_list.end(); ++state) {

[ygorshenin1, 2014/12/05 15:55:41]

nit: you can just write:
for (const auto& state : state_list) { ... }

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

Done.

+    // Determine the connection state and signal strength for |state|.
+    em::NetworkState::ConnectionState connection_state_enum =
+        em::NetworkState::UNKNOWN;
+    std::string connection_state_string = (*state)->connection_state();

[pneubeck (no reviews), 2014/12/09 03:00:09]

nit: const, use constructor instead of assignment

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

Done.

+    for (size_t i = 0; i < arraysize(kConnectionStateMap); ++i) {
+      if (connection_state_string == kConnectionStateMap[i].type_string) {
+        connection_state_enum = kConnectionStateMap[i].state_constant;
+        break;
+      }
+    }
+
+    // Copy fields from NetworkState into the status report.
+    em::NetworkState* proto_state = request->add_network_state();
+    proto_state->set_connection_state(connection_state_enum);
+    proto_state->set_signal_strength((*state)->signal_strength());
+    if (!(*state)->device_path().empty())
+      proto_state->set_device_path((*state)->device_path());
   }
 }
 
@@ -447,12 +507,9 @@ void DeviceStatusCollector::GetUsers(em::DeviceStatusReportRequest* request) {
   }
 }
 
-void DeviceStatusCollector::GetStatus(em::DeviceStatusReportRequest* request) {
-  // TODO(mnissler): Remove once the old cloud policy stack is retired. The old
-  // stack doesn't support reporting successful submissions back to here, so
-  // just assume whatever ends up in |request| gets submitted successfully.
-  GetDeviceStatus(request);
-  OnSubmittedSuccessfully();
+void DeviceStatusCollector::GetHardwareStatus(
+    em::DeviceStatusReportRequest* status) {

[pneubeck (no reviews), 2014/12/09 03:00:10]

why is this empty function necessary?
if it will be filled eventually better just punt this part of the CL.

[Andrew T Wilson (Slow), 2014/12/11 00:09:54]

I'd like to keep this in place because I'd like to keep the surrounding logic
(checking the report_hardware_status_ setting, etc in this CL). I've added a
TODO here.

+
 }
 
 bool DeviceStatusCollector::GetDeviceStatus(
@@ -476,6 +533,9 @@ bool DeviceStatusCollector::GetDeviceStatus(
     GetUsers(status);
   }
 
+  if (report_hardware_status_)
+    GetHardwareStatus(status);
+
   return true;
 }