Improve Cellular support in networkingPrivate

chromeos/network/managed_network_configuration_handler_impl.cc

Index: chromeos/network/managed_network_configuration_handler_impl.cc
diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc
index 9411f4b1b983b942dca6ab51c0756b8b6bb1c260..3a93baf5837efa223d67437aa45990e1f7cb7a2a 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -76,14 +76,37 @@ void LogErrorWithDict(const tracked_objects::Location& from_where,
                              device_event_log::LOG_LEVEL_ERROR, error_name);
 }
 
-const base::DictionaryValue* GetByGUID(const GuidToPolicyMap& policies,
-                                       const std::string& guid) {
+const base::DictionaryValue* GetPolicyByGUID(const GuidToPolicyMap& policies,
+                                             const std::string& guid) {
   GuidToPolicyMap::const_iterator it = policies.find(guid);
   if (it == policies.end())
     return NULL;
   return it->second;
 }
 
+scoped_ptr<base::DictionaryValue> ExtractShillDeviceSettings(
+    base::DictionaryValue* user_settings) {
+  scoped_ptr<base::DictionaryValue> shill_device_settings(
+      new base::DictionaryValue);
+  // Only extract specific properties that can be set in Shill.
+  base::DictionaryValue* cellular;
+  if (user_settings->GetDictionaryWithoutPathExpansion(
+          ::onc::network_config::kCellular, &cellular)) {
+    scoped_ptr<base::Value> value;
+    if (cellular->RemoveWithoutPathExpansion(::onc::cellular::kCarrier,
+                                             &value)) {
+      shill_device_settings->SetWithoutPathExpansion(shill::kCarrierProperty,
+                                                     value.Pass());
+    }
+    if (cellular->RemoveWithoutPathExpansion(::onc::cellular::kAllowRoaming,
+                                             &value)) {
+      shill_device_settings->SetWithoutPathExpansion(
+          shill::kCellularAllowRoamingProperty, value.Pass());
+    }
+  }
+  return shill_device_settings.Pass();
+}
+
 }  // namespace
 
 struct ManagedNetworkConfigurationHandlerImpl::Policies {
@@ -178,7 +201,7 @@ void ManagedNetworkConfigurationHandlerImpl::SendManagedProperties(
       return;
     }
     if (!guid.empty())
-      network_policy = GetByGUID(policies->per_network_config, guid);
+      network_policy = GetPolicyByGUID(policies->per_network_config, guid);
     global_policy = &policies->global_network_config;
   }
 
@@ -226,7 +249,7 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
     const std::string& service_path,
     const base::DictionaryValue& user_settings,
     const base::Closure& callback,
-    const network_handler::ErrorCallback& error_callback) const {
+    const network_handler::ErrorCallback& error_callback) {
   const NetworkState* state =
       network_state_handler_->GetNetworkStateFromServicePath(
           service_path, true /* configured_only */);
@@ -267,6 +290,10 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
       network_util::TranslateShillTypeToONC(state->type()));
   user_settings_copy->MergeDictionary(&user_settings);
 
+  // Extract any Device properties to apply separately, before validation.

[pneubeck (no reviews), 2015/03/24 16:19:52]

any reason to do this before validation?
So far, all details of validation were separated out into the validator.
I'd like to avoid any adhoc translatin/validation code accumulating in this
class (which is already too large).

+  scoped_ptr<base::DictionaryValue> shill_device_dictionary(
+      ExtractShillDeviceSettings(user_settings_copy.get()));
+
   // Validate the ONC dictionary. We are liberal and ignore unknown field
   // names. User settings are only partial ONC, thus we ignore missing fields.
   onc::Validator validator(false,  // Ignore unknown fields.
@@ -288,13 +315,11 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
     LOG(WARNING) << "Validation of ONC user settings produced warnings.";
 
   // Fill in HexSSID field from contents of SSID field if not set already.
-  if (user_settings_copy) {
-    onc::FillInHexSSIDFieldsInOncObject(onc::kNetworkConfigurationSignature,
-                                        validated_user_settings.get());
-  }
+  onc::FillInHexSSIDFieldsInOncObject(onc::kNetworkConfigurationSignature,
+                                      validated_user_settings.get());
 
   const base::DictionaryValue* network_policy =
-      GetByGUID(policies->per_network_config, guid);
+      GetPolicyByGUID(policies->per_network_config, guid);
   VLOG(2) << "This configuration is " << (network_policy ? "" : "not ")
           << "managed.";
 
@@ -305,8 +330,69 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
                                             network_policy,
                                             validated_user_settings.get()));
 
