Notify ARC of default network changes

components/arc/net/arc_net_host_impl.cc

Index: components/arc/net/arc_net_host_impl.cc
diff --git a/components/arc/net/arc_net_host_impl.cc b/components/arc/net/arc_net_host_impl.cc
index 3ffab61ca33da84bbe185c923dcfd17459a8cc07..0c8e0e3565f41b9d2e41ad7dbdb4687450e6d3aa 100644
--- a/components/arc/net/arc_net_host_impl.cc
+++ b/components/arc/net/arc_net_host_impl.cc
@@ -370,6 +370,249 @@ void ArcNetHostImpl::ScanCompleted(const chromeos::DeviceState* /*unused*/) {
   arc_bridge_service()->net_instance()->ScanCompleted();
 }
 
+mojom::WiFiPtr TranslateONCWifi(const base::DictionaryValue* dict) {
+  mojom::WiFiPtr mojo = mojom::WiFi::New();
+  std::string tmp;
+
+  if (!dict->GetInteger(onc::wifi::kFrequency, &mojo->frequency))
+    NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

Frequency is not a required ONC property, we should provide a reasonable
default.

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+  if (!dict->GetString(onc::wifi::kBSSID, &tmp))
+    NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

Ditto.

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+  DCHECK(!tmp.empty());
+  mojo->bssid = tmp;
+
+  tmp.clear();
+  if (!dict->GetString(onc::wifi::kHexSSID, &tmp))
+    NOTREACHED();
+  DCHECK(!tmp.empty());
+  mojo->hex_ssid = tmp;

[stevenjb, 2016/05/16 19:29:35]

This pattern is used a lot. I think the code in this file would be a lot more
readable as:
mojo->hex_ssid = GetStringFromOncDictionary(dict, onc::wifi::kHexSSID, true /*
required */);

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+  if (!dict->GetBoolean(onc::wifi::kHiddenSSID, &mojo->hidden_ssid))
+    NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

Optional, defaults to false.

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+  tmp.clear();
+  if (!dict->GetString(onc::wifi::kSecurity, &tmp))
+    NOTREACHED();
+  DCHECK(!tmp.empty());
+  if (tmp == onc::wifi::kWEP_PSK)
+    mojo->security = mojom::SecurityType::WEP_PSK;
+  else if (tmp == onc::wifi::kWEP_8021X)
+    mojo->security = mojom::SecurityType::WEP_8021X;
+  else if (tmp == onc::wifi::kWPA_PSK)
+    mojo->security = mojom::SecurityType::WPA_PSK;
+  else if (tmp == onc::wifi::kWPA_EAP)
+    mojo->security = mojom::SecurityType::WPA_EAP;
+  else
+    NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

nit: Move this conversion to a helper function.

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+  if (!dict->GetInteger(onc::wifi::kSignalStrength, &mojo->signal_strength))
+    NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

Optional, defaults to 0

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+  return mojo;
+}
+
+mojo::Array<mojo::String> TranslateStringArray(const base::ListValue* list) {
+  mojo::Array<mojo::String> mojos = mojo::Array<mojo::String>::New(0);
+
+  for (size_t i = 0; i < list->GetSize(); i++) {
+    std::string tmp;
+    if (!list->GetString(i, &tmp))
+      NOTREACHED();
+    DCHECK(!tmp.empty());
+
+    mojo::String element;
+    element = tmp;
+    mojos.push_back(element);
+  }
+
+  return mojos;
+}

[stevenjb, 2016/05/16 19:29:35]

This seems like a generally useful utility function?

[Kevin Cernekee, 2016/05/17 01:56:27]

Acknowledged.

