Pepper: Introduce ThreadAwareCallback.

ppapi/shared_impl/thread_aware_callback.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[dmichael (off chromium), 2013/01/15 22:43:52]

nit: 2013 for new files

[yzshen1, 2013/01/16 18:55:59]

Done. How time flies! :)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PPAPI_SHARED_IMPL_THREAD_AWARE_CALLBACK_H_
+#define PPAPI_SHARED_IMPL_THREAD_AWARE_CALLBACK_H_
+
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "ppapi/shared_impl/ppapi_shared_export.h"
+#include "ppapi/shared_impl/proxy_lock.h"
+
+namespace ppapi {
+
+class MessageLoopShared;
+
+namespace internal {
+
+class PPAPI_SHARED_EXPORT ThreadAwareCallbackBase {
+ protected:
+  ThreadAwareCallbackBase();
+  ~ThreadAwareCallbackBase();
+
+  bool ShouldPostToTargetLoop();
+  void RunIfNotAborted(const base::Closure& closure);
+
+ private:
+  class Core;
+
+  scoped_refptr<MessageLoopShared> target_loop_;
+  scoped_refptr<Core> core_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThreadAwareCallbackBase);
+};
+
+}  // namespace internal
+
+// Some PPB interfaces have methods that set a custom callback. Usually, the
+// callback has to be called on the same thread as the one it was set on.
+// ThreadAwareCallback keeps track of the target thread, and posts a task to run
+// on it if requested from a different thread.
+//
+// Please note that:
+// - Unlike TrackedCallback, there is not restriction of how many times the

[dmichael (off chromium), 2013/01/15 22:43:52]

nit: "not restriction of" -> "no restriction on"

[yzshen1, 2013/01/16 18:55:59]

Done.

+//   callback will be called.
+// - When a ThreadAwareCallback object is destroyed, all pending tasks to run
+//   the callback will be ignored. It is designed this way so that when the
+//   resource is destroyed or the callback is cancelled by the plugin, we can
+//   simply delete the ThreadAwareCallback object to prevent touching the
+//   callback later.
+template <class FuncType>
+class ThreadAwareCallback;

[dmichael (off chromium), 2013/01/15 22:43:52]

It's worth noting that if called on the target thread, the callback runs
immediately (that might not always be what the caller wants, if they don't want
to be blocked while the callback runs. Not sure if we'll run in to that, but
good to make it obvious to somebody using this).

Not sure the best place to put that comment. You could add some unimplemented
declarations here, just so you have a place to put comments...

[yzshen1, 2013/01/16 18:55:59]

Done. I added one more bullet point above.

On 2013/01/15 22:43:52, dmichael wrote:

+
+template <>
+class ThreadAwareCallback<void (*)()>

[dmichael (off chromium), 2013/01/15 22:43:52]

Do we really need all the specializations?

What about taking advantage of base::Callback/Bind, to put it in one class
template and save code? Maybe something like:

template <class FunctionType>
class ThreadAwareCallback {
 public:
  explicit ThreadAwareCallback(FunctionType func)
      : func_(func) {}
  void RunOnTargetThread() {
    InternalRunOnTargetThread(callback_);
  }
  template <class P1>
  void RunOnTargetThread(const P1& p1) {
    InternalRunOnTargetThread(base::Bind(func_, p1));
  }
  template <class P1, class P2>
  void RunOnTargetThread(const P1& p1, const P2& p2) {
    InternalRunOnTargetThread(base::Bind(func_, p1, p2));
  }
  // etc; 1 overload per number of params you want to support.
  // Note the compiler won't instantiate them unless they're
  // used.
 private:
  void InternalRunOnTargetThread(const base::Closure& cb) {
    if (ShouldPostToTargetLoop())
      RunIfNotAborted(cb);
    else
      ppapi::CallWhileUnlocked(cb);
  }
  FunctionType func_;
};

Should work the same...  only difference I think is you'll get a different
compile error if you mess up.

[yzshen1, 2013/01/16 18:55:59]

First, thanks a lot for writing the detailed comments! :)

