Make CallWrappers multiuse. No longer make CallWrapper->Run() self deleting. Since the SimpleThread API backs to joinable threads, the common use case makes it simple for the caller to manage the CallWrapper memory, and this makes everything much clearer. This also allows a CallWrapper's Run() to be called multiple times. This also moves CallWrapper into the base namespace, which I'm told is all the rage these days.

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

5 files changed, 90 insertions, 51 deletions

diff --git a/base/call_wrapper.h b/base/call_wrapper.h
index c968f76..ae9738d0 100644
--- a/base/call_wrapper.h
+++ b/base/call_wrapper.h
@@ -30,42 +30,47 @@
 #ifndef BASE_CALL_WRAPPER_H_
 #define BASE_CALL_WRAPPER_H_
 
-// WARNING: This is currently in competition with Task related callbacks, see
-//          task.h, you should maybe be using those interfaces instead.
+// NOTE: If you want a callback that takes at-call-time parameters, you should
+// use Callback (see task.h).  CallWrapper only supports creation-time binding.
 //
 // A function / method call invocation wrapper.  This creates a "closure" of
 // sorts, storing a function pointer (or method pointer), along with a possible
 // list of arguments.  The objects have a single method Run(), which will call
-// the function / method with the arguments unpacked.  Memory is automatically
-// managed, a Wrapper is only good for 1 Run(), and will delete itself after.
+// the function / method with the arguments unpacked.  The arguments to the
+// wrapped call are supplied at CallWrapper creation time, and no arguments can
+// be supplied to the Run() method.
 //
 // All wrappers should be constructed through the two factory methods:
 //   CallWrapper* NewFunctionCallWrapper(funcptr, ...);
 //   CallWrapper* NewMethodCallWrapper(obj, &Klass::Method, ...);
 //
-// The arguments are copied by value, and kept within the CallWrapper object.
-// The parameters should be simple types, or pointers to more complex types.
-// The copied parameters will be automatically destroyed, but any pointers,
-// or other objects that need to be managed should be managed by the callback.
-// If your Run method does custom cleanup, and Run is never called, this could
-// of course leak that manually managed memory.
+// The classes memory should be managed like any other dynamically allocated
+// object, and deleted when no longer in use.  It is of course wrong to destroy
+// the object while a Run() is in progress, or to call Run() after the object
+// has been destroyed.  Arguments are copied by value, and kept within the
+// CallWrapper object.  You should only pass-by-value simple parameters, or
+// pointers to more complex types.  If your parameters need custom memory
+// management, then you should probably do this at the same point you destroy
+// the CallWrapper.  You should be aware of how your CallWrapper is used, and
+// it is valid for Run() to be called 0 or more times.
 //
 // Some example usage:
 //   CallWrapper* wrapper = NewFunctionCallWrapper(&my_func);
-//   wrapper->Run();  // my_func(); delete wrapper;
-//   // wrapper is no longer valid.
+//   wrapper->Run();  // my_func();
+//   delete wrapper;
 //
-//   CallWrapper* wrapper = NewFunctionCallWrapper(&my_func, 10);
-//   wrapper->Run();  // my_func(10); delete wrapper;
-//   // wrapper is no longer valid.
+//   scoped_ptr<CallWrapper> wrapper(NewFunctionCallWrapper(&my_func, 10));
+//   wrapper->Run();  // my_func(10);
 //
 //   MyObject obj;
-//   CallWrapper* wrapper = NewMethodCallWrapper(&obj, &MyObject::Foo, 1, 2);
-//   wrapper->Run();  // obj->Foo(1, 2); delete wrapper;
-//   // wrapper is no longer valid.
+//   scoped_ptr<CallWrapper> wrapper(
+//      NewMethodCallWrapper(&obj, &MyObject::Foo, 1, 2));
+//   wrapper->Run();  // obj->Foo(1, 2);
 
 #include "base/tuple.h"
 
