Implements AudioMessageFilter as member in RenderThread

Ensures that AudioMessageFilter now lives on the main render thread.

In addition, removes usage of routing ID by utilizing control messages instead of routed messages.

BUG=none
TESTS=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92138

[henrika_dont_use, 2011-06-14 15:24:27]

This patch has been tested on Windows 7 for HTML5 audio/video and WebRTC but not
in combination with Pepper or WebAudio.

The AudioInputMessageFilter has not been implemented yet. Waiting for review
comments first.

[Chris Rogers, 2011-06-14 20:04:00]

This is easily fixed, but gcc on Mac OS X is giving a compiler warning (treated
as error) that AudioDevice::stream_id_ is initialized in a different order in
the constructor.  Same problem with AudioMessageFilter::message_loop_

On 2011/06/14 15:24:27, henrika wrote:
> This patch has been tested on Windows 7 for HTML5 audio/video and WebRTC but
not
> in combination with Pepper or WebAudio.
> 
> The AudioInputMessageFilter has not been implemented yet. Waiting for review
> comments first.

[Chris Rogers, 2011-06-14 20:19:20]

Built on Mac OS X with this patch and the web audio API seems to work just fine
in my limited testing.  Later I'll try it on Windows, but I expect it should
work fine.

[scherkus (not reviewing), 2011-06-14 22:22:12]

looking good so far!

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_thr...
content/renderer/render_thread.cc:175:
AudioMessageFilterCreator::SharedFilter();
indent two more spaces

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_thr...
content/renderer/render_thread.cc:200:
AudioMessageFilterCreator::SharedFilter();
ditto

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
content/renderer/render_view.cc:1352: switch (message.level) {
?

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
content/renderer/render_view.h:214: AudioMessageFilter* audio_message_filter() {
will you move this to RenderThread or get rid of this method entirely?

[Chris Rogers, 2011-06-14 22:57:10]

Hi Henrik, I don't have time to check the window's build for web audio API right
now with this patch.  But I'm assuming that my check on Mac OS X is sufficient
so let's not worry about that.

I have one or two comments below:

http://codereview.chromium.org/7157001/diff/11001/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/media/audi...
content/renderer/media/audio_device.h:39: AudioMessageFilter* filter);
Can't the AudioDevice simply call AudioMessageFilterCreator::SharedFilter()
itself instead of having this passed as a constructor argument?

http://codereview.chromium.org/7157001/diff/11001/content/renderer/renderer_w...
File content/renderer/renderer_webaudiodevice_impl.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/renderer_w...
content/renderer/renderer_webaudiodevice_impl.cc:18:
AudioMessageFilterCreator::SharedFilter();
Is there any reason why AudioDevice can't call
AudioMessageFilterCreator::SharedFilter() itself to avoid having to pass this in
as a constructor argument?  AudioDevice was supposed to be an abstraction to
hide the underlying implementation details of the IPC.  This seems to be
requiring clients of AudioDevice to know more than they should have to about
IPC.

[henrika_dont_use, 2011-06-15 15:16:39]

Think I've ensured that both reviewers are happy for now.
Still, I am not 100% sure that this design will be perfect. Will add more
details using e-mail to avoid spamming this CL.

http://codereview.chromium.org/7157001/diff/11001/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/media/audi...
content/renderer/media/audio_device.h:39: AudioMessageFilter* filter);
Good point. Will make such a change and upload for review. Guess Andrew wants to
comment as well.

On 2011/06/14 22:57:10, Chris Rogers wrote:
> Can't the AudioDevice simply call AudioMessageFilterCreator::SharedFilter()
> itself instead of having this passed as a constructor argument?

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_thr...
content/renderer/render_thread.cc:175:
AudioMessageFilterCreator::SharedFilter();
On 2011/06/14 22:22:12, scherkus wrote:
> indent two more spaces

