ARC: Remove error status setting in GetNetworks.

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 da7f4024d1c4a90eef73399225a938c9290a91c6..1ced53f0c34fa46d5497e7db87dd6e5971e7d78f 100644
--- a/components/arc/net/arc_net_host_impl.cc
+++ b/components/arc/net/arc_net_host_impl.cc
@@ -52,58 +52,79 @@ void ArcNetHostImpl::OnNetInstanceReady() {
   arc_bridge_service()->net_instance()->Init(std::move(host));
 }
 
+void ArcNetHostImpl::OnNetInstanceClosed() {
+  VLOG(1) << "OnNetInstanceClosed()";
+}
+
 void ArcNetHostImpl::GetNetworks(bool configured_only,
                                  bool visible_only,
                                  const GetNetworksCallback& callback) {

[Luis Héctor Chávez, 2016/03/01 16:22:31]

nit: add DCHECK(thread_checker_.CalledOnValidThread()); here and all the rest of
the methods (except where you expect a different thread to be used. In that case
comment what thread you expect.

-  NetworkDataPtr data = NetworkData::New();
-  data->status = NetworkResult::SUCCESS;
-
-  // Retrieve list of nearby wifi networks
-  chromeos::NetworkTypePattern network_pattern =
-      chromeos::onc::NetworkTypePatternFromOncType(onc::network_type::kWiFi);
-  scoped_ptr<base::ListValue> network_properties_list =
-      chromeos::network_util::TranslateNetworkListToONC(
-          network_pattern, configured_only, visible_only,
-          kGetNetworksListLimit);
-
-  // Extract info for each network and add it to the list.
-  for (base::Value* value : *network_properties_list) {
-    WifiConfigurationPtr wc = WifiConfiguration::New();
-
-    base::DictionaryValue* network_dict = nullptr;
-    value->GetAsDictionary(&network_dict);
-    DCHECK(network_dict);
-
-    // kName is a post-processed version of kHexSSID.
-    std::string tmp;
-    network_dict->GetString(onc::network_config::kName, &tmp);
-    DCHECK(!tmp.empty());
-    wc->ssid = tmp;
-
-    base::DictionaryValue* wifi_dict = nullptr;
-    network_dict->GetDictionary(onc::network_config::kWiFi, &wifi_dict);
-    DCHECK(wifi_dict);
-
-    if (!wifi_dict->GetInteger(onc::wifi::kFrequency, &wc->frequency))
-      wc->frequency = 0;
-    if (!wifi_dict->GetInteger(onc::wifi::kSignalStrength,
-                               &wc->signal_strength))
-      wc->signal_strength = 0;
-
-    if (!wifi_dict->GetString(onc::wifi::kSecurity, &tmp))
-      NOTREACHED();
-    DCHECK(!tmp.empty());
-    wc->security = tmp;
-
-    if (!wifi_dict->GetString(onc::wifi::kBSSID, &tmp))
-      NOTREACHED();
-    DCHECK(!tmp.empty());
-    wc->bssid = tmp;
-
-    data->networks.push_back(std::move(wc));
+  NetworkDataPtr network_data = NetworkData::New();
+
+  // This API supports either configured or visible networks only. Return with a
+  // failure if both flags are specified.
+  if (configured_only && visible_only) {

[Luis Héctor Chávez, 2016/03/01 16:22:31]

Hmmm this is problematic. Can you substitute these two flags with a three-value
enum?

[abhishekbh, 2016/03/02 19:16:31]

I don't think that gets us anything for the following reasons - 
1. With the enum I will have something like REQUEST_CONFIGURED_ONLY,
REQUEST_VISIBLE_ONLY, REQUEST_INVALID. Essentially it's just an int being passed
over which I would still have to check and I"ll end up doing the same thing.

2. This really isn't a true public interface. It is being used by the Instance,
so for all intents and purposes this is "internal" to the system.  And any gross
problems in malformed args should ideally be caught during development and fixed
on the Instance side since we know what we are sending down. This simple check
should suffice for this case.

+    network_data->status = NetworkResult::FAILURE;
+    callback.Run(std::move(network_data));
+    return;
   }
 
-  callback.Run(std::move(data));
+  network_data->status = NetworkResult::SUCCESS;
+  // The network list is populated only if it wasn't invalidated by a scan
+  // completed event, else send over the cached networks list.
+  visible_networks_lock_.Acquire();

[Luis Héctor Chávez, 2016/03/01 16:22:31]

This is the UI thred, so this is not allowed.

How about you get rid of this lock and then if ScanCompleted arrives on a
different thread than expected, post a task to the main thread?

+  if (cached_visible_networks_.size() == 0) {
+    // Retrieve list of nearby wifi networks
+    chromeos::NetworkTypePattern network_pattern =
+        chromeos::onc::NetworkTypePatternFromOncType(onc::network_type::kWiFi);
+    scoped_ptr<base::ListValue> network_properties_list =
+        chromeos::network_util::TranslateNetworkListToONC(
+            network_pattern, configured_only, visible_only,
+            kGetNetworksListLimit);
+
+    // Extract info for each network and add it to the list.
+    for (base::Value* value : *network_properties_list) {
+      WifiConfigurationPtr wc = WifiConfiguration::New();
+
+      base::DictionaryValue* network_dict = nullptr;
+      value->GetAsDictionary(&network_dict);
+      DCHECK(network_dict);
+
+      // kName is a post-processed version of kHexSSID.
+      std::string tmp;
+      network_dict->GetString(onc::network_config::kName, &tmp);
+      DCHECK(!tmp.empty());
+      wc->ssid = tmp;
+
+      base::DictionaryValue* wifi_dict = nullptr;
+      network_dict->GetDictionary(onc::network_config::kWiFi, &wifi_dict);
+      DCHECK(wifi_dict);
+
+      if (!wifi_dict->GetInteger(onc::wifi::kFrequency, &wc->frequency))
+        wc->frequency = 0;
+      if (!wifi_dict->GetInteger(onc::wifi::kSignalStrength,
+                                 &wc->signal_strength))
+        wc->signal_strength = 0;
+
+      if (!wifi_dict->GetString(onc::wifi::kSecurity, &tmp))
+        NOTREACHED();
+      DCHECK(!tmp.empty());
+      wc->security = tmp;
+
+      if (!wifi_dict->GetString(onc::wifi::kBSSID, &tmp))
+        NOTREACHED();
+      DCHECK(!tmp.empty());
+      wc->bssid = tmp;
+
+      network_data->networks.push_back(std::move(wc));
+    }
+    cached_visible_networks_ = network_data->networks.Clone();
+  } else {
+    network_data->networks = cached_visible_networks_.Clone();
+  }
+  visible_networks_lock_.Release();
+
+  callback.Run(std::move(network_data));
 }
 
 void ArcNetHostImpl::GetWifiEnabledState(
@@ -124,6 +145,11 @@ void ArcNetHostImpl::ScanCompleted(const chromeos::DeviceState* /*unused*/) {
     return;
   }
 
+  // Invalidate the networks list on a scan completion.
+  visible_networks_lock_.Acquire();
+  cached_visible_networks_.SetToEmpty();
+  visible_networks_lock_.Release();
+
   arc_bridge_service()->net_instance()->ScanCompleted();
 }