Created a MessageLoopProxy interface. This provides a thread-safe refcounted ...

Created a MessageLoopProxy interface. This provides a thread-safe refcounted interface to the Post* methods
 of a message loop. This class can outlive the target message loop. Changed ChromeThread to vend an implementation of this proxy.
BUG=None
TEST=ChromeThread unit-tests modified.


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

[darin (slow to review), 2010-04-28 22:05:49]

This looks good to me.

I am wondering what your implementation of MessageLoopProxy will look like when
you don't have ChromeThread backing it.  I also wondering if there isn't a way
to reduce layers (e.g., perhaps MessageLoop should just be reference counted!). 
<-- these wonderings don't need to slow you down right now.

http://codereview.chromium.org/1800008/diff/1/2
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/1800008/diff/1/2#newcode14
base/message_loop_proxy.h:14: // The return value from the Post* methods of this
class is false if the task
there's some redundancy between this comment block and the one below.  probably
good to prune that out.

http://codereview.chromium.org/1800008/diff/1/2#newcode20
base/message_loop_proxy.h:20: MessageLoopProxy() { }
nit: insert new line after ctor or just delete the ctor since it matches what
the compiler will auto-generate.

http://codereview.chromium.org/1800008/diff/1/2#newcode46
base/message_loop_proxy.h:46: }
nit: insert new line before private

[sanjeevr, 2010-04-28 22:30:49]

Made the changes and uploaded a new patch.

I was also wondering about the implementation that I would provide for this
interface in the service process this afternoon. I think I will have the service
process itself vend one of these for the IO thread in that process. The Post*
implementation would check if that thread still existed.

I will wait for John's comments also.

http://codereview.chromium.org/1800008/diff/1/2
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/1800008/diff/1/2#newcode14
base/message_loop_proxy.h:14: // The return value from the Post* methods of this
class is false if the task
On 2010/04/28 22:05:49, darin wrote:
> there's some redundancy between this comment block and the one below. 
probably
> good to prune that out.

Done.

http://codereview.chromium.org/1800008/diff/1/2#newcode20
base/message_loop_proxy.h:20: MessageLoopProxy() { }
On 2010/04/28 22:05:49, darin wrote:
> nit: insert new line after ctor or just delete the ctor since it matches what
> the compiler will auto-generate.

Done.

Inserted a newline. Not having a c'tor generates a compile error (not sure why,
looking into that).

http://codereview.chromium.org/1800008/diff/1/2#newcode46
base/message_loop_proxy.h:46: }
On 2010/04/28 22:05:49, darin wrote:
> nit: insert new line before private

Done.

[jam, 2010-04-28 22:42:46]

http://codereview.chromium.org/1800008/diff/8001/3002
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/1800008/diff/8001/3002#newcode16
base/message_loop_proxy.h:16: MessageLoopProxy() { }
nit: since it's empty no need to have for now

http://codereview.chromium.org/1800008/diff/8001/3004
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/1800008/diff/8001/3004#newcode22
chrome/browser/chrome_thread.cc:22: class ChromeThread::MessageLoopProxyImpl :
public MessageLoopProxy {
nit: just curious, why is this class part of ChromeThread?  Since it doesn't use
any private members or methods, it doesn't need to right?

http://codereview.chromium.org/1800008/diff/8001/3004#newcode28
chrome/browser/chrome_thread.cc:28: Task* task);
nit: if you're defining this class in the .cc file, then we usually put the
implementation inline as well.

http://codereview.chromium.org/1800008/diff/8001/3005
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/1800008/diff/8001/3005#newcode132
chrome/browser/chrome_thread.h:132: static scoped_refptr<MessageLoopProxy>
GetMessageLoopProxyForThread(
nit: we normally return refcounted objects as naked pointers

http://codereview.chromium.org/1800008/diff/8001/3006
File chrome/browser/chrome_thread_unittest.cc (right):

http://codereview.chromium.org/1800008/diff/8001/3006#newcode134
chrome/browser/chrome_thread_unittest.cc:134: }
a test that exercise the primary reason for this class would be nice as well
(i.e. posting a task after the ML is gone)

