Support the ModemManager1 interfaces for SMS messages

chromeos/dbus/sms_client.cc

Index: chromeos/dbus/sms_client.cc
diff --git a/chromeos/dbus/sms_client.cc b/chromeos/dbus/sms_client.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b8a93515c0a6d792fe2a3c2ffebfee9cad237d40
--- /dev/null
+++ b/chromeos/dbus/sms_client.cc
@@ -0,0 +1,153 @@
+// Copyright (c) 2012 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 "chromeos/dbus/sms_client.h"
+
+#include <map>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
+#include "base/stringprintf.h"
+#include "base/stl_util.h"
+#include "base/values.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+#include "dbus/values_util.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+// A class which makes method calls for SMS services via the
+// org.freedesktop.ModemManager1.Sms object using the DBusProperties
+// interface.
+class SMSProxy {
+ public:
+  typedef base::Callback<void(const base::DictionaryValue& sms)> GetAllCallback;
+
+  SMSProxy(dbus::Bus* bus,
+           const std::string& service_name,
+           const dbus::ObjectPath& object_path)
+      : proxy_(bus->GetObjectProxy(service_name, object_path)),
+        weak_ptr_factory_(this) {
+  }
+
+  // Gets all the properties of the SMS.
+  void GetAll(const GetAllCallback& callback) {
+    dbus::MethodCall method_call(dbus::kDBusPropertiesInterface,
+                                 dbus::kDBusPropertiesGetAll);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(modemmanager::kModemManager1SmsInterface);
+    proxy_->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                       base::Bind(&SMSProxy::OnGetAll,
+                                  weak_ptr_factory_.GetWeakPtr(),
+                                  callback));
+  }
+
+ private:
+  // Handles responses of GetAll method calls.
+  void OnGetAll(const GetAllCallback& callback, dbus::Response* response) {
+    if (!response) {
+      // Must invoke the callback, even if there is no message.
+      callback.Run(base::DictionaryValue());
+      return;
+    }
+    dbus::MessageReader reader(response);
+    scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
+    base::DictionaryValue* dictionary_value = NULL;
+    if (!value.get() || !value->GetAsDictionary(&dictionary_value)) {
+      LOG(WARNING) << "Invalid response: " << response->ToString();
+      callback.Run(base::DictionaryValue());
+      return;
+    }
+    callback.Run(*dictionary_value);
+  }
+
+  void OnSignalConnected(const std::string& interface,

[hashimoto, 2012/06/06 07:19:44]

No need to have this method.

[Jason Glasgow, 2012/06/06 20:54:20]

Done.

+                         const std::string& signal,
+                         bool successed) {
+    LOG_IF(ERROR, !successed) << "Connect to " << interface << " "
+                              << signal << " failed.";
+  }
+
+  dbus::ObjectProxy* proxy_;
+  base::WeakPtrFactory<SMSProxy> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SMSProxy);
+};
+
+// SMSClient is used to communicate with the
+// org.freedesktop.ModemManager1.SMS service.  All methods should be
+// called from the origin thread (UI thread) which initializes the
+// DBusThreadManager instance.
+class SMSClientImpl : public SMSClient {
+ public:
+  explicit SMSClientImpl(dbus::Bus* bus) : bus_(bus), weak_ptr_factory_(this) {}
+  virtual ~SMSClientImpl() {}
+
+  // Calls GetAll method.  |callback| is called after the method call succeeds.
+  virtual void GetAll(const std::string& service_name,
+                      const dbus::ObjectPath& object_path,
+                      const GetAllCallback& callback) OVERRIDE {
+    // The callback passed to GetAll owns the |proxy| pointer.
+    SMSProxy *proxy = new SMSProxy(bus_, service_name, object_path);
+    CHECK(proxy);

[hashimoto, 2012/06/06 07:19:44]

No need to CHECK

[Jason Glasgow, 2012/06/06 20:54:20]

Done.

+    proxy->GetAll(base::Bind(&SMSClientImpl::OnGetAll,
+                             weak_ptr_factory_.GetWeakPtr(),
+                             base::Owned(proxy), callback));

[hashimoto, 2012/06/06 07:19:44]

First I thought this trick (deleting |proxy| on OnGetAll) was fine.

But, it turns out that the callback passed to dbus::ObjectProxy is not
guaranteed to be deleted on UI thread.
This can be a problem because an object which owns a WeakPtrFactory must be
deleted on the same thread where it is created. (see base/memory/weak_ptr.h for
detail)

So, given conditions below:
1. 'delete proxy' in OnGetAll does not work since it can leak if the callback is
not called for some reason.
2. base::Owned(proxy) does not work since it cannot guarantee the proxy is
deleted on UI thread.
I think here we should have ProxyMap to ensure proxy is deleted (like
ModemMessagingClient), or merge SMSProxy to SMSClientImpl so we don't have
|proxy| and there is no need to delete it at all (it's easy since we are not
connecting to signal)

[Jason Glasgow, 2012/06/06 20:54:20]

I'm still working on a solution....
On 2012/06/06 07:19:44, hashimoto wrote:
Done.

+  }
+
+ private:
+  void OnGetAll(SMSProxy *proxy,
+                const GetAllCallback& callback,
+                const base::DictionaryValue& sms) {
+    callback.Run(sms);
+  }
+
+  dbus::Bus* bus_;
+  base::WeakPtrFactory<SMSClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SMSClientImpl);
+};
+
+class SMSClientStubImpl : public SMSClient {
+ public:
+  SMSClientStubImpl() {}
+  virtual ~SMSClientStubImpl() {}
+
+  virtual void GetAll(const std::string& service_name,
+                      const dbus::ObjectPath& object_path,
+                      const GetAllCallback& callback) OVERRIDE {
+    // TODO(jglasgow): implement for unit tests
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SMSClientStubImpl);
+};
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// SMSClient
+
+SMSClient::SMSClient() {}
+
+SMSClient::~SMSClient() {}
+
+
+// static
+SMSClient* SMSClient::Create(DBusClientImplementationType type,
+                             dbus::Bus* bus) {
+  if (type == REAL_DBUS_CLIENT_IMPLEMENTATION) {
+    return new SMSClientImpl(bus);
+  }
+  DCHECK_EQ(STUB_DBUS_CLIENT_IMPLEMENTATION, type);
+  return new SMSClientStubImpl();
+}
+
+}  // namespace chromeos