Call Device.SetCarrier when the ONC Carrier property is specified.

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..ca7a63c2a00c23f81a2ccd1483217dd89733535b 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -226,7 +226,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 */);
@@ -305,6 +305,33 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
                                             network_policy,
                                             validated_user_settings.get()));
 
+  // 'Carrier' needs to be handled specially if set.

[pneubeck (no reviews), 2015/03/27 18:26:46]

BIG NOTE:
you do this now only for SetProperties but not for the initial CreateNetwork

btw. in the current form, you can also considering moving it to NCH, which might
catch more uses and might be cleaner.

[stevenjb, 2015/03/27 23:18:04]

I prefer keeping this as an ONC special case instead of a Shill special case.

Yeah, I'm not sure what to do about createConfiguration. It doesn't make sense
to have CreateConfiguration fail if SetCarrier fails since the configuration
will be created, but I don't like not providing feedback either. For now I added
documentation to the property in networking_private.idl specifying that it can
be specified in setProperties but not createConfiguration.

+  base::DictionaryValue* cellular;

[pneubeck (no reviews), 2015/03/27 18:26:46]

pls initialize to nullptr

[stevenjb, 2015/03/27 23:18:04]

We always disagree on this. It is really not necessary checking the result of
Value::Get*. I'll set it as always because it's not worthy of discussion.

+  if (validated_user_settings->GetDictionaryWithoutPathExpansion(
+          ::onc::network_config::kCellular, &cellular)) {
+    std::string carrier;
+    if (cellular->GetStringWithoutPathExpansion(::onc::cellular::kCarrier,
+                                                &carrier)) {
+      network_device_handler_->SetCarrier(
+          state->device_path(), carrier,

[pneubeck (no reviews), 2015/03/27 18:26:46]

do you have to check whether device_path is valid?

[stevenjb, 2015/03/27 23:18:04]

device_path must always be valid for NetworkState.

+          base::Bind(
+              &ManagedNetworkConfigurationHandlerImpl::SetShillProperties,

[pneubeck (no reviews), 2015/03/27 18:26:46]

We really need a helper to collect callbacks of several >>parallel<< callbacks.
These callback sequences are a pain ...

[stevenjb, 2015/03/27 23:18:04]

Agreed.

+              weak_ptr_factory_.GetWeakPtr(), service_path,
+              base::Passed(&shill_dictionary), callback, error_callback),
+          error_callback);
+      return;
+    }
+  }
+
+  SetShillProperties(service_path, shill_dictionary.Pass(), callback,
+                     error_callback);
+}
+
+void ManagedNetworkConfigurationHandlerImpl::SetShillProperties(
+    const std::string& service_path,
+    scoped_ptr<base::DictionaryValue> shill_dictionary,
+    const base::Closure& callback,
+    const network_handler::ErrorCallback& error_callback) {
   network_configuration_handler_->SetShillProperties(
       service_path, *shill_dictionary,
       NetworkConfigurationObserver::SOURCE_USER_ACTION, callback,