Pepper: Introduce ThreadAwareCallback.

ppapi/shared_impl/thread_aware_callback.cc

Index: ppapi/shared_impl/thread_aware_callback.cc
diff --git a/ppapi/shared_impl/thread_aware_callback.cc b/ppapi/shared_impl/thread_aware_callback.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b06fb8c8b22243d7e32ae845068b0b13755c2d4b
--- /dev/null
+++ b/ppapi/shared_impl/thread_aware_callback.cc
@@ -0,0 +1,58 @@
+// 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/thread_aware_callback.h"
+
+#include "base/callback.h"
+#include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/ppb_message_loop_shared.h"
+
+namespace ppapi {
+namespace internal {
+
+class ThreadAwareCallbackBase::Core : public base::RefCountedThreadSafe<Core> {
+ public:
+  Core() : aborted_(false) {
+  }
+
+  void MarkAsAborted() { aborted_ = true; }
+
+  void RunIfNotAborted(const base::Closure& closure) {
+    ProxyAutoLock auto_lock;

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

You could use "RunWhileLocked" where you Post this instead; that would be more
consistent. I think it gets confusing if some methods explicitly lock and some
don't. The others (e.g. MarkAsAborted) have the lock already when they are
entered.

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

RunWhileLocked() adds another level of 'Bind', which is overhead.
I thought this was internal implementation of ThreadAwareCallbackBase, and
should be clear enough.

I added a comment. Please let me know if you still think that I should make the
change.

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

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

I really don't think the Bind is a big deal; it's the way we do this everywhere
else. If we find via profiling that it takes too much time, then we can look at
taking it back out. In all likelihood, acquiring the lock is way more
significant anyway.

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

Done. Thanks for the feedback. :)

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

+
+    if (!aborted_)
+      CallWhileUnlocked(closure);
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<Core>;
+  ~Core() {
+  }
+
+  bool aborted_;
+};
+
+ThreadAwareCallbackBase::ThreadAwareCallbackBase()
+    : target_loop_(PpapiGlobals::Get()->GetCurrentMessageLoop()),
+      core_(new Core()) {

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

DCHECK(target_loop_);
?
There's no guarantee that the plugin attached a MessageLoopResource to this
thread.

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

A DCHECK() won't help plugin developers who develop with a release Chrome or the
end users, right?

If they don't attach a message loop resource to their non-main threads, this
callback will be run on the main thread. It seems (somewhat) reasonable to me.

I noticed that in TrackedCallback, we also have similar problem:
"// On a non-main thread, there must be a valid target loop for non-
 // blocking callbacks, or we will have no place to run them."
"If the callback is required, there's no nice way to tell the plugin."

Do you want me to log an error and never run the callback?

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

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

Ah. I was assuming that the API that uses this would check the thread and return
error when the function is called if there's no attached thread. I think of this
DCHECK as a way to protect people updating APIs to make sure they check the
thread. Maybe we should route everything through a factory function (e.g.
CreateThreadAwareCallback) that can do the checking in one place.

Where possible, TrackedCallback returns PP_ERROR_NOMESSAGELOOP. The exception is
"required" callbacks, which we've always claimed only ever return
PP_OK_COMPLETIONPENDING, so there's not a good way to return error there.

Here, it seems like you're free to make MonitorDeviceChange return
PP_ERROR_NOMESSAGELOOP
https://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/c/pp_completion_cal...

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

Yeah, I like this idea. I have made changes accordingly.
And because that would add significant amount of code to my current way of
implementation. I changed to the way you suggested. :)
Now that we don't have a "required" concept for those custom callbacks, we
probably don't have this problem here.

+}
+
+ThreadAwareCallbackBase::~ThreadAwareCallbackBase() {
+  core_->MarkAsAborted();
+}
+
+bool ThreadAwareCallbackBase::ShouldPostToTargetLoop() {
+  return target_loop_.get() &&
+         target_loop_ != PpapiGlobals::Get()->GetCurrentMessageLoop();
+}
+
+void ThreadAwareCallbackBase::RunIfNotAborted(
+    const base::Closure& closure) {
+  target_loop_->PostClosure(FROM_HERE,
+                            base::Bind(&Core::RunIfNotAborted, core_, closure),
+                            0);
+}
+
+}  // namespace internal
+}  // namespace ppapi