+
+mojo::Array<mojom::IPConfigurationPtr> TranslateONCIPConfigs(
+    const base::ListValue* list) {
+  mojo::Array<mojom::IPConfigurationPtr> mojos =
+      mojo::Array<mojom::IPConfigurationPtr>::New(0);
+
+  for (size_t i = 0; i < list->GetSize(); i++) {
+    const base::DictionaryValue* ip_dict = nullptr;
+    mojom::IPConfigurationPtr mojo = mojom::IPConfiguration::New();
+
+    list->GetDictionary(i, &ip_dict);
+    DCHECK(ip_dict);
+
+    std::string tmp;
+    if (!ip_dict->GetString(onc::ipconfig::kGateway, &tmp))
+      NOTREACHED();
+    DCHECK(!tmp.empty());
+    mojo->gateway = tmp;
+
+    tmp.clear();
+    if (!ip_dict->GetString(onc::ipconfig::kIPAddress, &tmp))
+      NOTREACHED();
+    DCHECK(!tmp.empty());
+    mojo->ip_address = tmp;
+
+    const base::ListValue* dns_list;
+    if (ip_dict->GetList(onc::ipconfig::kNameServers, &dns_list))
+      mojo->name_servers = TranslateStringArray(dns_list);
+    else
+      NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

This should be written:
if (!...GetList...)
  NOTREACHED();
mojo->name_servers = ...

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+    if (!ip_dict->GetInteger(onc::ipconfig::kRoutingPrefix,
+                             &mojo->routing_prefix))
+      NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

{}

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+    tmp.clear();
+    if (!ip_dict->GetString(onc::ipconfig::kType, &tmp))
+      NOTREACHED();
+    DCHECK(!tmp.empty());
+    if (tmp == onc::ipconfig::kIPv4)
+      mojo->type = mojom::IPAddressType::IPV4;
+    else if (tmp == onc::ipconfig::kIPv6)
+      mojo->type = mojom::IPAddressType::IPV6;
+    else
+      NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

More simply:
mojo->type = tmp == onc::ipconfig::kIPv6 ? mojom::IPAddressType::IPV6 :
mojom::IPAddressType::IPV4;

[Kevin Cernekee, 2016/05/17 01:56:26]

Done.

+
+    tmp.clear();
+    if (!ip_dict->GetString(onc::ipconfig::kWebProxyAutoDiscoveryUrl, &tmp))
+      NOTREACHED();
+    DCHECK(!tmp.empty());
+    mojo->web_proxy_auto_discovery_url = tmp;
+
+    mojos.push_back(std::move(mojo));
+  }
+  return mojos;
+}
+
+mojom::NetworkConfigurationPtr TranslateONCConfiguration(
+    const base::DictionaryValue* dict) {
+  mojom::NetworkConfigurationPtr mojo = mojom::NetworkConfiguration::New();
+  std::string tmp;
+
+  if (!dict->GetString(onc::network_config::kConnectionState, &tmp))
+    NOTREACHED();
+  DCHECK(!tmp.empty());
+  if (tmp == onc::connection_state::kConnected)
+    mojo->connection_state = mojom::ConnectionStateType::CONNECTED;
+  else if (tmp == onc::connection_state::kConnecting)
+    mojo->connection_state = mojom::ConnectionStateType::CONNECTING;
+  else if (tmp == onc::connection_state::kNotConnected)
+    mojo->connection_state = mojom::ConnectionStateType::NOT_CONNECTED;
+  else
+    NOTREACHED();

[stevenjb, 2016/05/16 19:29:35]

nit: helper function

[Kevin Cernekee, 2016/05/17 01:56:27]

Done.

+
+  tmp.clear();
+  if (!dict->GetString(onc::network_config::kGUID, &tmp))
+    NOTREACHED();
+  DCHECK(!tmp.empty());
+  mojo->guid = tmp;
+
+  const base::ListValue* ip_config_list = nullptr;
+  if (dict->GetList(onc::network_config::kIPConfigs, &ip_config_list)) {
+    DCHECK(ip_config_list);
+    mojo->ip_configs = TranslateONCIPConfigs(ip_config_list);
+  }
+
+  tmp.clear();
+  if (dict->GetString(onc::network_config::kMacAddress, &tmp)) {
+    DCHECK(!tmp.empty());
+    mojo->mac_address = tmp;
+  }
+
+  tmp.clear();
+  if (!dict->GetString(onc::network_config::kType, &tmp))
+    NOTREACHED();
+  DCHECK(!tmp.empty());
+  if (tmp == onc::network_type::kCellular) {
+    mojo->type = mojom::NetworkType::CELLULAR;
+  } else if (tmp == onc::network_type::kEthernet) {
+    mojo->type = mojom::NetworkType::ETHERNET;
+  } else if (tmp == onc::network_type::kVPN) {
+    mojo->type = mojom::NetworkType::VPN;
+  } else if (tmp == onc::network_type::kWiFi) {
+    mojo->type = mojom::NetworkType::WIFI;
+
+    const base::DictionaryValue* wifi_dict = nullptr;
+    dict->GetDictionary(onc::network_config::kWiFi, &wifi_dict);
+    DCHECK(wifi_dict);
+    mojo->wifi = TranslateONCWifi(wifi_dict);
+  } else if (tmp == onc::network_type::kWimax) {
+    mojo->type = mojom::NetworkType::WIMAX;
+  }
+
+  return mojo;
+}
+
+void GetDefaultNetworkSuccessCallback(
+    const ArcNetHostImpl::GetDefaultNetworkCallback& callback,
+    const std::string& service_path,
+    const base::DictionaryValue& dictionary) {
+  // TODO(cernekee): Figure out how to query Chrome for the default physical
+  // service if a VPN is connected, rather than just reporting the
+  // default logical service in both fields.

[stevenjb, 2016/05/16 19:29:35]

string type;
if (dictionary->GetString(kType, &type) && type == VPN) {
  physical_network =
GetNetworkStateHandler()->ConnectedNetworkByType(NetworkTypePattern::NonVirtual());
  if (physical_network) {
    GetManagedConfigurationHandler()->GetProperties(
           user_id_hash, physical_network->service_path(),
           base::Bind(&GetDefaultNetworkSuccessCallback, callback),
           base::Bind(&GetDefaultNetworkFailureCallback, callback));
    return;
  }
}  

[Kevin Cernekee, 2016/05/17 01:56:27]

Ack, will handle this in a separate CL.

+  callback.Run(TranslateONCConfiguration(&dictionary),
+               TranslateONCConfiguration(&dictionary));
+}
+
+void GetDefaultNetworkFailureCallback(
+    const ArcNetHostImpl::GetDefaultNetworkCallback& callback,
+    const std::string& error_name,
+    std::unique_ptr<base::DictionaryValue> error_data) {
+  LOG(ERROR) << "Failed to query default logical network";
+  callback.Run(nullptr, nullptr);
+}
+
+void ArcNetHostImpl::GetDefaultNetwork(
+    const GetDefaultNetworkCallback& callback) {
+  std::string user_id_hash = chromeos::LoginState::Get()->primary_user_hash();
+  GetManagedConfigurationHandler()->GetProperties(
+      user_id_hash, default_network_,
+      base::Bind(&GetDefaultNetworkSuccessCallback, callback),
+      base::Bind(&GetDefaultNetworkFailureCallback, callback));
+}
+
+void DefaultNetworkSuccessCallback(ArcNetHostImpl* instance,
+                                   const std::string& service_path,
+                                   const base::DictionaryValue& dictionary) {
+  instance->arc_bridge_service()->net_instance()->DefaultNetworkChanged(
+      TranslateONCConfiguration(&dictionary),
+      TranslateONCConfiguration(&dictionary));
+}
+
+void DefaultNetworkFailureCallback(
+    const std::string& error_name,
+    std::unique_ptr<base::DictionaryValue> error_data) {
+  LOG(ERROR) << "Failed to query default logical network";
+}
+
+void ArcNetHostImpl::DefaultNetworkChanged(
+    const chromeos::NetworkState* network) {
+  if (!network) {
+    default_network_.clear();
+    VLOG(1) << "No default network";
+  } else {
+    default_network_ = network->path();
+    VLOG(1) << "New default network: " << default_network_;
+  }
+
+  if (arc_bridge_service()->net_version() < 2) {
+    VLOG(1) << "ArcBridgeService does not support DefaultNetworkChanged.";
+    return;
+  }
+
+  if (default_network_.empty()) {

[stevenjb, 2016/05/16 19:29:35]

Not possible. (You can add a DCHECK to document that).

[Kevin Cernekee, 2016/05/17 01:56:26]

We hit this case when network is NULL (i.e. all interfaces are down/disabled).

[stevenjb, 2016/05/17 18:02:41]

I see. I would restructure this function:

if (!network) {
  VLOG...
  arc_bridge_service()->net_instance()->DefaultNetworkChanged(nullptr, nullptr);
  return;
}

+    arc_bridge_service()->net_instance()->DefaultNetworkChanged(nullptr,
+                                                                nullptr);
+  } else {
+    std::string user_id_hash = chromeos::LoginState::Get()->primary_user_hash();
+    GetManagedConfigurationHandler()->GetProperties(
+        user_id_hash, default_network_,
+        base::Bind(&DefaultNetworkSuccessCallback, base::Unretained(this)),

[stevenjb, 2016/05/16 19:29:35]

You should just bind default_network here instead of setting it as a member.

[Kevin Cernekee, 2016/05/17 01:56:26]

We set it as a member because we usually get a DefaultNetworkChanged event
before ArcBridgeService comes up.  In that case, ARC needs to explicitly query
the default network through the GetDefaultNetwork mojo call, so it is convenient
to have it cached.

[stevenjb, 2016/05/17 18:02:41]

As noted: I think it is better to let NetworkStateHandler to track the default
network and use GetNetworkStateHandler()->DefaultNetwork() instead.

+        base::Bind(&DefaultNetworkFailureCallback));
+  }
+}
+
 void ArcNetHostImpl::OnShuttingDown() {
   GetStateHandler()->RemoveObserver(this, FROM_HERE);
 }