First step towards moving AudioDevice from content/ to media/audio.

First step towards moving AudioDevice and AudioInputDevice from content/ to media/audio.  This cl introduces new IPC interface files in media/audio that have the definitions of an IPC layer for AudioDevice and AudioInputDevice.
AudioMessageFilter, AudionInputMessageFilter and others have been updated to use definitions from these file but in order to keep the diffs simple, I haven't actually moved the files over to media/audio. That will be the next step (and then no code changes should be needed).

TEST=There should be no functional changes here.
  If there are problems, they should be caught by our existing unit
  tests or build errors.

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

[tommi (sloooow) - chröme, 2012-07-23 16:47:23]

Hey Andrew - here's a stab at what we discussed :)
I'm trying to do it in small steps so that we can work out any issues early.

[scherkus (not reviewing), 2012-07-23 22:57:51]

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
content/browser/renderer_host/media/audio_renderer_host.cc:315: Send(new
AudioMsg_NotifyStreamStateChanged(stream_id,
fix indent (drop stream_id to next line?)

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
File content/browser/renderer_host/media/audio_renderer_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:122: if
(state == media::kAudioStreamPlaying) {
seems like a good use case for a switch!

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/common/medi...
File content/common/media/audio_stream_state.h (left):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/common/medi...
content/common/media/audio_stream_state.h:11: enum AudioStreamState {
we had a whole file dedicated to this?!? haha

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_device_unittest.cc:138: return new
AudioDevice(audio_message_filter_,
fix indent (drop a_m_f_ to next line?)

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_input_message_filter.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_input_message_filter.h:37: virtual void
OnStateChanged(media::AudioStreamState state) = 0;
AIMF should have its own set of state enums

case in point: AudioInputDevice(which impls AIMF::Delegate) has TODOs for
kAudioStreamPaused saying it should be Stopped and a NOTIMPLEMENTED() for
kAudioStreamPlaying since playing doesn't make sense for input (perhaps it
should be capture or even better removed entirely if it's not used?)

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter.cc:93: // Captures the channel for
IPC.
this comment is useless -- mind removing?

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter.cc:125: DLOG(WARNING) << "Got audio
stream event for a non-existent or removed"
I wonder who's passing in bad ids?

if a reference to some sort of stream interface was returned instead of a stream
id it would be equivalent to a use-after-free bug

may want to consider upgrading to DCHECK one day :)

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_message_filter.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter.h:31: // Add a delegate to the map
and return id of the entry.
replace this comment + one below with "media::AudioDeviceIPCDelegate
implementation." since the definition for ADIPCDelegate has the docs for these
methods

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_message_filter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:126:
AudioMsg_NotifyStreamStateChanged(stream_id1,
mixed indent: drop stream_id1 to next line

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:135:
AudioMsg_NotifyStreamStateChanged(stream_id2,
mixed indent: drop stream_id1 to next line

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:145:
AudioMsg_NotifyStreamStateChanged(stream_id1,
mixed indent: drop stream_id1 to next line

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:152:
AudioMsg_NotifyStreamStateChanged(stream_id2,
mixed indent: drop stream_id1 to next line

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.h:40: //
AudioMessageFilter::Delegate.
update docs

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
File media/audio/audio_device_ipc.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:17: enum AudioStreamState {
define inside Delegate as "State" and drop kAudioStream prefix?

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:23: class AudioDeviceIPCDelegate {
any thoughts as to replacing this interface with two base::Bind() parameters?

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:25: 
remove blank line

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:26: // Called when state of an audio stream has
changed in the browser process.
nit: I'd drop the "in the browser process" bit -- browser vs renderer is a
src/content/ concept

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:29: // Called when an audio stream has been
created in the browser process.
ditto

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:29: // Called when an audio stream has been
created in the browser process.
can we get docs on what these handles and params mean?

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:34: virtual ~AudioDeviceIPCDelegate() {}
even this it's OK to inline ctor/dtor in this particular instance, us media
folks have decided to avoid dealing with style exceptions and we impl the
ctor/dtor inside a .cc

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:37: class AudioDeviceIPC {
needs docs

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:39: // Registers an AudioDeviceIPCDelegate and
returns a stream_id that must
|stream_id|

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:41: virtual int
AddDelegate(AudioDeviceIPCDelegate* sink) = 0;
s/sink/delegate?

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:43: // Remove a delegate referenced by
|stream_id| from the map.
map? what map!? (update docs :))

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:46: virtual void CreateStream(int stream_id,
const AudioParameters& params) = 0;
docs?

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:54: virtual ~AudioDeviceIPC() {}
ditto

[tommi (sloooow) - chröme, 2012-07-24 09:49:43]

All done except for updating the input interfaces.  Will take care of that in
the next patch set.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
content/browser/renderer_host/media/audio_renderer_host.cc:315: Send(new
AudioMsg_NotifyStreamStateChanged(stream_id,
On 2012/07/23 22:57:51, scherkus wrote:
> fix indent (drop stream_id to next line?)

I can fix this but I thought this was in line with the styleguide?  Do you mind
setting me straight?
(I thought I was doing something similar to "AndAnotherLongFunctionCall" in
http://www.chromium.org/developers/coding-style)

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
File content/browser/renderer_host/media/audio_renderer_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
content/browser/renderer_host/media/audio_renderer_host_unittest.cc:122: if
(state == media::kAudioStreamPlaying) {
On 2012/07/23 22:57:51, scherkus wrote:
> seems like a good use case for a switch!

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/common/medi...
File content/common/media/audio_stream_state.h (left):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/common/medi...
content/common/media/audio_stream_state.h:11: enum AudioStreamState {
On 2012/07/23 22:57:51, scherkus wrote:
> we had a whole file dedicated to this?!? haha

enum files are the new black! hehe, iirc this was necessary since both renderer
and browser needed the definition.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_device_unittest.cc:138: return new
AudioDevice(audio_message_filter_,
On 2012/07/23 22:57:51, scherkus wrote:
> fix indent (drop a_m_f_ to next line?)

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_input_message_filter.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_input_message_filter.h:37: virtual void
OnStateChanged(media::AudioStreamState state) = 0;
On 2012/07/23 22:57:51, scherkus wrote:
> AIMF should have its own set of state enums
> 
> case in point: AudioInputDevice(which impls AIMF::Delegate) has TODOs for
> kAudioStreamPaused saying it should be Stopped and a NOTIMPLEMENTED() for
> kAudioStreamPlaying since playing doesn't make sense for input (perhaps it
> should be capture or even better removed entirely if it's not used?)

Makes sense.  I'll make interface definitions for input in the ipc header file
under media/audio.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter.cc:125: DLOG(WARNING) << "Got audio
stream event for a non-existent or removed"
On 2012/07/23 22:57:51, scherkus wrote:
> I wonder who's passing in bad ids?
> 
> if a reference to some sort of stream interface was returned instead of a
stream
> id it would be equivalent to a use-after-free bug
> 
> may want to consider upgrading to DCHECK one day :)

Upgraded to NOTREACHED.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_message_filter.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter.h:31: // Add a delegate to the map
and return id of the entry.
On 2012/07/23 22:57:51, scherkus wrote:
> replace this comment + one below with "media::AudioDeviceIPCDelegate
> implementation." since the definition for ADIPCDelegate has the docs for these
> methods

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
File content/renderer/media/audio_message_filter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:126:
AudioMsg_NotifyStreamStateChanged(stream_id1,
On 2012/07/23 22:57:51, scherkus wrote:
> mixed indent: drop stream_id1 to next line

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:135:
AudioMsg_NotifyStreamStateChanged(stream_id2,
On 2012/07/23 22:57:51, scherkus wrote:
> mixed indent: drop stream_id1 to next line

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:145:
AudioMsg_NotifyStreamStateChanged(stream_id1,
On 2012/07/23 22:57:51, scherkus wrote:
> mixed indent: drop stream_id1 to next line

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/me...
content/renderer/media/audio_message_filter_unittest.cc:152:
AudioMsg_NotifyStreamStateChanged(stream_id2,
On 2012/07/23 22:57:51, scherkus wrote:
> mixed indent: drop stream_id1 to next line

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.h:40: //
AudioMessageFilter::Delegate.
On 2012/07/23 22:57:51, scherkus wrote:
> update docs

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
File media/audio/audio_device_ipc.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:17: enum AudioStreamState {
On 2012/07/23 22:57:51, scherkus wrote:
> define inside Delegate as "State" and drop kAudioStream prefix?

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:23: class AudioDeviceIPCDelegate {
On 2012/07/23 22:57:51, scherkus wrote:
> any thoughts as to replacing this interface with two base::Bind() parameters?

I thought about that but decided not to do that right now for these reasons:

* Too many changes to the existing code.
* Registering/unregistering the callbacks can get more messy.  E.g. If an
AudioDevice implementation gets lazy and doesn't unregister, but rather null's a
callback, the AudioMessageFilter's callback vectors could fill up rather
quickly.
* It doesn't really give us more convenience over what we have currently and
generates more code.

Also, I was thinking about adding a callback to this interface, OnIPCClosed()
and make it a part of the contract that any cached references to the
AudioDeviceIPC object must be cleared at that point. WDYT?

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:25: 
On 2012/07/23 22:57:51, scherkus wrote:
> remove blank line

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:26: // Called when state of an audio stream has
changed in the browser process.
On 2012/07/23 22:57:51, scherkus wrote:
> nit: I'd drop the "in the browser process" bit -- browser vs renderer is a
> src/content/ concept

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:29: // Called when an audio stream has been
created in the browser process.
On 2012/07/23 22:57:51, scherkus wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:29: // Called when an audio stream has been
created in the browser process.
On 2012/07/23 22:57:51, scherkus wrote:
> can we get docs on what these handles and params mean?

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:34: virtual ~AudioDeviceIPCDelegate() {}
On 2012/07/23 22:57:51, scherkus wrote:
> even this it's OK to inline ctor/dtor in this particular instance, us media
> folks have decided to avoid dealing with style exceptions and we impl the
> ctor/dtor inside a .cc

ah, wasn't aware of that. Thanks for the heads up.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:37: class AudioDeviceIPC {
On 2012/07/23 22:57:51, scherkus wrote:
> needs docs

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:39: // Registers an AudioDeviceIPCDelegate and
returns a stream_id that must
On 2012/07/23 22:57:51, scherkus wrote:
> |stream_id|

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:41: virtual int
AddDelegate(AudioDeviceIPCDelegate* sink) = 0;
On 2012/07/23 22:57:51, scherkus wrote:
> s/sink/delegate?

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:43: // Remove a delegate referenced by
|stream_id| from the map.
On 2012/07/23 22:57:51, scherkus wrote:
> map? what map!? (update docs :))

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:46: virtual void CreateStream(int stream_id,
const AudioParameters& params) = 0;
On 2012/07/23 22:57:51, scherkus wrote:
> docs?

Done.

https://chromiumcodereview.appspot.com/10790121/diff/3003/media/audio/audio_d...
media/audio/audio_device_ipc.h:54: virtual ~AudioDeviceIPC() {}
On 2012/07/23 22:57:51, scherkus wrote:
> ditto

Done.

[tommi (sloooow) - chröme, 2012-07-24 12:27:54]

Changes for AudioInputDevice uploaded and cl description amended.  ptal.

[tommi (sloooow) - chröme, 2012-07-24 12:36:15]

+darin for content/content_common.gypi

[scherkus (not reviewing), 2012-07-24 17:57:48]

man AudioInputDeviceIPCDelegate is a long name!!! I wonder if we can think of
something more concise :\

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/ren...
content/browser/renderer_host/media/audio_renderer_host.cc:315: Send(new
AudioMsg_NotifyStreamStateChanged(stream_id,
On 2012/07/24 09:49:44, tommi wrote:
> On 2012/07/23 22:57:51, scherkus wrote:
> > fix indent (drop stream_id to next line?)
> 
> I can fix this but I thought this was in line with the styleguide?  Do you
mind
> setting me straight?
> (I thought I was doing something similar to "AndAnotherLongFunctionCall" in
> http://www.chromium.org/developers/coding-style)

Hmm!!

I've always done either ThisIsARatherLongFunctionCall or
ThisIsAnotherLongFunctionCall but never AndAnotherLongFunctionCall as that seems
like a mixing of indentation styles

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
doesn't explicitly forbid the mixing of indentation styles but does seem to
either wrap at the ( or drop to next line + 4-space indent

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/ren...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/ren...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:463:
std::string()))
nit: fits on previous line

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/ren...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:509:
std::string()))
nit: fits on previous line

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
File content/renderer/media/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
content/renderer/media/audio_input_device.cc:193: agc_is_enabled_);
indenting

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
content/renderer/media/audio_input_device.cc:203: ipc_ = NULL;
FYI we seem to guard against calling ipc_ with stream_id_ -- this may lead to
null deref

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
File content/renderer/media/audio_input_message_filter.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
content/renderer/media/audio_input_message_filter.h:64: void
OnStreamStateChanged(int stream_id,
indent

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
File content/renderer/media/audio_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
content/renderer/media/audio_message_filter.cc:134: // tests (ppapi_ppb_audio,
PPB_Audio::StartPlayback, StopPlayback).
file a bug?

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
File content/renderer/media/audio_message_filter.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
content/renderer/media/audio_message_filter.h:31: //
media::AudioDeviceIPCDelegate implementation
period

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/me...
content/renderer/media/audio_message_filter.h:70:
media::AudioDeviceIPCDelegate::State state);
indent

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:123: ipc_ = NULL;
null deref if stream_id != 0?

https://chromiumcodereview.appspot.com/10790121/diff/5027/media/audio/audio_i...
File media/audio/audio_input_device_ipc.h (right):

https://chromiumcodereview.appspot.com/10790121/diff/5027/media/audio/audio_i...
media/audio/audio_input_device_ipc.h:50: // IPC channel has been closed and no
more ipc requests can be made.
nit:s/ipc/IPC

[tommi (sloooow) - chröme, 2012-07-24 20:35:51]

Some ideas...
* skip 'Device' in the name
* introduce a new namespace, audio,  to shorten names and group code.
* s/Input/In s/Output/Out.
Wdyt?
On Jul 24, 2012 7:57 PM, <scherkus@chromium.org> wrote:

> man AudioInputDeviceIPCDelegate is a long name!!! I wonder if we can think
> of
> something more concise :\
>
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
3003/content/browser/renderer_**host/media/audio_renderer_**host.cc<https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/renderer_host/media/audio_renderer_host.cc>
> File content/browser/renderer_host/**media/audio_renderer_host.cc (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
> 3003/content/browser/renderer_**host/media/audio_renderer_**
>
host.cc#newcode315<https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/renderer_host/media/audio_renderer_host.cc#newcode315>
> content/browser/renderer_host/**media/audio_renderer_host.cc:**315:
> Send(new
> AudioMsg_**NotifyStreamStateChanged(**stream_id,
> On 2012/07/24 09:49:44, tommi wrote:
>
>> On 2012/07/23 22:57:51, scherkus wrote:
>> > fix indent (drop stream_id to next line?)
>>
>
>  I can fix this but I thought this was in line with the styleguide?  Do
>>
> you mind
>
>> setting me straight?
>> (I thought I was doing something similar to
>>
> "AndAnotherLongFunctionCall" in
>
>>
http://www.chromium.org/**developers/coding-style<http://www.chromium.org/dev...
>> )
>>
>
> Hmm!!
>
> I've always done either ThisIsARatherLongFunctionCall or
> ThisIsAnotherLongFunctionCall but never AndAnotherLongFunctionCall as
> that seems like a mixing of indentation styles
>
> http://google-styleguide.**googlecode.com/svn/trunk/**
>
cppguide.xml?showone=Function_**Calls#Function_Calls<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Calls#Function_Calls>
> doesn't explicitly forbid the mixing of indentation styles but does seem
> to either wrap at the ( or drop to next line + 4-space indent
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
> 5027/content/browser/renderer_**host/media/audio_input_device_**
>
manager_unittest.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc>
> File
> content/browser/renderer_host/**media/audio_input_device_**
> manager_unittest.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
> 5027/content/browser/renderer_**host/media/audio_input_device_**
>
manager_unittest.cc#newcode463<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc#newcode463>
> content/browser/renderer_host/**media/audio_input_device_**
> manager_unittest.cc:463:
> std::string()))
> nit: fits on previous line
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
> 5027/content/browser/renderer_**host/media/audio_input_device_**
>
manager_unittest.cc#newcode509<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc#newcode509>
> content/browser/renderer_host/**media/audio_input_device_**
> manager_unittest.cc:509:
> std::string()))
> nit: fits on previous line
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_input_device.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_device.cc>
> File content/renderer/media/audio_**input_device.cc (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_input_device.cc#**newcode193<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_device.cc#newcode193>
> content/renderer/media/audio_**input_device.cc:193: agc_is_enabled_);
> indenting
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_input_device.cc#**newcode203<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_device.cc#newcode203>
> content/renderer/media/audio_**input_device.cc:203: ipc_ = NULL;
> FYI we seem to guard against calling ipc_ with stream_id_ -- this may
> lead to null deref
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_input_message_filter.h<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_message_filter.h>
> File content/renderer/media/audio_**input_message_filter.h (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_input_message_filter.h#**newcode64<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_message_filter.h#newcode64>
> content/renderer/media/audio_**input_message_filter.h:64: void
> OnStreamStateChanged(int stream_id,
> indent
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_message_filter.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.cc>
> File content/renderer/media/audio_**message_filter.cc (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_message_filter.cc#**newcode134<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.cc#newcode134>
> content/renderer/media/audio_**message_filter.cc:134: // tests
> (ppapi_ppb_audio, PPB_Audio::StartPlayback, StopPlayback).
> file a bug?
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_message_filter.h<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.h>
> File content/renderer/media/audio_**message_filter.h (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_message_filter.h#**newcode31<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.h#newcode31>
> content/renderer/media/audio_**message_filter.h:31: //
> media::AudioDeviceIPCDelegate implementation
> period
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/media/**audio_message_filter.h#**newcode70<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.h#newcode70>
> content/renderer/media/audio_**message_filter.h:70:
> media::AudioDeviceIPCDelegate:**:State state);
> indent
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/content/renderer/pepper/**pepper_platform_audio_input_**impl.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/pepper/pepper_platform_audio_input_impl.cc>
> File content/renderer/pepper/**pepper_platform_audio_input_**impl.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
> 5027/content/renderer/pepper/**pepper_platform_audio_input_**
>
impl.cc#newcode123<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode123>
> content/renderer/pepper/**pepper_platform_audio_input_**impl.cc:123: ipc_
> =
> NULL;
> null deref if stream_id != 0?
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/media/audio/audio_input_**device_ipc.h<https://chromiumcodereview.appspot.com/10790121/diff/5027/media/audio/audio_input_device_ipc.h>
> File media/audio/audio_input_**device_ipc.h (right):
>
> https://chromiumcodereview.**appspot.com/10790121/diff/**
>
5027/media/audio/audio_input_**device_ipc.h#newcode50<https://chromiumcodereview.appspot.com/10790121/diff/5027/media/audio/audio_input_device_ipc.h#newcode50>
> media/audio/audio_input_**device_ipc.h:50: // IPC channel has been closed
> and no more ipc requests can be made.
> nit:s/ipc/IPC
>
>
https://chromiumcodereview.**appspot.com/10790121/<https://chromiumcodereview...
>

[scherkus (not reviewing), 2012-07-24 20:49:39]

On 2012/07/24 20:35:51, tommi wrote:
> Some ideas...
> * skip 'Device' in the name
> * introduce a new namespace, audio,  to shorten names and group code.
> * s/Input/In s/Output/Out.
> Wdyt?

Let's get rid of Device -- IPC is the more important part in the name instead of
Device.

> On Jul 24, 2012 7:57 PM, <mailto:scherkus@chromium.org> wrote:
> 
> > man AudioInputDeviceIPCDelegate is a long name!!! I wonder if we can think
> > of
> > something more concise :\
> >
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
3003/content/browser/renderer_**host/media/audio_renderer_**host.cc<https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/renderer_host/media/audio_renderer_host.cc>
> > File content/browser/renderer_host/**media/audio_renderer_host.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> > 3003/content/browser/renderer_**host/media/audio_renderer_**
> >
>
host.cc#newcode315<https://chromiumcodereview.appspot.com/10790121/diff/3003/content/browser/renderer_host/media/audio_renderer_host.cc#newcode315>
> > content/browser/renderer_host/**media/audio_renderer_host.cc:**315:
> > Send(new
> > AudioMsg_**NotifyStreamStateChanged(**stream_id,
> > On 2012/07/24 09:49:44, tommi wrote:
> >
> >> On 2012/07/23 22:57:51, scherkus wrote:
> >> > fix indent (drop stream_id to next line?)
> >>
> >
> >  I can fix this but I thought this was in line with the styleguide?  Do
> >>
> > you mind
> >
> >> setting me straight?
> >> (I thought I was doing something similar to
> >>
> > "AndAnotherLongFunctionCall" in
> >
> >>
>
http://www.chromium.org/**developers/coding-style%3Chttp://www.chromium.org/d...>
> >> )
> >>
> >
> > Hmm!!
> >
> > I've always done either ThisIsARatherLongFunctionCall or
> > ThisIsAnotherLongFunctionCall but never AndAnotherLongFunctionCall as
> > that seems like a mixing of indentation styles
> >
> > http://google-styleguide.**googlecode.com/svn/trunk/**
> >
>
cppguide.xml?showone=Function_**Calls#Function_Calls<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Calls#Function_Calls>
> > doesn't explicitly forbid the mixing of indentation styles but does seem
> > to either wrap at the ( or drop to next line + 4-space indent
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> > 5027/content/browser/renderer_**host/media/audio_input_device_**
> >
>
manager_unittest.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc>
> > File
> > content/browser/renderer_host/**media/audio_input_device_**
> > manager_unittest.cc
> > (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> > 5027/content/browser/renderer_**host/media/audio_input_device_**
> >
>
manager_unittest.cc#newcode463<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc#newcode463>
> > content/browser/renderer_host/**media/audio_input_device_**
> > manager_unittest.cc:463:
> > std::string()))
> > nit: fits on previous line
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> > 5027/content/browser/renderer_**host/media/audio_input_device_**
> >
>
manager_unittest.cc#newcode509<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc#newcode509>
> > content/browser/renderer_host/**media/audio_input_device_**
> > manager_unittest.cc:509:
> > std::string()))
> > nit: fits on previous line
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_input_device.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_device.cc>
> > File content/renderer/media/audio_**input_device.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_input_device.cc#**newcode193<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_device.cc#newcode193>
> > content/renderer/media/audio_**input_device.cc:193: agc_is_enabled_);
> > indenting
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_input_device.cc#**newcode203<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_device.cc#newcode203>
> > content/renderer/media/audio_**input_device.cc:203: ipc_ = NULL;
> > FYI we seem to guard against calling ipc_ with stream_id_ -- this may
> > lead to null deref
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_input_message_filter.h<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_message_filter.h>
> > File content/renderer/media/audio_**input_message_filter.h (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_input_message_filter.h#**newcode64<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_input_message_filter.h#newcode64>
> > content/renderer/media/audio_**input_message_filter.h:64: void
> > OnStreamStateChanged(int stream_id,
> > indent
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_message_filter.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.cc>
> > File content/renderer/media/audio_**message_filter.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_message_filter.cc#**newcode134<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.cc#newcode134>
> > content/renderer/media/audio_**message_filter.cc:134: // tests
> > (ppapi_ppb_audio, PPB_Audio::StartPlayback, StopPlayback).
> > file a bug?
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_message_filter.h<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.h>
> > File content/renderer/media/audio_**message_filter.h (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_message_filter.h#**newcode31<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.h#newcode31>
> > content/renderer/media/audio_**message_filter.h:31: //
> > media::AudioDeviceIPCDelegate implementation
> > period
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/media/**audio_message_filter.h#**newcode70<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/media/audio_message_filter.h#newcode70>
> > content/renderer/media/audio_**message_filter.h:70:
> > media::AudioDeviceIPCDelegate:**:State state);
> > indent
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/content/renderer/pepper/**pepper_platform_audio_input_**impl.cc<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/pepper/pepper_platform_audio_input_impl.cc>
> > File content/renderer/pepper/**pepper_platform_audio_input_**impl.cc
> > (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> > 5027/content/renderer/pepper/**pepper_platform_audio_input_**
> >
>
impl.cc#newcode123<https://chromiumcodereview.appspot.com/10790121/diff/5027/content/renderer/pepper/pepper_platform_audio_input_impl.cc#newcode123>
> > content/renderer/pepper/**pepper_platform_audio_input_**impl.cc:123: ipc_
> > =
> > NULL;
> > null deref if stream_id != 0?
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/media/audio/audio_input_**device_ipc.h<https://chromiumcodereview.appspot.com/10790121/diff/5027/media/audio/audio_input_device_ipc.h>
> > File media/audio/audio_input_**device_ipc.h (right):
> >
> > https://chromiumcodereview.**appspot.com/10790121/diff/**
> >
>
5027/media/audio/audio_input_**device_ipc.h#newcode50<https://chromiumcodereview.appspot.com/10790121/diff/5027/media/audio/audio_input_device_ipc.h#newcode50>
> > media/audio/audio_input_**device_ipc.h:50: // IPC channel has been closed
> > and no more ipc requests can be made.
> > nit:s/ipc/IPC
> >
> >
>
https://chromiumcodereview.**appspot.com/10790121/%3Chttps://chromiumcoderevi...>
> >

[darin (slow to review), 2012-07-24 23:52:30]

On 2012/07/24 12:36:15, tommi wrote:
> +darin for content/content_common.gypi

LGTM for content_common.gypi

[tommi (sloooow) - chröme, 2012-07-25 13:46:16]

Thanks Andrew.  All done, including changing AudioInputDeviceIPCDelegate to
AudioInputIPCDelegate.

I also updated the AudioInputDevice to be consistent with AudioDevice.  As you
know, these two classes often get out of sync so I decided to keep them in sync 
in this change although I started making the change for AudioDevice only. 
Creating an input device is now done the same way as for an output device and
the factory function names are consistent.  Please see patch set #10 (diff #9)
for that change alone.

http://codereview.chromium.org/10790121/diff/5027/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

http://codereview.chromium.org/10790121/diff/5027/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:463:
std::string()))
On 2012/07/24 17:57:48, scherkus wrote:
> nit: fits on previous line

Done.

http://codereview.chromium.org/10790121/diff/5027/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:509:
std::string()))
On 2012/07/24 17:57:48, scherkus wrote:
> nit: fits on previous line

Done.

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
File content/renderer/media/audio_input_device.cc (right):

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:193: agc_is_enabled_);
On 2012/07/24 17:57:48, scherkus wrote:
> indenting

Done.

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
content/renderer/media/audio_input_device.cc:203: ipc_ = NULL;
On 2012/07/24 17:57:48, scherkus wrote:
> FYI we seem to guard against calling ipc_ with stream_id_ -- this may lead to
> null deref

The stream_id_ checks are not really meant to guard the ipc_ variable.  Some
checks are for if (stream_id_), others if (!stream_id_) and in some places
there's no check.  I see your point though and I have thought about it.  In
practice, the audio message filter always outlives the audio devices, but I
intentionally added this callback to force a NULL crash in case we make a
mistake (and not accessing deleted memory).

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
File content/renderer/media/audio_input_message_filter.h (right):

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
content/renderer/media/audio_input_message_filter.h:64: void
OnStreamStateChanged(int stream_id,
On 2012/07/24 17:57:48, scherkus wrote:
> indent

Done.

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

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
content/renderer/media/audio_message_filter.cc:134: // tests (ppapi_ppb_audio,
PPB_Audio::StartPlayback, StopPlayback).
On 2012/07/24 17:57:48, scherkus wrote:
> file a bug?

Done.

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

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:31: //
media::AudioDeviceIPCDelegate implementation
On 2012/07/24 17:57:48, scherkus wrote:
> period

Done.

http://codereview.chromium.org/10790121/diff/5027/content/renderer/media/audi...
content/renderer/media/audio_message_filter.h:70:
media::AudioDeviceIPCDelegate::State state);
On 2012/07/24 17:57:48, scherkus wrote:
> indent

Done.

http://codereview.chromium.org/10790121/diff/5027/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

http://codereview.chromium.org/10790121/diff/5027/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:123: ipc_ = NULL;
On 2012/07/24 17:57:48, scherkus wrote:
> null deref if stream_id != 0?

See other comment. I didn't want to add reference counting to the ipc interface
as in practice it isn't needed.  This is to force NULL crashes instead of
referencing deleted memory in case we make a mistake.

http://codereview.chromium.org/10790121/diff/5027/media/audio/audio_input_dev...
File media/audio/audio_input_device_ipc.h (right):

http://codereview.chromium.org/10790121/diff/5027/media/audio/audio_input_dev...
media/audio/audio_input_device_ipc.h:50: // IPC channel has been closed and no
more ipc requests can be made.
On 2012/07/24 17:57:48, scherkus wrote:
> nit:s/ipc/IPC

Done.

[tommi (sloooow) - chröme, 2012-07-25 16:14:57]

sorry for the 'one more' fix, but since I'm in there I took care of a little
thing that Victoria wanted to fix a while ago, use |int| instead of int32 and
uint32 in the interface.  Hope that's all right.  Hands off now.

[scherkus (not reviewing), 2012-07-25 17:43:36]

LGTM!!! I thought you were also going to drop Device from AudioOutputDeviceIPC
-- see my note in that file

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
content/renderer/media/audio_device.h:112: const
scoped_refptr<base::MessageLoopProxy>& io_loop);
indent

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
File content/renderer/media/audio_input_device.h (right):

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
content/renderer/media/audio_input_device.h:148: double sample_rate() const {
return audio_parameters_.sample_rate();   }
remove extra spacing at end

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_device_i...
File media/audio/audio_device_ipc.h (right):

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_device_i...
media/audio/audio_device_ipc.h:58: class MEDIA_EXPORT AudioDeviceIPC {
Shoot. I thought you were going to rename this AudioOutputIPC to mirror
AudioInputIPC -- I should have articulated that instead of assuming you would
read my mind from halfway around the world!!!

It's a purely OCD thing on my part but I think it makes sense to have our audio
code state Input/Output in their names instead of optionally leaving out
"Output" and having that imply that it's an output device

WDYT?

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_input_de...
File media/audio/audio_input_device_ipc.h (right):

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_input_de...
media/audio/audio_input_device_ipc.h:5: #ifndef
MEDIA_AUDIO_AUDIO_INPUT_DEVICE_IPC_H_
fix filename + header guards

[tommi (sloooow) - chröme, 2012-07-25 20:10:26]

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
content/renderer/media/audio_device.h:112: const
scoped_refptr<base::MessageLoopProxy>& io_loop);
On 2012/07/25 17:43:37, scherkus wrote:
> indent

Done.

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
File content/renderer/media/audio_input_device.h (right):

http://codereview.chromium.org/10790121/diff/25041/content/renderer/media/aud...
content/renderer/media/audio_input_device.h:148: double sample_rate() const {
return audio_parameters_.sample_rate();   }
On 2012/07/25 17:43:37, scherkus wrote:
> remove extra spacing at end

Done.

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_device_i...
File media/audio/audio_device_ipc.h (right):

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_device_i...
media/audio/audio_device_ipc.h:58: class MEDIA_EXPORT AudioDeviceIPC {
On 2012/07/25 17:43:37, scherkus wrote:
> Shoot. I thought you were going to rename this AudioOutputIPC to mirror
> AudioInputIPC -- I should have articulated that instead of assuming you would
> read my mind from halfway around the world!!!

I must admit that the thought crossed my mind but then I realized that that also
implied changing the name of the file, so I erred on the side of lazyness :)

> 
> It's a purely OCD thing on my part but I think it makes sense to have our
audio
> code state Input/Output in their names instead of optionally leaving out
> "Output" and having that imply that it's an output device
> 
> WDYT?

Absolutely agree!  glad to see it's not just me :) I'll make the change.

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_input_de...
File media/audio/audio_input_device_ipc.h (right):

http://codereview.chromium.org/10790121/diff/25041/media/audio/audio_input_de...
media/audio/audio_input_device_ipc.h:5: #ifndef
MEDIA_AUDIO_AUDIO_INPUT_DEVICE_IPC_H_
On 2012/07/25 17:43:37, scherkus wrote:
> fix filename + header guards

Done.

[scherkus (not reviewing), 2012-07-25 22:34:11]

lgtm++ w/ nits

http://codereview.chromium.org/10790121/diff/32004/content/common/media/audio...
File content/common/media/audio_messages.h (right):

http://codereview.chromium.org/10790121/diff/32004/content/common/media/audio...
content/common/media/audio_messages.h:17: #include
"media/audio/audio_output_ipc.h"
a->z ordering

http://codereview.chromium.org/10790121/diff/32004/content/common/media/audio...
content/common/media/audio_messages.h:25:
IPC_ENUM_TRAITS(media::AudioOutputIPCDelegate::State)
a->z ordering

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
File content/renderer/media/audio_device_factory.h (right):

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:28: static
media::AudioRendererSink* NewOutputDevice();
hrmm.. these names are a bit strange

it's an AudioDeviceFactory that creates AudioRendererSinks?!

I don't have any real concrete suggestions though :\

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:46: private:
indent

[henrika (OOO until Aug 14), 2012-07-26 07:45:47]

Really nice work Tommi.

Today, we have rather detailed comments in the header files for both AudioDevice
and AudioInputDevice. It would be great if you could clean it up since I don't
think all of it is correct now with this new patch. My view is that we should
remove parts of it.

[tommi (sloooow) - chröme, 2012-07-26 09:19:36]

All done.  Also updated the documentation for AudioInputDevice.

http://codereview.chromium.org/10790121/diff/32004/content/common/media/audio...
File content/common/media/audio_messages.h (right):

http://codereview.chromium.org/10790121/diff/32004/content/common/media/audio...
content/common/media/audio_messages.h:17: #include
"media/audio/audio_output_ipc.h"
On 2012/07/25 22:34:11, scherkus wrote:
> a->z ordering

Done.

http://codereview.chromium.org/10790121/diff/32004/content/common/media/audio...
content/common/media/audio_messages.h:25:
IPC_ENUM_TRAITS(media::AudioOutputIPCDelegate::State)
On 2012/07/25 22:34:11, scherkus wrote:
> a->z ordering

Done.

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
File content/renderer/media/audio_device_factory.h (right):

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:28: static
media::AudioRendererSink* NewOutputDevice();
On 2012/07/25 22:34:11, scherkus wrote:
> hrmm.. these names are a bit strange
> 
> it's an AudioDeviceFactory that creates AudioRendererSinks?!
> 
> I don't have any real concrete suggestions though :\

I agree.  I think AudioDeviceFactory is fine but AudioRendererSink could do with
improving.  We could rename that interface to something like AudioOutputClient,
AudioOutputProxy, AudioOutputInterface etc and add a similar definition for
AudioInputDevice.

Alternatively, we could rename the implementations of these interfaces and use
AudioOutputDevice, AudioInputDevice as names for the interfaces and rename the
implementation classes to Audio[Output|Input]DeviceImpl.  Since that pattern is
pretty common in Chrome so maybe that's the way to go.

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:46: private:
On 2012/07/25 22:34:11, scherkus wrote:
> indent

Done.

[scherkus (not reviewing), 2012-07-26 17:57:31]

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
File content/renderer/media/audio_device_factory.h (right):

http://codereview.chromium.org/10790121/diff/32004/content/renderer/media/aud...
content/renderer/media/audio_device_factory.h:28: static
media::AudioRendererSink* NewOutputDevice();
On 2012/07/26 09:19:36, tommi wrote:
> On 2012/07/25 22:34:11, scherkus wrote:
> > hrmm.. these names are a bit strange
> > 
> > it's an AudioDeviceFactory that creates AudioRendererSinks?!
> > 
> > I don't have any real concrete suggestions though :\
> 
> I agree.  I think AudioDeviceFactory is fine but AudioRendererSink could do
with
> improving.  We could rename that interface to something like
AudioOutputClient,
> AudioOutputProxy, AudioOutputInterface etc and add a similar definition for
> AudioInputDevice.
> 
> Alternatively, we could rename the implementations of these interfaces and use
> AudioOutputDevice, AudioInputDevice as names for the interfaces and rename the
> implementation classes to Audio[Output|Input]DeviceImpl.  Since that pattern
is
> pretty common in Chrome so maybe that's the way to go.

I think the *Impl idea has some legs -- the one thing to watch out for (having
committed this crime myself!) is that we don't end up with two Impls in
different namespaces (i.e., one in media:: for testing and one in content::
that's the "real" one)