Split out audio debug recording from RenderProcessHostImpl.

This removes almost all code related to webrtc's audio debug recording from RenderProcessHostImpl. Splitting the code out makes RenderProcessHostImpl smaller and more readable. 

At the same time, switch the IPC code to use Mojo instead of message filters at each end. Mojo presents a function-call-style, instead of handling and routing individual messages.

BUG=542828

[Anand Mistry (off Chromium), 2015-10-13 03:12:38]

This removes almost all code related to webrtc's audio debug recording from
RenderProcessHostImpl. Splitting the code out makes RenderProcessHostImpl
smaller and more readable. 

At the same time, switch the IPC code to use Mojo instead of message filters at
each end. Mojo presents a function-call-style instead of handling and routing
individual messages.

[Anand Mistry (off Chromium), 2015-10-13 06:55:57]

amistry@chromium.org changed reviewers:
+ grunell@chromium.org, rockot@chromium.org, xhwang@chromium.org

[Anand Mistry (off Chromium), 2015-10-13 06:55:58]

{xhwang,grunell}@chromium.org: For all things media.
rockot: For Mojoyness.

Hey all. I brought this up a few months ago, but never got around to cleaning up
my CL and sending it out. Well, now I have (mostly). Hopefully it's acceptable,
but if you're thinking "WTF is this clown doing", let me know.

This CL can be thought of as doing two things:
1. Splitting out audio recording out of RenderProcessHostImpl. This is the
primary goal. RPHL is big and hairy and really needs to go on a diet. This was
the lowest hanging fruit that would have given the largest result.
2. Mojo! Mojo is great. Can't you tell by how clear and readable the result is?

[Henrik Grunell, 2015-10-13 08:16:14]

On 2015/10/13 06:55:58, Anand Mistry wrote:
> mailto:{xhwang,grunell}@chromium.org: For all things media.
> rockot: For Mojoyness.
> 
> Hey all. I brought this up a few months ago, but never got around to cleaning
up
> my CL and sending it out. Well, now I have (mostly). Hopefully it's
acceptable,
> but if you're thinking "WTF is this clown doing", let me know.
> 
> This CL can be thought of as doing two things:
> 1. Splitting out audio recording out of RenderProcessHostImpl. This is the
> primary goal. RPHL is big and hairy and really needs to go on a diet. This was
> the lowest hanging fruit that would have given the largest result.
> 2. Mojo! Mojo is great. Can't you tell by how clear and readable the result
is?

Great that you do this!

Could this be split in several CLs? At least (1) breakout from RPHI, (2) switch
to mojo, (3) rename message filter in renderer.

[Anand Mistry (off Chromium), 2015-10-13 08:42:55]

On 2015/10/13 08:16:14, Henrik Grunell wrote:
> On 2015/10/13 06:55:58, Anand Mistry wrote:
> > mailto:{xhwang,grunell}@chromium.org: For all things media.
> > rockot: For Mojoyness.
> > 
> > Hey all. I brought this up a few months ago, but never got around to
cleaning
> up
> > my CL and sending it out. Well, now I have (mostly). Hopefully it's
> acceptable,
> > but if you're thinking "WTF is this clown doing", let me know.
> > 
> > This CL can be thought of as doing two things:
> > 1. Splitting out audio recording out of RenderProcessHostImpl. This is the
> > primary goal. RPHL is big and hairy and really needs to go on a diet. This
was
> > the lowest hanging fruit that would have given the largest result.
> > 2. Mojo! Mojo is great. Can't you tell by how clear and readable the result
> is?
> 
> Great that you do this!
> 
> Could this be split in several CLs? At least (1) breakout from RPHI, (2)
switch
> to mojo, (3) rename message filter in renderer.

I'll see what I can do. But using Mojo is very much a core part of this change.
It's what makes it easy to decouple everything from RenderProcessHostImpl and
doing it in two steps is a large amount of work.

[Henrik Grunell, 2015-10-14 09:01:16]

