Pepper: Introduce ThreadAwareCallback.

Pepper: Introduce ThreadAwareCallback.

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.

BUG=None
TEST=newly added unittests.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177319

[yzshen1, 2013-01-11 20:14:27]

Hi, David.

Would you please take a look? Thanks!

[yzshen, 2013-01-15 17:41:10]

Friendly ping. :)


On Fri, Jan 11, 2013 at 12:14 PM, <yzshen@chromium.org> wrote:

> Reviewers: dmichael,
>
> Message:
> Hi, David.
>
> Would you please take a look? Thanks!
>
> Description:
> Pepper: Introduce ThreadAwareCallback.
>
> 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.
>
> BUG=None
> TEST=newly added unittests.
>
>
> Please review this at
https://codereview.chromium.**org/11859015/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/component_**updater/test/component_**
> installers_unittest.cc
>   M ppapi/ppapi_shared.gypi
>   M ppapi/ppapi_tests.gypi
>   M ppapi/proxy/device_**enumeration_resource_helper.h
>   M ppapi/proxy/device_**enumeration_resource_helper.cc
>   M ppapi/proxy/plugin_globals.h
>   M ppapi/proxy/plugin_globals.cc
>   M ppapi/proxy/ppapi_proxy_test.h
>   M ppapi/proxy/ppapi_proxy_test.**cc
>   M ppapi/proxy/ppb_message_loop_**proxy.h
>   M ppapi/shared_impl/DEPS
>   M ppapi/shared_impl/ppapi_**globals.h
>   M ppapi/shared_impl/ppapi_**globals.cc
>   M ppapi/shared_impl/test_**globals.h
>   M ppapi/shared_impl/test_**globals.cc
>   A ppapi/shared_impl/thread_**aware_callback.h
>   A ppapi/shared_impl/thread_**aware_callback.cc
>   A ppapi/shared_impl/thread_**aware_callback_unittest.cc
>   M webkit/plugins/ppapi/host_**globals.h
>   M webkit/plugins/ppapi/host_**globals.cc
>
>
>


-- 
Best regards,
Yuzhu Shen.

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

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
File ppapi/proxy/ppapi_proxy_test.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.cc:323: // PP_OK_COMPLETIONPENDING. Either fix the
comment or the implementation.
I would lean towards PP_OK and just updating the doc. Thanks for catching that!

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
File ppapi/proxy/ppapi_proxy_test.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.h:91: explicit PluginProxyTestHarness(bool
per_thread_globals);
I almost always prefer a simple enum over bool for readability.

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.h:200: void CheckOnValidThread(bool main_thread);
Ditto... an enum would be more readable, e.g.:
AssertOnThread(MAIN)/AssertOnThread(BACKGROUND), or just use two functions

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.h:202: // They can be called on any thread.
nit s/They/These

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/ppapi_gl...
File ppapi/shared_impl/ppapi_globals.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/ppapi_gl...
ppapi/shared_impl/ppapi_globals.h:46: struct ForPerThreadTest {};
Since it's really the PpapiGlobals that's "Per-thread", not the test, how about:
PerThreadForTest

