Use GUID instead of servicePath in network settings

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 d84e355a0bcb34da53dfa9348ca7cf6fbd5b1741..ab91cc721377ce990145f190a25b87664df25155 100644
--- a/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
+++ b/chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
@@ -71,8 +71,6 @@ const char kNetworkInfoKeyPolicyManaged[] = "policyManaged";
 // Functions we call in JavaScript.
 const char kRefreshNetworkDataFunction[] =
     "options.network.NetworkList.refreshNetworkData";
-const char kGetManagedPropertiesResultFunction[] =
-    "options.internet.DetailsInternetPage.getManagedPropertiesResult";
 const char kUpdateConnectionDataFunction[] =
     "options.internet.DetailsInternetPage.updateConnectionData";
 const char kUpdateCarrierFunction[] =
@@ -86,9 +84,7 @@ const char kSimOperationMessage[] = "simOperation";
 
 // TODO(stevenjb): Replace these with the matching networkingPrivate methods.
 // crbug.com/279351.
-const char kGetManagedPropertiesMessage[] = "getManagedProperties";
 const char kStartConnectMessage[] = "startConnect";
-const char kSetPropertiesMessage[] = "setProperties";
 
 // TODO(stevenjb): Add these to networkingPrivate.
 const char kRemoveNetworkMessage[] = "removeNetwork";
@@ -119,8 +115,6 @@ const char kTagWiredList[] = "wiredList";
 const char kTagWirelessList[] = "wirelessList";
 
 // Pseudo-ONC chrome specific properties appended to the ONC dictionary.
-const char kNetworkInfoKeyServicePath[] = "servicePath";
-const char kTagErrorMessage[] = "errorMessage";
 const char kTagShowViewAccountButton[] = "showViewAccountButton";
 
 void ShillError(const std::string& function,
@@ -140,6 +134,13 @@ const NetworkState* GetNetworkState(const std::string& service_path) {
       GetNetworkState(service_path);
 }
 
+std::string ServicePathFromGuid(const std::string& guid) {

[pneubeck (no reviews), 2015/03/12 20:53:58]

to consider:
should this handle an empty |guid|?

[stevenjb, 2015/03/13 01:20:14]

Do you mean log an error? It will already return an empty service path if no
networks match. I think it is fine as-is, it will be handled the same as for an
invalid guid.

+  const NetworkState* network =
+      NetworkHandler::Get()->network_state_handler()->GetNetworkStateFromGuid(
+          guid);
+  return network ? network->path() : "";
+}
+
 // Builds a dictionary with network information for the NetworkList on the
 // settings page. Ownership of the returned pointer is transferred to the
 // caller. TODO(stevenjb): Replace with calls to networkingPrivate.getNetworks.
@@ -153,8 +154,6 @@ base::DictionaryValue* BuildNetworkDictionary(
       profile_prefs, g_browser_process->local_state(), *network);
   network_info->SetBoolean(kNetworkInfoKeyPolicyManaged, has_policy);
 
-  network_info->SetString(kNetworkInfoKeyServicePath, network->path());
-
   return network_info.release();
 }
 
@@ -264,27 +263,23 @@ void InternetOptionsHandler::RegisterMessages() {
                  base::Unretained(this)));
 
   // networkingPrivate methods
-  web_ui()->RegisterMessageCallback(kGetManagedPropertiesMessage,
-      base::Bind(&InternetOptionsHandler::GetManagedPropertiesCallback,
-                 base::Unretained(this)));
   web_ui()->RegisterMessageCallback(kStartConnectMessage,
       base::Bind(&InternetOptionsHandler::StartConnectCallback,
                  base::Unretained(this)));
