Fix ODR violations in media unit tests.

media/video/capture/fake_video_capture_device_unittest.cc

Index: media/video/capture/fake_video_capture_device_unittest.cc
diff --git a/media/video/capture/fake_video_capture_device_unittest.cc b/media/video/capture/fake_video_capture_device_unittest.cc
index 9a9f0464446a59fbca0bda5e89b82acde8761c0d..8adaeffe669078d2d8dfb9e8e46afad71f785ef4 100644
--- a/media/video/capture/fake_video_capture_device_unittest.cc
+++ b/media/video/capture/fake_video_capture_device_unittest.cc
@@ -19,33 +19,34 @@ using ::testing::SaveArg;
 
 namespace media {
 
-class MockClient : public media::VideoCaptureDevice::Client {
+namespace {

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

I'm not 100% sure making an anonymous namespace
inside media namespace is consistent with 
media/ idioms. I'll defer to tommi@ for this.

[Lei Zhang, 2015/02/10 00:04:15]

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?

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

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.

+
+class MockClient : public VideoCaptureDevice::Client {
  public:
   MOCK_METHOD2(ReserveOutputBuffer,
-               scoped_refptr<Buffer>(media::VideoFrame::Format format,
+               scoped_refptr<Buffer>(VideoFrame::Format format,
                                      const gfx::Size& dimensions));
   MOCK_METHOD0(OnErr, void());
 
   explicit MockClient(base::Callback<void(const VideoCaptureFormat&)> frame_cb)
       : main_thread_(base::MessageLoopProxy::current()), frame_cb_(frame_cb) {}
 
-  virtual void OnError(const std::string& error_message) override {
+  void OnError(const std::string& error_message) override {
     OnErr();
   }
 
-  virtual void OnIncomingCapturedData(const uint8* data,
-                                      int length,
-                                      const VideoCaptureFormat& format,
-                                      int rotation,
-                                      base::TimeTicks timestamp) override {
+  void OnIncomingCapturedData(const uint8* data,
+                              int length,
+                              const VideoCaptureFormat& format,
+                              int rotation,
+                              base::TimeTicks timestamp) override {
     main_thread_->PostTask(FROM_HERE, base::Bind(frame_cb_, format));
   }
 
-  virtual void OnIncomingCapturedVideoFrame(
-      const scoped_refptr<Buffer>& buffer,
-      const media::VideoCaptureFormat& buffer_format,
-      const scoped_refptr<media::VideoFrame>& frame,
-      base::TimeTicks timestamp) override {
+  void OnIncomingCapturedVideoFrame(const scoped_refptr<Buffer>& buffer,
+                                    const VideoCaptureFormat& buffer_format,
+                                    const scoped_refptr<VideoFrame>& frame,
+                                    base::TimeTicks timestamp) override {
     NOTREACHED();
   }
 
@@ -58,10 +59,10 @@ class DeviceEnumerationListener :
     public base::RefCounted<DeviceEnumerationListener> {
  public:
   MOCK_METHOD1(OnEnumeratedDevicesCallbackPtr,
-               void(media::VideoCaptureDevice::Names* names));
+               void(VideoCaptureDevice::Names* names));
   // GMock doesn't support move-only arguments, so we use this forward method.
   void OnEnumeratedDevicesCallback(
-      scoped_ptr<media::VideoCaptureDevice::Names> names) {
+      scoped_ptr<VideoCaptureDevice::Names> names) {
     OnEnumeratedDevicesCallbackPtr(names.release());
   }
 
@@ -72,7 +73,7 @@ class DeviceEnumerationListener :
 
 class FakeVideoCaptureDeviceTest : public testing::Test {
  protected:
-  typedef media::VideoCaptureDevice::Client Client;
+  typedef VideoCaptureDevice::Client Client;
 
   FakeVideoCaptureDeviceTest()
       : loop_(new base::MessageLoop()),
@@ -83,8 +84,6 @@ class FakeVideoCaptureDeviceTest : public testing::Test {
     device_enumeration_listener_ = new DeviceEnumerationListener();
   }
 
-  void SetUp() override {}
-
   void OnFrameCaptured(const VideoCaptureFormat& format) {
     last_format_ = format;
     run_loop_->QuitClosure().Run();
@@ -95,8 +94,8 @@ class FakeVideoCaptureDeviceTest : public testing::Test {
     run_loop_->Run();
   }
 
-  scoped_ptr<media::VideoCaptureDevice::Names> EnumerateDevices() {
-    media::VideoCaptureDevice::Names* names;
+  scoped_ptr<VideoCaptureDevice::Names> EnumerateDevices() {
+    VideoCaptureDevice::Names* names;
     EXPECT_CALL(*device_enumeration_listener_.get(),
                 OnEnumeratedDevicesCallbackPtr(_)).WillOnce(SaveArg<0>(&names));
 
@@ -104,7 +103,7 @@ class FakeVideoCaptureDeviceTest : public testing::Test {
         base::Bind(&DeviceEnumerationListener::OnEnumeratedDevicesCallback,
                    device_enumeration_listener_));
     base::MessageLoop::current()->RunUntilIdle();
-    return scoped_ptr<media::VideoCaptureDevice::Names>(names);
+    return scoped_ptr<VideoCaptureDevice::Names>(names);
   }
 
   const VideoCaptureFormat& last_format() const { return last_format_; }
@@ -119,9 +118,9 @@ class FakeVideoCaptureDeviceTest : public testing::Test {
 };
 
 TEST_F(FakeVideoCaptureDeviceTest, Capture) {
-  scoped_ptr<media::VideoCaptureDevice::Names> names(EnumerateDevices());
+  scoped_ptr<VideoCaptureDevice::Names> names(EnumerateDevices());
 
-  ASSERT_GT(static_cast<int>(names->size()), 0);
+  ASSERT_FALSE(names->empty());
 
   scoped_ptr<VideoCaptureDevice> device(
       video_capture_device_factory_->Create(names->front()));
@@ -145,28 +144,26 @@ TEST_F(FakeVideoCaptureDeviceTest, GetDeviceSupportedFormats) {
   scoped_ptr<VideoCaptureDevice::Names> names(EnumerateDevices());
 
   VideoCaptureFormats supported_formats;
-  VideoCaptureDevice::Names::iterator names_iterator;
 
-  for (names_iterator = names->begin(); names_iterator != names->end();
-       ++names_iterator) {
+  for (const auto& names_iterator : *names) {

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

Oh yeah!

     video_capture_device_factory_->GetDeviceSupportedFormats(
-        *names_iterator, &supported_formats);
-    EXPECT_EQ(supported_formats.size(), 4u);
+        names_iterator, &supported_formats);
+    ASSERT_EQ(supported_formats.size(), 4u);

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

Shouldn't this still be EXPECT_EQ? Why change it?

[Lei Zhang, 2015/02/10 00:04:15]

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.

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

Gotcha, right.

     EXPECT_EQ(supported_formats[0].frame_size.width(), 320);
     EXPECT_EQ(supported_formats[0].frame_size.height(), 240);
-    EXPECT_EQ(supported_formats[0].pixel_format, media::PIXEL_FORMAT_I420);
+    EXPECT_EQ(supported_formats[0].pixel_format, PIXEL_FORMAT_I420);
     EXPECT_GE(supported_formats[0].frame_rate, 20);
     EXPECT_EQ(supported_formats[1].frame_size.width(), 640);
     EXPECT_EQ(supported_formats[1].frame_size.height(), 480);
-    EXPECT_EQ(supported_formats[1].pixel_format, media::PIXEL_FORMAT_I420);
+    EXPECT_EQ(supported_formats[1].pixel_format, PIXEL_FORMAT_I420);
     EXPECT_GE(supported_formats[1].frame_rate, 20);
     EXPECT_EQ(supported_formats[2].frame_size.width(), 1280);
     EXPECT_EQ(supported_formats[2].frame_size.height(), 720);
-    EXPECT_EQ(supported_formats[2].pixel_format, media::PIXEL_FORMAT_I420);
+    EXPECT_EQ(supported_formats[2].pixel_format, PIXEL_FORMAT_I420);
     EXPECT_GE(supported_formats[2].frame_rate, 20);
     EXPECT_EQ(supported_formats[3].frame_size.width(), 1920);
     EXPECT_EQ(supported_formats[3].frame_size.height(), 1080);
-    EXPECT_EQ(supported_formats[3].pixel_format, media::PIXEL_FORMAT_I420);
+    EXPECT_EQ(supported_formats[3].pixel_format, PIXEL_FORMAT_I420);
     EXPECT_GE(supported_formats[3].frame_rate, 20);
   }
 }
@@ -182,7 +179,7 @@ TEST_F(FakeVideoCaptureDeviceTest, DISABLED_CaptureVariableResolution) {
   capture_params.resolution_change_policy =
       RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT;
 
-  ASSERT_GT(static_cast<int>(names->size()), 0);
+  ASSERT_FALSE(names->empty());
 
   scoped_ptr<VideoCaptureDevice> device(
       video_capture_device_factory_->Create(names->front()));
@@ -195,8 +192,7 @@ TEST_F(FakeVideoCaptureDeviceTest, DISABLED_CaptureVariableResolution) {
   static_cast<FakeVideoCaptureDevice*>(device.get())->
       PopulateVariableFormatsRoster(formats);
 
-  EXPECT_CALL(*client_, OnErr())
-      .Times(0);
+  EXPECT_CALL(*client_, OnErr()).Times(0);
   int action_count = 200;
 
   device->AllocateAndStart(capture_params, client_.Pass());
@@ -209,4 +205,6 @@ TEST_F(FakeVideoCaptureDeviceTest, DISABLED_CaptureVariableResolution) {
   device->StopAndDeAllocate();
 }
 
+}  // namespace
+
 };  // namespace media