Use GUID instead of servicePath in network settings

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/options/chromeos/internet_options_handler.h"
 
 #include <ctype.h>
 
 #include <map>
 #include <string>
@@ skipping 53 matching lines @@
 const char kShowCarrierSelectKey[] = "showCarrierSelect";
 const char kNetworkDataKey[] = "networkData";
 
 // 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 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[] =
     "options.internet.DetailsInternetPage.updateCarrier";
 
 // Setter methods called from JS that still need to be converted to match
 // networkingPrivate methods.
 const char kSetCarrierMessage[] = "setCarrier";
 const char kShowMorePlanInfoMessage[] = "showMorePlanInfo";
 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";
 
 // TODO(stevenjb): Deprecate these and integrate with settings Web UI.
 const char kAddConnectionMessage[] = "addConnection";
 const char kConfigureNetworkMessage[] = "configureNetwork";
 const char kActivateNetworkMessage[] = "activateNetwork";
 
 // These are strings used to communicate with JavaScript.
@@ skipping 10 matching lines @@
 const char kTagSimOpUnlock[] = "unlock";
 const char kTagVpnList[] = "vpnList";
 const char kTagWifiAvailable[] = "wifiAvailable";
 const char kTagWifiEnabled[] = "wifiEnabled";
 const char kTagWimaxAvailable[] = "wimaxAvailable";
 const char kTagWimaxEnabled[] = "wimaxEnabled";
 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,
                 const std::string& error_name,
                 scoped_ptr<base::DictionaryValue> error_data) {
   // UpdateConnectionData may send requests for stale services; ignore
   // these errors.
   if (function == "UpdateConnectionData" &&
       error_name == network_handler::kDBusFailedError)
     return;
   NET_LOG_ERROR("Shill Error from InternetOptionsHandler: " + error_name,
                 function);
 }
 
 const NetworkState* GetNetworkState(const std::string& service_path) {
   return NetworkHandler::Get()->network_state_handler()->
       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.
 base::DictionaryValue* BuildNetworkDictionary(
     const NetworkState* network,
     const PrefService* profile_prefs) {
   scoped_ptr<base::DictionaryValue> network_info =
       network_util::TranslateNetworkStateToONC(network);
 
   bool has_policy = onc::HasPolicyForNetwork(
       profile_prefs, g_browser_process->local_state(), *network);
   network_info->SetBoolean(kNetworkInfoKeyPolicyManaged, has_policy);
 
-  network_info->SetString(kNetworkInfoKeyServicePath, network->path());
-
   return network_info.release();
 }
 
 bool ShowViewAccountButton(const NetworkState* cellular) {
   if (cellular->activation_state() != shill::kActivationStateActivating &&
       cellular->activation_state() != shill::kActivationStateActivated)
     return false;
 
   const DeviceState* device =
       NetworkHandler::Get()->network_state_handler()->GetDeviceState(
@@ skipping 89 matching lines @@
       base::Bind(&InternetOptionsHandler::ShowMorePlanInfoCallback,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback(kSetCarrierMessage,
       base::Bind(&InternetOptionsHandler::SetCarrierCallback,
                  base::Unretained(this)));
   web_ui()->RegisterMessageCallback(kSimOperationMessage,
       base::Bind(&InternetOptionsHandler::SimOperationCallback,
                  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() {
   NetworkStateHandler* handler = NetworkHandler::Get()->network_state_handler();
   const DeviceState* device =
       handler->GetDeviceStateByType(NetworkTypePattern::Cellular());
   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());
     }
   }
   UpdateCarrier();
 }
 
 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;
   }
   const DeviceState* device = NetworkHandler::Get()->network_state_handler()->
       GetDeviceStateByType(NetworkTypePattern::Cellular());
   if (!device) {
     LOG(WARNING) << "SetCarrierCallback with no cellular device.";
     return;
   }
   NetworkHandler::Get()->network_device_handler()->SetCarrier(
@@ skipping 34 matching lines @@
     return;
   }
   SimDialogDelegate::ShowDialog(GetNativeWindow(), mode);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // networkingPrivate implementation methods. TODO(stevenjb): Use the
 // 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)) {
+void InternetOptionsHandler::StartConnectCallback(const base::ListValue* args) {
+  std::string guid;
+  if (!args->GetString(0, &guid)) {
     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)) {
-    NOTREACHED();
-    return;
-  }
-  ui::NetworkConnect::Get()->ConnectToNetwork(service_path);
+  std::string service_path = ServicePathFromGuid(guid);
+  if (!service_path.empty())
+    ui::NetworkConnect::Get()->ConnectToNetwork(service_path);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
 void InternetOptionsHandler::RefreshNetworkData() {
   base::DictionaryValue dictionary;
   FillNetworkInfo(&dictionary);
   web_ui()->CallJavascriptFunction(kRefreshNetworkDataFunction, dictionary);
 }
 
 void InternetOptionsHandler::UpdateConnectionData(
     const std::string& service_path) {
   NetworkHandler::Get()
       ->managed_network_configuration_handler()
       ->GetManagedProperties(
           LoginState::Get()->primary_user_hash(), service_path,
           base::Bind(&InternetOptionsHandler::GetManagedPropertiesResult,
                      weak_factory_.GetWeakPtr(), kUpdateConnectionDataFunction),
           base::Bind(&ShillError, "UpdateConnectionData"));
 }
 
 void InternetOptionsHandler::GetManagedPropertiesResult(
     const std::string& js_callback_function,
     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,
                              ShowViewAccountButton(network));
     }
   }
   web_ui()->CallJavascriptFunction(js_callback_function, *dictionary);
 }
 
 void InternetOptionsHandler::UpdateCarrier() {
   web_ui()->CallJavascriptFunction(kUpdateCarrierFunction);
 }
 
 void InternetOptionsHandler::DeviceListChanged() {
   if (!web_ui())
     return;
   RefreshNetworkData();
 }
 
 void InternetOptionsHandler::NetworkListChanged() {
   if (!web_ui())
     return;
   RefreshNetworkData();
 }
 
 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.

     const NetworkState* network) {
   if (!web_ui())
     return;
   RefreshNetworkData();
-  if (network->path() == details_path_)
-    UpdateConnectionData(network->path());
+  UpdateConnectionData(network->path());
 }
 
 void InternetOptionsHandler::DevicePropertiesUpdated(
     const DeviceState* device) {
   if (!web_ui())
     return;
   if (device->type() != shill::kTypeCellular)
     return;
   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 {
   return web_ui()->GetWebContents()->GetTopLevelNativeWindow();
 }
 
 const PrefService* InternetOptionsHandler::GetPrefs() const {
   return Profile::FromWebUI(web_ui())->GetPrefs();
 }
 
 void InternetOptionsHandler::AddConnection(const base::ListValue* args) {
   std::string onc_type;
   if (args->GetSize() != 1 || !args->GetString(0, &onc_type)) {
     NOTREACHED();
     return;
   }
   if (onc_type == ::onc::network_type::kWiFi) {
     NetworkConfigView::ShowForType(shill::kTypeWifi, GetNativeWindow());
   } else if (onc_type == ::onc::network_type::kVPN) {
     NetworkConfigView::ShowForType(shill::kTypeVPN, GetNativeWindow());
   } else if (onc_type == ::onc::network_type::kCellular) {
     ChooseMobileNetworkDialog::ShowDialog(GetNativeWindow());
   } else {
     LOG(ERROR) << "Unsupported type for AddConnection";
   }
 }
 
 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),
                             base::Bind(&ShillError, "RemoveNetwork"));
 }
 
 base::ListValue* InternetOptionsHandler::GetWiredList() {
   base::ListValue* list = new base::ListValue();
   const NetworkState* network = NetworkHandler::Get()->network_state_handler()->
       FirstNetworkByType(NetworkTypePattern::Ethernet());
@@ skipping 87 matching lines @@
   dictionary->SetBoolean(
       kTagWimaxAvailable,
       handler->IsTechnologyAvailable(NetworkTypePattern::Wimax()));
   dictionary->SetBoolean(
       kTagWimaxEnabled,
       handler->IsTechnologyEnabled(NetworkTypePattern::Wimax()));
 }
 
 }  // namespace options
 }  // namespace chromeos