Add flag for enabling WebRTC AEC debug recordings.

Add flag for enabling WebRTC AEC debug recordings.

BUG= https://code.google.com/p/webrtc/issues/detail?id=830

Once libjingle has been rolled and sticks a hard coded string shall be changed to a variable, see todo.

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

[Henrik Grunell, 2013-05-13 14:33:01]

Scott: content/browser/*
Joi: content/public/*
Tommi: content/renderer/media/*

[Jói, 2013-05-13 14:45:24]

Looked at the whole change. LGTM with a nit.

https://codereview.chromium.org/14893006/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/14893006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_process_host_impl.cc:933:
switches::kEnableWebRtcAecRecordings,
I think the section above is in mostly alphabetical order; as such this should
be inserted as line 858.

[tommi (sloooow) - chröme, 2013-05-13 14:56:23]

https://codereview.chromium.org/14893006/diff/1/content/public/common/content...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/14893006/diff/1/content/public/common/content...
content/public/common/content_switches.cc:812: const char
kEnableWebRtcAecRecordings[] = "enable-webrtc-aec-recordings";
keep with other webrtc flags?

https://codereview.chromium.org/14893006/diff/1/content/public/common/content...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/14893006/diff/1/content/public/common/content...
content/public/common/content_switches.h:257: extern const char
kEnableWebRtcAecRecordings[];
There are some webrtc related flags at around line ~80.  Should this be there?

https://codereview.chromium.org/14893006/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/14893006/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:473:
enable_aec_recording));
does this depend on a new roll?  if so, can you add that info to the cl
description?

[Henrik Grunell, 2013-05-14 08:39:06]

https://chromiumcodereview.appspot.com/14893006/diff/1/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/14893006/diff/1/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:933:
switches::kEnableWebRtcAecRecordings,
On 2013/05/13 14:45:24, Jói wrote:
> I think the section above is in mostly alphabetical order; as such this should
> be inserted as line 858.

Done.

https://chromiumcodereview.appspot.com/14893006/diff/1/content/public/common/...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/14893006/diff/1/content/public/common/...
content/public/common/content_switches.cc:812: const char
kEnableWebRtcAecRecordings[] = "enable-webrtc-aec-recordings";
On 2013/05/13 14:56:23, tommi wrote:
> keep with other webrtc flags?

sgtm, done.

https://chromiumcodereview.appspot.com/14893006/diff/1/content/public/common/...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/14893006/diff/1/content/public/common/...
content/public/common/content_switches.h:257: extern const char
kEnableWebRtcAecRecordings[];
On 2013/05/13 14:56:23, tommi wrote:
> There are some webrtc related flags at around line ~80.  Should this be there?

sgtm, done.

https://chromiumcodereview.appspot.com/14893006/diff/1/content/renderer/media...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/14893006/diff/1/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:473:
enable_aec_recording));
On 2013/05/13 14:56:23, tommi wrote:
> does this depend on a new roll?  if so, can you add that info to the cl
> description?

Yes it does, added that info.

[tommi (sloooow) - chröme, 2013-05-14 09:53:29]

lgtm with the below fixed.

https://chromiumcodereview.appspot.com/14893006/diff/14001/content/renderer/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/14893006/diff/14001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:468:
command_line.HasSwitch(switches::kEnableWebRtcAecRecordings);
since the constant only exists when ENABLE_WEBRTC is 1, you need that #ifdef
here as well.

[Henrik Grunell, 2013-05-14 10:08:44]

https://chromiumcodereview.appspot.com/14893006/diff/14001/content/renderer/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/14893006/diff/14001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:468:
command_line.HasSwitch(switches::kEnableWebRtcAecRecordings);
On 2013/05/14 09:53:29, tommi wrote:
> since the constant only exists when ENABLE_WEBRTC is 1, you need that #ifdef
> here as well.

This file is only compiled if WebRTC is enabled, i.e. if ENABLE_WEBRTC is
defined.

[tommi (sloooow) - chröme, 2013-05-14 12:10:26]

https://chromiumcodereview.appspot.com/14893006/diff/14001/content/renderer/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/14893006/diff/14001/content/renderer/m...
content/renderer/media/media_stream_dependency_factory.cc:468:
command_line.HasSwitch(switches::kEnableWebRtcAecRecordings);
On 2013/05/14 10:08:44, Henrik Grunell wrote:
> On 2013/05/14 09:53:29, tommi wrote:
> > since the constant only exists when ENABLE_WEBRTC is 1, you need that #ifdef
> > here as well.
> 
> This file is only compiled if WebRTC is enabled, i.e. if ENABLE_WEBRTC is
> defined.

ah, lgtm then :)

[sky, 2013-05-14 16:56:15]

LGTM

[Henrik Grunell, 2013-05-16 11:05:06]

Libjingle interface has changed. Tommi, ptal at
content/renderer/media/media_stream_dependency_factory.cc. The other changes are
rebase only.

[tommi (sloooow) - chröme, 2013-05-16 11:59:35]

lgtm

[Henrik Grunell, 2013-05-22 07:57:53]

Tommi, please review again; new libjingle interface. Only changes to
content/renderer/media*
In media_stream_dependency_factory.cc only the CommandLine section has been
moved and changed. The rest is rebase changes.

Rest of the files are rebase changes only.

[tommi (sloooow) - chröme, 2013-05-22 08:01:57]

lgtm, but you'll have to wait for this one to go in first:
https://chromiumcodereview.appspot.com/15120009/

[Henrik Grunell, 2013-05-22 08:09:03]

Once more Tommi. I'd like not to depend on libjingle roll.

[tommi (sloooow) - chröme, 2013-05-22 08:23:15]

lgtm

[commit-bot: I haz the power, 2013-05-22 08:30:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/14893006/40001

[commit-bot: I haz the power, 2013-05-22 12:52:56]

Change committed as 201499