Use Managed properties for Preferred and Provider.

chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc

Index: chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
diff --git a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
index c4c8e95f2931efb44c9e65ae3dcc5b18710ae516..39d6c770a8e14fbf4d2d857e8845d0bb9a41778f 100644
--- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
+++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
@@ -43,6 +43,7 @@
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_util.h"
 #include "chromeos/network/onc/onc_signature.h"
+#include "chromeos/network/onc/onc_translation_tables.h"
 #include "chromeos/network/onc/onc_translator.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "components/onc/onc_constants.h"
@@ -62,6 +63,10 @@ namespace options {
 
 namespace {
 
+// The key in a Managed Value dictionary for translated values.
+// TODO(stevenjb): Consider making this part of the ONC spec.
+const char kTranslatedKey[] = "Translated";
+
 // Keys for the network description dictionary passed to the web ui. Make sure
 // to keep the strings in sync with what the JavaScript side uses.
 const char kNetworkInfoKeyIconURL[] = "iconURL";
@@ -149,8 +154,6 @@ const char kTagNetworkId[] = "networkId";
 const char kTagOptions[] = "options";
 const char kTagPassword[] = "password";
 const char kTagPolicy[] = "policy";
-const char kTagPreferred[] = "preferred";
-const char kTagProviderType[] = "providerType";
 const char kTagProviderApnList[] = "providerApnList";
 const char kTagRecommended[] = "recommended";
 const char kTagRecommendedValue[] = "recommendedValue";
@@ -158,7 +161,6 @@ const char kTagRemembered[] = "remembered";
 const char kTagRememberedList[] = "rememberedList";
 const char kTagRestrictedPool[] = "restrictedPool";
 const char kTagRoamingState[] = "roamingState";
-const char kTagServerHostname[] = "serverHostname";
 const char kTagCarriers[] = "carriers";
 const char kTagCurrentCarrierIndex[] = "currentCarrierIndex";
 const char kTagShared[] = "shared";
@@ -337,14 +339,14 @@ const char* GetOncSettingString(::onc::ONCSource onc_source) {
 // is true, or the enforced value if |recommended| is false.
 // Note(stevenjb): This is bridge code until we use GetManagedProperties to
 // retrieve Shill properties.
-void SetManagedValueDictionary(const char* key,
-                               const base::Value* value,
-                               ::onc::ONCSource onc_source,
-                               bool recommended,
-                               const base::Value* default_value,
-                               base::DictionaryValue* settings) {
+void SetManagedValueDictionaryEx(const char* settings_dict_key,
+                                 const base::Value* value,
+                                 ::onc::ONCSource onc_source,
+                                 bool recommended,
+                                 const base::Value* default_value,
+                                 base::DictionaryValue* settings_dict) {
   base::DictionaryValue* dict = new base::DictionaryValue();
-  settings->Set(key, dict);
+  settings_dict->Set(settings_dict_key, dict);

[pneubeck (no reviews), 2014/08/19 19:26:38]

nit:
maybe clarify at the comment of this function whether keys with '.' are
supported or not.
Accordingly this should indeed either be 'Set' as it is or
SetWithoutPathExpansion.

Maybe say something like 'base::DictionaryValue's path expansion is applied to
|settings_dict_key|.' ?

[stevenjb, 2014/08/19 23:20:17]

This was not intended to support path expansion, settings_dict_key is expected
to be an ONC key. I added a comment and changed to SetWithout...

 
   DCHECK(value);
   dict->Set(::onc::kAugmentationActiveSetting, value->DeepCopy());
@@ -370,6 +372,41 @@ void SetManagedValueDictionary(const char* key,
   }
 }
 
+void SetManagedValueDictionary(const std::string& guid,
+                               const char* settings_dict_key,
+                               const base::Value* value,
+                               const std::string& onc_key,
+                               base::DictionaryValue* settings_dict) {
+

[pneubeck (no reviews), 2014/08/19 19:26:37]

nit: drop empty line

[stevenjb, 2014/08/19 23:20:18]

Done.

+  ::onc::ONCSource onc_source = ::onc::ONC_SOURCE_NONE;
+  const base::DictionaryValue* onc =
+      onc::FindPolicyForActiveUser(guid, &onc_source);
+  DCHECK_EQ(onc == NULL, onc_source == ::onc::ONC_SOURCE_NONE);
+  const base::Value* default_value = NULL;
+  if (onc)
+    onc->Get(onc_key, &default_value);
+  SetManagedValueDictionaryEx(settings_dict_key,
+                              value,
+                              onc_source,
+                              onc::IsRecommendedValue(onc, onc_key),
+                              default_value,
+                              settings_dict);
+}
+
+// Creates a GetManagedProperties style dictionary with an Active value and
+// a Translated value, and adds it to |settings|.
+// Note(stevenjb): This is bridge code until we use GetManagedProperties to
+// retrieve Shill properties and include Translated values.
+void SetTranslatedDictionary(const char* settings_dict_key,
+                             const std::string& value,
+                             const std::string& translated_value,
+                             base::DictionaryValue* settings_dict) {
+  base::DictionaryValue* dict = new base::DictionaryValue();
+  settings_dict->Set(settings_dict_key, dict);
+  dict->SetString(::onc::kAugmentationActiveSetting, value);
+  dict->SetString(kTranslatedKey, translated_value);
+}
+
 std::string CopyStringFromDictionary(const base::DictionaryValue& source,
                                      const std::string& src_key,
                                      const std::string& dest_key,
@@ -387,46 +424,53 @@ void PopulateVPNDetails(const NetworkState* vpn,
                         base::DictionaryValue* dictionary) {
   // Name and Remembered are set in PopulateConnectionDetails().
   // Provider properties are stored in the "Provider" dictionary.
-  const base::DictionaryValue* provider_properties = NULL;
+  const base::DictionaryValue* shill_provider_properties = NULL;
   if (!shill_properties.GetDictionaryWithoutPathExpansion(
-          shill::kProviderProperty, &provider_properties)) {
+          shill::kProviderProperty, &shill_provider_properties)) {
     LOG(ERROR) << "No provider properties for VPN: " << vpn->path();
     return;
   }
-  std::string provider_type;
-  provider_properties->GetStringWithoutPathExpansion(
-      shill::kTypeProperty, &provider_type);
-  dictionary->SetString(kTagProviderType,
-                        internet_options_strings::ProviderTypeString(
-                            provider_type,
-                            *provider_properties));
+  base::DictionaryValue* vpn_dictionary = new base::DictionaryValue;
+  dictionary->Set(::onc::network_config::kVPN, vpn_dictionary);
+
+  std::string shill_provider_type;
+  if (!shill_provider_properties->GetStringWithoutPathExpansion(
+          shill::kTypeProperty, &shill_provider_type)) {
+    LOG(ERROR) << "Shill VPN has no Provider.Type: " << vpn->path();
+    return;
+  }
+  std::string onc_provider_type;
+  onc::TranslateStringToONC(
+      onc::kVPNTypeTable, shill_provider_type, &onc_provider_type);
+  SetTranslatedDictionary(
+      ::onc::vpn::kType,
+      onc_provider_type,
+      internet_options_strings::ProviderTypeString(shill_provider_type,
+                                                   *shill_provider_properties),
+      vpn_dictionary);
+
+  base::DictionaryValue* provider_dictionary = new base::DictionaryValue;

[pneubeck (no reviews), 2014/08/19 19:26:37]

I have the impression that 'provider' is not the right term in this case, but
maybe I'm wrong.
Maybe something like vpn_subdictionary / vpn_details .. ?

[stevenjb, 2014/08/19 23:20:17]

Renamed provider_type_dictionary since that is what the key is. (Which, FWIW,
seems a little wonky to me, and certainly adds code complexity).

+  vpn_dictionary->Set(onc_provider_type, provider_dictionary);

[pneubeck (no reviews), 2014/08/19 19:26:38]

This will not work: The possible subdictionaries are OpenVPN, L2TP, IPsec
There is a VPN type 'L2TP-IPsec' but no dictionary 'L2TP-IPsec'.

I mean, it will work for now, but not anymore when you change to
GetManagedProperties

Since you set only the username for L2TP below, this should be easy to fix

[stevenjb, 2014/08/19 23:20:18]

Gah. OK, so... what? Let me look at the doc again...

 
   std::string username;
-  if (provider_type == shill::kProviderOpenVpn) {
-    provider_properties->GetStringWithoutPathExpansion(
+  if (shill_provider_type == shill::kProviderOpenVpn) {
+    shill_provider_properties->GetStringWithoutPathExpansion(
         shill::kOpenVPNUserProperty, &username);
   } else {
-    provider_properties->GetStringWithoutPathExpansion(
+    shill_provider_properties->GetStringWithoutPathExpansion(
         shill::kL2tpIpsecUserProperty, &username);
   }
-  dictionary->SetString(kTagUsername, username);
+  provider_dictionary->SetString(::onc::vpn::kUsername, username);

[pneubeck (no reviews), 2014/08/19 19:26:37]

The username is never set in IPsec but only in L2TP

[stevenjb, 2014/08/19 23:20:18]

OK, I think I maybe have this right now...

 
-  ::onc::ONCSource onc_source = ::onc::ONC_SOURCE_NONE;
-  const base::DictionaryValue* onc =
-      onc::FindPolicyForActiveUser(vpn->guid(), &onc_source);
-
-  NetworkPropertyUIData hostname_ui_data;
-  hostname_ui_data.ParseOncProperty(
-      onc_source,
-      onc,
-      ::onc::network_config::VpnProperty(::onc::vpn::kHost));
   std::string provider_host;
-  provider_properties->GetStringWithoutPathExpansion(
+  shill_provider_properties->GetStringWithoutPathExpansion(
       shill::kHostProperty, &provider_host);
-  SetValueDictionary(kTagServerHostname,
-                     new base::StringValue(provider_host),
-                     hostname_ui_data,
-                     dictionary);
+  SetManagedValueDictionary(
+      vpn->guid(),
+      ::onc::vpn::kHost,

[pneubeck (no reviews), 2014/08/19 19:26:38]

optional nit: having the same key with different prefixes two times in a call is
rather confusing. could you add argument comments that one refers to the key in
the result and one to the path in ONC ?

OR:
just use the same path and don't pass in 'vpn_dictionary' but 'dictionary' ?
That would also ensure that the structure of the resulting dictionary matches
ONC.

[stevenjb, 2014/08/19 23:20:17]

I really don't like using Value::Set with '.' separators, especially in
networking code where Shill sometimes uses '.' in key names.

I'll try to clarify the comments, but... there's no way this isn't going to be
confusing for now.

+      new base::StringValue(provider_host),

[pneubeck (no reviews), 2014/08/19 19:26:38]

memory leak, see other comment.

[stevenjb, 2014/08/19 23:20:17]

Bah, this is what happens when... nevermind. Fixed.

+      ::onc::network_config::VpnProperty(::onc::vpn::kHost),
+      vpn_dictionary);
 }
 
 // Given a list of supported carrier's by the device, return the index of
@@ -1332,11 +1376,11 @@ void InternetOptionsHandler::PopulateDictionaryDetailsCallback(
   int priority = 0;
   shill_properties.GetIntegerWithoutPathExpansion(
       shill::kPriorityProperty, &priority);
-  bool preferred = priority > 0;
-  SetValueDictionary(kTagPreferred,
-                     new base::FundamentalValue(preferred),
-                     property_ui_data,
-                     dictionary.get());
+  SetManagedValueDictionary(network->guid(),
+                            ::onc::network_config::kPriority,
+                            new base::FundamentalValue(priority),

[pneubeck (no reviews), 2014/08/19 19:26:38]

this will leak the FundamentalValue.
Please, please change the functions SetManagedValueDictionary(Ex) to take a
(const Value& value) to prevent this mistake.

[stevenjb, 2014/08/19 23:20:17]

Fixed.

+                            ::onc::network_config::kPriority,
+                            dictionary.get());
 
   std::string onc_path_to_auto_connect;
   if (network->Matches(NetworkTypePattern::WiFi())) {
@@ -1400,12 +1444,12 @@ void InternetOptionsHandler::PopulateDictionaryDetailsCallback(
         auto_connect_value.reset(new base::FundamentalValue(false));
       }
     }
-    SetManagedValueDictionary(shill::kAutoConnectProperty,
-                              auto_connect_value.get(),
-                              auto_connect_onc_source,
-                              auto_connect_recommended,
-                              auto_connect_default_value,
-                              dictionary.get());
+    SetManagedValueDictionaryEx(shill::kAutoConnectProperty,
+                                auto_connect_value.get(),
+                                auto_connect_onc_source,
+                                auto_connect_recommended,
+                                auto_connect_default_value,
+                                dictionary.get());
   }
 
   // Show details dialog