Done.

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_thr...
content/renderer/render_thread.cc:200:
AudioMessageFilterCreator::SharedFilter();
On 2011/06/14 22:22:12, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
content/renderer/render_view.cc:1352: switch (message.level) {
Resolves lint warning. Should it not be a space here?

On 2011/06/14 22:22:12, scherkus wrote:
> ?

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
content/renderer/render_view.h:214: AudioMessageFilter* audio_message_filter() {
Removed. Now uses singleton directly.

On 2011/06/14 22:22:12, scherkus wrote:
> will you move this to RenderThread or get rid of this method entirely?

http://codereview.chromium.org/7157001/diff/11001/content/renderer/renderer_w...
File content/renderer/renderer_webaudiodevice_impl.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/renderer_w...
content/renderer/renderer_webaudiodevice_impl.cc:18:
AudioMessageFilterCreator::SharedFilter();
On 2011/06/14 22:57:10, Chris Rogers wrote:
> Is there any reason why AudioDevice can't call
> AudioMessageFilterCreator::SharedFilter() itself to avoid having to pass this
in
> as a constructor argument?  AudioDevice was supposed to be an abstraction to
> hide the underlying implementation details of the IPC.  This seems to be
> requiring clients of AudioDevice to know more than they should have to about
> IPC.

Done.

[scherkus (not reviewing), 2011-06-16 21:56:22]

few more sanity checking questions but overall I think we're good here

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7157001/diff/11001/content/renderer/render_vie...
content/renderer/render_view.cc:1352: switch (message.level) {
On 2011/06/15 15:16:39, henrika wrote:
> Resolves lint warning. Should it not be a space here?
> 
> On 2011/06/14 22:22:12, scherkus wrote:
> > ?
> 

nope that's right -- I just had a brainfart :)

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_device.h:20: // TODO(henrika): add comments here...
want to add comments right away?  don't see a reason to punt on documentation!

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_device.h:73: void InitializeTask(const
AudioParameters& params);
instead of xxxTask suffix, I wonder if it'd be cleaner to use a xxxOnIOThread
suffix

InitializeOnIOThread
StartOnIOThread

etc...

WDYT?

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_message_filter_creator.cc (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_message_filter_creator.cc:22: return
Singleton<AudioMessageFilterCreator>::get();
hmmmmmmmm.... I'm wondering....

is there any reason to not use Singleton<AudioMessageFilter>::get() as opposed
to this creator class?

guess I'm a bit confused

I'm also wondering about whether classes should be injecting the
AudioMessageFilter as opposed to calling the singleton directly (just concerned
about testability... singletons usually harm testability)

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_message_filter_creator.h (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_message_filter_creator.h:18: 
nit:remove blank line

[henrika_dont_use, 2011-06-17 11:52:01]

Improved comments and removed creator layer based on recommendation from Andrew.

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_device.h:20: // TODO(henrika): add comments here...
On 2011/06/16 21:56:22, scherkus wrote:
> want to add comments right away?  don't see a reason to punt on documentation!

Done.

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_device.h:73: void InitializeTask(const
AudioParameters& params);
Agree. Will change back. Used notation in existing AudioRendererImpl. Should I
modify this class as well to be consistent?

On 2011/06/16 21:56:22, scherkus wrote:
> instead of xxxTask suffix, I wonder if it'd be cleaner to use a xxxOnIOThread
> suffix
> 
> InitializeOnIOThread
> StartOnIOThread
> 
> etc...
> 
> WDYT?

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_message_filter_creator.cc (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_message_filter_creator.cc:22: return
Singleton<AudioMessageFilterCreator>::get();
Good point. I agree (used same approach as was done for video) and will modify
the design. Creator class is removed. Using new factory design in
AudioMessageFilter instead.

On 2011/06/16 21:56:22, scherkus wrote:
> hmmmmmmmm.... I'm wondering....
> 
> is there any reason to not use Singleton<AudioMessageFilter>::get() as opposed
> to this creator class?
> 
> guess I'm a bit confused
> 
> I'm also wondering about whether classes should be injecting the
> AudioMessageFilter as opposed to calling the singleton directly (just
concerned
> about testability... singletons usually harm testability)

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_message_filter_creator.h (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_message_filter_creator.h:18: 
On 2011/06/16 21:56:22, scherkus wrote:
> nit:remove blank line

Done.

[henrika_dont_use, 2011-06-17 14:20:53]

Removed the creator class. 

Tried to cache result of <>:get to remove some overhead but received the
following presubmit errors:

** Presubmit ERRORS **
Found Singleton<T> in the following header files.
Please move them to an appropriate source file so that the template gets
instantiated in a single compilation unit.
  content\renderer\media\audio_device.h \
  content\renderer\media\audio_renderer_impl.h

Removed current scoped_refptr<AudioMessageFilter> filter_ declarations and use
singleton access method in all implementations instead.

I will modify input sides as well when Andrew and Chris are both OK with this
CL.

I must also add that the new design modifies the current behavior in Chrome.
Take HTML5 a/v as an example. Today, two RenderViews in one render thread will
create two AudioMessageFilters with different routing IDs. Using this CL, only
one filter is used and the routing ID will be modified upon creating the second
RenderView. Might require more detailed studies here.

[scherkus (not reviewing), 2011-06-17 17:50:09]

LGTM after fixing nits

again thanks for doing this -- big improvement in readability/design!

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/19001/content/renderer/media/audi...
content/renderer/media/audio_device.h:73: void InitializeTask(const
AudioParameters& params);
On 2011/06/17 11:52:02, henrika wrote:
> Agree. Will change back. Used notation in existing AudioRendererImpl. Should I
> modify this class as well to be consistent?

We can get to that later :)

> 
> On 2011/06/16 21:56:22, scherkus wrote:
> > instead of xxxTask suffix, I wonder if it'd be cleaner to use a
xxxOnIOThread
> > suffix
> > 
> > InitializeOnIOThread
> > StartOnIOThread
> > 
> > etc...
> > 
> > WDYT?
>

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
content/renderer/media/audio_device.h:5: #ifndef
CONTENT_RENDERER_MEDIA_AUDIO_DEVICE_H_
nice docs!

thanks for writing them up henrika!

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:350:
AudioMessageFilter::GetInstance()->Send(message);
nice!

might want to add a DCHECK for io_loop_

http://codereview.chromium.org/7157001/diff/25010/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:994: : public
fileapi::FileSystemCallbackDispatcher {
indentation

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_thr...
content/renderer/render_thread.cc:178: AddFilter(audio_message_filter);
inline the call to AudioMessageFilter::GetInstance() ?

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_thr...
content/renderer/render_thread.cc:202: RemoveFilter(audio_message_filter);
ditto?

your call

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_vie...
content/renderer/render_view.cc:48: #include
"content/renderer/media/audio_message_filter.h"
I don't think we need this anymore either!

[Chris Rogers, 2011-06-17 18:38:40]

Henrik, thanks so much for doing this.  It's a vast improvement over the current
code!

[henrika_dont_use, 2011-06-21 16:43:38]

Rewrote the message filter using new Traits for reference counted objects.

Ensured that the routing ID for the message filter is only set one time by the
first user. Can be HTML5 a/v, Pepper, WebAudio or WebRTC. HTML a/v and Pepper
uses the RenderView routing ID, Pepper uses 2 and both WebAudio and WebRTC uses
1 in this revision. 

Note that, if HTML5 a/v user calls the message filter first, all other users
will use the same routing ID as the initial RenderView.

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
content/renderer/media/audio_device.h:5: // Low-latency audio rendering unit
utilizing audio output stream provided
Thx.
On 2011/06/17 17:50:10, scherkus wrote:
> nice docs!
> 
> thanks for writing them up henrika!

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:350:
AudioMessageFilter::GetInstance()->Send(message);
On 2011/06/17 17:50:10, scherkus wrote:
> nice!
> 
> might want to add a DCHECK for io_loop_

Done.

http://codereview.chromium.org/7157001/diff/25010/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:994: : public
fileapi::FileSystemCallbackDispatcher {
On 2011/06/17 17:50:10, scherkus wrote:
> indentation

Done.

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_thr...
content/renderer/render_thread.cc:178: AddFilter(audio_message_filter);
On 2011/06/17 17:50:10, scherkus wrote:
> inline the call to AudioMessageFilter::GetInstance() ?

Done.

http://codereview.chromium.org/7157001/diff/25010/content/renderer/render_thr...
content/renderer/render_thread.cc:202: RemoveFilter(audio_message_filter);
On 2011/06/17 17:50:10, scherkus wrote:
> ditto?
> 
> your call

Done.

[scherkus (not reviewing), 2011-06-21 17:23:59]

more nits

brettw: base/memory/singleton.h
jam: routing_id usage


FYI the discussion on chromium-dev seemed to lean towards using RenderThread as
opposed to introducing additional singleton traits:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
content/renderer/media/audio_device.cc:32:
audio_message_filter->SetRoutingID(DEFAULT_ROUTING_ID);
strange this actually works :)

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
content/renderer/media/audio_device.h:76: DEFAULT_ROUTING_ID = 1,
this constant doesn't need to be in .h file

also use kCamelCaseStyle

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.h (right):

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:58: ROUTING_ID_HAS_NOT_BEEN_SET =
-1,
this doesn't need to be in .h file

also kCamelCaseStyle

so you could move this to .cc and have kNoRoutingId = -1

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:66: void SetRoutingID(int32 id);
I'm not too hot on having every callee remember to call these two methods in
order to use AudioMessageFilter...

how about changing GetInstance to accept a routing_id

that way the check-and-set of the routing_id can be contained inside
GetInstance() and we're guaranteed that when returning the AudioMessageFilter
it's ready to use immediately

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:81: int32 routing_id() { return
routing_id_; }
I don't think I saw anyone actually call this method.. we might be able to
remove it

http://codereview.chromium.org/7157001/diff/27019/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/27019/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:864: new
PlatformAudioImpl(AudioMessageFilter::GetInstance()));
since you have render_view object, why not use its routing_id() ?

[brettw, 2011-06-21 17:26:39]

I think I agree with your assessment that it will be better to attach to
RenderThread than add more singleton traits. A refcounted singleton sounds weird
to me. I assume this means you won't have to change singleton.h then?

[jam, 2011-06-21 19:02:04]

the changes to singleton aren't needed per the email thread on chromium-dev

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.h (right):

http://codereview.chromium.org/7157001/diff/27019/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:58: ROUTING_ID_HAS_NOT_BEEN_SET =
-1,
we already have a value for this: MSG_ROUTING_NONE

http://codereview.chromium.org/7157001/diff/27019/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/27019/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:174:
filter_->SetRoutingID(DEFAULT_ROUTING_ID);
you can't just pick a routing a id in the renderer process, since this is most
likely already used by a renderer.

stepping back a bit, it's not clear to me why a routing_id is needed.  It looks
like this logic was copied from RenderView.  That's a different situation, and
is used because it's convenient for code to call RenderView's Send() without
knowing the RenderView's routing id.  The IPC message needs a routing id though,
because that's how it's matched to a unique RenderViewHost in the browser
process.

In the case of a singleton in the renderer that only talks to one object in the
browser per renderer process, there's no need to have routing IDs.  You should
use control messages.  Please have a look at
http://dev.chromium.org/developers/design-documents/inter-process-communication.

[jam, 2011-06-21 19:17:37]

CCing chromium-reviews.  This is added by default.  Please do not remove it.

[jam, 2011-06-22 21:36:51]

btw here's how the video guys changed routing messages to control, it might be
an inspiration: http://codereview.chromium.org/7222024/

[henrika_dont_use, 2011-06-23 14:02:57]

Thanks John, I'm inspired.

[henrika_dont_use, 2011-06-23 14:18:40]

New design based on offline discussions.

Functionality for HTML5 a/v verified using this suitable video
http://youtu.be/UBo8KXc83tc.

Remains to verify for Pepper and WebAudio.

Summary:

- The one and only audio message filter now lives on the main render thread. A
public accessor has also been added.

- Usage of routing ID has been removed. All IPC messages are now of control
type. Stream ID is used as key.

- All audio clients using the audio message filter must be on the render thread.

Note:

- The input side is not finalized since I wanted to wait for LGTM for the output
side first. I had to do some initial modifications in the AudioInputRendererHost
to comply with the new control IPC messages. AudioInputDevice will be patched in
a separate CL.

- Unit test will fail in content\renderer\media\audio_renderer_impl_unittest.cc
since it breaks the "must be on the main renderer thread" restriction. Please
advice.

[jam, 2011-06-23 18:59:55]

looks much better :)  almost there, just some nits.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:26: DCHECK(RenderThread::current());
nit: no need

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:28: DCHECK(filter_);
nit: no need

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:30: DCHECK(io_loop_);
nit: don't cache

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:106: DCHECK(MessageLoop::current() ==
io_loop_);
these dchecks are redundant, since this is a private method that is only invoked
using PostTask.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:186: DCHECK(MessageLoop::current() ==
io_loop_);
not needed (filter is safe to call on any thread anyways)

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.h:76: DEFAULT_ROUTING_ID = 1,
this isn't used anywhere, why is it added?

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.h:165: MessageLoop* io_loop_;
don't need this, just use ChildProcess::current()->io_message_loop()

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_message_filter.cc:56: message_loop_ =
MessageLoop::current();
sorry I know this isn't part of this change, but you don't need to keep a
pointer to the IO message loop. just do
ChildProcess::current()->io_message_loop

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:37:
DCHECK(RenderThread::current());
nit: no need

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:39: DCHECK(filter_);
ditto

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:40: io_loop_ =
filter_->message_loop();
don't need this, just use ChildProcess::current()->io_message_loop()

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:352: DCHECK(MessageLoop::current()
== io_loop_);
nit: this isn't needed since filter_ posttasks to the IO thread if it's called
on another thread

http://codereview.chromium.org/7157001/diff/39015/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:165:
DCHECK(RenderThread::current());
nit: we usually avoid dchecks against a pointer before using it. on release
builds, this does nothing. on debug builds, there'll be a crash right away so it
doesn't add much value.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
File content/renderer/render_thread.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
content/renderer/render_thread.h:170: return audio_message_filter_.get();
nit: two space indent. also, the style is not to add comments for simple getters

[scherkus (not reviewing), 2011-06-23 19:14:43]

few nits but wow this is like a breath of fresh air or something -- thanks a ton
for doing this!

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_message_filter.cc:56: message_loop_ =
MessageLoop::current();
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> sorry I know this isn't part of this change, but you don't need to keep a
> pointer to the IO message loop. just do
> ChildProcess::current()->io_message_loop

It's think this code might have been around before ChildProcess had such methods
:)

I agree w/ the change though!

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:35: // Construction of
AudioRendererImpl only works on the main renderer thread.
could you paste the stack trace?

it's probably because something that lives in thread-local-storage (i.e.,
RenderThread::current()) doesn't exist in the unit test context

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
content/renderer/render_thread.cc:198:
VideoCaptureMessageFilterCreator::SharedFilter();
nit: 4 space indent

[wjia(left Chromium), 2011-06-23 19:43:22]

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.h:123: void
SendErrorMessage(int32 stream_id);
it would be good to use int, instead of int32, to keep consistent.

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:177: void
SendErrorMessage(int32 stream_id);
int32 --> int?

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.h:14: //   RenderMessageFilter  <--------->
AudioMessageFilter
From the code (how AudioRendererHost is registered), there is no
RenderMessageFilter between AudioRendererHost and AudioMessageFilter. Please
double check.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:38: filter_ =
RenderThread::current()->audio_message_filter();
it's easier to test AudioRendererImpl if |filter_| is injected as before.
RenderThread::current() uses TLS which might not be available in test
environment.

[henrika_dont_use, 2011-06-27 15:05:44]

Tried to take all input into account.

However, I do end up in a state where I find it rather difficult to maintain the
unit test of AudioRenderImpl. I will spin off a thread and discuss this topic
separately.

For now, the unit test is still crashing.

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_renderer_host.h:123: void
SendErrorMessage(int32 stream_id);
On 2011/06/23 19:43:22, wjia wrote:
> it would be good to use int, instead of int32, to keep consistent.

Done.

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_renderer_host.h:177: void
SendErrorMessage(int32 stream_id);
On 2011/06/23 19:43:22, wjia wrote:
> int32 --> int?

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:26: DCHECK(RenderThread::current());
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: no need

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:28: DCHECK(filter_);
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: no need

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:30: DCHECK(io_loop_);
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: don't cache

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:106: DCHECK(MessageLoop::current() ==
io_loop_);
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> these dchecks are redundant, since this is a private method that is only
invoked
> using PostTask.

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.cc:186: DCHECK(MessageLoop::current() ==
io_loop_);
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> not needed (filter is safe to call on any thread anyways)

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.h:14: //   RenderMessageFilter  <--------->
AudioMessageFilter
Might be correct. Any suggestion on what to use instead?

On 2011/06/23 19:43:22, wjia wrote:
> From the code (how AudioRendererHost is registered), there is no
> RenderMessageFilter between AudioRendererHost and AudioMessageFilter. Please
> double check.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.h:76: DEFAULT_ROUTING_ID = 1,
Removed.

On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> this isn't used anywhere, why is it added?

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_device.h:165: MessageLoop* io_loop_;
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> don't need this, just use ChildProcess::current()->io_message_loop()

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_message_filter.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_message_filter.cc:56: message_loop_ =
MessageLoop::current();
Removed, but the unit tests sets the message_loop_ member.
Will start up a separate discussion regarding the unit test.

On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> sorry I know this isn't part of this change, but you don't need to keep a
> pointer to the IO message loop. just do
> ChildProcess::current()->io_message_loop

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_message_filter.cc:56: message_loop_ =
MessageLoop::current();
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> sorry I know this isn't part of this change, but you don't need to keep a
> pointer to the IO message loop. just do
> ChildProcess::current()->io_message_loop

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:37:
DCHECK(RenderThread::current());
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: no need

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:38: filter_ =
RenderThread::current()->audio_message_filter();
Should be OK without injecting since it is possible to create a test environment
where render thread exists.

On 2011/06/23 19:43:22, wjia wrote:
> it's easier to test AudioRendererImpl if |filter_| is injected as before.
> RenderThread::current() uses TLS which might not be available in test
> environment.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:39: DCHECK(filter_);
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:40: io_loop_ =
filter_->message_loop();
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> don't need this, just use ChildProcess::current()->io_message_loop()

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:352: DCHECK(MessageLoop::current()
== io_loop_);
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: this isn't needed since filter_ posttasks to the IO thread if it's called
> on another thread

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:35: // Construction of
AudioRendererImpl only works on the main renderer thread.
See separate e-mail discussion.

On 2011/06/23 19:14:43, scherkus wrote:
> could you paste the stack trace?
> 
> it's probably because something that lives in thread-local-storage (i.e.,
> RenderThread::current()) doesn't exist in the unit test context

http://codereview.chromium.org/7157001/diff/39015/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:165:
DCHECK(RenderThread::current());
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: we usually avoid dchecks against a pointer before using it. on release
> builds, this does nothing. on debug builds, there'll be a crash right away so
it
> doesn't add much value.

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
content/renderer/render_thread.cc:198:
VideoCaptureMessageFilterCreator::SharedFilter();
On 2011/06/23 19:14:43, scherkus wrote:
> nit: 4 space indent

Done.

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
File content/renderer/render_thread.h (right):

http://codereview.chromium.org/7157001/diff/39015/content/renderer/render_thr...
content/renderer/render_thread.h:170: return audio_message_filter_.get();
On 2011/06/23 18:59:55, John Abd-El-Malek wrote:
> nit: two space indent. also, the style is not to add comments for simple
getters

Done.

[jam, 2011-06-27 16:18:19]

lgtm

http://codereview.chromium.org/7157001/diff/46001/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/46001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:172:
ChildProcess::current()->io_message_loop()->PostTask(FROM_HERE,
nit: MessageLoop::current()-> is smaller :)

http://codereview.chromium.org/7157001/diff/46001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/46001/content/renderer/render_thr...
content/renderer/render_thread.cc:606:
command_line.HasSwitch(switches::kEnableMediaStream));
nit: 4 spaces

[scherkus (not reviewing), 2011-06-27 19:02:08]

LGTM

will you be disabling the crashing tests or did we resolve that issue?

[henrika_dont_use, 2011-06-28 11:57:40]

Fixed last two minor comments from John.

http://codereview.chromium.org/7157001/diff/46001/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/46001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:172:
ChildProcess::current()->io_message_loop()->PostTask(FROM_HERE,
On 2011/06/27 16:18:19, John Abd-El-Malek wrote:
> nit: MessageLoop::current()-> is smaller :)

Done.

http://codereview.chromium.org/7157001/diff/46001/content/renderer/render_thr...
File content/renderer/render_thread.cc (right):

http://codereview.chromium.org/7157001/diff/46001/content/renderer/render_thr...
content/renderer/render_thread.cc:606:
command_line.HasSwitch(switches::kEnableMediaStream));
On 2011/06/27 16:18:19, John Abd-El-Malek wrote:
> nit: 4 spaces

