WiFi client for GCD bootstrapping

chrome/browser/local_discovery/wifi/wifi_service_client.cc

+// Copyright 2014 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 "base/threading/sequenced_worker_pool.h"
+#include "base/threading/thread.h"
+#include "chrome/browser/local_discovery/wifi/wifi_service_client.h"
+#include "components/onc/onc_constants.h"
+#include "components/wifi/wifi_service.h"
+#include "content/public/browser/browser_thread.h"
+
+using wifi::WiFiService;
+
+namespace local_discovery {
+
+namespace wifi {
+
+namespace {
+
+scoped_ptr<base::DictionaryValue> MakeProperties(const std::string& ssid,
+                                                 const std::string& password) {
+  scoped_ptr<base::DictionaryValue> properties(new base::DictionaryValue);
+
+  properties->SetString(onc::network_config::kType, onc::network_type::kWiFi);
+  base::DictionaryValue* wifi = new base::DictionaryValue;
+  properties->Set(onc::network_config::kWiFi, wifi);
+
+  wifi->SetString(onc::wifi::kSSID, ssid);
+  wifi->SetString(onc::wifi::kPassphrase, password);
+
+  return properties.Pass();
+}
+
+}  // namespace
+
+class WifiServiceContainer {

[tbarzic, 2014/05/08 21:54:06]

the name is somewhat confusing.
It sounds more like something for stashing multiple wifi_service instances.

[Noam Samuel, 2014/05/12 23:54:12]

Renamed WifiServiceWrapper

+ public:
+  explicit WifiServiceContainer(
+      const base::WeakPtr<WifiServiceClient> service_client)
+      : service_client_(service_client), weak_factory_(this) {}

[stevenjb, 2014/05/09 18:46:15]

One arg per line

[Noam Samuel, 2014/05/12 23:54:12]

Done.

+
+  ~WifiServiceContainer() {}
+
+  void Start() {
+    wifi_service_.reset(WiFiService::Create());
+
+    wifi_service_->Initialize(base::MessageLoopProxy::current());
+
+    wifi_service_->SetEventObservers(
+        base::MessageLoopProxy::current(),
+        base::Bind(&WifiServiceContainer::OnNetworksChangedEvent,

[stevenjb, 2014/05/09 18:46:15]

OnNetworksChangedEvent is a no-op, so why pass it at all?

[Noam Samuel, 2014/05/12 23:54:12]

Done.

+                   base::Unretained(this)),
+        base::Bind(&WifiServiceContainer::OnNetworkListChangedEvent,
+                   base::Unretained(this)));

[stevenjb, 2014/05/09 18:46:15]

Rather than using Unretained (which implies that this might be unsafe), make
OnNetworkListChangedEvent a local function (since it doesn't actually have any
class dependencies).

[Noam Samuel, 2014/05/12 23:54:12]

OnNetworkListChangedEvent does have class dependencies (service_client_ in this
version or network_list_update_). I could bind them and make it a local
function, but I think that's somewhat clunky.

Also, since this class manages WifiService, the use of Unretained doesn't imply
lack of safety. I can use a weak pointer, but I think it's unnecessary and
implies more complexity than exists.

[stevenjb, 2014/05/13 00:18:06]

My bad, I didn't read OnNetworkListChangedEvent closely enough. And you're
correct, since wifi_service_ is owned, Unretained is fine.

+  }
+
+  void GetSSIDList(const WifiClient::SSIDListCallback& callback) {

[stevenjb, 2014/05/09 18:46:15]

For a complex implementation like this it is nicer to declare the class
separately from the implementation. (For smaller classes inlining the
implementation is fine).

[Noam Samuel, 2014/05/12 23:54:12]

Done.

+    base::ListValue visible_networks;
+
+    wifi_service_->GetVisibleNetworks(onc::network_type::kWiFi,
+                                      &visible_networks);
+
+    std::vector<NetworkProperties> network_property_list;
+
+    for (size_t i = 0; i < visible_networks.GetSize(); i++) {
+      const base::DictionaryValue* network_value = NULL;
+      NetworkProperties network_properties;
+      std::string connection_status;
+
+      if (!visible_networks.GetDictionary(i, &network_value) ||
+          !network_value->GetString(onc::network_config::kName,
+                                    &network_properties.ssid) ||
+          !network_value->GetString(onc::network_config::kGUID,
+                                    &network_properties.internal_id) ||
+          !network_value->GetString(onc::network_config::kConnectionState,
+                                    &connection_status)) {
+        NOTREACHED();
+        continue;
+      }
+
+      network_properties.connected =
+          (connection_status == onc::connection_state::kConnected);
+
+      network_property_list.push_back(network_properties);
+    }
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&WifiServiceClient::PostClosure,
+                   service_client_,
+                   base::Bind(callback, network_property_list)));
+  }
+
+  void ConnectToNetwork(const std::string& ssid,
+                        const std::string& internal_id,
+                        const std::string& password,
+                        const WifiClient::SuccessCallback& callback) {
+    scoped_ptr<base::DictionaryValue> properties =
+        MakeProperties(ssid, password);
+    std::string network_guid;
+    bool error = false;
+    std::string error_string;
+
+    if (!internal_id.empty()) {
+      network_guid = internal_id;
+      wifi_service_->SetProperties(
+          network_guid, properties.Pass(), &error_string);
+
+      if (!error_string.empty()) {
+        LOG(ERROR) << "Could not set properties on network: " << error_string;
+        error = true;
+      }
+    } else {
+      wifi_service_->CreateNetwork(
+          false, properties.Pass(), &network_guid, &error_string);
+
+      if (!error_string.empty()) {
+        LOG(ERROR) << "Could not create network: " << error_string;
+        error = true;
+      }
+    }
+
+    if (!error) {
+      wifi_service_->StartConnect(network_guid, &error_string);
+
+      if (!error_string.empty()) {
+        LOG(ERROR) << "Could not connect to network: " << error_string;
+        error = true;
+      }
+    }
+
+    content::BrowserThread::PostTask(content::BrowserThread::UI,

[tbarzic, 2014/05/08 21:54:06]

just a warning:
StartConnect succeeding does not necessary mean that the connect request
succeeded; the network may still fail to connect.

[Noam Samuel, 2014/05/12 23:54:12]

Could you expand on that? Will the network either have connected or failed to
connect, or could the connection state of the network only be apparent later? 

[stevenjb, 2014/05/13 00:18:06]

I'm less familiar with WifiService, but connecting to a service can take a
while, so StartConnect generally just sends a connection request to the system.
The state at this point may or may not even have changed to "Connecting" yet. An
error would generally only indicate that the service is unknown or unconfigured
so a connect request would be guaranteed to fail.

[Noam Samuel, 2014/05/13 23:51:55]

Refactored the code to be asynchronous and use a timeout , returning the user to
their old network upon failure.

+                                     FROM_HERE,
+                                     base::Bind(&WifiServiceClient::PostClosure,
+                                                service_client_,
+                                                base::Bind(callback, !error)));
+  }
+
+  void OnNetworksChangedEvent(const std::vector<std::string>& network_guids) {}
+
+  void OnNetworkListChangedEvent(
+      const std::vector<std::string>& network_guids) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&WifiServiceClient::OnNetworkListChanged, service_client_));
+  }
+
+  base::WeakPtr<WifiServiceContainer> AsWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  void RequestScan() { wifi_service_->RequestNetworkScan(); }
+
+  void ConnectToNetworkByID(const std::string& network_guid,
+                            const WifiClient::SuccessCallback& callback) {
+    std::string error_string;
+    bool error = false;
+
+    wifi_service_->StartConnect(network_guid, &error_string);
+
+    if (!error_string.empty()) {
+      LOG(ERROR) << "Could not connect to network by ID: " << error_string;
+      error = true;
+    }
+
+    content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                     FROM_HERE,
+                                     base::Bind(&WifiServiceClient::PostClosure,
+                                                service_client_,
+                                                base::Bind(callback, !error)));