On 2015/10/13 08:42:55, Anand Mistry wrote:
> On 2015/10/13 08:16:14, Henrik Grunell wrote:
> > On 2015/10/13 06:55:58, Anand Mistry wrote:
> > > mailto:{xhwang,grunell}@chromium.org: For all things media.
> > > rockot: For Mojoyness.
> > > 
> > > Hey all. I brought this up a few months ago, but never got around to
> cleaning
> > up
> > > my CL and sending it out. Well, now I have (mostly). Hopefully it's
> > acceptable,
> > > but if you're thinking "WTF is this clown doing", let me know.
> > > 
> > > This CL can be thought of as doing two things:
> > > 1. Splitting out audio recording out of RenderProcessHostImpl. This is the
> > > primary goal. RPHL is big and hairy and really needs to go on a diet. This
> was
> > > the lowest hanging fruit that would have given the largest result.
> > > 2. Mojo! Mojo is great. Can't you tell by how clear and readable the
result
> > is?
> > 
> > Great that you do this!
> > 
> > Could this be split in several CLs? At least (1) breakout from RPHI, (2)
> switch
> > to mojo, (3) rename message filter in renderer.
> 
> I'll see what I can do. But using Mojo is very much a core part of this
change.
> It's what makes it easy to decouple everything from RenderProcessHostImpl and
> doing it in two steps is a large amount of work.

OK, if doing it all in one CL makes more sense, I'm fine with that. Let me know.

[Anand Mistry (off Chromium), 2015-10-15 03:59:42]

On 2015/10/14 09:01:16, Henrik Grunell wrote:
> On 2015/10/13 08:42:55, Anand Mistry wrote:
> > On 2015/10/13 08:16:14, Henrik Grunell wrote:
> > > On 2015/10/13 06:55:58, Anand Mistry wrote:
> > > > mailto:{xhwang,grunell}@chromium.org: For all things media.
> > > > rockot: For Mojoyness.
> > > > 
> > > > Hey all. I brought this up a few months ago, but never got around to
> > cleaning
> > > up
> > > > my CL and sending it out. Well, now I have (mostly). Hopefully it's
> > > acceptable,
> > > > but if you're thinking "WTF is this clown doing", let me know.
> > > > 
> > > > This CL can be thought of as doing two things:
> > > > 1. Splitting out audio recording out of RenderProcessHostImpl. This is
the
> > > > primary goal. RPHL is big and hairy and really needs to go on a diet.
This
> > was
> > > > the lowest hanging fruit that would have given the largest result.
> > > > 2. Mojo! Mojo is great. Can't you tell by how clear and readable the
> result
> > > is?
> > > 
> > > Great that you do this!
> > > 
> > > Could this be split in several CLs? At least (1) breakout from RPHI, (2)
> > switch
> > > to mojo, (3) rename message filter in renderer.
> > 
> > I'll see what I can do. But using Mojo is very much a core part of this
> change.
> > It's what makes it easy to decouple everything from RenderProcessHostImpl
and
> > doing it in two steps is a large amount of work.
> 
> OK, if doing it all in one CL makes more sense, I'm fine with that. Let me
know.

Yeah, I've taken another look. This needs to be looked at as a whole. But I'll
keep this in mind for future changes, so they're less invasive.

[Henrik Grunell, 2015-10-15 12:17:03]