Done.

[henrika_dont_use, 2011-06-28 13:01:46]

Disabled all failing unit tests for now. Will continue discussion in a separate
e-mail thread.

[henrika_dont_use, 2011-06-28 15:38:47]

Sorry, some additional modifications based on similar work on the audio input
side.

- Added input message filter to RenderThread in this CL.
- Moved all other input parts to separate "input CL" (see
http://codereview.chromium.org/7277042).
- Updated some comments in the AudioDevice to ensure that it is in sync with the
new AudioInputDevice.

[henrika_dont_use, 2011-06-29 08:13:25]

Removed dependency of separate "input CL" to ensure that this CL can be
committed as is.

[henrika_dont_use, 2011-06-29 16:31:14]

Enabled major parts of the previously disabled unit tests for 
AudioRendererImpl. Goal has been to run all tests in an environment which is as
realistic as possible following all the existing constraints both in the
renderer and in the audio message filter.

Three tests (all of the same type and related to a killed message loop) are
still disabled since I was not able to find a suitable way to introduce them
again. We loose very little functionality in the test be disabling these.

[tommi (sloooow) - chröme, 2011-06-30 14:11:23]

nice job Henrik.  I'll wait with my final approval for the last update that
enables the 3 currently disabled unit tests.

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_device.cc:55:
ChildProcess::current()->io_message_loop()->PostTask(
should we retain the old comment about why we're accessing the io loop directly?

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_device.h:44: //    memory.
nice

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:96: CHANNEL_LAYOUT_MONO,
indent parameters to the same indent as 16

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:159: FROM_HERE, new
WaitTask(event_.get()));
indent

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:170: FROM_HERE, new
WaitTask(event_.get()));
same.  also below.  lint should help.

