Add browser test for AEC dump.

Add browser test for AEC dump.

Also:
- Code changes to avoid race between enable and disable dump. (This is extremely unlikely in practice due to the file picker dialog when enabling, but may happen in tests.)
- Allow enable dump after PeerConnectionFactory creation. This avoids race in tests and also has the benefit of letting a user enable the dump during an ongoing call.

R=jochen@chromium.org, phoglund@chromium.org

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

[Henrik Grunell, 2013-12-18 14:05:01]

Patrik, can you take a first look?

[phoglund_chromium, 2013-12-18 14:20:46]

Test look good in general aside from some minor comments. Don't know about the
production code though: feel free to loop in someone else on that.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_aecdump_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_aecdump_browsertest.cc:26: // Set up file name.
You should be able to use
https://code.google.com/p/chromium/codesearch#chromium/src/base/file_util.h&q...
instead: that way you'll get a path that is guaranteed to be writable and you
won't have to delete anything. Also make the comment a bit more informational,
like "Find a safe place to write the dump during the test".

The extension doesn't matter right?

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_aecdump_browsertest.cc:51: dump_file_);
You should have this too:

    // The video playback will not work without a GPU, so force its use here.
    // This may not be available on all VMs though.
    command_line->AppendSwitch(switches::kUseGpuInTests);

Otherwise video detection will not work reliably. I'm surprised you managed to
get it to work without the gpu flag.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:643:
ASSERT_TRUE(base::GetTempDir(&dump_file));
Again use CreateTemporaryFile here.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:658:
EXPECT_TRUE(base::PathExists(dump_file));
You may as well assert on all of these: further errors will just be confusing.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:673: // As above, but enable and
disable sump before starting a call. The file should
sump?

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:683:
ASSERT_TRUE(base::GetTempDir(&dump_file));
CreateTemporaryFile

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_internals.h:91:
FRIEND_TEST_ALL_PREFIXES(WebrtcBrowserTest, CallWithAecDumpEnabledThenDisabled);
Nit: line length

https://codereview.chromium.org/101063003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1527:
base::Bind(&DisableAecDumpOnFileThread),
So you queue something on the file thread just to make sure it has processed all
previous requests?

[Henrik Grunell, 2013-12-19 08:43:13]

Code review fixes.

Jochen, Patrik has mainly looked at the two tests. Please review the rest.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_aecdump_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_aecdump_browsertest.cc:26: // Set up file name.
On 2013/12/18 14:20:46, phoglund wrote:
> You should be able to use
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/file_util.h&q...
> instead: that way you'll get a path that is guaranteed to be writable and you
> won't have to delete anything.

I suppose you mean delete it here; when test is done it must be deleted.

> Also make the comment a bit more informational,
> like "Find a safe place to write the dump during the test".
> 
> The extension doesn't matter right?

No it doesn't.

Done.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_aecdump_browsertest.cc:51: dump_file_);
On 2013/12/18 14:20:46, phoglund wrote:
> You should have this too:
> 
>     // The video playback will not work without a GPU, so force its use here.
>     // This may not be available on all VMs though.
>     command_line->AppendSwitch(switches::kUseGpuInTests);
> 
> Otherwise video detection will not work reliably. I'm surprised you managed to
> get it to work without the gpu flag.

Done.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:643:
ASSERT_TRUE(base::GetTempDir(&dump_file));
On 2013/12/18 14:20:46, phoglund wrote:
> Again use CreateTemporaryFile here.

Done.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:658:
EXPECT_TRUE(base::PathExists(dump_file));
On 2013/12/18 14:20:46, phoglund wrote:
> You may as well assert on all of these: further errors will just be confusing.

But that won't delete the file, which afaik is not acceptable. Keep this or make
a member var deleted in TearDown is path is non-empty?

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:673: // As above, but enable and
disable sump before starting a call. The file should
On 2013/12/18 14:20:46, phoglund wrote:
> sump?

That's dump's evil twin. But I'll write a test for that some other time.

Done.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:683:
ASSERT_TRUE(base::GetTempDir(&dump_file));
On 2013/12/18 14:20:46, phoglund wrote:
> CreateTemporaryFile

Done.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_internals.h:91:
FRIEND_TEST_ALL_PREFIXES(WebrtcBrowserTest, CallWithAecDumpEnabledThenDisabled);
On 2013/12/18 14:20:46, phoglund wrote:
> Nit: line length

