Add CompletionTask to PPAPI TrackedCallback.

ppapi/shared_impl/tracked_callback.cc

 // 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 "ppapi/shared_impl/tracked_callback.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 110 matching lines @@
     // Wait()s.
     operation_completed_condvar_->Signal();
   } else {
     // If there's a target_loop_, and we're not on the right thread, we need to
     // post to target_loop_.
     if (target_loop_.get() &&
         target_loop_.get() != PpapiGlobals::Get()->GetCurrentMessageLoop()) {
       PostRun(result);
       return;
     }
+
+    result = RunCompletionTask(result);

[yzshen1, 2013/08/07 21:09:10]

One thing that may be tricky is that this completion task may do things such as
running the tracked callback again. I think we should handle that case
carefully.

For example, if the completion task run the tracked callback, we will probably
end up running the PP_CompletionCallback twice. (Because at this point we don't
test whether the callback is in completed state anymore.)

[dmichael (off chromium), 2013/08/08 17:29:58]

I think the answer is "don't do that". The completion task needs to do fairly
simple things like write output parameters. IMHO, a comment would suffice.

That said, we should probably move this RunCompletionTask until after
MarkAsCompleted, so that it's consistent with the blocking case. That would make
doing "Run()" a no-op, if a CompletionTask tries to do Run() and a DCHECK if it
does PostRun.

[bbudge, 2013/08/08 18:34:00]

I moved the task invocation to after MarkAsCompleted. We first have to copy the
task to a local given the comment below. However I'm not sure it's safe to
access object state in BlockUntilComplete after receiving the signal to wake up.

+
     // Copy |callback_| now, since |MarkAsCompleted()| may delete us.
     PP_CompletionCallback callback = callback_;
     // Do this before running the callback in case of reentrancy (which
     // shouldn't happen, but avoid strange failures).
     MarkAsCompleted();
     // TODO(dmichael): Associate a message loop with the callback; if it's not
     // the same as the current thread's loop, then post it to the right loop.
     CallWhileUnlocked(PP_RunCompletionCallback, &callback, result);
   }
 }
@@ skipping 16 matching lines @@
     // classes protect against having a null target_loop_ otherwise).
     DCHECK(IsMainThread());
     DCHECK(PpapiGlobals::Get()->IsHostGlobals());
     base::MessageLoop::current()->PostTask(FROM_HERE, callback_closure);
   } else {
     target_loop_->PostClosure(FROM_HERE, callback_closure, 0);
   }
   is_scheduled_ = true;
 }
 
+void TrackedCallback::SetCompletionTask(const CompletionTask& completion_task) {
+  DCHECK(completion_task_.is_null());
+  completion_task_ = completion_task;
+}
+
 // static
 bool TrackedCallback::IsPending(
     const scoped_refptr<TrackedCallback>& callback) {
   if (!callback.get())
     return false;
   if (callback->aborted())
     return false;
   return !callback->completed();
 }
 
@@ skipping 11 matching lines @@
   // BlockUntilComplete should never be called for in-process plugins, where
   // blocking callbacks are not supported.
   CHECK(operation_completed_condvar_.get());
   if (!is_blocking() || !operation_completed_condvar_.get()) {
     NOTREACHED();
     return PP_ERROR_FAILED;
   }
 
   while (!completed())
     operation_completed_condvar_->Wait();
-  return result_for_blocked_callback_;
+
+  return RunCompletionTask(result_for_blocked_callback_);
 }
 
 void TrackedCallback::MarkAsCompleted() {
   DCHECK(!completed());
 
   // We will be removed; maintain a reference to ensure we won't be deleted
   // until we're done.
   scoped_refptr<TrackedCallback> thiz = this;
   completed_ = true;
   // We may not have a valid resource, in which case we're not in the tracker.
   if (resource_id_)
     tracker_->Remove(thiz);
   tracker_ = NULL;
 }
 
+int32_t TrackedCallback::RunCompletionTask(int32_t result) {
+  if (!completion_task_.is_null())
+    result = completion_task_.Run(result);
+  return result;

[teravest, 2013/08/08 16:49:52]

nit: I think you want to call completion_task_.Reset() here.

While it's strange, there's nothing stopping someone from calling
BlockUntilComplete() multiple times for a given TrackedCallback, but I think you
only want to run the completion_task_ once.

[bbudge, 2013/08/08 18:34:00]

Yes, good catch. Another danger is if the task bundles up a ref to this, we have
a cycle and the callback will never be deleted. Done.

+}
+
 }  // namespace ppapi