+  if (!shill_device_dictionary->empty() && network_device_handler_) {
+    SetShillDeviceProperties(
+        state->device_path(), shill_device_dictionary.Pass(),
+        base::Bind(
+            &ManagedNetworkConfigurationHandlerImpl::SetShillServiceProperties,
+            weak_ptr_factory_.GetWeakPtr(), service_path,
+            base::Passed(&shill_dictionary), callback, error_callback),
+        error_callback);
+  } else {
+    SetShillServiceProperties(service_path, shill_dictionary.Pass(), callback,
+                              error_callback);
+  }
+}
+
+void ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties(

[pneubeck (no reviews), 2015/03/24 16:19:52]

this should really, really move to the NetworkDeviceHandler

ideally we could avoid this complexity by also having a SetDeviceProperties
exposed by Shill (at least that should be filed as a bug). This must be covered
by a test.

+    const std::string& device_path,
+    scoped_ptr<base::DictionaryValue> shill_device_dictionary,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
+  // Note: The 'Remove' calls here are crucial to ensure we do not start an
+  // infinte callback chain.
+  scoped_ptr<base::Value> shill_value;
+  if (shill_device_dictionary->RemoveWithoutPathExpansion(
+          shill::kCarrierProperty, &shill_value)) {
+    std::string carrier;
+    shill_value->GetAsString(&carrier);
+    NET_LOG(USER) << "SetCarrier: " << device_path << ": " << carrier;
+    network_device_handler_->SetCarrier(
+        device_path, carrier,
+        base::Bind(
+            &ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties,
+            weak_ptr_factory_.GetWeakPtr(), device_path,
+            base::Passed(&shill_device_dictionary), callback, error_callback),
+        error_callback);
+    return;
+  }
+  if (shill_device_dictionary->empty()) {
+    callback.Run();
+    return;
+  }
+  // We have to set Device properites one at a time, but setting them is rare.
+  base::DictionaryValue::Iterator iter(*shill_device_dictionary);
+  std::string key = iter.key();
+  if (shill_device_dictionary->RemoveWithoutPathExpansion(key, &shill_value)) {
+    NET_LOG(USER) << "SetDeviceProperty: " << device_path << ": " << key << "="
+                  << *shill_value;
+    network_device_handler_->SetDeviceProperty(
+        device_path, key, *shill_value,
+        base::Bind(
+            &ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties,
+            weak_ptr_factory_.GetWeakPtr(), device_path,
+            base::Passed(&shill_device_dictionary), callback, error_callback),
+        error_callback);
+  }
+}
+
+void ManagedNetworkConfigurationHandlerImpl::SetShillServiceProperties(
+    const std::string& service_path,
+    scoped_ptr<base::DictionaryValue> shill_service_dictionary,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
   network_configuration_handler_->SetShillProperties(
-      service_path, *shill_dictionary,
+      service_path, *shill_service_dictionary,
       NetworkConfigurationObserver::SOURCE_USER_ACTION, callback,
       error_callback);
 }
@@ -572,7 +658,7 @@ ManagedNetworkConfigurationHandlerImpl::FindPolicyByGUID(
     const Policies* user_policies = GetPoliciesForUser(userhash);
     if (user_policies) {
       const base::DictionaryValue* policy =
-          GetByGUID(user_policies->per_network_config, guid);
+          GetPolicyByGUID(user_policies->per_network_config, guid);
       if (policy) {
         *onc_source = ::onc::ONC_SOURCE_USER_POLICY;
         return policy;
@@ -583,7 +669,7 @@ ManagedNetworkConfigurationHandlerImpl::FindPolicyByGUID(
   const Policies* device_policies = GetPoliciesForUser(std::string());
   if (device_policies) {
     const base::DictionaryValue* policy =
-        GetByGUID(device_policies->per_network_config, guid);
+        GetPolicyByGUID(device_policies->per_network_config, guid);
     if (policy) {
       *onc_source = ::onc::ONC_SOURCE_DEVICE_POLICY;
       return policy;
@@ -628,7 +714,7 @@ ManagedNetworkConfigurationHandlerImpl::FindPolicyByGuidAndProfile(
   if (!policies)
     return NULL;
 
-  return GetByGUID(policies->per_network_config, guid);
+  return GetPolicyByGUID(policies->per_network_config, guid);
 }
 
 const ManagedNetworkConfigurationHandlerImpl::Policies*