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_
+#define CHROMEOS_DBUS_PRIVET_DAEMON_MANAGER_CLIENT_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"

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

nit: Seems unused.

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

Done.

+#include "base/macros.h"
+#include "base/values.h"

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

ditto.

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

Done.

+#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;
+
+    bool operator==(

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

Our style guide disallows operator overloading in general.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Operator_Ove...

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

It was used in the test case; which is permitted because of the STL == operator
used on the map. But I've since change this not to use a map per your
suggestion.

+        const chromeos::PrivetDaemonManagerClient::PairingInfoVariant& other)
+        const {
+      return type == other.type && string == other.string && blob == other.blob;
+    }
+  };
+
+  class PairingInfoVariantMapProperty : public dbus::PropertyBase {
+   public:
+    using PairingInfoVariantMap =
+        std::map<std::string,
+                 chromeos::PrivetDaemonManagerClient::PairingInfoVariant>;

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

nit: "chromeos::PrivetDaemonManagerClient::" is not needed.

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

Done.

+
+    bool PopValueFromReader(dbus::MessageReader* reader) override;
+    void AppendSetValueToWriter(dbus::MessageWriter* writer) override;
+    void ReplaceValueWithSetValue() override;
+    void ReplaceSetValueForTesting(const PairingInfoVariantMap& value);
+
+    const PairingInfoVariantMap& value() const { return value_; }
+
+   private:
+    PairingInfoVariantMap value_;
+    PairingInfoVariantMap set_value_;

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

It seems this member is non-empty only in tests.
Is this needed?

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

The PropertyBase makes special use of set_value vs value.. specifically there
are overrides. I don't want to deviate too far from the desired implementation
that PropertyBase teaches. It is mainly here for writing...

[hashimoto, 2015/03/20 06:41:48]

What I'm curious about is if there is any chance AppendSetValueToWriter(),
ReplaceValueWithSetValue() and |set_value_| get used.
IIUC ParingInfoProperty is used in read-only fashion because Properties doesn't
provide non-const access to |paring_info_|.

If we don't need write-access of this property for foreseeable future, there is
no reason to keep unused code being maintained.
You should remove |set_value_|, and the related methods should be just marked as
NOTIMPLEMENTED() if there is no possibility they get actually used.
(It's unfortunate dbus::PropertySet doesn't provide a way to implement read-only
property cleanly, but that's another topic.)

[dtapuska, 2015/03/20 13:47:55]

Done.

+  };
+
+  // Structure of properties associated with a privet Manager.
+  class ManagerProperties : public dbus::PropertySet {

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

nit: PrivetDaemonManagerClient::ManagerProperties sounds a bit redundant.
How about just calling this class Properties?

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

Done.

+   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.

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

Why don't you just provide getter methods for each property listed below?

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

Done.

+    //  "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.

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

Please add a URL to a publicly available document.

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

Adjusted comments per class rewrite.

+    const std::map<std::string, PairingInfoVariant>& pairing_info() const {

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

nit: Please use the typedef above.

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

Done.

+      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(); }
+
+   private:
+    dbus::Property<std::string> wifi_bootstrap_state_;
+    dbus::Property<std::string> gcd_bootstrap_state_;
+    PairingInfoVariantMapProperty 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() = 0;
+
+    // Called when the manager has been removed.
+    virtual void ManagerRemoved() = 0;
+
+    // Called when the manager has a change in value of the property named
+    // |property_name|.
+    virtual void ManagerPropertyChanged(const std::string& property_name) = 0;
+  };
+
+  ~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.
+  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_