Done.

https://codereview.chromium.org/101063003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1527:
base::Bind(&DisableAecDumpOnFileThread),
On 2013/12/18 14:20:46, phoglund wrote:
> So you queue something on the file thread just to make sure it has processed
all
> previous requests?

Exactly. I don't know if there's another way that's common or better. This will
at least for sure avoid races, though it's kind of weird to just post back and
forth and do nothing on the file thread.

Added comment to clarify.

[phoglund_chromium, 2013-12-19 11:57:04]

tests: lgtm

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_aecdump_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_aecdump_browsertest.cc:26: // Set up file name.
Right, I mean you don't have to delete it here just to make sure.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:658:
EXPECT_TRUE(base::PathExists(dump_file));
Hmm, yeah that's true. Like I said offline you should delete the file but it's
not a huge deal if you don't. You can keep them as EXPECT.

https://codereview.chromium.org/101063003/diff/60001/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:673: // As above, but enable and
disable sump before starting a call. The file should
hehe

https://codereview.chromium.org/101063003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/101063003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1527:
base::Bind(&DisableAecDumpOnFileThread),
I think it kind of makes sense. I can't think of a better way spontanously.

[jochen (gone - plz use gerrit), 2013-12-19 12:41:05]

https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
File content/browser/media/webrtc_aecdump_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
content/browser/media/webrtc_aecdump_browsertest.cc:27:
ASSERT_TRUE(CreateTemporaryFile(&dump_file_));
why not use a scoped temp directory?

https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
content/browser/media/webrtc_aecdump_browsertest.cc:67: };
disallow copy & assign

https://codereview.chromium.org/101063003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/101063003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1530:
base::Bind(&DisableAecDumpOnFileThread),
would it be enough to create an empty base::Closure()?

[Henrik Grunell, 2013-12-19 13:46:09]

https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
File content/browser/media/webrtc_aecdump_browsertest.cc (right):

https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
content/browser/media/webrtc_aecdump_browsertest.cc:27:
ASSERT_TRUE(CreateTemporaryFile(&dump_file_));
On 2013/12/19 12:41:06, jochen wrote:
> why not use a scoped temp directory?

Didn't know about that class. :) Seems like a good fit; changed.

But I don't think it's improving the other tests (in the other file) so I kept
it as is there.

https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
content/browser/media/webrtc_aecdump_browsertest.cc:67: };
On 2013/12/19 12:41:06, jochen wrote:
> disallow copy & assign

Done.

https://codereview.chromium.org/101063003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/101063003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1530:
base::Bind(&DisableAecDumpOnFileThread),
On 2013/12/19 12:41:06, jochen wrote:
> would it be enough to create an empty base::Closure()?

Well, it just crashes for me. Is that supposed to work?

[jochen (gone - plz use gerrit), 2013-12-19 13:48:45]

On 2013/12/19 13:46:09, Henrik Grunell wrote:
>
https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
> File content/browser/media/webrtc_aecdump_browsertest.cc (right):
> 
>
https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
> content/browser/media/webrtc_aecdump_browsertest.cc:27:
> ASSERT_TRUE(CreateTemporaryFile(&dump_file_));
> On 2013/12/19 12:41:06, jochen wrote:
> > why not use a scoped temp directory?
> 
> Didn't know about that class. :) Seems like a good fit; changed.
> 
> But I don't think it's improving the other tests (in the other file) so I kept
> it as is there.
> 
>
https://codereview.chromium.org/101063003/diff/120001/content/browser/media/w...
> content/browser/media/webrtc_aecdump_browsertest.cc:67: };
> On 2013/12/19 12:41:06, jochen wrote:
> > disallow copy & assign
> 
> Done.
> 
>
https://codereview.chromium.org/101063003/diff/120001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/101063003/diff/120001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:1530:
> base::Bind(&DisableAecDumpOnFileThread),
> On 2013/12/19 12:41:06, jochen wrote:
> > would it be enough to create an empty base::Closure()?
> 
> Well, it just crashes for me. Is that supposed to work?

ok

lgtm

[commit-bot: I haz the power, 2013-12-19 14:09:34]

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

[commit-bot: I haz the power, 2013-12-19 15:48:49]

Retried try job too often on win_rel for step(s) mini_installer_test
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Henrik Grunell, 2013-12-20 08:34:01]

Committed patchset #10 manually as r242042 (presubmit successful).