Add an ONC property for the third-party VPN provider extension ID

chromeos/network/network_state.cc

Index: chromeos/network/network_state.cc
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index 609c9f0f1c7b589ab30ffc2b52ccf1a37ce353da..3387f6532c7168e8513170825524ba839e43ddee 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -4,6 +4,7 @@
 
 #include "chromeos/network/network_state.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -166,25 +167,27 @@ bool NetworkState::PropertyChanged(const std::string& key,
     }
     return true;
   } else if (key == shill::kProviderProperty) {
+    third_party_vpn_provider_extension_id_.clear();

[pneubeck (no reviews), 2015/03/18 19:35:29]

I found the reason why I said we must be cautious with the clear()s here.
See the documentation of this function:
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...

If you return false, the state _must not_ have changed.
Please move this clear into the
 if (vpn_provider_type_ != shill::kProviderThirdPartyVpn)
case

Then there's still one corner-case left if vpn_provider_type_ changed to 3rd
party and fails in line 187.

[bartfab (slow), 2015/03/18 20:02:14]

Fixed, including the corner case.

     const base::DictionaryValue* dict;
-    std::string provider_type;
     if (!value.GetAsDictionary(&dict) ||
         !dict->GetStringWithoutPathExpansion(shill::kTypeProperty,
-                                             &provider_type)) {
+                                             &vpn_provider_type_)) {
+      NET_LOG(ERROR) << "Failed to parse " << path() << "." << key;
       return false;
     }
 
-    if (provider_type != shill::kProviderThirdPartyVpn) {
-      // If the network uses the built-in OpenVPN and L2TP support, set the
-      // provider extension ID to an empty string.
-      vpn_provider_extension_id_.clear();
+    if (vpn_provider_type_ != shill::kProviderThirdPartyVpn)
       return true;
-    }
 
     // If the network uses a third-party VPN provider, copy over the provider's
     // extension ID, which is held in |shill::kHostProperty|.
-    return dict->GetStringWithoutPathExpansion(shill::kHostProperty,
-                                               &vpn_provider_extension_id_);
+    if (!dict->GetStringWithoutPathExpansion(
+            shill::kHostProperty, &third_party_vpn_provider_extension_id_)) {
+      NET_LOG(ERROR) << "Failed to parse " << path() << "." << key;
+      return false;
+    }
+
+    return true;
   }
   return false;
 }
@@ -232,6 +235,20 @@ void NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
                                               connection_state());
   }
 
+  // VPN properties.
+  if (NetworkTypePattern::VPN().MatchesType(type())) {
+    scoped_ptr<base::DictionaryValue> provider_property(

[pneubeck (no reviews), 2015/03/18 19:35:29]

nit: drop a comment before this line, like:

"Shill is sending VPN provider properties in a nested dictionary (but expects to
receive a flat dictionary). |dictionary| must have the same structure."

[bartfab (slow), 2015/03/18 20:02:14]

Done.

+        new base::DictionaryValue);
+    provider_property->SetStringWithoutPathExpansion(shill::kTypeProperty,
+                                                     vpn_provider_type_);
+    if (vpn_provider_type_ == shill::kProviderThirdPartyVpn) {
+      provider_property->SetStringWithoutPathExpansion(
+          shill::kHostProperty, third_party_vpn_provider_extension_id_);
+    }
+    dictionary->SetWithoutPathExpansion(shill::kProviderProperty,
+                                        provider_property.release());
+  }
+
   // Wireless properties
   if (!NetworkTypePattern::Wireless().MatchesType(type()))
     return;