-  web_ui()->RegisterMessageCallback(kSetPropertiesMessage,
-      base::Bind(&InternetOptionsHandler::SetPropertiesCallback,
-                 base::Unretained(this)));
 }
 
 void InternetOptionsHandler::ShowMorePlanInfoCallback(
     const base::ListValue* args) {
   if (!web_ui())
     return;
-  std::string service_path;
-  if (args->GetSize() != 1 || !args->GetString(0, &service_path)) {
+  std::string guid;
+  if (args->GetSize() != 1 || !args->GetString(0, &guid)) {
     NOTREACHED();
     return;
   }
-  ui::NetworkConnect::Get()->ShowMobileSetup(service_path);
+  std::string service_path = ServicePathFromGuid(guid);
+  if (!service_path.empty())
+    ui::NetworkConnect::Get()->ShowMobileSetup(service_path);
 }
 
 void InternetOptionsHandler::CarrierStatusCallback() {
@@ -294,7 +289,7 @@ void InternetOptionsHandler::CarrierStatusCallback() {
   if (device && (device->carrier() == shill::kCarrierSprint)) {
     const NetworkState* network =
         handler->FirstNetworkByType(NetworkTypePattern::Cellular());
-    if (network && network->path() == details_path_) {
+    if (network) {
       ui::NetworkConnect::Get()->ActivateCellular(network->path());
       UpdateConnectionData(network->path());
     }
@@ -303,11 +298,8 @@ void InternetOptionsHandler::CarrierStatusCallback() {
 }
 
 void InternetOptionsHandler::SetCarrierCallback(const base::ListValue* args) {
-  std::string service_path;
   std::string carrier;
-  if (args->GetSize() != 2 ||
-      !args->GetString(0, &service_path) ||
-      !args->GetString(1, &carrier)) {
+  if (args->GetSize() != 1 || !args->GetString(1, &carrier)) {
     NOTREACHED();
     return;
   }
@@ -362,33 +354,15 @@ void InternetOptionsHandler::SimOperationCallback(const base::ListValue* args) {
 // networkingPrivate API directly in the settings JS and deprecate these
 // methods. crbug.com/279351.
 
-void InternetOptionsHandler::GetManagedPropertiesCallback(
-    const base::ListValue* args) {
-  std::string service_path;
-  if (!args->GetString(0, &service_path)) {
-    NOTREACHED();
-    return;
-  }
-  // This is only ever called to provide properties for the details page, so
-  // set |details_path_| (used by the NetworkState observers) here.
-  details_path_ = service_path;
-  NetworkHandler::Get()
-      ->managed_network_configuration_handler()
-      ->GetManagedProperties(
-          LoginState::Get()->primary_user_hash(), service_path,
-          base::Bind(&InternetOptionsHandler::GetManagedPropertiesResult,
-                     weak_factory_.GetWeakPtr(),
-                     kGetManagedPropertiesResultFunction),
-          base::Bind(&ShillError, "GetManagedProperties"));
-}
-
 void InternetOptionsHandler::StartConnectCallback(const base::ListValue* args) {
-  std::string service_path;
-  if (!args->GetString(0, &service_path)) {
+  std::string guid;
+  if (!args->GetString(0, &guid)) {
     NOTREACHED();
     return;
   }
-  ui::NetworkConnect::Get()->ConnectToNetwork(service_path);
+  std::string service_path = ServicePathFromGuid(guid);
+  if (!service_path.empty())
+    ui::NetworkConnect::Get()->ConnectToNetwork(service_path);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -415,17 +389,8 @@ void InternetOptionsHandler::GetManagedPropertiesResult(
     const std::string& service_path,
     const base::DictionaryValue& onc_properties) {
   scoped_ptr<base::DictionaryValue> dictionary(onc_properties.DeepCopy());
-  // Add service path for now.
-  dictionary->SetString(kNetworkInfoKeyServicePath, service_path);
-
   const NetworkState* network = GetNetworkState(service_path);
   if (network) {
-    // Add a Chrome specific translated error message. TODO(stevenjb): Figure
-    // out a more robust way to track errors. Service.Error is transient so we
-    // use NetworkState.error() which accurately tracks the "last" error.
-    dictionary->SetString(kTagErrorMessage,
-                          ui::NetworkConnect::Get()->GetShillErrorString(
-                              network->error(), service_path));
     // Add additional non-ONC cellular properties to inform the UI.
     if (network->type() == shill::kTypeCellular) {
       dictionary->SetBoolean(kTagShowViewAccountButton,
@@ -455,8 +420,7 @@ void InternetOptionsHandler::NetworkConnectionStateChanged(
     const NetworkState* network) {
   if (!web_ui())
     return;

[pneubeck (no reviews), 2015/03/12 20:53:58]

is that such a good idea to send all network properties on every state or
properties change of any network to JS?

[stevenjb, 2015/03/13 01:20:14]

The JS already ignores updates to other networks. This is how the
networkingPrivate events will work when we switch to using those in the JS
instead of forwarding them here. If that overhead is a problem then we have to
rethink our approach, but it shouldn't be. These events occur with a frequency
that is O(seconds) not milliseconds.

[pneubeck (no reviews), 2015/03/13 09:54:20]

That's not what the API currently exposes.
All events send lists of GUIDs (which we considered extending to list of state
properties) but because of the overhead not all configuration properties.
JS will have to filter from the GUIDs and request state properties or all
properties, depending on what it needs.
I remember someone caring about hundreds of Wi-Fi networks in range...
If that isn't a problem, it's fine with me.

[stevenjb, 2015/03/13 17:01:14]

Sure, that's fair, this will send more data than just the guids. And we do have
to be aware of ~100 networks in range, but in practice Shill limits the number
of networks it will send, e.g. strength updates for.

By eliminating GetManagedPropertiesCallback we lost our convenient way to track
the guid, but I could track it explicitly to alleviate that concern.

-  if (network->path() == details_path_)
-    UpdateConnectionData(network->path());
+  UpdateConnectionData(network->path());
 }
 
 void InternetOptionsHandler::NetworkPropertiesUpdated(

[pneubeck (no reviews), 2015/03/12 20:53:58]

this is triggered for every signalStrength change of any network

[stevenjb, 2015/03/13 01:20:14]

Still infrequent as these things go. Previously we were sending entire icon
updates with every signal strength change.

[pneubeck (no reviews), 2015/03/13 09:54:20]

fair enough.

@@ -464,8 +428,7 @@ void InternetOptionsHandler::NetworkPropertiesUpdated(
   if (!web_ui())
     return;
   RefreshNetworkData();
-  if (network->path() == details_path_)
-    UpdateConnectionData(network->path());
+  UpdateConnectionData(network->path());
 }
 
 void InternetOptionsHandler::DevicePropertiesUpdated(
@@ -477,24 +440,7 @@ void InternetOptionsHandler::DevicePropertiesUpdated(
   const NetworkState* network =
       NetworkHandler::Get()->network_state_handler()->FirstNetworkByType(
           NetworkTypePattern::Cellular());
-  if (network && network->path() == details_path_)
-    UpdateConnectionData(network->path());
-}
-
-void InternetOptionsHandler::SetPropertiesCallback(
-    const base::ListValue* args) {
-  std::string service_path;
-  const base::DictionaryValue* properties;
-  if (args->GetSize() < 2 ||
-      !args->GetString(0, &service_path) ||
-      !args->GetDictionary(1, &properties)) {
-    NOTREACHED();
-    return;
-  }
-  NetworkHandler::Get()->managed_network_configuration_handler()->SetProperties(
-      service_path, *properties,
-      base::Bind(&base::DoNothing),
-      base::Bind(&ShillError, "SetProperties"));
+  UpdateConnectionData(network->path());
 }
 
 gfx::NativeWindow InternetOptionsHandler::GetNativeWindow() const {
@@ -523,29 +469,36 @@ void InternetOptionsHandler::AddConnection(const base::ListValue* args) {
 }
 
 void InternetOptionsHandler::ConfigureNetwork(const base::ListValue* args) {
-  std::string service_path;
-  if (args->GetSize() != 1 || !args->GetString(0, &service_path)) {
+  std::string guid;
+  if (args->GetSize() != 1 || !args->GetString(0, &guid)) {
     NOTREACHED();
     return;
   }
-  NetworkConfigView::Show(service_path, GetNativeWindow());
+  std::string service_path = ServicePathFromGuid(guid);
+  if (!service_path.empty())
+    NetworkConfigView::Show(service_path, GetNativeWindow());
 }
 
 void InternetOptionsHandler::ActivateNetwork(const base::ListValue* args) {
-  std::string service_path;
-  if (args->GetSize() != 1 || !args->GetString(0, &service_path)) {
+  std::string guid;
+  if (args->GetSize() != 1 || !args->GetString(0, &guid)) {
     NOTREACHED();
     return;
   }
-  ui::NetworkConnect::Get()->ActivateCellular(service_path);
+  std::string service_path = ServicePathFromGuid(guid);
+  if (!service_path.empty())
+    ui::NetworkConnect::Get()->ActivateCellular(service_path);
 }
 
 void InternetOptionsHandler::RemoveNetwork(const base::ListValue* args) {
-  std::string service_path;
-  if (args->GetSize() != 1 || !args->GetString(0, &service_path)) {
+  std::string guid;
+  if (args->GetSize() != 1 || !args->GetString(0, &guid)) {
     NOTREACHED();
     return;
   }
+  std::string service_path = ServicePathFromGuid(guid);
+  if (service_path.empty())
+    return;
   NetworkHandler::Get()
       ->managed_network_configuration_handler()
       ->RemoveConfiguration(service_path, base::Bind(&base::DoNothing),