[darin (slow to review), 2010-04-28 23:23:05]

Another choice would be to build an implementation on top of MessageLoop
that uses MessageLoop's DestructionObserver mechanism.  Then
MessageLoopProxy would not need to have virtual methods, and we could
provide a constructor on MessageLoopProxy that takes a MessageLoop pointer.
 This would allow it to be used more generically throughout Chrome.  On the
downside, it would not integrate so nicely with ChromeThread, and maybe that
is a deal breaker.  I haven't thought about this enough.

-Darin

On Wed, Apr 28, 2010 at 3:30 PM, <sanjeevr@chromium.org> wrote:

> Made the changes and uploaded a new patch.
>
> I was also wondering about the implementation that I would provide for this
> interface in the service process this afternoon. I think I will have the
> service
> process itself vend one of these for the IO thread in that process. The
> Post*
> implementation would check if that thread still existed.
>
> I will wait for John's comments also.
>
>
>
> http://codereview.chromium.org/1800008/diff/1/2
> File base/message_loop_proxy.h (right):
>
> http://codereview.chromium.org/1800008/diff/1/2#newcode14
> base/message_loop_proxy.h:14: // The return value from the Post* methods
> of this class is false if the task
> On 2010/04/28 22:05:49, darin wrote:
>
>> there's some redundancy between this comment block and the one below.
>>
> probably
>
>> good to prune that out.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1800008/diff/1/2#newcode20
> base/message_loop_proxy.h:20: MessageLoopProxy() { }
> On 2010/04/28 22:05:49, darin wrote:
>
>> nit: insert new line after ctor or just delete the ctor since it
>>
> matches what
>
>> the compiler will auto-generate.
>>
>
> Done.
>
> Inserted a newline. Not having a c'tor generates a compile error (not
> sure why, looking into that).
>
>
> http://codereview.chromium.org/1800008/diff/1/2#newcode46
> base/message_loop_proxy.h:46: }
> On 2010/04/28 22:05:49, darin wrote:
>
>> nit: insert new line before private
>>
>
> Done.
>
>
> http://codereview.chromium.org/1800008/show
>

[darin (slow to review), 2010-04-28 23:24:34]

Another choice would be to build an implementation on top of MessageLoop
that uses MessageLoop's DestructionObserver mechanism.  Then
MessageLoopProxy would not need to have virtual methods, and we could
provide a constructor on MessageLoopProxy that takes a MessageLoop pointer.
 This would allow it to be used more generically throughout Chrome.  On the
downside, it would not integrate so nicely with ChromeThread, and maybe that
is a deal breaker.  I haven't thought about this enough.

-Darin

On Wed, Apr 28, 2010 at 3:30 PM, <sanjeevr@chromium.org> wrote:

> Made the changes and uploaded a new patch.
>
> I was also wondering about the implementation that I would provide for this
> interface in the service process this afternoon. I think I will have the
> service
> process itself vend one of these for the IO thread in that process. The
> Post*
> implementation would check if that thread still existed.
>
> I will wait for John's comments also.
>
>
>
> http://codereview.chromium.org/1800008/diff/1/2
> File base/message_loop_proxy.h (right):
>
> http://codereview.chromium.org/1800008/diff/1/2#newcode14
> base/message_loop_proxy.h:14: // The return value from the Post* methods
> of this class is false if the task
> On 2010/04/28 22:05:49, darin wrote:
>
>> there's some redundancy between this comment block and the one below.
>>
> probably
>
>> good to prune that out.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1800008/diff/1/2#newcode20
> base/message_loop_proxy.h:20: MessageLoopProxy() { }
> On 2010/04/28 22:05:49, darin wrote:
>
>> nit: insert new line after ctor or just delete the ctor since it
>>
> matches what
>
>> the compiler will auto-generate.
>>
>
> Done.
>
> Inserted a newline. Not having a c'tor generates a compile error (not
> sure why, looking into that).
>
>
> http://codereview.chromium.org/1800008/diff/1/2#newcode46
> base/message_loop_proxy.h:46: }
> On 2010/04/28 22:05:49, darin wrote:
>
>> nit: insert new line before private
>>
>
> Done.
>
>
> http://codereview.chromium.org/1800008/show
>

