Use Managed properties for Preferred and Provider.

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 dc01b2a53dd51335958cc529ae46db46780fe5c8..61580f56dff09888ffc5ce5443f57f40226ba9b9 100644
--- a/chromeos/network/onc/onc_translator_shill_to_onc.cc
+++ b/chromeos/network/onc/onc_translator_shill_to_onc.cc
@@ -89,6 +89,12 @@ class ShillToONCTranslator {
   // |onc_field_name|.
   void TranslateAndAddNestedObject(const std::string& onc_field_name);
 
+  // Sets |onc_field_name| in dictionary |onc_dictionary_name| in |onc_object_|
+  // to |value| if the dictionary exists.
+  void SetNestedOncValue(const std::string& onc_dictionary_name,
+                         const std::string& onc_field_name,
+                         const base::Value& value);
+
   // Translates a list of nested objects and adds the list to |onc_object_| at
   // |onc_field_name|. If there are errors while parsing individual objects or
   // if the resulting list contains no entries, the result will not be added to
@@ -182,8 +188,7 @@ void ShillToONCTranslator::TranslateOpenVPN() {
   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;
-    if (onc_field_name == ::onc::vpn::kSaveCredentials ||
-        onc_field_name == ::onc::openvpn::kRemoteCertKU ||
+    if (onc_field_name == ::onc::openvpn::kRemoteCertKU ||
         onc_field_name == ::onc::openvpn::kServerCAPEMs) {
       CopyProperty(field_signature);
       continue;
@@ -233,18 +238,52 @@ void ShillToONCTranslator::TranslateIPsec() {
 }
 
 void ShillToONCTranslator::TranslateVPN() {
-  TranslateWithTableAndSet(
-      shill::kProviderTypeProperty, kVPNTypeTable, ::onc::vpn::kType);
   CopyPropertiesAccordingToSignature();
 
-  std::string vpn_type;
-  if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kType,
-                                                 &vpn_type)) {
-    if (vpn_type == ::onc::vpn::kTypeL2TP_IPsec) {
-      TranslateAndAddNestedObject(::onc::vpn::kIPsec);
-      TranslateAndAddNestedObject(::onc::vpn::kL2TP);
-    } else {
-      TranslateAndAddNestedObject(vpn_type);
+  // Parse Shill Provider dictionary.
+  const base::DictionaryValue* provider = NULL;
+  if (!shill_dictionary_->GetDictionaryWithoutPathExpansion(
+          shill::kProviderProperty, &provider)) {
+    LOG(ERROR) << "Shill VPN object with no Provider dictionary";
+    return;
+  }
+  std::string shill_provider_type, onc_provider_type;
+  if (!provider->GetStringWithoutPathExpansion(shill::kTypeProperty,
+                                               &shill_provider_type)) {
+    LOG(ERROR) << "Shill VPN object with no Provider.Type";

[pneubeck (no reviews), 2014/08/20 08:16:16]

alternatively, instead of catching a missing input (which will be handled by
TranslateStringToONC as well),
you could check the result of TranslateStringToONC, to also catch unknown Shill
types.

[stevenjb, 2014/08/20 17:18:37]

Done.

+    return;
+  }
+  TranslateStringToONC(kVPNTypeTable, shill_provider_type, &onc_provider_type);
+  onc_object_->SetStringWithoutPathExpansion(::onc::vpn::kType,
+                                             onc_provider_type);
+  std::string provider_host;
+  if (provider->GetStringWithoutPathExpansion(shill::kHostProperty,

[pneubeck (no reviews), 2014/08/20 08:16:16]

The translation tables were meant for the cases where the translation is
symmetric.

please comment out
  { ::onc::vpn::kHost, shill::kProviderHostProperty},
in
  vpn_fields in onc_translation_tables.cc
with a note
 // Is handled differently for Shill -> ONC, see onc_translator_*. )

and add the translation in the other direction explicitly to TranslateVPN() in
onc_translator_onc_to_shill.cc:

std::string host;
if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kHost, &host) {
  shill_dictionary_->SetStringWithoutPathExpansion(shill::kHostProperty, host);
}

[stevenjb, 2014/08/20 17:18:37]

Ugh. Yeah, I understand your point, but this is tedious. It seems like we have
enough special cases at this point to provide some better support for asymmetry
and nested dictionaries. Another day. Done.

[pneubeck (no reviews), 2014/08/20 19:49:36]

Yes, it's tedious. To get rid of it, I would prefer to work on the root cause
and align ONC and the format of Shill. Neither should be seen as static.

+                                              &provider_host)) {
+    onc_object_->SetStringWithoutPathExpansion(::onc::vpn::kHost,
+                                               provider_host);
+  }
+
+  const base::Value* save_credentials = NULL;
+  shill_dictionary_->GetWithoutPathExpansion(shill::kSaveCredentialsProperty,

[pneubeck (no reviews), 2014/08/20 08:16:16]

like the Host property above,
please comment out in
  openvpn_fields and ipsec_fields

and add it explicitly to TranslateOpenVPN and to a new TranslateIPsec.

[stevenjb, 2014/08/20 17:18:37]

Done.

+                                             &save_credentials);
+  if (save_credentials && !save_credentials->IsType(base::Value::TYPE_BOOLEAN))
+    save_credentials = NULL;
+
+  // Translate the nested dictionary.
+  if (onc_provider_type == ::onc::vpn::kTypeL2TP_IPsec) {
+    TranslateAndAddNestedObject(::onc::vpn::kIPsec, *provider);
+    TranslateAndAddNestedObject(::onc::vpn::kL2TP, *provider);
+    if (save_credentials) {
+      SetNestedOncValue(
+          ::onc::vpn::kIPsec, ::onc::vpn::kSaveCredentials, *save_credentials);
+      SetNestedOncValue(

[pneubeck (no reviews), 2014/08/20 08:16:16]

nit: wasn't set before and could be removed.

[stevenjb, 2014/08/20 17:18:37]

Done.

+          ::onc::vpn::kL2TP, ::onc::vpn::kSaveCredentials, *save_credentials);
+    }
+  } else {
+    TranslateAndAddNestedObject(onc_provider_type, *provider);
+    if (save_credentials) {
+      SetNestedOncValue(
+          onc_provider_type, ::onc::vpn::kSaveCredentials, *save_credentials);
     }
   }
 }
@@ -404,11 +443,23 @@ void ShillToONCTranslator::TranslateAndAddNestedObject(
                                          *field_signature->value_signature);
   scoped_ptr<base::DictionaryValue> nested_object =
       nested_translator.CreateTranslatedONCObject();
-  if (nested_object->empty())
-    return;
+  // Note: this may create an empty nested object which is necessary in case

[pneubeck (no reviews), 2014/08/20 08:16:16]

Instead of potentially creating empty nested objects, SetNestedOncValue could
create the missing Dictionary.

[stevenjb, 2014/08/20 17:18:38]

I was trying to avoid code duplication since an empty dictionary should be
harmless, but done.

+  // properties are added later with SetNestedOncValue.
   onc_object_->SetWithoutPathExpansion(onc_field_name, nested_object.release());
 }
 
+void ShillToONCTranslator::SetNestedOncValue(
+    const std::string& onc_dictionary_name,
+    const std::string& onc_field_name,
+    const base::Value& value) {
+  base::DictionaryValue* nested;
+  if (!onc_object_->GetDictionaryWithoutPathExpansion(
+          onc_dictionary_name, &nested)) {
+    NOTREACHED();
+  }
+  nested->SetWithoutPathExpansion(onc_field_name, value.DeepCopy());
+}
+
 void ShillToONCTranslator::TranslateAndAddListOfObjects(
     const std::string& onc_field_name,
     const base::ListValue& list) {