I took a first shot at this. I haven't looked at the mojo details, defer to
other reviewers. Besides that change it's mostly code moving, so I've looked at
that. Looks good in general. Mostly comments on naming and commenting the code.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
File content/browser/media/audio_debug_controller.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_controller.h:15: template <typename T>
Nit: Empty line before.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_controller.h:19: }
// namespace base
Nit: Empty line before.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_controller.h:26: class AudioDebugController {
Add comment with a description of the class. On which thread does it live?

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:25: const
base::FilePath::CharType kAudioDebugRecordingFileNameAddition[] =
kAecDumpFileNameAddition

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:30: mojo::ScopedHandle
CreateAudioDebugRecordingFile(base::FilePath file_path) {
CreateAecDumpFile()

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:54: void
DisableAudioDebugRecordingOnFileThread() {
Would be great if you could remove this and replace with base::DoNothing().

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:105:
AudioDebugRecordingImpl* dump_impl_;
Rename to audio_debug_recording_impl_

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:119: void
AudioDebugRecordingImpl::DumpObserver::Enable(const base::FilePath& file) {
You might as well put the definition with the declaration as done with the other
functions.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:164: for (auto& observer :
observers_)
Add comment that this enables AEC dump.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:24: class
AudioDebugRecordingImpl : public AudioDebugRecording {
Comment with class description and threading story. Will there be one object per
render process host?

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:35: // Enables/disables AEC
dumping.
AEC dumping -> audio debug recordings.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:32: #include
"base/strings/string_util.h"
I guess you added this for something else already in the code?

https://codereview.chromium.org/1246283003/diff/140001/content/common/media/a...
File content/common/media/audio_debug_recording.mojom (right):

https://codereview.chromium.org/1246283003/diff/140001/content/common/media/a...
content/common/media/audio_debug_recording.mojom:9: Enable(handle file_handle);
Today we have two different recordings. This specifically enables AEC dump. The
other kind (mic input) is in the browser process. For extension simplicity I
think we can keep the generic naming AudioDebugRecording here and for the
classes. But let the handle naming reflect that it's an AEC dump file, e.g.
"aec_dump_file_handle".

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/DEPS
File content/renderer/DEPS (right):

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/DEPS#...
content/renderer/DEPS:32: 'render_thread_impl_browsertest.cc': [
Why is this needed?

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/media...
File content/renderer/media/audio_debug_recorder.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/media...
content/renderer/media/audio_debug_recorder.h:69: using ObserverMap =
base::ScopedPtrMap<int, scoped_ptr<Observer>>;
What are the observers for? Comment would be good.

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.h:532: // Used for communicating registering
AEC dump consumers with the browser and
Update comment. "AEC dump" -> "audio debug recording" for consistency.

[Henrik Grunell, 2015-10-15 12:20:16]

Oh, and unit tests would be extremely appreciated. :D (At least it would be
great to make the new classes testable.)

[Anand Mistry (off Chromium), 2015-10-21 03:58:24]

I added a couple of unit tests. I _didn't_ add a test for AudioDebugController
because the class is essentially a map and doesn't have any non-trivial logic,
and making it testable would be a pain. I could, but I'd rather not.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
File content/browser/media/audio_debug_controller.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_controller.h:15: template <typename T>
On 2015/10/15 12:17:02, Henrik Grunell wrote:
> Nit: Empty line before.

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_controller.h:19: }
On 2015/10/15 12:17:02, Henrik Grunell wrote:
> // namespace base
> Nit: Empty line before.

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_controller.h:26: class AudioDebugController {
On 2015/10/15 12:17:02, Henrik Grunell wrote:
> Add comment with a description of the class. On which thread does it live? 

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:25: const
base::FilePath::CharType kAudioDebugRecordingFileNameAddition[] =
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> kAecDumpFileNameAddition

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:30: mojo::ScopedHandle
CreateAudioDebugRecordingFile(base::FilePath file_path) {
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> CreateAecDumpFile()

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:54: void
DisableAudioDebugRecordingOnFileThread() {
On 2015/10/15 12:17:02, Henrik Grunell wrote:
> Would be great if you could remove this and replace with base::DoNothing().

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:105:
AudioDebugRecordingImpl* dump_impl_;
On 2015/10/15 12:17:02, Henrik Grunell wrote:
> Rename to audio_debug_recording_impl_

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:119: void
AudioDebugRecordingImpl::DumpObserver::Enable(const base::FilePath& file) {
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> You might as well put the definition with the declaration as done with the
other
> functions.

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:164: for (auto& observer :
observers_)
On 2015/10/15 12:17:02, Henrik Grunell wrote:
> Add comment that this enables AEC dump.

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:24: class
AudioDebugRecordingImpl : public AudioDebugRecording {
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> Comment with class description and threading story. Will there be one object
per
> render process host?

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:35: // Enables/disables AEC
dumping.
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> AEC dumping -> audio debug recordings.

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/140001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:32: #include
"base/strings/string_util.h"
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> I guess you added this for something else already in the code?

Hmmm. It compiles without it. I guess it was needed in a previous version.
Removed.

https://codereview.chromium.org/1246283003/diff/140001/content/common/media/a...
File content/common/media/audio_debug_recording.mojom (right):

https://codereview.chromium.org/1246283003/diff/140001/content/common/media/a...
content/common/media/audio_debug_recording.mojom:9: Enable(handle file_handle);
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> Today we have two different recordings. This specifically enables AEC dump.
The
> other kind (mic input) is in the browser process. For extension simplicity I
> think we can keep the generic naming AudioDebugRecording here and for the
> classes. But let the handle naming reflect that it's an AEC dump file, e.g.
> "aec_dump_file_handle".

Done.

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/DEPS
File content/renderer/DEPS (right):

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/DEPS#...
content/renderer/DEPS:32: 'render_thread_impl_browsertest.cc': [
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> Why is this needed?

This specific test needs to initialise Mojo. Normally, Mojo is initialised by
the browsertest test runner. But this specific test is more like a unit test,
and Mojo doesn't get initialised.

I don't like this either.

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/media...
File content/renderer/media/audio_debug_recorder.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/media...
content/renderer/media/audio_debug_recorder.h:69: using ObserverMap =
base::ScopedPtrMap<int, scoped_ptr<Observer>>;
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> What are the observers for? Comment would be good.

Done. Updated a bunch of comments.

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/rende...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/1246283003/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.h:532: // Used for communicating registering
AEC dump consumers with the browser and
On 2015/10/15 12:17:03, Henrik Grunell wrote:
> Update comment. "AEC dump" -> "audio debug recording" for consistency.

Done.

[Henrik Grunell, 2015-10-21 14:08:59]

Thanks for adding the tests.

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:163: if
(audio_input_renderer_host_) {
Inject a mock AIRH instead of null. And check the expected calls on it.

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.h (right):

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:49: // Returns the number of
registered observers. For tests only.
Make the test(s) friend and access the member directly.

[Anand Mistry (off Chromium), 2015-10-26 06:17:49]

I'm looking into the windows build failures. Please continue reviewing while I
try to fix it.

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:163: if
(audio_input_renderer_host_) {
On 2015/10/21 14:08:59, Henrik Grunell wrote:
> Inject a mock AIRH instead of null. And check the expected calls on it.

Done.

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.h (right):

https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:49: // Returns the number of
registered observers. For tests only.
On 2015/10/21 14:08:59, Henrik Grunell wrote:
> Make the test(s) friend and access the member directly.

Done.

[Henrik Grunell, 2015-10-28 14:35:55]

On 2015/10/26 06:17:49, Anand Mistry wrote:
> I'm looking into the windows build failures. Please continue reviewing while I
> try to fix it.
> 
>
https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
> File content/browser/media/audio_debug_recording_impl.cc (right):
> 
>
https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
> content/browser/media/audio_debug_recording_impl.cc:163: if
> (audio_input_renderer_host_) {
> On 2015/10/21 14:08:59, Henrik Grunell wrote:
> > Inject a mock AIRH instead of null. And check the expected calls on it.
> 
> Done.
> 
>
https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
> File content/browser/media/audio_debug_recording_impl.h (right):
> 
>
https://codereview.chromium.org/1246283003/diff/190034/content/browser/media/...
> content/browser/media/audio_debug_recording_impl.h:49: // Returns the number
of
> registered observers. For tests only.
> On 2015/10/21 14:08:59, Henrik Grunell wrote:
> > Make the test(s) friend and access the member directly.
> 
> Done.

I think it looks good. Ping me when the build errors have been fixed.

Did you test this manually yet?

[Anand Mistry (off Chromium), 2015-10-30 00:35:07]

Windows build is fixed :D

Yes, I've tested it manually, and it appears to work.

[Henrik Grunell, 2015-10-30 13:20:43]

https://codereview.chromium.org/1246283003/diff/270001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.h:112: virtual
void EnableDebugRecording(const base::FilePath& file);
Why this change?

[Anand Mistry (off Chromium), 2015-11-02 23:46:46]

https://codereview.chromium.org/1246283003/diff/270001/content/browser/render...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/render...
content/browser/renderer_host/media/audio_input_renderer_host.h:112: virtual
void EnableDebugRecording(const base::FilePath& file);
On 2015/10/30 13:20:43, Henrik Grunell wrote:
> Why this change?

So that I could mock out the function.

[Henrik Grunell, 2015-11-03 07:33:27]

lgtm

[Ken Rockot(use gerrit already), 2015-11-09 19:05:44]

lgtm

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
File content/browser/media/audio_debug_controller.h (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
content/browser/media/audio_debug_controller.h:60:
std::set<AudioDebugRecordingImpl*> registered_impls_;
Just a thought: could you use a base::ScopedPtrHashMap instead, keyed on RHID?
Pro: more explicit about ownership; con: kind of superfluous and uses more
memory. I could go either way. Probably not worth adding a ScopedPtrSet, in any
case :)

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:132: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.h (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:75: std::set<DumpObserver*>
observers_;
Same thought here: ScopedPtrHashMap keyed on observer ID?

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
File content/renderer/media/audio_debug_recorder.cc (right):

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
content/renderer/media/audio_debug_recorder.cc:20: AudioDebugRecorder*
AudioDebugRecorder::g_filter = NULL;
nit (and maybe elsewhere): nullptr?

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
content/renderer/media/audio_debug_recorder.cc:38:
AudioDebugRecorder::Observer::Observer(
nit: Just inline all the method definitions on Observer?

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
content/renderer/media/media_stream_audio_processor.cc:331:
audio_debug_recorder_ = NULL;
nit: Not sure if we ever settled on a policy for this, but wanna update these
NULLs to nullptr while here?

[Anand Mistry (off Chromium), 2015-11-16 05:02:57]

amistry@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Anand Mistry (off Chromium), 2015-11-16 05:02:58]

xhwang: Ping! For content/*/media OWNERS.

pfeldman@chromium.org: Please review changes for non-media content OWNERS

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
File content/browser/media/audio_debug_controller.h (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
content/browser/media/audio_debug_controller.h:60:
std::set<AudioDebugRecordingImpl*> registered_impls_;
On 2015/11/09 19:05:43, Ken Rockot wrote:
> Just a thought: could you use a base::ScopedPtrHashMap instead, keyed on RHID?
> Pro: more explicit about ownership; con: kind of superfluous and uses more
> memory. I could go either way. Probably not worth adding a ScopedPtrSet, in
any
> case :)

I'd rather not. The reason RHID is passed is so that it can be forwarded to the
impl which uses it to generate file names. Otherwise, it's unnecessary, and I'd
prefer to avoid any further tieing. Also, this is logically a set, and I think
it should stay that way.

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.cc (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.cc:132: };
On 2015/11/09 19:05:43, Ken Rockot wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
File content/browser/media/audio_debug_recording_impl.h (right):

https://codereview.chromium.org/1246283003/diff/270001/content/browser/media/...
content/browser/media/audio_debug_recording_impl.h:75: std::set<DumpObserver*>
observers_;
On 2015/11/09 19:05:44, Ken Rockot wrote:
> Same thought here: ScopedPtrHashMap keyed on observer ID?

Changed to std::map now that we can use stl containers with move-only types.

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
File content/renderer/media/audio_debug_recorder.cc (right):

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
content/renderer/media/audio_debug_recorder.cc:20: AudioDebugRecorder*
AudioDebugRecorder::g_filter = NULL;
On 2015/11/09 19:05:44, Ken Rockot wrote:
> nit (and maybe elsewhere): nullptr?

Done.

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
content/renderer/media/audio_debug_recorder.cc:38:
AudioDebugRecorder::Observer::Observer(
On 2015/11/09 19:05:44, Ken Rockot wrote:
> nit: Just inline all the method definitions on Observer?

Done.

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/1246283003/diff/270001/content/renderer/media...
content/renderer/media/media_stream_audio_processor.cc:331:
audio_debug_recorder_ = NULL;
On 2015/11/09 19:05:44, Ken Rockot wrote:
> nit: Not sure if we ever settled on a policy for this, but wanna update these
> NULLs to nullptr while here?

Done.

[xhwang, 2015-11-16 19:56:40]

Sorry for the delay. However I am not familiar with webrtc code. Please replace
me with a webrtc media owner in the OWNERS list:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

[Henrik Grunell, 2016-06-08 12:11:06]

Will this CL be landed or can it be closed?

[xhwang, 2016-06-09 06:34:36]

xhwang@chromium.org changed reviewers:
- xhwang@chromium.org

[Anand Mistry (off Chromium), 2016-06-21 01:02:04]

On 2016/06/08 12:11:06, Henrik Grunell wrote:
> Will this CL be landed or can it be closed?

Oops. Sorry I dropped this on the floor. I would like to submit it, but it needs
a media owner to approve. Also, I need to sync/rebase.

[Henrik Grunell, 2016-06-23 14:00:19]

On 2016/06/21 01:02:04, Anand Mistry wrote:
> On 2016/06/08 12:11:06, Henrik Grunell wrote:
> > Will this CL be landed or can it be closed?
> 
> Oops. Sorry I dropped this on the floor. I would like to submit it, but it
needs
> a media owner to approve. Also, I need to sync/rebase.

After rebasing, tommi@ should be a suitable media reviewer.

[Anand Mistry (off Chromium), 2016-07-21 00:59:06]

On 2016/06/23 14:00:19, Henrik Grunell (OOO to Aug 15) wrote:
> On 2016/06/21 01:02:04, Anand Mistry wrote:
> > On 2016/06/08 12:11:06, Henrik Grunell wrote:
> > > Will this CL be landed or can it be closed?
> > 
> > Oops. Sorry I dropped this on the floor. I would like to submit it, but it
> needs
> > a media owner to approve. Also, I need to sync/rebase.
> 
> After rebasing, tommi@ should be a suitable media reviewer.

Unfortunately, I'm leaving google and won't be landing this. I've been busy on
other things and this just got left behind. Sorry.

[pfeldman, 2016-07-22 18:03:20]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org