Add MessageLoop::AssertIdle(). Use in ChromeOS audio threads.

Add MessageLoop::AssertIdle(). Use in ChromeOS audio threads.

BUG=chrome-os:11110
TEST=existing

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

[willchan no longer on Chromium, 2011-01-25 01:24:37]

darin: base/message_loop.*
davej: chromeos changes

[darin (slow to review), 2011-01-25 17:07:43]

http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc#newcode291
base/message_loop.cc:291: DCHECK(incoming_queue_.empty());
what about the work_queue_?

[willchan no longer on Chromium, 2011-01-25 17:12:01]

http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc
File base/message_loop.cc (right):

http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc#newcode291
base/message_loop.cc:291: DCHECK(incoming_queue_.empty());
On 2011/01/25 17:07:43, darin wrote:
> what about the work_queue_?

See my comment in the .h file:
// Asserts that the MessageLoop is "idle". in reality, we only check
// |incoming_queue_|, since we don't want to lock |work_queue_|.

|work_queue_| is only ever touched on the thread the MessageLoop runs on, so
unless I want to add a bunch of locks, I think it's unsafe to access. I am
concerned that an unlocked .empty() operation on |work_queue_| may lead to
crashes, although I guess that's implementation specific.

[darin (slow to review), 2011-01-25 18:35:42]

On Tue, Jan 25, 2011 at 9:12 AM, <willchan@chromium.org> wrote:

>
> http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc
> File base/message_loop.cc (right):
>
>
> http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc#newcode291
> base/message_loop.cc:291: DCHECK(incoming_queue_.empty());
> On 2011/01/25 17:07:43, darin wrote:
>
>> what about the work_queue_?
>>
>
> See my comment in the .h file:
>
> // Asserts that the MessageLoop is "idle". in reality, we only check
> // |incoming_queue_|, since we don't want to lock |work_queue_|.
>
> |work_queue_| is only ever touched on the thread the MessageLoop runs
> on, so unless I want to add a bunch of locks, I think it's unsafe to
> access. I am concerned that an unlocked .empty() operation on
> |work_queue_| may lead to crashes, although I guess that's
> implementation specific.
>
>
Oh, I see.  I did not realize that you wanted a way to query the queue state
from another thread.

I don't think that comment is best placed in the header file as the inner
details of the MessageLoop implementation is not something a consumer
of the MessageLoop interface should have to know about.

However, the fact that work_queue_ is not tested means that this function
does not really do a very good job of testing for idleness.  I guess it is
still
a useful assertion though.

I'd probably move the comment about only testing the incoming_work_queue_
to the implementation.  Consumers are just asking to assert idleness, and
the implementation is doing the best job it can without sacrificing perf.

-Darin


>
> http://codereview.chromium.org/6374010/
>

[darin (slow to review), 2011-01-25 18:36:43]

On Tue, Jan 25, 2011 at 10:35 AM, Darin Fisher <darin@chromium.org> wrote:

> On Tue, Jan 25, 2011 at 9:12 AM, <willchan@chromium.org> wrote:
>
>>
>> http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc
>> File base/message_loop.cc (right):
>>
>>
>> http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc#newcode291
>> base/message_loop.cc:291: DCHECK(incoming_queue_.empty());
>> On 2011/01/25 17:07:43, darin wrote:
>>
>>> what about the work_queue_?
>>>
>>
>> See my comment in the .h file:
>>
>> // Asserts that the MessageLoop is "idle". in reality, we only check
>> // |incoming_queue_|, since we don't want to lock |work_queue_|.
>>
>> |work_queue_| is only ever touched on the thread the MessageLoop runs
>> on, so unless I want to add a bunch of locks, I think it's unsafe to
>> access. I am concerned that an unlocked .empty() operation on
>> |work_queue_| may lead to crashes, although I guess that's
>> implementation specific.
>>
>>
> Oh, I see.  I did not realize that you wanted a way to query the queue
> state
> from another thread.
>

^^^ In fact, I can see that this function only makes sense to call across
threads
since it should always fail (even though it doesn't) if you call it on your
thread's
message loop :)



>
> I don't think that comment is best placed in the header file as the inner
> details of the MessageLoop implementation is not something a consumer
> of the MessageLoop interface should have to know about.
>
> However, the fact that work_queue_ is not tested means that this function
> does not really do a very good job of testing for idleness.  I guess it is
> still
> a useful assertion though.
>
> I'd probably move the comment about only testing the incoming_work_queue_
> to the implementation.  Consumers are just asking to assert idleness, and
> the implementation is doing the best job it can without sacrificing perf.
>
> -Darin
>
>
>>
>> http://codereview.chromium.org/6374010/
>>
>
>

[willchan no longer on Chromium, 2011-01-25 18:46:13]

On 2011/01/25 18:36:43, darin wrote:
> On Tue, Jan 25, 2011 at 10:35 AM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> 
> > On Tue, Jan 25, 2011 at 9:12 AM, <mailto:willchan@chromium.org> wrote:
> >
> >>
> >> http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc
> >> File base/message_loop.cc (right):
> >>
> >>
> >>
http://codereview.chromium.org/6374010/diff/1/base/message_loop.cc#newcode291
> >> base/message_loop.cc:291: DCHECK(incoming_queue_.empty());
> >> On 2011/01/25 17:07:43, darin wrote:
> >>
> >>> what about the work_queue_?
> >>>
> >>
> >> See my comment in the .h file:
> >>
> >> // Asserts that the MessageLoop is "idle". in reality, we only check
> >> // |incoming_queue_|, since we don't want to lock |work_queue_|.
> >>
> >> |work_queue_| is only ever touched on the thread the MessageLoop runs
> >> on, so unless I want to add a bunch of locks, I think it's unsafe to
> >> access. I am concerned that an unlocked .empty() operation on
> >> |work_queue_| may lead to crashes, although I guess that's
> >> implementation specific.
> >>
> >>
> > Oh, I see.  I did not realize that you wanted a way to query the queue
> > state
> > from another thread.
> >
> 
> ^^^ In fact, I can see that this function only makes sense to call across
> threads
> since it should always fail (even though it doesn't) if you call it on your
> thread's
> message loop :)

Hehe, yeah :)

> 
> 
> 
> >
> > I don't think that comment is best placed in the header file as the inner
> > details of the MessageLoop implementation is not something a consumer
> > of the MessageLoop interface should have to know about.
> >
> > However, the fact that work_queue_ is not tested means that this function
> > does not really do a very good job of testing for idleness.  I guess it is
> > still
> > a useful assertion though.
> >
> > I'd probably move the comment about only testing the incoming_work_queue_
> > to the implementation.  Consumers are just asking to assert idleness, and
> > the implementation is doing the best job it can without sacrificing perf.
> >

OK, I've moved it to the implementation.

> > -Darin
> >
> >
> >>
> >> http://codereview.chromium.org/6374010/
> >>
> >
> >

[darin (slow to review), 2011-01-25 18:47:45]

LGTM for message_loop.*

[davejcool, 2011-01-25 21:12:12]

LGTM for audio_mixer_ changes too.