ARC: Remove error status setting in GetNetworks.

components/arc/net/arc_net_host_impl.cc

 // Copyright 2015 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 "components/arc/net/arc_net_host_impl.h"
 
 #include <string>
 #include <vector>
 
 #include "base/location.h"
@@ skipping 34 matching lines @@
 }
 
 void ArcNetHostImpl::OnNetInstanceReady() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   NetHostPtr host;
   binding_.Bind(GetProxy(&host));
   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;
+  NetworkDataPtr network_data = NetworkData::New();
 
-  // 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));
+  // 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(
     const GetWifiEnabledStateCallback& callback) {
   bool is_enabled = GetStateHandler()->IsTechnologyEnabled(
       chromeos::NetworkTypePattern::WiFi());
 
   callback.Run(is_enabled);
 }
 
 void ArcNetHostImpl::StartScan() {
   GetStateHandler()->RequestScan();
 }
 
 void ArcNetHostImpl::ScanCompleted(const chromeos::DeviceState* /*unused*/) {
   if (arc_bridge_service()->net_version() < 1) {
     VLOG(1) << "ArcBridgeService does not support ScanCompleted.";
     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();
 }
 
 void ArcNetHostImpl::OnShuttingDown() {
   GetStateHandler()->RemoveObserver(this, FROM_HERE);
 }
 
 }  // namespace arc