[henrika_dont_use, 2011-06-30 14:59:51]

Fixes based on comments from Tommi.

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_device.cc:55:
ChildProcess::current()->io_message_loop()->PostTask(
Not needed since the filter now lives on the render thread and has been set up
already. The old comment was added because it was possible that the filter had
not yet been added to the IO thread when calling Start. Now, we know that this
race condition will not happen.

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:96: CHANNEL_LAYOUT_MONO,
On 2011/06/30 14:11:23, tommi wrote:
> indent parameters to the same indent as 16

Done.

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:159: FROM_HERE, new
WaitTask(event_.get()));
On 2011/06/30 14:11:23, tommi wrote:
> indent

Done.

http://codereview.chromium.org/7157001/diff/35005/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:170: FROM_HERE, new
WaitTask(event_.get()));
On 2011/06/30 14:11:23, tommi wrote:
> same.  also below.  lint should help.

Done.

[henrika_dont_use, 2011-06-30 15:09:14]

- Added support for all tests in the AudioRenderImpl unit test.
- Ensured that no warnings are given when IPC messages are sent.
- Extended comments in the unit test.

[tommi (sloooow) - chröme, 2011-06-30 20:24:44]

lgtm.

http://codereview.chromium.org/7157001/diff/65001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/65001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:162: const IPC::Message&
message) {
fix indent in this function.

[henrika_dont_use, 2011-07-01 08:41:25]

Fixed nit based on comment from Tommi.

http://codereview.chromium.org/7157001/diff/65001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/65001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:162: const IPC::Message&
message) {
On 2011/06/30 20:24:44, tommi wrote:
> fix indent in this function.

Done.

[henrika_dont_use, 2011-07-05 09:31:42]

Added the input parts as well to this CL to save time.

Please note that the AudioInputDevice and AudioInputMessageFilter are not
utilized in Chrome today hence they will not break any existing parts. The input
filter is however added to the RenderThread.

I have verified the performance of the input components using non-submitted
WebRTC tests.

[tommi (sloooow) - chröme, 2011-07-05 20:10:56]

LGTM++

Are any issues/lgtms pending?  I thought this had already been submitted.

On Tue, Jul 5, 2011 at 11:31 AM, <henrika@google.com> wrote:

> Added the input parts as well to this CL to save time.
>
> Please note that the AudioInputDevice and AudioInputMessageFilter are not
> utilized in Chrome today hence they will not break any existing parts. The
> input
> filter is however added to the RenderThread.
>
> I have verified the performance of the input components using non-submitted
> WebRTC tests.
>
>
>
http://codereview.chromium.**org/7157001/<http://codereview.chromium.org/7157...
>

[henrika_dont_use, 2011-07-06 06:50:35]

I think that Andrew would like to take a final look at the latest unit test for
AudioRenderImpl.

[scherkus (not reviewing), 2011-07-07 21:26:47]

mostly style nits + recommendations for organizing the unit tests but LGTM

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:26: class DelegateCaller:
public base::RefCountedThreadSafe<DelegateCaller> {
nit: space before :

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:29: : renderer_(renderer)
{}
indent by two more spaces

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:46: private:
nit: add blank line before public/private labels

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:48: ~DelegateCaller() {}
virtual

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:60: : event_(event) {}
nit: indent by two more spaces

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:71: class
AudioRendererImplTest : public ::testing::Test,
use initializer list style (i.e., drop : to next line, indent by 4)

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:79: static const int
kMaxWait = 500;
use base/test/test_timeouts.h instead

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:81:
AudioRendererImplTest() { }
nit: { } -> {}

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:84: // Test fixture's
SetUp.
nit: comment is useless (SetUp for a test case is common enough)

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:139: // Test fixture's
TearDown.
ditto w/ comment

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:144: // Fixture members
ditto w/ comment

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:161: bool
AudioRendererImplTest::OnMessageReceived(
nit: fits on one line

also typically we just inline these methods inside the class for unit tests

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:181:
ChildProcess::current()->io_message_loop()->PostTask(
what about having a PostIOTask() helper function defined in class?

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:195:
ChildProcess::current()->io_message_loop()->PostTask(
this code is duplicated a lot

typically I try to have tests that while might be a bit hairy in the fixture
class, at least read sensibly in the tests themselves

for example this bit of code where we post a task + wait could be encapsulated
inside a function called WaitForCompletion()

that way you don't need a comment describing the ~3-4 lines of code either

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:247:
ChildProcess::current()->io_message_loop()->PostTask(
ditto here with "DestroyIOMessageLoop()" helper function

http://codereview.chromium.org/7157001/diff/68001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/68001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:163: explicit
PlatformAudioImpl()
drop explicit

[henrika_dont_use, 2011-07-08 12:47:58]

Done modifications based on comments from Andrew.

Will make some additional code cleanup for the unit test in a separate CL.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:26: class DelegateCaller:
public base::RefCountedThreadSafe<DelegateCaller> {
On 2011/07/07 21:26:47, scherkus wrote:
> nit: space before :

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:29: : renderer_(renderer)
{}
On 2011/07/07 21:26:47, scherkus wrote:
> indent by two more spaces

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:46: private:
On 2011/07/07 21:26:47, scherkus wrote:
> nit: add blank line before public/private labels

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:48: ~DelegateCaller() {}
On 2011/07/07 21:26:47, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:60: : event_(event) {}
On 2011/07/07 21:26:47, scherkus wrote:
> nit: indent by two more spaces

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:71: class
AudioRendererImplTest : public ::testing::Test,
On 2011/07/07 21:26:47, scherkus wrote:
> use initializer list style (i.e., drop : to next line, indent by 4)

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:79: static const int
kMaxWait = 500;
On 2011/07/07 21:26:47, scherkus wrote:
> use base/test/test_timeouts.h instead

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:81:
AudioRendererImplTest() { }
On 2011/07/07 21:26:47, scherkus wrote:
> nit: { } -> {}

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:84: // Test fixture's
SetUp.
On 2011/07/07 21:26:47, scherkus wrote:
> nit: comment is useless (SetUp for a test case is common enough)

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:139: // Test fixture's
TearDown.
On 2011/07/07 21:26:47, scherkus wrote:
> ditto w/ comment

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:144: // Fixture members
On 2011/07/07 21:26:47, scherkus wrote:
> ditto w/ comment

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:161: bool
AudioRendererImplTest::OnMessageReceived(
On 2011/07/07 21:26:47, scherkus wrote:
> nit: fits on one line
> 
> also typically we just inline these methods inside the class for unit tests

Done.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:181:
ChildProcess::current()->io_message_loop()->PostTask(
Will fix it in a separate CL.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:195:
ChildProcess::current()->io_message_loop()->PostTask(
Will fix it in a separate CL.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl_unittest.cc:247:
ChildProcess::current()->io_message_loop()->PostTask(
Will fix it in a separate CL.

http://codereview.chromium.org/7157001/diff/68001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7157001/diff/68001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:163: explicit
PlatformAudioImpl()
On 2011/07/07 21:26:47, scherkus wrote:
> drop explicit

Done.

[henrika_dont_use, 2011-07-08 12:57:22]

I have now also merged the latest modifications by Eugene
(http://codereview.chromium.org/7253003) into this CL.

[commit-bot: I haz the power, 2011-07-08 14:18:21]

Try job failure for 7157001-74007 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[henrika_dont_use, 2011-07-08 15:55:20]

Sorry guys. I removed the marker from the commit button.

Seems to be some timing issue in the new unit test. I get error reports from the
trybots for Linux and Mac.

Will check this again on Monday.

[brettw, 2011-07-08 17:32:08]

content LGTM (just gave a quick once-over)

[henrika_dont_use, 2011-07-11 11:45:09]

Committed to try-bots again after rebasing on top of 7217025.

[henrika_dont_use, 2011-07-11 17:59:47]

Will be committed here: http://codereview.chromium.org/7327034/