[stevenjb, 2014/05/09 18:46:15]

It would be nice to share the common code between this and ConnectToNetwork.

[Noam Samuel, 2014/05/12 23:54:12]

Done.

+  }
+
+  void GetNetworkCredentials(const std::string& network_guid,
+                             const WifiClient::CredentialsCallback& callback) {
+    std::string error_string;
+    bool error = false;
+    std::string ssid;
+    std::string key;
+
+    base::DictionaryValue properties;
+
+    wifi_service_->GetProperties(network_guid, &properties, &error_string);
+
+    if (!error_string.empty()) {
+      LOG(ERROR) << "Could not get network properties: " << error_string;
+      error = true;
+    }
+
+    if (!properties.GetString(onc::network_config::kName, &ssid)) {
+      LOG(ERROR) << "Could not get network SSID";
+      error = true;
+    }
+
+    if (!error) {
+      wifi_service_->GetKeyFromSystem(network_guid, &key, &error_string);
+
+      if (!error_string.empty()) {
+        LOG(ERROR) << "Could not get key from system: " << error_string;
+        error = true;
+      }
+    }
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&WifiServiceClient::PostClosure,

[tbarzic, 2014/05/08 21:54:06]

I think it would make more sense to do this binding in WifiServiceClient when
GetNetworkCredentails is called (and get rid of dependecy on
wifi_service_client_ from WifiServiceClientContainer)

[Noam Samuel, 2014/05/12 23:54:12]

Done.

+                   service_client_,
+                   base::Bind(callback, !error, ssid, key)));
+  }
+
+ private:
+  scoped_ptr<WiFiService> wifi_service_;
+  base::WeakPtr<WifiServiceClient> service_client_;
+  base::WeakPtrFactory<WifiServiceContainer> weak_factory_;
+};
+
+scoped_ptr<WifiClient> WifiClient::Create() {
+  return scoped_ptr<WifiClient>(new WifiServiceClient());
+}
+
+WifiServiceClient::WifiServiceClient() : weak_factory_(this) {
+}
+
+WifiServiceClient::~WifiServiceClient() {
+}
+
+void WifiServiceClient::Start() {
+  task_runner_ = content::BrowserThread::GetMessageLoopProxyForThread(
+      content::BrowserThread::FILE);
+
+  scoped_ptr<WifiServiceContainer, ThreadSpecificDeleter<WifiServiceContainer> >
+      wifi_container(new WifiServiceContainer(weak_factory_.GetWeakPtr()),
+                     ThreadSpecificDeleter<WifiServiceContainer>(task_runner_));
+  wifi_container_.swap(wifi_container);

[Vitaly Buka (NO REVIEWS), 2014/05/05 22:05:58]

why do you need swap here?

why not just
wifi_container_.reset(new container)?

[Noam Samuel, 2014/05/12 23:54:12]

It was in order to have a stateful deleter with a task runner in it. However,
since we're using the file thread, I can change that to use
content::BrowserThread::DeleteOnThread.

+
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&WifiServiceContainer::Start, wifi_container_->AsWeakPtr()));