(That also leaves "ForTest" together, which makes it clearer, since a ForTest
suffix is meaningful in Chromium)

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:22: ProxyAutoLock auto_lock;
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.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:38: core_(new Core()) {
DCHECK(target_loop_);
?
There's no guarantee that the plugin attached a MessageLoopResource to this
thread.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: 2013 for new files

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:45: // - Unlike TrackedCallback, there
is not restriction of how many times the
nit: "not restriction of" -> "no restriction on"

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:53: class ThreadAwareCallback;
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...

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:56: class ThreadAwareCallback<void
(*)()>
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.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:70:
ppapi::CallWhileUnlocked<void>(func_);
You shouldn't need to pass a template parameter; it should be deduced.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback_unittest.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback_unittest.cc:56: class
ThreadAwareCallbackMultiThreadTest
A quick note for each test about what the test is checking for would be nice
(E.g. here, Test that a callback created on the main thread will run on the main
thread.)

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback_unittest.cc:126: // possible to be
called before this following statement. That is because
s/is possible to be/can

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

Thanks David!

Please take another look!

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
File ppapi/proxy/ppapi_proxy_test.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.cc:323: // PP_OK_COMPLETIONPENDING. Either fix the
comment or the implementation.
On 2013/01/15 22:43:52, dmichael wrote:
> I would lean towards PP_OK and just updating the doc. Thanks for catching
that!
That sounds good. I will make that a separate CL.

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
File ppapi/proxy/ppapi_proxy_test.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.h:91: explicit PluginProxyTestHarness(bool
per_thread_globals);
On 2013/01/15 22:43:52, dmichael wrote:
> I almost always prefer a simple enum over bool for readability.

Done.

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.h:200: void CheckOnValidThread(bool main_thread);
On 2013/01/15 22:43:52, dmichael wrote:
> Ditto... an enum would be more readable, e.g.:
> AssertOnThread(MAIN)/AssertOnThread(BACKGROUND), or just use two functions

Done.

https://codereview.chromium.org/11859015/diff/4002/ppapi/proxy/ppapi_proxy_te...
ppapi/proxy/ppapi_proxy_test.h:202: // They can be called on any thread.
On 2013/01/15 22:43:52, dmichael wrote:
> nit s/They/These

Done.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/ppapi_gl...
File ppapi/shared_impl/ppapi_globals.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/ppapi_gl...
ppapi/shared_impl/ppapi_globals.h:46: struct ForPerThreadTest {};
On 2013/01/15 22:43:52, dmichael wrote:
> Since it's really the PpapiGlobals that's "Per-thread", not the test, how
about:
> PerThreadForTest
> 
> (That also leaves "ForTest" together, which makes it clearer, since a ForTest
> suffix is meaningful in Chromium)

Done.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:22: ProxyAutoLock auto_lock;
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:
> 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.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:38: core_(new Core()) {
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:
> DCHECK(target_loop_);
> ?
> There's no guarantee that the plugin attached a MessageLoopResource to this
> thread.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/15 22:43:52, dmichael wrote:
> nit: 2013 for new files

Done. How time flies! :)

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:45: // - Unlike TrackedCallback, there
is not restriction of how many times the
On 2013/01/15 22:43:52, dmichael wrote:
> nit: "not restriction of" -> "no restriction on"

Done.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:53: class ThreadAwareCallback;
Done. I added one more bullet point above.

On 2013/01/15 22:43:52, dmichael wrote:
> 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...

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:56: class ThreadAwareCallback<void
(*)()>
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:
> 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.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:70:
ppapi::CallWhileUnlocked<void>(func_);
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:
> You shouldn't need to pass a template parameter; it should be deduced.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback_unittest.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback_unittest.cc:56: class
ThreadAwareCallbackMultiThreadTest
On 2013/01/15 22:43:52, dmichael wrote:
> A quick note for each test about what the test is checking for would be nice
> (E.g. here, Test that a callback created on the main thread will run on the
main
> thread.)

Done.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback_unittest.cc:126: // possible to be
called before this following statement. That is because
On 2013/01/15 22:43:52, dmichael wrote:
> s/is possible to be/can

Done.

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

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:22: ProxyAutoLock auto_lock;
On 2013/01/16 18:55:59, yzshen1 wrote:
> 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:
> > 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.
> 
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.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:38: core_(new Core()) {
On 2013/01/16 18:55:59, yzshen1 wrote:
> 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:
> > DCHECK(target_loop_);
> > ?
> > There's no guarantee that the plugin attached a MessageLoopResource to this
> > thread.
> 
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...

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:56: class ThreadAwareCallback<void
(*)()>
On 2013/01/16 18:55:59, yzshen1 wrote:
> 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.
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.

> (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?
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.

> 
> On 2013/01/15 22:43:52, dmichael wrote:
> > 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.
>

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:70:
ppapi::CallWhileUnlocked<void>(func_);
On 2013/01/16 18:55:59, yzshen1 wrote:
> 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:
> > You shouldn't need to pass a template parameter; it should be deduced.
> 
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)

https://codereview.chromium.org/11859015/diff/21001/ppapi/proxy/ppapi_proxy_t...
File ppapi/proxy/ppapi_proxy_test.h (right):

https://codereview.chromium.org/11859015/diff/21001/ppapi/proxy/ppapi_proxy_t...
ppapi/proxy/ppapi_proxy_test.h:209: void CheckOnValidThread(ThreadType
thread_type);
nit: I think it's clearer now without "Valid"

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

Thanks David!
Please take another look.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.cc (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:22: ProxyAutoLock auto_lock;
Done. Thanks for the feedback. :)

On 2013/01/16 19:21:00, dmichael wrote:
> On 2013/01/16 18:55:59, yzshen1 wrote:
> > 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:
> > > 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.
> > 
> 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.

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.cc:38: core_(new Core()) {
> 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.
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. :)


> 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.
Now that we don't have a "required" concept for those custom callbacks, we
probably don't have this problem here.

> 
> 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...

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
File ppapi/shared_impl/thread_aware_callback.h (right):

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:56: class ThreadAwareCallback<void
(*)()>
I have made changes in the process of adding a Create() method. Thanks!

On 2013/01/16 19:21:00, dmichael wrote:
> On 2013/01/16 18:55:59, yzshen1 wrote:
> > 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.
> 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.
> 
> > (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?
> 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.
> 
> > 
> > On 2013/01/15 22:43:52, dmichael wrote:
> > > 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.
> > 
>

https://codereview.chromium.org/11859015/diff/4002/ppapi/shared_impl/thread_a...
ppapi/shared_impl/thread_aware_callback.h:70:
ppapi::CallWhileUnlocked<void>(func_);
On 2013/01/16 19:21:00, dmichael wrote:
> On 2013/01/16 18:55:59, yzshen1 wrote:
> > 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:
> > > You shouldn't need to pass a template parameter; it should be deduced.
> > 
> Ah, I see. Presumably it fails for const-ref arguments?
I think so, haven't looked careful enough to find out if there is anything else.
> 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)

https://codereview.chromium.org/11859015/diff/21001/ppapi/proxy/ppapi_proxy_t...
File ppapi/proxy/ppapi_proxy_test.h (right):

https://codereview.chromium.org/11859015/diff/21001/ppapi/proxy/ppapi_proxy_t...
ppapi/proxy/ppapi_proxy_test.h:209: void CheckOnValidThread(ThreadType
thread_type);
On 2013/01/16 19:21:01, dmichael wrote:
> nit: I think it's clearer now without "Valid"

Done.

[dmichael (off chromium), 2013-01-16 22:53:48]

Thanks, looks great! There seems to be a unit test failure, but otherwise lgtm

[dmichael (off chromium), 2013-01-16 22:54:40]

Ah, I was looking at an old patch set when I noted the failing unit tests. You
probably already took care of it.

Anyway, still lgtm

[yzshen1, 2013-01-16 23:01:59]

Hi, Brett.

I need an OWNER stamp for:
chrome/browser/component_updater/test/component_installers_unittest.cc

Thanks! :)

[brettw, 2013-01-16 23:03:05]

random unit test lgtm

[commit-bot: I haz the power, 2013-01-17 00:40:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11859015/37001

[commit-bot: I haz the power, 2013-01-17 02:38:26]

Change committed as 177319