Revert 111479 - chrome: dbus: support asynchronous method replies

BUG=chromium-os:23241
TEST=Unit tests and manual testing on device.

Change-Id: I4d665897687030f4ab2379e4f6ddb9b3ebe02af4


Review URL: http://codereview.chromium.org/8637002

TBR=vlaviano@chromium.org
Review URL: http://codereview.chromium.org/8682032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111487 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 44 insertions, 135 deletions

diff --git a/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc b/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc
index a564c1d..921cf80 100644
--- a/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc
+++ b/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc
@@ -224,9 +224,8 @@ bool ProxyResolutionServiceProvider::OnOriginThread() {
   return base::PlatformThread::CurrentId() == origin_thread_id_;
 }
 
-void ProxyResolutionServiceProvider::ResolveProxyHandler(
-    dbus::MethodCall* method_call,
-    dbus::ExportedObject::ResponseSender response_sender) {
+dbus::Response* ProxyResolutionServiceProvider::ResolveProxyHandler(
+    dbus::MethodCall* method_call) {
   DCHECK(OnOriginThread());
   VLOG(1) << "Handing method call: " << method_call->ToString();
   // The method call should contain the three string parameters.
@@ -238,8 +237,7 @@ void ProxyResolutionServiceProvider::ResolveProxyHandler(
       !reader.PopString(&signal_interface) ||
       !reader.PopString(&signal_name)) {
     LOG(ERROR) << "Unexpected method call: " << method_call->ToString();
-    response_sender.Run(NULL);
-    return;
+    return NULL;
   }
 
   resolver_->ResolveProxy(source_url,
@@ -250,20 +248,18 @@ void ProxyResolutionServiceProvider::ResolveProxyHandler(
   // Return an empty response for now. We'll send a signal once the
   // network proxy resolution is completed.
   dbus::Response* response = dbus::Response::FromMethodCall(method_call);
-  response_sender.Run(response);
+  return response;
 }
 
 // static
-void ProxyResolutionServiceProvider::CallResolveProxyHandler(
+dbus::Response* ProxyResolutionServiceProvider::CallResolveProxyHandler(
     base::WeakPtr<ProxyResolutionServiceProvider> provider_weak_ptr,
-    dbus::MethodCall* method_call,
-    dbus::ExportedObject::ResponseSender response_sender) {
+    dbus::MethodCall* method_call) {
   if (!provider_weak_ptr) {
     LOG(WARNING) << "Called after the object is deleted";
-    response_sender.Run(NULL);
-    return;
+    return NULL;
   }
-  provider_weak_ptr->ResolveProxyHandler(method_call, response_sender);
+  return provider_weak_ptr->ResolveProxyHandler(method_call);
 }
 
 ProxyResolutionServiceProvider* ProxyResolutionServiceProvider::Create() {
diff --git a/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h b/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h
index 383c082..5a26358 100644
--- a/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h
+++ b/chrome/browser/chromeos/dbus/proxy_resolution_service_provider.h
@@ -15,9 +15,9 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
 #include "chrome/browser/chromeos/dbus/cros_dbus_service.h"
-#include "dbus/exported_object.h"
 
 namespace dbus {
+class ExportedObject;
 class MethodCall;
 class Response;
 }
@@ -104,15 +104,13 @@ class ProxyResolutionServiceProvider
   // Callback to be invoked when ChromeOS clients send network proxy
   // resolution requests to the service running in chrome executable.
   // Called on UI thread from dbus request.
-  void ResolveProxyHandler(dbus::MethodCall* method_call,
-      dbus::ExportedObject::ResponseSender response_sender);
+  dbus::Response* ResolveProxyHandler(dbus::MethodCall* method_call);
 
   // Calls ResolveProxyHandler() if weak_ptr is not NULL. Used to ensure a
   // safe shutdown.
-  static void CallResolveProxyHandler(
+  static dbus::Response* CallResolveProxyHandler(
       base::WeakPtr<ProxyResolutionServiceProvider> weak_ptr,
-      dbus::MethodCall* method_call,
-      dbus::ExportedObject::ResponseSender response_sender);
+      dbus::MethodCall* method_call);
 
   // Returns true if the current thread is on the origin thread.
   bool OnOriginThread();
diff --git a/dbus/bus.h b/dbus/bus.h
index 4af07ce..89fba91 100644
--- a/dbus/bus.h
+++ b/dbus/bus.h
@@ -100,15 +100,11 @@ class ObjectProxy;
 //
 // Exporting a method:
 //
-//   void Echo(dbus::MethodCall* method_call,
-//             dbus::ExportedObject::ResponseSender response_sender) {
+//   Response* Echo(dbus::MethodCall* method_call) {
 //     // Do something with method_call.
 //     Response* response = Response::FromMethodCall(method_call);
 //     // Build response here.
-//     // Can send an immediate response here to implement a synchronous service
-//     // or store the response_sender and send a response later to implement an
-//     // asynchronous service.
-//     response_sender.Run(response);
+//     return response;
 //   }
 //
 //   void OnExported(const std::string& interface_name,
diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index 5c98fe9..2f486f4 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -223,24 +223,6 @@ TEST_F(EndToEndAsyncTest, Timeout) {
   ASSERT_EQ("", response_strings_[0]);
 }
 
-// Tests calling a method that sends its reply asynchronously.
-TEST_F(EndToEndAsyncTest, AsyncEcho) {
-  const char* kHello = "hello";
-
-  // Create the method call.
-  dbus::MethodCall method_call("org.chromium.TestInterface", "AsyncEcho");
-  dbus::MessageWriter writer(&method_call);
-  writer.AppendString(kHello);
-
-  // Call the method.
-  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
-  CallMethod(&method_call, timeout_ms);
-
-  // Check the response.
-  WaitForResponses(1);
-  EXPECT_EQ(kHello, response_strings_[0]);
-}
-
 TEST_F(EndToEndAsyncTest, NonexistentMethod) {
   dbus::MethodCall method_call("org.chromium.TestInterface", "Nonexistent");
 
diff --git a/dbus/exported_object.cc b/dbus/exported_object.cc
index de25168..e4a1641 100644
--- a/dbus/exported_object.cc
+++ b/dbus/exported_object.cc
@@ -224,13 +224,12 @@ DBusHandlerResult ExportedObject::HandleMessage(
                                             method_call.release(),
                                             start_time));
   } else {
-    // If the D-Bus thread is not used, just call the method directly.
-    MethodCall* released_method_call = method_call.release();
-    iter->second.Run(released_method_call,
-                     base::Bind(&ExportedObject::SendResponse,
-                                this,
-                                start_time,
-                                released_method_call));
+    // If the D-Bus thread is not used, just call the method directly. We
+    // don't need the complicated logic to wait for the method call to be
+    // complete.
+    // |response| will be deleted in OnMethodCompleted().
+    Response* response = iter->second.Run(method_call.get());
+    OnMethodCompleted(method_call.release(), response, start_time);
   }
 
   // It's valid to say HANDLED here, and send a method response at a later
@@ -242,27 +241,14 @@ void ExportedObject::RunMethod(MethodCallCallback method_call_callback,
                                MethodCall* method_call,
                                base::TimeTicks start_time) {
   bus_->AssertOnOriginThread();
-  method_call_callback.Run(method_call,
-                           base::Bind(&ExportedObject::SendResponse,
-                                      this,
-                                      start_time,
-                                      method_call));
-}
 
-void ExportedObject::SendResponse(base::TimeTicks start_time,
-                                  MethodCall* method_call,
-                                  Response* response) {
-  DCHECK(method_call);
-  if (bus_->HasDBusThread()) {
-    bus_->PostTaskToDBusThread(FROM_HERE,
-                               base::Bind(&ExportedObject::OnMethodCompleted,
-                                          this,
-                                          method_call,
-                                          response,
-                                          start_time));
-  } else {
-    OnMethodCompleted(method_call, response, start_time);
-  }
+  Response* response = method_call_callback.Run(method_call);
+  bus_->PostTaskToDBusThread(FROM_HERE,
+                             base::Bind(&ExportedObject::OnMethodCompleted,
+                                        this,
+                                        method_call,
+                                        response,
+                                        start_time));
 }
 
 void ExportedObject::OnMethodCompleted(MethodCall* method_call,
diff --git a/dbus/exported_object.h b/dbus/exported_object.h
index 7ac2f88..bc67bcd 100644
--- a/dbus/exported_object.h
+++ b/dbus/exported_object.h
@@ -38,15 +38,9 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
                  const std::string& service_name,
                  const std::string& object_path);
 
-  // Called to send a response from an exported method. Response* is the
-  // response message. Callers should pass a NULL Response* in the event
-  // of an error that prevents the sending of a response.
-  typedef base::Callback<void (Response*)> ResponseSender;
-
   // Called when an exported method is called. MethodCall* is the request
-  // message. ResponseSender is the callback that should be used to send a
-  // response.
-  typedef base::Callback<void (MethodCall*, ResponseSender)> MethodCallCallback;
+  // message.
+  typedef base::Callback<Response* (MethodCall*)> MethodCallCallback;
 
   // Called when method exporting is done.
   // Parameters:
@@ -130,14 +124,7 @@ class ExportedObject : public base::RefCountedThreadSafe<ExportedObject> {
                  MethodCall* method_call,
                  base::TimeTicks start_time);
 
-  // Callback invoked by service provider to send a response to a method call.
-  // Can be called immediately from a MethodCallCallback to implement a
-  // synchronous service or called later to implement an asynchronous service.
-  void SendResponse(base::TimeTicks start_time,
-                    MethodCall* method_call,
-                    Response* response);
-
-  // Called on completion of the method run from SendResponse().
+  // Called on completion of the method run from RunMethod().
   // Takes ownership of |method_call| and |response|.
   void OnMethodCompleted(MethodCall* method_call,
                          Response* response,
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index ea8e360..b0207b0 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -13,8 +13,8 @@
 
 namespace dbus {
 
-// Echo, SlowEcho, AsyncEcho, BrokenMethod.
-const int TestService::kNumMethodsToExport = 4;
+// Echo, SlowEcho, BrokenMethod.
+const int TestService::kNumMethodsToExport = 3;
 
 TestService::Options::Options() {
 }
@@ -148,15 +148,6 @@ void TestService::Run(MessageLoop* message_loop) {
 
   exported_object_->ExportMethod(
       "org.chromium.TestInterface",
-      "AsyncEcho",
-      base::Bind(&TestService::AsyncEcho,
-                 base::Unretained(this)),
-      base::Bind(&TestService::OnExported,
-                 base::Unretained(this)));
-  ++num_methods;
-
-  exported_object_->ExportMethod(
-      "org.chromium.TestInterface",
       "BrokenMethod",
       base::Bind(&TestService::BrokenMethod,
                  base::Unretained(this)),
@@ -172,44 +163,25 @@ void TestService::Run(MessageLoop* message_loop) {
   message_loop->Run();
 }
 
-void TestService::Echo(MethodCall* method_call,
-                       dbus::ExportedObject::ResponseSender response_sender) {
+Response* TestService::Echo(MethodCall* method_call) {
   MessageReader reader(method_call);
   std::string text_message;
-  if (!reader.PopString(&text_message)) {
-    response_sender.Run(NULL);
-    return;
-  }
+  if (!reader.PopString(&text_message))
+    return NULL;
 
   Response* response = Response::FromMethodCall(method_call);
   MessageWriter writer(response);
   writer.AppendString(text_message);
-  response_sender.Run(response);
+  return response;
 }
 
-void TestService::SlowEcho(
-    MethodCall* method_call,
-    dbus::ExportedObject::ResponseSender response_sender) {
+Response* TestService::SlowEcho(MethodCall* method_call) {
   base::PlatformThread::Sleep(TestTimeouts::tiny_timeout_ms());
-  Echo(method_call, response_sender);
-}
-
-void TestService::AsyncEcho(
-    MethodCall* method_call,
-    dbus::ExportedObject::ResponseSender response_sender) {
-  // Schedule a call to Echo() to send an asynchronous response after we return.
-  message_loop()->PostDelayedTask(FROM_HERE,
-                                  base::Bind(&TestService::Echo,
-                                             base::Unretained(this),
-                                             method_call,
-                                             response_sender),
-                                  TestTimeouts::tiny_timeout_ms());
+  return Echo(method_call);
 }
 
-void TestService::BrokenMethod(
-    MethodCall* method_call,
-    dbus::ExportedObject::ResponseSender response_sender) {
-  response_sender.Run(NULL);
+Response* TestService::BrokenMethod(MethodCall* method_call) {
+  return NULL;
 }
 
 }  // namespace dbus
diff --git a/dbus/test_service.h b/dbus/test_service.h
index ea3e5ad..153613e 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -10,7 +10,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/threading/thread.h"
 #include "base/synchronization/waitable_event.h"
-#include "dbus/exported_object.h"
 
 namespace base {
 class MessageLoopProxy;
@@ -19,6 +18,7 @@ class MessageLoopProxy;
 namespace dbus {
 
 class Bus;
+class ExportedObject;
 class MethodCall;
 class Response;
 
@@ -89,22 +89,14 @@ class TestService : public base::Thread {
   //
 
   // Echos the text message received from the method call.
-  void Echo(MethodCall* method_call,
-            dbus::ExportedObject::ResponseSender response_sender);
+  Response* Echo(MethodCall* method_call);
 
   // Echos the text message received from the method call, but sleeps for
   // TestTimeouts::tiny_timeout_ms() before returning the response.
-  void SlowEcho(MethodCall* method_call,
-                dbus::ExportedObject::ResponseSender response_sender);
-
-  // Echos the text message received from the method call, but sends its
-  // response asynchronously after this callback has returned.
-  void AsyncEcho(MethodCall* method_call,
-                 dbus::ExportedObject::ResponseSender response_sender);
+  Response* SlowEcho(MethodCall* method_call);
 
   // Returns NULL, instead of a valid Response.
-  void BrokenMethod(MethodCall* method_call,
-                    dbus::ExportedObject::ResponseSender response_sender);
+  Response* BrokenMethod(MethodCall* method_call);
 
   scoped_refptr<base::MessageLoopProxy> dbus_thread_message_loop_proxy_;
   base::WaitableEvent on_all_methods_exported_;