[tbarzic, 2014/05/08 21:54:06]

AsWeakPtr is no particularly useful here:
AsWeakPtr will return a weak ptr bound to the current thread, which is not the
thread it's used on; so this the weak ptr being set will not guarantee that
wifi_container is actually alive.

(this will probably crash on debug build)

[Noam Samuel, 2014/05/12 23:54:12]

Are you sure about this?
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
doesn't say anything about the thread the WeakPtr is issued on, it says that the
WeakPtr is bound to the thread it's dereferenced on.

I get the weak ptr on UI thread, but dereference and invalidate it only on the
file thread. If the constraints in the comments of weak_ptr are accurate, that
should be a valid way to use weak pointers.

[tbarzic, 2014/05/15 20:50:07]

yeah, looks like I was wrong about what's allowed.
Though, getting weak ptr on UI thread and later using it on FILE thread still
seems fragile, and not really useful in this case. It just makes ownership model
for the wrapper more confusing.

+}
+
+void WifiServiceClient::GetSSIDList(const SSIDListCallback& callback) {
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceContainer::GetSSIDList,
+                                    wifi_container_->AsWeakPtr(),
+                                    callback));
+}
+
+void WifiServiceClient::RequestScan(const SuccessCallback& callback) {
+  scan_callbacks_.push_back(callback);

[tbarzic, 2014/05/08 21:54:06]

I wouldn't rely on RequestScan causing OnNetworkListChanged event (the visible
network list may not change for a while) or that the next network list event is
necessary the result of the scan 

[Noam Samuel, 2014/05/12 23:54:12]

Thanks, changed the model a bit to add observers instead.

+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceContainer::RequestScan,
+                                    wifi_container_->AsWeakPtr()));
+}
+
+void WifiServiceClient::OnNetworkListChanged() {
+  std::vector<SuccessCallback> scan_callbacks;
+  scan_callbacks.swap(scan_callbacks_);
+
+  for (std::vector<SuccessCallback>::iterator i = scan_callbacks.begin();
+       i != scan_callbacks.end();
+       i++) {
+    i->Run(true);
+  }
+}
+
+void WifiServiceClient::PostClosure(const base::Closure& callback) {
+  callback.Run();

[Vitaly Buka (NO REVIEWS), 2014/05/05 22:05:58]

Thread checks around would be useful for understanding this code. 

[stevenjb, 2014/05/09 18:46:15]

No need for this to be a member function (unless thread checks against a member
of WifiServiceClient are added).

[Noam Samuel, 2014/05/12 23:54:12]

Done.

[Noam Samuel, 2014/05/12 23:54:12]

This needs to be a member function because we use the WeakPtr to ensure
callbacks are not called after the WifiServiceClient (now WifiManager) is
deleted.

This is explained in a comment in wifi_manager_nonchromeos.h
(wifi_service_client.h)

+}
+
+void WifiServiceClient::ConnectToNetwork(const std::string& ssid,
+                                         const std::string& internal_id,
+                                         const std::string& password,
+                                         const SuccessCallback& callback) {
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceContainer::ConnectToNetwork,
+                                    wifi_container_->AsWeakPtr(),
+                                    ssid,
+                                    internal_id,
+                                    password,
+                                    callback));
+}
+
+void WifiServiceClient::ConnectToNetworkByID(const std::string& internal_id,
+                                             const SuccessCallback& callback) {
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&WifiServiceContainer::ConnectToNetworkByID,
+                                    wifi_container_->AsWeakPtr(),
+                                    internal_id,
+                                    callback));
+}
+
+void WifiServiceClient::GetNetworkCredentials(
+    const std::string& internal_id,
+    const CredentialsCallback& callback) {
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&WifiServiceContainer::GetNetworkCredentials,
+                 wifi_container_->AsWeakPtr(),
+                 internal_id,
+                 callback));
+}
+
+}  // namespace wifi
+
+}  // namespace local_discovery