[sanjeevr, 2010-04-29 00:29:04]

Made the changes as we discussed. Please take another look.

http://codereview.chromium.org/1800008/diff/8001/3002
File base/message_loop_proxy.h (right):

http://codereview.chromium.org/1800008/diff/8001/3002#newcode16
base/message_loop_proxy.h:16: MessageLoopProxy() { }
On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> nit: since it's empty no need to have for now

Done.

http://codereview.chromium.org/1800008/diff/8001/3004
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/1800008/diff/8001/3004#newcode22
chrome/browser/chrome_thread.cc:22: class ChromeThread::MessageLoopProxyImpl :
public MessageLoopProxy {
On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> nit: just curious, why is this class part of ChromeThread?  Since it doesn't
use
> any private members or methods, it doesn't need to right?

It is part of ChromeThread because it is ChromeThread's implementation of
MessageLoopProxy.

http://codereview.chromium.org/1800008/diff/8001/3004#newcode28
chrome/browser/chrome_thread.cc:28: Task* task);
On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> nit: if you're defining this class in the .cc file, then we usually put the
> implementation inline as well.

Done.

http://codereview.chromium.org/1800008/diff/8001/3005
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/1800008/diff/8001/3005#newcode132
chrome/browser/chrome_thread.h:132: static scoped_refptr<MessageLoopProxy>
GetMessageLoopProxyForThread(
On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> nit: we normally return refcounted objects as naked pointers

Left this as is as per our discussion.

http://codereview.chromium.org/1800008/diff/8001/3006
File chrome/browser/chrome_thread_unittest.cc (right):

http://codereview.chromium.org/1800008/diff/8001/3006#newcode134
chrome/browser/chrome_thread_unittest.cc:134: }
On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> a test that exercise the primary reason for this class would be nice as well
> (i.e. posting a task after the ML is gone)

Done.

[jam, 2010-04-29 00:53:31]

lgtm

[jam, 2010-04-29 01:11:06]

On Wed, Apr 28, 2010 at 5:29 PM, <sanjeevr@chromium.org> wrote:

> Made the changes as we discussed. Please take another look.
>
>
> http://codereview.chromium.org/1800008/diff/8001/3002
>
> File base/message_loop_proxy.h (right):
>
> http://codereview.chromium.org/1800008/diff/8001/3002#newcode16
> base/message_loop_proxy.h:16: MessageLoopProxy() { }
>
> On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
>
>> nit: since it's empty no need to have for now
>>
>
> Done.
>
> http://codereview.chromium.org/1800008/diff/8001/3004
> File chrome/browser/chrome_thread.cc (right):
>
> http://codereview.chromium.org/1800008/diff/8001/3004#newcode22
> chrome/browser/chrome_thread.cc:22: class
> ChromeThread::MessageLoopProxyImpl : public MessageLoopProxy {
>
> On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
>
>> nit: just curious, why is this class part of ChromeThread?  Since it
>>
> doesn't use
>
>> any private members or methods, it doesn't need to right?
>>
>
> It is part of ChromeThread because it is ChromeThread's implementation
> of MessageLoopProxy.


note: i still don't understand why this class is part of ChromeThread.  This
class both uses and is used by ChromeThread, so it seems they're peers
instead of dependent relationship which is why one would normally use a
nested class.


>
> http://codereview.chromium.org/1800008/diff/8001/3004#newcode28
> chrome/browser/chrome_thread.cc:28: Task* task);
>
> On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
>
>> nit: if you're defining this class in the .cc file, then we usually
>>
> put the
>
>> implementation inline as well.
>>
>
> Done.
>
> http://codereview.chromium.org/1800008/diff/8001/3005
> File chrome/browser/chrome_thread.h (right):
>
> http://codereview.chromium.org/1800008/diff/8001/3005#newcode132
> chrome/browser/chrome_thread.h:132: static
> scoped_refptr<MessageLoopProxy> GetMessageLoopProxyForThread(
> On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
>
>> nit: we normally return refcounted objects as naked pointers
>>
>
> Left this as is as per our discussion.
>
>
> http://codereview.chromium.org/1800008/diff/8001/3006
> File chrome/browser/chrome_thread_unittest.cc (right):
>
> http://codereview.chromium.org/1800008/diff/8001/3006#newcode134
> chrome/browser/chrome_thread_unittest.cc:134: }
> On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
>
>> a test that exercise the primary reason for this class would be nice
>>
> as well
>
>> (i.e. posting a task after the ML is gone)
>>
>
> Done.
>
> http://codereview.chromium.org/1800008/show
>

