WiFi client for GCD bootstrapping

chrome/browser/local_discovery/wifi/wifi_manager.h

+// 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.
+
+#ifndef CHROME_BROWSER_LOCAL_DISCOVERY_WIFI_WIFI_MANAGER_H_
+#define CHROME_BROWSER_LOCAL_DISCOVERY_WIFI_WIFI_MANAGER_H_
+
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+
+namespace local_discovery {
+
+namespace wifi {
+
+struct NetworkProperties {
+  std::string internal_id;
+  std::string ssid;
+  bool connected;
+};
+
+class NetworkListObserver {
+ public:
+  virtual ~NetworkListObserver() {}
+
+  virtual void Start() = 0;

[stevenjb, 2014/05/15 18:16:26]

Please comment.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+};
+
+// Credentials for WiFi networks. Currently only supports PSK-based networks.
+// TODO(noamsml): Support for 802.11X and other authentication methods.
+struct WifiCredentials {
+  static WifiCredentials FromPSK(const std::string& psk);
+
+  std::string psk;
+};
+
+class WifiManager {

[stevenjb, 2014/05/15 18:16:26]

Please add a comment describing the purpose of this class.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+ public:
+  typedef base::Callback<void(const std::vector<NetworkProperties>& ssids)>
+      SSIDListCallback;
+  typedef base::Callback<void(bool success)> SuccessCallback;
+  typedef base::Callback<
+      void(bool success, const std::string& ssid, const std::string& password)>
+      CredentialsCallback;
+
+  virtual ~WifiManager() {}
+
+  static scoped_ptr<WifiManager> Create();
+
+  virtual void Start() = 0;

[stevenjb, 2014/05/15 18:16:26]

Please comment.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+  // Get the list of visible SSIDs in the vicinity. This does not initiate a
+  // scan, but merely gets the list of networks from the system.
+  virtual void GetSSIDList(const SSIDListCallback& callback) = 0;
+
+  // Request a scan for networks nearby. |callback| will be called once the scan
+  // is initiated and may be null.

[stevenjb, 2014/05/15 18:16:26]

Since this is just requesting a scan, does it really need a callback? Since
there is no success or failure indication, and the scan itself is asynchronous,
it doesn't seem like it would be useful.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+  virtual void RequestScan(const base::Closure& callback) = 0;
+
+  // Connect to a network with a given SSID and password. |callback| will be

[stevenjb, 2014/05/15 18:16:26]

This should probably say "Configure and connect to a network". I might also
optionally suggest naming this ConfigureNetworkAndConnect.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+  // called once the network is connected or after it has failed to connect.
+  virtual void ConnectToNetwork(const std::string& ssid,
+                                const WifiCredentials& credentials,
+                                const SuccessCallback& callback) = 0;
+
+  // Connect to a network with a given network ID. |callback| will be called
+  // once the network is connected or after it has failed to connect.
+  virtual void ConnectToNetworkByID(const std::string& internal_id,
+                                    const SuccessCallback& callback) = 0;
+
+  // Get the credentials for a network with a given network ID. |callback| will
+  // be called with credentials if they can be retrieved.
+  virtual void GetNetworkCredentials(const std::string& internal_id,
+                                     const CredentialsCallback& callback) = 0;

[stevenjb, 2014/05/15 18:16:26]

My first assumption was that this was caching the credentials which seemed bad.
I would consider renaming this RequestNetworkCredentials, and would definitely
put "from the system" somewhere in the comment.

[Noam Samuel, 2014/05/20 21:19:53]

Done.

+
+  // Create an observer for changes in the visible network list. As long as the
+  // observer is live, |callback| will be called with the new SSID list whenever
+  // it changes.

[stevenjb, 2014/05/15 18:16:26]

It's not clear to me how this is intended to be used, and I don't see it getting
called in this CL. I think it might be better to leave this call out of this CL
and add it when it is being used so that the reviewers can more easily
understand the intention.

Usually I would expect to see an implementer inherit from NetworkListObserver
and Add/Remove itself from the observed class (WifiManager).

[Noam Samuel, 2014/05/16 17:56:02]

This is an RAII observer tied to a callback. An object of some other class would
call this with a callback, and get back a scoped_ptr it can store. Then, when
the object is destroyed, the callback will automatically stop being called.
Maybe "observer" is the wrong word, does "watcher" make more sense?

[stevenjb, 2014/05/19 16:29:52]

Hmm. I'm not convinced that this would be a better model here than just making
"some other class" the observer and Add/Remove itself. Again, it would be easier
to judge if this were added when it is implemented. "Watcher" would be better
than Observer, but it's really more of a "CallbackWrapper".

[Noam Samuel, 2014/05/20 21:19:53]

Named to watcher because it's in line with ServiceWatcher, used in the (somewhat
related) service discovery code.

+  virtual scoped_ptr<NetworkListObserver> CreateNetworkListObserver(
+      const SSIDListCallback& callback) = 0;

[stevenjb, 2014/05/15 18:16:26]

DISALLOW_COPY_AND_ASSIGN

[Noam Samuel, 2014/05/20 21:19:53]

Causes compile errors. Also, unnecessary for abstract class?

+};
+
+}  // namespace wifi
+
+}  // namespace local_discovery
+
+#endif  // CHROME_BROWSER_LOCAL_DISCOVERY_WIFI_WIFI_MANAGER_H_