Created a stock implementation of the MessageLoopProxy interface than can be ...

Created a stock implementation of the MessageLoopProxy interface than can be used to create an implementation that targets the current thread's message loop.
BUG=None
TEST=Unit tests provided.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46591

[Paweł Hajdan Jr., 2010-05-04 04:39:47]

Drive-by with a test comment.

http://codereview.chromium.org/1837003/diff/11001/12004
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/1837003/diff/11001/12004#newcode23
base/message_loop_proxy_impl_unittest.cc:23:
CHECK(io_thread_->message_loop_proxy()->BelongsToCurrentThread());
Could you replace the CHECKs with ASSERT_TRUEs? We don't want to crash entire
base_unittests because of a test failure. Please also change the method names
(Check -> Assert probably).

[sanjeevr, 2010-05-04 17:23:28]

http://codereview.chromium.org/1837003/diff/11001/12004
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/1837003/diff/11001/12004#newcode23
base/message_loop_proxy_impl_unittest.cc:23:
CHECK(io_thread_->message_loop_proxy()->BelongsToCurrentThread());
On 2010/05/04 04:39:52, Paweł Hajdan Jr. wrote:
> Could you replace the CHECKs with ASSERT_TRUEs? We don't want to crash entire
> base_unittests because of a test failure. Please also change the method names
> (Check -> Assert probably).

Done.

[Paweł Hajdan Jr., 2010-05-04 17:25:34]

http://codereview.chromium.org/1837003/diff/17001/18004
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/1837003/diff/17001/18004#newcode83
base/message_loop_proxy_impl_unittest.cc:83: CHECK(false);
There is still one CHECK here. However, the NeverDeleted class seems to be
unused at all.

[sanjeevr, 2010-05-04 17:35:01]

This code is largely a copy-paste from chrome_thread_unittest.cc. There are
equivalent CHECKs there which we also need to convert to ASSERT_TRUEs. There is
one more CHECK left in this code but I think that should be there because it is
in the Run method of a task that never runs. That should crash, IMHO.

http://codereview.chromium.org/1837003/diff/17001/18004
File base/message_loop_proxy_impl_unittest.cc (right):

http://codereview.chromium.org/1837003/diff/17001/18004#newcode83
base/message_loop_proxy_impl_unittest.cc:83: CHECK(false);
On 2010/05/04 17:25:34, Paweł Hajdan Jr. wrote:
> There is still one CHECK here. However, the NeverDeleted class seems to be
> unused at all.

Done.

[Paweł Hajdan Jr., 2010-05-04 17:39:43]

Code I commented in the drive-by LGTM.

[darin (slow to review), 2010-05-05 06:33:10]

http://codereview.chromium.org/1837003/diff/24001/25003
File base/message_loop_proxy_impl.cc (right):

http://codereview.chromium.org/1837003/diff/24001/25003#newcode13
base/message_loop_proxy_impl.cc:13: class MessageLoopProxyImpl::Core
I think you can avoid the Core class if you instead define a Traits class for
the RefCountedThreadSafe that MessageLoopProxy extends from.  Define a protected
OnDestruct method that implementations of MessageLoopProxy can implement.

Then you can arrange for the destruction of MessageLoopProxyImpl to occur on the
thread corresponding to target_message_loop_.  You can use NewRunnableFunction
to call a static helper function with a MessageLoopProxyImpl* as its parameter.

WDYT?

http://codereview.chromium.org/1837003/diff/24001/25003#newcode49
base/message_loop_proxy_impl.cc:49: delete task;
we should probably avoid deleting the task while holding the message_loop_lock_.
 what if the task's dtor tried to call PostTask again?  this looks to be an
issue with ChromeThread as well!

http://codereview.chromium.org/1837003/diff/24001/25002
File base/message_loop_proxy_impl.h (right):

http://codereview.chromium.org/1837003/diff/24001/25002#newcode12
base/message_loop_proxy_impl.h:12: 
I should have mentioned this when you added MessageLoopProxy, but that class as
well as the ones defined here should really be declared in the "base" namespace.

http://codereview.chromium.org/1837003/diff/24001/25002#newcode35
base/message_loop_proxy_impl.h:35: static scoped_refptr<MessageLoopProxyImpl>
CreateForCurrentThread();
I think this method would be better on MessageLoopProxy.  That way consumers
won't need to include this implementation header.

http://codereview.chromium.org/1837003/diff/24001/25002#newcode47
base/message_loop_proxy_impl.h:47: class ThreadWithMessageLoopProxy : public
base::Thread {
I think we should just change Thread to vend a MessageLoopProxy instead of a
MessageLoop.

[sanjeevr, 2010-05-05 23:34:07]

Made all the changes. The CL has more files now because of all the places I had
to change MessageLoopProxy to base::MessageLoopProxy.

http://codereview.chromium.org/1837003/diff/24001/25003
File base/message_loop_proxy_impl.cc (right):

http://codereview.chromium.org/1837003/diff/24001/25003#newcode13
base/message_loop_proxy_impl.cc:13: class MessageLoopProxyImpl::Core
On 2010/05/05 06:33:10, darin wrote:
> I think you can avoid the Core class if you instead define a Traits class for
> the RefCountedThreadSafe that MessageLoopProxy extends from.  Define a
protected
> OnDestruct method that implementations of MessageLoopProxy can implement.
> 
> Then you can arrange for the destruction of MessageLoopProxyImpl to occur on
the
> thread corresponding to target_message_loop_.  You can use NewRunnableFunction
> to call a static helper function with a MessageLoopProxyImpl* as its
parameter.
> 
> WDYT?

Done.

http://codereview.chromium.org/1837003/diff/24001/25003#newcode49
base/message_loop_proxy_impl.cc:49: delete task;
On 2010/05/05 06:33:10, darin wrote:
> we should probably avoid deleting the task while holding the
message_loop_lock_.
>  what if the task's dtor tried to call PostTask again?  this looks to be an
> issue with ChromeThread as well!

Done.

http://codereview.chromium.org/1837003/diff/24001/25002
File base/message_loop_proxy_impl.h (right):

http://codereview.chromium.org/1837003/diff/24001/25002#newcode12
base/message_loop_proxy_impl.h:12: 
On 2010/05/05 06:33:10, darin wrote:
> I should have mentioned this when you added MessageLoopProxy, but that class
as
> well as the ones defined here should really be declared in the "base"
namespace.

Done.

http://codereview.chromium.org/1837003/diff/24001/25002#newcode35
base/message_loop_proxy_impl.h:35: static scoped_refptr<MessageLoopProxyImpl>
CreateForCurrentThread();
On 2010/05/05 06:33:10, darin wrote:
> I think this method would be better on MessageLoopProxy.  That way consumers
> won't need to include this implementation header.

Done.

http://codereview.chromium.org/1837003/diff/24001/25002#newcode47
base/message_loop_proxy_impl.h:47: class ThreadWithMessageLoopProxy : public
base::Thread {
On 2010/05/05 06:33:10, darin wrote:
> I think we should just change Thread to vend a MessageLoopProxy instead of a
> MessageLoop.

Done.

[darin (slow to review), 2010-05-06 18:29:20]

LGTM