+namespace base {
+
 class CallWrapper {
  public:
   virtual ~CallWrapper() { }
@@ -82,7 +87,6 @@ class FunctionCallWrapper : public CallWrapper {
       : func_(func), params_(params) { }
   virtual void Run() {
     DispatchToFunction(func_, params_);
-    delete this;
   }
  private:
   FuncType func_;
@@ -98,7 +102,6 @@ class MethodCallWrapper : public CallWrapper {
       : obj_(obj), meth_(meth), params_(params) { }
   virtual void Run() {
     DispatchToMethod(obj_, meth_, params_);
-    delete this;
   }
  private:
   ObjType* obj_;
@@ -231,4 +234,6 @@ inline CallWrapper* NewMethodCallWrapper(ObjType* obj, MethType meth,
       obj, meth, MakeTuple(arg0, arg1, arg2, arg3, arg4));
 }
 
+}  // namespace base
+
 #endif  // BASE_CALL_WRAPPER_H_
diff --git a/base/call_wrapper_unittest.cc b/base/call_wrapper_unittest.cc
index 45a156a..b1e2518 100644
--- a/base/call_wrapper_unittest.cc
+++ b/base/call_wrapper_unittest.cc
@@ -28,6 +28,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include "base/call_wrapper.h"
+#include "base/scoped_ptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -59,34 +60,44 @@ TEST(CallWrapperTest, FunctionCall) {
   // Function call with 0 arguments.
   {
     EXPECT_EQ(0, global_int);
-    CallWrapper* wrapper = NewFunctionCallWrapper(SetGlobalInt5);
+    scoped_ptr<base::CallWrapper> wrapper(
+        base::NewFunctionCallWrapper(&SetGlobalInt5));
 
     EXPECT_EQ(0, global_int);
     wrapper->Run();
     EXPECT_EQ(5, global_int);
+
+    global_int = 0;
+    wrapper->Run();
+    EXPECT_EQ(5, global_int);
   }
   // Function call with 1 argument.
   {
     EXPECT_EQ(5, global_int);
-    CallWrapper* wrapper = NewFunctionCallWrapper(SetGlobalInt, 0);
+    scoped_ptr<base::CallWrapper> wrapper(
+        base::NewFunctionCallWrapper(&SetGlobalInt, 0));
 
     EXPECT_EQ(5, global_int);
     wrapper->Run();
     EXPECT_EQ(0, global_int);
+
+    global_int = 5;
+    wrapper->Run();
+    EXPECT_EQ(0, global_int);
   }
   // Function call with 2 arguments.
   {
     int stack_int = 4;
-    CallWrapper* wrapper;
+    scoped_ptr<base::CallWrapper> wrapper;
 
-    wrapper = NewFunctionCallWrapper(SetInt, &global_int, 8);
+    wrapper.reset(base::NewFunctionCallWrapper(&SetInt, &global_int, 8));
     EXPECT_EQ(4, stack_int);
     EXPECT_EQ(0, global_int);
     wrapper->Run();
     EXPECT_EQ(4, stack_int);
     EXPECT_EQ(8, global_int);
 
-    wrapper = NewFunctionCallWrapper(SetInt, &stack_int, 8);
+    wrapper.reset(base::NewFunctionCallWrapper(&SetInt, &stack_int, 8));
     EXPECT_EQ(4, stack_int);
     EXPECT_EQ(8, global_int);
     wrapper->Run();
@@ -96,22 +107,29 @@ TEST(CallWrapperTest, FunctionCall) {
   // Function call with 3-5 arguments.
   {
     int stack_int = 12;
-    CallWrapper* wrapper;
+    scoped_ptr<base::CallWrapper> wrapper;
 
-    wrapper = NewFunctionCallWrapper(SetIntAdd2, &stack_int, 1, 6);
+    wrapper.reset(
+        base::NewFunctionCallWrapper(&SetIntAdd2, &stack_int, 1, 6));
     EXPECT_EQ(12, stack_int);
     wrapper->Run();
     EXPECT_EQ(7, stack_int);
 
-    wrapper = NewFunctionCallWrapper(SetIntAdd3, &stack_int, 1, 6, 2);
+    wrapper.reset(
+        base::NewFunctionCallWrapper(&SetIntAdd3, &stack_int, 1, 6, 2));
     EXPECT_EQ(7, stack_int);
     wrapper->Run();
     EXPECT_EQ(9, stack_int);
 
-    wrapper = NewFunctionCallWrapper(SetIntAdd4, &stack_int, 1, 6, 2, 3);
+    wrapper.reset(
+        base::NewFunctionCallWrapper(&SetIntAdd4, &stack_int, 1, 6, 2, 3));
     EXPECT_EQ(9, stack_int);
     wrapper->Run();
     EXPECT_EQ(12, stack_int);
+
+    global_int = 2;
+    wrapper->Run();
+    EXPECT_EQ(12, stack_int);
   }
 }
 
@@ -136,37 +154,51 @@ TEST(CallWrapperTest, MethodCall) {
   {
     int stack_int = 0;
     Incrementer incr(&stack_int);
-    CallWrapper* wrapper;
+    scoped_ptr<base::CallWrapper> wrapper;
 
-    wrapper = NewMethodCallWrapper(&incr, &Incrementer::Increment);
+    wrapper.reset(
+        base::NewMethodCallWrapper(&incr, &Incrementer::Increment));
     EXPECT_EQ(0, stack_int);
     wrapper->Run();
     EXPECT_EQ(1, stack_int);
 
-    wrapper = NewMethodCallWrapper(&incr, &Incrementer::IncrementBy, 10);
+    wrapper.reset(
+        base::NewMethodCallWrapper(&incr, &Incrementer::IncrementBy, 10));
     EXPECT_EQ(1, stack_int);
     wrapper->Run();
     EXPECT_EQ(11, stack_int);
+    wrapper->Run();
+    EXPECT_EQ(21, stack_int);
+    wrapper->Run();
+    EXPECT_EQ(31, stack_int);
   }
   // Method call with 2-5 arguments.
   {
     int stack_int = 0;
     Incrementer incr(&stack_int);
-    CallWrapper* wrapper;
+    scoped_ptr<base::CallWrapper> wrapper;
 
-    wrapper = NewMethodCallWrapper(&incr, &Incrementer::SetIntAdd2, 1, 5);
+    wrapper.reset(
+        base::NewMethodCallWrapper(&incr, &Incrementer::SetIntAdd2, 1, 5));
     EXPECT_EQ(0, stack_int);
     wrapper->Run();
     EXPECT_EQ(6, stack_int);
 
-    wrapper = NewMethodCallWrapper(&incr, &Incrementer::SetIntAdd3, 1, 5, 7);
+    wrapper.reset(
+        base::NewMethodCallWrapper(&incr, &Incrementer::SetIntAdd3, 1, 5, 7));
     EXPECT_EQ(6, stack_int);
     wrapper->Run();
     EXPECT_EQ(13, stack_int);
 
-    wrapper = NewMethodCallWrapper(&incr, &Incrementer::SetIntAdd4, 1, 5, 7, 2);
+    wrapper.reset(
+        base::NewMethodCallWrapper(&incr,
+                                   &Incrementer::SetIntAdd4, 1, 5, 7, 2));
     EXPECT_EQ(13, stack_int);
     wrapper->Run();
     EXPECT_EQ(15, stack_int);
+
+    stack_int = 2;
+    wrapper->Run();
+    EXPECT_EQ(15, stack_int);
   }
 }
diff --git a/base/simple_thread.cc b/base/simple_thread.cc
index 797843d..1d88dada 100644
--- a/base/simple_thread.cc
+++ b/base/simple_thread.cc
@@ -75,8 +75,4 @@ void CallWrapperSimpleThread::Run() {
   wrapper_ = NULL;
 }
 
-CallWrapperSimpleThread::~CallWrapperSimpleThread() {
-  DCHECK(!wrapper_) << "CallWrapper was never released.";
-}
-
 }  // namespace base
diff --git a/base/simple_thread.h b/base/simple_thread.h
index fe9a136..44b397d 100644
--- a/base/simple_thread.h
+++ b/base/simple_thread.h
@@ -43,14 +43,15 @@
 // NOTE: You *MUST* call Join on the thread to clean up the underlying thread
 // resources.  You are also responsible for destructing the SimpleThread object.
 // It is invalid to destroy a SimpleThread while it is running, or without
-// Start() having been called (and a thread never created).
+// Start() having been called (and a thread never created).  The CallWrapper
+// object should live as long as a CallWrapperSimpleThread.
 //
 // Thread Safety: A SimpleThread is not completely thread safe.  It is safe to
 // access it from the creating thread or from the newly created thread.  This
 // implies that the creator thread should be the thread that calls Join.
 //
 // Example:
-//   CallWrapper* wrapper = NewMethodCallWrapper(obj, &Foo::Main);
+//   scoped_ptr<CallWrapper> wrapper(NewMethodCallWrapper(obj, &Foo::Main));
 //   scoped_ptr<SimpleThread> thread(new CallWrapperSimpleThread(wrapper));
 //   thread->Start();
 //   // Start will return after the Thread has been successfully started and
@@ -58,7 +59,7 @@
 //   // until it returns.  The CallWrapper will then delete itself.
 //   thread->Join();  // Wait until the thread has exited.  You MUST Join!
 //   // The SimpleThread object is still valid, however you may not call Join
-//   // or Start again.  In this example the scoper will destroy the object.
+//   // or Start again.  In this example the scopers will destroy the objects.
 
 #ifndef BASE_SIMPLE_THREAD_H_
 #define BASE_SIMPLE_THREAD_H_
@@ -69,10 +70,10 @@
 #include "base/waitable_event.h"
 #include "base/platform_thread.h"
 
-class CallWrapper;
-
 namespace base {
 
+class CallWrapper;
+
 // This is the base SimpleThread.  You can derive from it and implement the
 // virtual Run method, or you can use the CallWrapperSimpleThread interface.
 class SimpleThread : public PlatformThread::Delegate {
@@ -151,7 +152,7 @@ class CallWrapperSimpleThread : public SimpleThread {
                           const std::string& name_prefix)
       : SimpleThread(options, name_prefix), wrapper_(wrapper) { }
 
-  virtual ~CallWrapperSimpleThread();
+  virtual ~CallWrapperSimpleThread() { }
   virtual void Run();
  private:
   CallWrapper* wrapper_;
diff --git a/base/simple_thread_unittest.cc b/base/simple_thread_unittest.cc
index c894637..fcdabda 100644
--- a/base/simple_thread_unittest.cc
+++ b/base/simple_thread_unittest.cc
@@ -53,10 +53,11 @@ void SignalEvent(base::WaitableEvent* event) {
 TEST(SimpleThreadTest, CreateAndJoin) {
   int stack_int = 0;
 
-  CallWrapper* wrapper = NewFunctionCallWrapper(SetInt, &stack_int, 7);
+  scoped_ptr<base::CallWrapper> wrapper(
+      base::NewFunctionCallWrapper(SetInt, &stack_int, 7));
   EXPECT_EQ(0, stack_int);
   scoped_ptr<base::SimpleThread> thread(
-      new base::CallWrapperSimpleThread(wrapper));
+      new base::CallWrapperSimpleThread(wrapper.get()));
   EXPECT_FALSE(thread->HasBeenStarted());
   EXPECT_FALSE(thread->HasBeenJoined());
   EXPECT_EQ(0, stack_int);
@@ -75,8 +76,10 @@ TEST(SimpleThreadTest, WaitForEvent) {
   // Create a thread, and wait for it to signal us.
   base::WaitableEvent event(true, false);
 
-  scoped_ptr<base::SimpleThread> thread(new base::CallWrapperSimpleThread(
-      NewFunctionCallWrapper(SignalEvent, &event)));
+  scoped_ptr<base::CallWrapper> wrapper(
+      base::NewFunctionCallWrapper(SignalEvent, &event));
+  scoped_ptr<base::SimpleThread> thread(
+      new base::CallWrapperSimpleThread(wrapper.get()));
 
   EXPECT_FALSE(event.IsSignaled());
   thread->Start();
@@ -89,8 +92,10 @@ TEST(SimpleThreadTest, Named) {
   base::WaitableEvent event(true, false);
 
   base::SimpleThread::Options options;
+  scoped_ptr<base::CallWrapper> wrapper(
+      base::NewFunctionCallWrapper(SignalEvent, &event));
   scoped_ptr<base::SimpleThread> thread(new base::CallWrapperSimpleThread(
-      NewFunctionCallWrapper(SignalEvent, &event), options, "testy"));
+      wrapper.get(), options, "testy"));
   EXPECT_EQ(thread->name_prefix(), "testy");
   EXPECT_FALSE(event.IsSignaled());