I considered this alternative, and decided to use the current way because it
saves one 'Bind' when called on the same thread.
(The compilation error when given wrong number of arguments would be slightly
more clear too, but there isn't a big difference.)

I don't think the code is significantly more complicated.
Do you think it is okay to leave it as it is?

On 2013/01/15 22:43:52, dmichael wrote:

[dmichael (off chromium), 2013/01/16 19:21:00]

Bind is pretty cheap; I would worry more about readability. I kind of think it
would be nice to have most of the code shared and only a 1 line function for
each number of arguments, rather than copying more stuff and having more
possible ways to mess up. I also suspect overloading is more widely known than
partial specializations. So I still prefer the 1 template class way. But it's
not that important to me.
I prefer the shorter way that has less copy/paste. But it's up to you. I do
think the compile error for passing the wrong number of parameters will be nicer
your way.

[yzshen1, 2013/01/16 22:37:36]

I have made changes in the process of adding a Create() method. Thanks!

On 2013/01/16 19:21:00, dmichael wrote:

+    : public internal::ThreadAwareCallbackBase {
+ public:
+  typedef void (*FuncType)();
+
+  explicit ThreadAwareCallback(FuncType func) : func_(func) {
+  }
+  ~ThreadAwareCallback() {
+  }
+
+  void RunOnTargetThread() {
+    if (ShouldPostToTargetLoop())
+      RunIfNotAborted(base::Bind(func_));
+    else
+      ppapi::CallWhileUnlocked<void>(func_);

[dmichael (off chromium), 2013/01/15 22:43:52]

You shouldn't need to pass a template parameter; it should be deduced.

[yzshen1, 2013/01/16 18:55:59]

For this one, it is probably okay. But for those with input parameters below,
sometimes those parameter types cannot be deduced. (I realized that when I wrote
the tests.) So I added the template parameters explicitly. This one was added so
that the code looks more consistent.

On 2013/01/15 22:43:52, dmichael wrote:

[dmichael (off chromium), 2013/01/16 19:21:00]

Ah, I see. Presumably it fails for const-ref arguments? That's fine, then. If
you prefer, you can fix CallWhileUnlocked to allow that; I've been meaning to
(just add more template parameters, so the parameters in the function type don't
have to be an exact match to the parameters that are passed)

[yzshen1, 2013/01/16 22:37:36]

I think so, haven't looked careful enough to find out if there is anything else.

+  }
+
+ private:
+  FuncType func_;
+};
+
+template <class P1>
+class ThreadAwareCallback<void (*)(P1)>
+    : public internal::ThreadAwareCallbackBase {
+ public:
+  typedef void (*FuncType)(P1);
+
+  explicit ThreadAwareCallback(FuncType func) : func_(func) {
+  }
+  ~ThreadAwareCallback() {
+  }
+
+  void RunOnTargetThread(const P1& p1) {
+    if (ShouldPostToTargetLoop())
+      RunIfNotAborted(base::Bind(func_, p1));
+    else
+      ppapi::CallWhileUnlocked<void, P1>(func_, p1);
+  }
+
+ private:
+  FuncType func_;
+};
+
+template <class P1, class P2>
+class ThreadAwareCallback<void (*)(P1, P2)>
+    : public internal::ThreadAwareCallbackBase {
+ public:
+  typedef void (*FuncType)(P1, P2);
+
+  explicit ThreadAwareCallback(FuncType func) : func_(func) {
+  }
+  ~ThreadAwareCallback() {
+  }
+
+  void RunOnTargetThread(const P1& p1, const P2& p2) {
+    if (ShouldPostToTargetLoop())
+      RunIfNotAborted(base::Bind(func_, p1, p2));
+    else
+      ppapi::CallWhileUnlocked<void, P1, P2>(func_, p1, p2);
+  }
+
+ private:
+  FuncType func_;
+};
+
+template <class P1, class P2, class P3>
+class ThreadAwareCallback<void (*)(P1, P2, P3)>
+    : public internal::ThreadAwareCallbackBase {
+ public:
+  typedef void (*FuncType)(P1, P2, P3);
+
+  explicit ThreadAwareCallback(FuncType func) : func_(func) {
+  }
+  ~ThreadAwareCallback() {
+  }
+
+  void RunOnTargetThread(const P1& p1, const P2& p2, const P3& p3) {
+    if (ShouldPostToTargetLoop())
+      RunIfNotAborted(base::Bind(func_, p1, p2, p3));
+    else
+      ppapi::CallWhileUnlocked<void, P1, P2, P3>(func_, p1, p2, p3);
+  }
+
+ private:
+  FuncType func_;
+};
+
+template <class P1, class P2, class P3, class P4>
+class ThreadAwareCallback<void (*)(P1, P2, P3, P4)>
+    : public internal::ThreadAwareCallbackBase {
+ public:
+  typedef void (*FuncType)(P1, P2, P3, P4);
+
+  explicit ThreadAwareCallback(FuncType func) : func_(func) {
+  }
+  ~ThreadAwareCallback() {
+  }
+
+  void RunOnTargetThread(const P1& p1,
+                         const P2& p2,
+                         const P3& p3,
+                         const P4& p4) {
+    if (ShouldPostToTargetLoop())
+      RunIfNotAborted(base::Bind(func_, p1, p2, p3, p4));
+    else
+      ppapi::CallWhileUnlocked<void, P1, P2, P3, P4>(func_, p1, p2, p3, p4);
+  }
+
+ private:
+  FuncType func_;
+};
+
+template <class P1, class P2, class P3, class P4, class P5>
+class ThreadAwareCallback<void (*)(P1, P2, P3, P4, P5)>
+    : public internal::ThreadAwareCallbackBase {
+ public:
+  typedef void (*FuncType)(P1, P2, P3, P4, P5);
+
+  explicit ThreadAwareCallback(FuncType func) : func_(func) {
+  }
+  ~ThreadAwareCallback() {
+  }
+
+  void RunOnTargetThread(const P1& p1,
+                         const P2& p2,
+                         const P3& p3,
+                         const P4& p4,
+                         const P5& p5) {
+    if (ShouldPostToTargetLoop()) {
+      RunIfNotAborted(base::Bind(func_, p1, p2, p3, p4, p5));
+    } else {
+      ppapi::CallWhileUnlocked<void, P1, P2, P3, P4, P5>(func_, p1, p2, p3, p4,
+                                                         p5);
+    }
+  }
+
+ private:
+  FuncType func_;
+};
+
+}  // namespace ppapi
+
+#endif  // PPAPI_SHARED_IMPL_THREAD_AWARE_CALLBACK_H_