Correctly translate Cellular Device properties to ONC

chromeos/network/onc/onc_translator_shill_to_onc.cc

Index: chromeos/network/onc/onc_translator_shill_to_onc.cc
diff --git a/chromeos/network/onc/onc_translator_shill_to_onc.cc b/chromeos/network/onc/onc_translator_shill_to_onc.cc
index 3c00e40ec1082881975849179530b5ecd8d9e761..38d3708d236de38ce43f990528cc463970ebe84d 100644
--- a/chromeos/network/onc/onc_translator_shill_to_onc.cc
+++ b/chromeos/network/onc/onc_translator_shill_to_onc.cc
@@ -66,6 +66,7 @@ class ShillToONCTranslator {
   void TranslateVPN();
   void TranslateWiFiWithState();
   void TranslateCellularWithState();
+  void TranslateCellularDevice();
   void TranslateNetworkWithState();
   void TranslateIPConfig();
 
@@ -133,6 +134,8 @@ ShillToONCTranslator::CreateTranslatedONCObject() {
     TranslateWiFiWithState();
   } else if (onc_signature_ == &kCellularWithStateSignature) {
     TranslateCellularWithState();
+  } else if (onc_signature_ == &kCellularDeviceSignature) {
+    TranslateCellularDevice();
   } else if (onc_signature_ == &kIPConfigSignature) {
     TranslateIPConfig();
   } else {
@@ -264,30 +267,26 @@ void ShillToONCTranslator::TranslateCellularWithState() {
     TranslateAndAddListOfObjects(::onc::cellular::kAPNList, *shill_apns);
   }
 
+  // Merge the Device dictionary with this one (Cellular) using the
+  // CellularDevice signiture.
   const base::DictionaryValue* device_dictionary = NULL;
   if (!shill_dictionary_->GetDictionaryWithoutPathExpansion(
           shill::kDeviceProperty, &device_dictionary)) {
     return;
   }
+  ShillToONCTranslator nested_translator(*device_dictionary,
+                                         kCellularDeviceSignature);
+  scoped_ptr<base::DictionaryValue> nested_object =
+      nested_translator.CreateTranslatedONCObject();
+  onc_object_->MergeDictionary(nested_object.get());
+}
 
-  // Iterate through all fields of the CellularWithState signature and copy
-  // values from the device properties according to the separate
-  // CellularDeviceTable.
-  for (const OncFieldSignature* field_signature = onc_signature_->fields;
-       field_signature->onc_field_name != NULL; ++field_signature) {
-    const std::string& onc_field_name = field_signature->onc_field_name;
-
-    std::string shill_property_name;
-    const base::Value* shill_value = NULL;
-    if (!GetShillPropertyName(field_signature->onc_field_name,
-                              kCellularDeviceTable,
-                              &shill_property_name) ||
-        !device_dictionary->GetWithoutPathExpansion(shill_property_name,
-                                                    &shill_value)) {
-      continue;
-    }
-    onc_object_->SetWithoutPathExpansion(onc_field_name,
-                                         shill_value->DeepCopy());
+void ShillToONCTranslator::TranslateCellularDevice() {
+  CopyPropertiesAccordingToSignature();
+  const base::DictionaryValue* dictionary = NULL;
+  if (shill_dictionary_->GetDictionaryWithoutPathExpansion(
+          shill::kSIMLockStatusProperty, &dictionary)) {
+    TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);

[armansito, 2014/07/18 21:21:52]

I wonder if we should just place all the nested fields of the shill
SIMLockStatus property at the top level device properties dictionary to avoid
yet another nested entry. I don't have a strong opinion on this (since the way
you have it is at least consistent with what's returned from shill) but I have a
feeling that all the nested dictionaries end up complicating the data that we
have to deal with while using networkingPrivate.

[stevenjb, 2014/07/18 21:31:50]

Fortunately in JS, nested dictionaries are easy to deal with, and in some ways
it is actually more convenient to have a nested dictionary of related
properties.

Now, it would be nice if we would automatically handle translation of these
dictionaries, and I considered looking into that, but decided that I would
probably be more work than I am willing to do right now.

   }
 }