privetd: Expose dbus API

chromeos/dbus/privet_daemon_manager_client.h

+// 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.
+#ifndef CHROMEOS_DBUS_PRIVET_DAEMON_MANAGER_CLIENT_H_

[wtc, 2015/03/17 17:31:05]

Nit: add a blank line before this line.

[dtapuska, 2015/03/18 14:28:54]

Done.

+#define CHROMEOS_DBUS_PRIVET_DAEMON_MANAGER_CLIENT_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/values.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/dbus/dbus_client.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "dbus/object_path.h"
+#include "dbus/property.h"
+
+namespace chromeos {
+
+// PrivetDaemonManagerClient is used to communicate with the
+// privetd service.  All methods should be called from
+// the origin thread which initializes the DBusThreadManager instance.
+class CHROMEOS_EXPORT PrivetDaemonManagerClient : public DBusClient {
+ public:
+  struct PairingInfoVariant {
+    enum class Type {
+      BLOB,
+      STRING,
+    };
+    Type type;
+    std::string string;
+    std::vector<uint8_t> blob;

[wtc, 2015/03/17 17:31:05]

Nit: this is usually defined as a union in C code. I don't know what is the C++
way, but only one of |string| and |blob| is used, depending on the value of
|type|.

[satorux1, 2015/03/17 17:39:11]

Do we need to have two different types for the storage? I thought std::string
and std::vector<uint8_t> were essentially the same thing.

[dtapuska, 2015/03/18 14:28:54]

Well yes they are an array of bytes (although one is signed and the other
isn't).

What I am specifically calling out is that std::string is likely printable; and
although I could put the binary code in a string; I felt that then somebody
would feel it is printable which it certainly isn't. ie; it isn't ASCII or
UTF-8; it is a blob of binary data.

[dtapuska, 2015/03/18 14:28:54]

before C++ 11 you weren't allowed to put non-POD types in a union. It wasn't
clear in the style guide if I can use unions of non-POD types; the guide
declares if it is not explicitly called out don't use it. This is why I did it
this way.

[hashimoto, 2015/03/19 08:00:22]

How about replacing this map of variant with a class like this?
class ParingInfo {
public:
  std::string sessionId() const;
  std::string mode() const;
  std::string code() const;
...
};

If you really want to have a variant type in C++, there is already base::Value.
If you use base::Value, you can use base::DictionaryValue instead of std::map.
You can also use functions declared in dbus/values_util.h to pop/append
base::Values to D-Bus messages.

[dtapuska, 2015/03/19 18:04:24]

I wasn't sure what would be the desired approach here. Done.

+  };
+
+  // Structure of properties associated with a privet Manager.
+  class ManagerProperties : public dbus::PropertySet {
+   public:
+    ManagerProperties(dbus::ObjectProxy* object_proxy,
+                      const std::string& interface_name,
+                      const PropertyChangedCallback& callback);
+    ~ManagerProperties() override;
+
+    // State of WiFi bootstrapping.
+    // Values are "disabled", "waiting", "connecting", "monitoring".
+    const std::string& wifi_bootstrap_state() const {
+      return wifi_bootstrap_state_.value();
+    }
+
+    // State of GCD bootstrapping.
+    // Values are "disabled", "offline", "connecting", "waiting", "registering",
+    // "online".
+    const std::string& gcd_boostrap_state() const {
+      return gcd_bootstrap_state_.value();
+    }
+
+    // State of device pairing. The map will contain the following keys.
+    //  "sessionId" - ID of the pairing session; generated by device
+    //  "mode" - Selected type of pairing from /privet/v3/pairing/start
+    //                    (e.g. "pinCode" or "embeddedCode")
+    //  "code" - The pin code or embedded code as appropriate to the
+    //           "mode" value.  See design document.
+    const std::map<std::string, PairingInfoVariant>& pairing_info() const {
+      return pairing_info_.value();
+    }
+
+    //  Concise note describing a peer.  Suitable for display to the user.
+    const std::string& description() const { return description_.value(); }
+
+    // Concise name describing a peer.  Suitable for display to the user.
+    const std::string& name() const { return name_.value(); }

[wtc, 2015/03/17 17:31:05]

Just wanted to make sure you considered the issue of whether these strings need
to be internationalized.

[dtapuska, 2015/03/18 14:28:54]

These are coming from the dbus API.. I don't have much influence putting an
interface on top of it.. 

+
+   private:
+    dbus::Property<std::string> wifi_bootstrap_state_;
+    dbus::Property<std::string> gcd_bootstrap_state_;
+    dbus::Property<std::map<std::string, PairingInfoVariant>> pairing_info_;
+    dbus::Property<std::string> description_;
+    dbus::Property<std::string> name_;
+
+    DISALLOW_COPY_AND_ASSIGN(ManagerProperties);
+  };
+
+  // Interface for observing changes from a apmanager daemon.
+  class Observer {
+   public:
+    virtual ~Observer();
+
+    // Called when the manager has been added.
+    virtual void ManagerAdded();
+
+    // Called when the manager has been removed.
+    virtual void ManagerRemoved();
+
+    // Called when the manager has a change in value of the property named
+    // |property_name|.
+    virtual void ManagerPropertyChanged(const std::string& property_name);

[wtc, 2015/03/17 17:31:05]

Should these methods be declared as abstract (= 0)?

[dtapuska, 2015/03/18 14:28:54]

According to Google C++ style likely. Although the general pattern in the
chromeos/dbus classes were not to make them completely abstract.

+  };
+
+  ~PrivetDaemonManagerClient() override;
+
+  // Factory function, creates a new instance which is owned by the caller.
+  // For normal usage, access the singleton via DBusThreadManager::Get().
+  static PrivetDaemonManagerClient* Create();
+
+  // Adds and removes observers for events on all apmanager
+  // events. Check the |object_path| parameter of observer methods to

[wtc, 2015/03/17 17:31:05]

|object_path| is not used in the Observer interface. Did you mean
|property_name|?

[dtapuska, 2015/03/18 14:28:54]

Done.

+  // determine which group is issuing the event.
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
+
+  // Calls SetDescription method.
+  // |callback| is called with its |call_status| argument set to
+  // DBUS_METHOD_CALL_SUCCESS if the method call succeeds. Otherwise,
+  // |callback| is called with |call_status| set to DBUS_METHOD_CALL_FAILURE.
+  virtual void SetDescription(const std::string& description,
+                              const VoidDBusMethodCallback& callback) = 0;
+
+  // Obtains the properties for the manager any values should be
+  // copied if needed.
+  virtual const ManagerProperties* GetManagerProperties() = 0;
+
+ protected:
+  // Create() should be used instead.
+  PrivetDaemonManagerClient();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PrivetDaemonManagerClient);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_DBUS_PRIVET_DAEMON_MANAGER_CLIENT_H_