Fix ODR violations in media unit tests.

Fix ODR violations in media unit tests.

Do some cleanup at the same time.

BUG=449754
Committed: https://crrev.com/22a8af1afb44b29cfcee081f492ac1f434b54201
Cr-Commit-Position: refs/heads/master@{#315645}

[Lei Zhang, 2015-02-05 21:53:46]

thestig@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2015-02-09 23:41:04]

mcasas@chromium.org changed reviewers:
+ tommi@chromium.org

[mcasas, 2015-02-09 23:41:05]

LGTM % nits.

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:22: namespace {
I'm not 100% sure making an anonymous namespace
inside media namespace is consistent with 
media/ idioms. I'll defer to tommi@ for this.

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:148: for (const auto&
names_iterator : *names) {
Oh yeah!

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:151:
ASSERT_EQ(supported_formats.size(), 4u);
Shouldn't this still be EXPECT_EQ? Why change it?

[Lei Zhang, 2015-02-10 00:04:16]

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:22: namespace {
On 2015/02/09 23:41:05, mcasas wrote:
> I'm not 100% sure making an anonymous namespace
> inside media namespace is consistent with 
> media/ idioms. I'll defer to tommi@ for this.

Since nothing in this file is intended to be shared with the outside world, I
don't see any downsides other than potentially having two tests with the same
name in two different files. I can keep FakeVideoCaptureDeviceTest in the media
namespace but put MockClient and DeviceEnumerationListener in the
media::anonymous namespace. SG?

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:151:
ASSERT_EQ(supported_formats.size(), 4u);
On 2015/02/09 23:41:05, mcasas wrote:
> Shouldn't this still be EXPECT_EQ? Why change it?

No, it shouldn't. Below you are accessing |supported_formats[n]|, which can
potentially go out of bounds!

With EXPECT_EQ(), if |supported_formats| is too small, you go out of bounds
below, and media_unittests as a whole can crash. Then none of the other test
cases in media_unittests get a chance to run.

With ASSERT_EQ(), you fail the assert and the test case fails. You never go out
of bounds below and the other test cases in media_unittests gets a chance to
run.

[Lei Zhang, 2015-02-10 20:15:23]

New patchsets have been uploaded after l-g-t-m from mcasas@chromium.org

[Lei Zhang, 2015-02-10 20:23:06]

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:22: namespace {
On 2015/02/10 00:04:15, Lei Zhang wrote:
> On 2015/02/09 23:41:05, mcasas wrote:
> > I'm not 100% sure making an anonymous namespace
> > inside media namespace is consistent with 
> > media/ idioms. I'll defer to tommi@ for this.
> 
> Since nothing in this file is intended to be shared with the outside world, I
> don't see any downsides other than potentially having two tests with the same
> name in two different files. I can keep FakeVideoCaptureDeviceTest in the
media
> namespace but put MockClient and DeviceEnumerationListener in the
> media::anonymous namespace. SG?

I've done this in patch set 3. I'll land this CL this afternoon. If you have
strong opinions about namespaces, let me know and I'll be happy to address your
concerns in this CL or in a followup CL.

[mcasas, 2015-02-10 20:38:00]

On 2015/02/10 20:23:06, Lei Zhang wrote:
>
https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
> File media/video/capture/fake_video_capture_device_unittest.cc (right):
> 
>
https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
> media/video/capture/fake_video_capture_device_unittest.cc:22: namespace {
> On 2015/02/10 00:04:15, Lei Zhang wrote:
> > On 2015/02/09 23:41:05, mcasas wrote:
> > > I'm not 100% sure making an anonymous namespace
> > > inside media namespace is consistent with 
> > > media/ idioms. I'll defer to tommi@ for this.
> > 
> > Since nothing in this file is intended to be shared with the outside world,
I
> > don't see any downsides other than potentially having two tests with the
same
> > name in two different files. I can keep FakeVideoCaptureDeviceTest in the
> media
> > namespace but put MockClient and DeviceEnumerationListener in the
> > media::anonymous namespace. SG?
> 
> I've done this in patch set 3. I'll land this CL this afternoon. If you have
> strong opinions about namespaces, let me know and I'll be happy to address
your
> concerns in this CL or in a followup CL.

I think I dug up some old thread on that, and anonymous namespaces inside
media namespace are ok, while anonymous namespaces out of media namespace
are discouraged, due to media/ particularities. So I guess either of
what you have done should be fine.

[mcasas, 2015-02-10 20:38:36]

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
File media/video/capture/fake_video_capture_device_unittest.cc (right):

https://codereview.chromium.org/874883003/diff/1/media/video/capture/fake_vid...
media/video/capture/fake_video_capture_device_unittest.cc:151:
ASSERT_EQ(supported_formats.size(), 4u);
On 2015/02/10 00:04:15, Lei Zhang wrote:
> On 2015/02/09 23:41:05, mcasas wrote:
> > Shouldn't this still be EXPECT_EQ? Why change it?
> 
> No, it shouldn't. Below you are accessing |supported_formats[n]|, which can
> potentially go out of bounds!
> 
> With EXPECT_EQ(), if |supported_formats| is too small, you go out of bounds
> below, and media_unittests as a whole can crash. Then none of the other test
> cases in media_unittests get a chance to run.
> 
> With ASSERT_EQ(), you fail the assert and the test case fails. You never go
out
> of bounds below and the other test cases in media_unittests gets a chance to
> run.

Gotcha, right.

[Lei Zhang, 2015-02-10 21:30:08]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-02-10 21:31:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874883003/40001

[commit-bot: I haz the power, 2015-02-10 21:35:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-10 21:35:43]

Patchset 3 (id:??) landed as
https://crrev.com/22a8af1afb44b29cfcee081f492ac1f434b54201
Cr-Commit-Position: refs/heads/master@{#315645}