[sanjeevr, 2010-04-29 01:34:40]

Moved it out of the class and uploaded. Please take a look.

On 2010/04/29 01:11:06, John Abd-El-Malek wrote:
> On Wed, Apr 28, 2010 at 5:29 PM, <mailto:sanjeevr@chromium.org> wrote:
> 
> > Made the changes as we discussed. Please take another look.
> >
> >
> > http://codereview.chromium.org/1800008/diff/8001/3002
> >
> > File base/message_loop_proxy.h (right):
> >
> > http://codereview.chromium.org/1800008/diff/8001/3002#newcode16
> > base/message_loop_proxy.h:16: MessageLoopProxy() { }
> >
> > On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> >
> >> nit: since it's empty no need to have for now
> >>
> >
> > Done.
> >
> > http://codereview.chromium.org/1800008/diff/8001/3004
> > File chrome/browser/chrome_thread.cc (right):
> >
> > http://codereview.chromium.org/1800008/diff/8001/3004#newcode22
> > chrome/browser/chrome_thread.cc:22: class
> > ChromeThread::MessageLoopProxyImpl : public MessageLoopProxy {
> >
> > On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> >
> >> nit: just curious, why is this class part of ChromeThread?  Since it
> >>
> > doesn't use
> >
> >> any private members or methods, it doesn't need to right?
> >>
> >
> > It is part of ChromeThread because it is ChromeThread's implementation
> > of MessageLoopProxy.
> 
> 
> note: i still don't understand why this class is part of ChromeThread.  This
> class both uses and is used by ChromeThread, so it seems they're peers
> instead of dependent relationship which is why one would normally use a
> nested class.
> 
> 
> >
> > http://codereview.chromium.org/1800008/diff/8001/3004#newcode28
> > chrome/browser/chrome_thread.cc:28: Task* task);
> >
> > On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> >
> >> nit: if you're defining this class in the .cc file, then we usually
> >>
> > put the
> >
> >> implementation inline as well.
> >>
> >
> > Done.
> >
> > http://codereview.chromium.org/1800008/diff/8001/3005
> > File chrome/browser/chrome_thread.h (right):
> >
> > http://codereview.chromium.org/1800008/diff/8001/3005#newcode132
> > chrome/browser/chrome_thread.h:132: static
> > scoped_refptr<MessageLoopProxy> GetMessageLoopProxyForThread(
> > On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> >
> >> nit: we normally return refcounted objects as naked pointers
> >>
> >
> > Left this as is as per our discussion.
> >
> >
> > http://codereview.chromium.org/1800008/diff/8001/3006
> > File chrome/browser/chrome_thread_unittest.cc (right):
> >
> > http://codereview.chromium.org/1800008/diff/8001/3006#newcode134
> > chrome/browser/chrome_thread_unittest.cc:134: }
> > On 2010/04/28 22:42:46, John Abd-El-Malek wrote:
> >
> >> a test that exercise the primary reason for this class would be nice
> >>
> > as well
> >
> >> (i.e. posting a task after the ML is gone)
> >>
> >
> > Done.
> >
> > http://codereview.chromium.org/1800008/show
> >
>