Pass camera facing info to audio client

content/browser/renderer_host/media/audio_input_renderer_host_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/media/audio_input_renderer_host.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "component/arc/video_facing.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/media/capture/audio_mirroring_manager.h"
 #include "content/browser/renderer_host/media/audio_input_device_manager.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/media/media_stream_ui_proxy.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread.h"
@@ skipping 215 matching lines @@
     EXPECT_GT(controller_list_.size(), i);
     return controller_list_[i].get();
   }
 
   MOCK_METHOD0(ControllerCreated, void());
 
  private:
   std::vector<scoped_refptr<MockController>> controller_list_;
 };
 
+class MockVideoCaptureObserver : public component::VideoCaptureObserver {
+ public:
+  MOCK_METHOD1(OnVideoCaptureStarted, void(component::VideoFacingMode));
+  MOCK_METHOD1(OnVideoCaptureStopped, void(component::VideoFacingMode));
+};

[chfremer, 2017/01/20 18:23:52]

I count a total of 9 places where we have unit tests that need this exact same
mock.
It seems awfully redundant to define it 9 times. Would it be possible to move it
to a separate file that can be shared between all the tests that need it?

[shenghao, 2017/01/24 12:52:01]

Done.

+
 }  // namespace
 
 class AudioInputRendererHostTest : public testing::Test {
  public:
   AudioInputRendererHostTest() {
     base::CommandLine* flags = base::CommandLine::ForCurrentProcess();
     flags->AppendSwitch(switches::kUseFakeDeviceForMediaStream);
     flags->AppendSwitch(switches::kUseFakeUIForMediaStream);
 
     audio_manager_.reset(new media::FakeAudioManager(
         base::ThreadTaskRunnerHandle::Get(),
         base::ThreadTaskRunnerHandle::Get(), &log_factory_));
     media_stream_manager_ =
-        base::MakeUnique<MediaStreamManager>(audio_manager_.get());
+        base::MakeUnique<MediaStreamManager>(audio_manager_.get(),
+                                             new MockVideoCaptureObserver());

[chfremer, 2017/01/20 18:23:53]

Since neither MediaStreamManager nor VideoCaptureManager take ownership of this,
this looks like a memory leak (here and in the 8 other tests that do the same).

We seem to have a situation where sometimes we want VideoCaptureManager to take
ownership (e.g. in all the tests), and sometimes we don't, i.e. when passing in
a pointer to a global object.

My recommendation here is to make |capture_observer_| in VideoCaptureManager a
std::unique_ptr<>. Then, to pass in CrasAudioHandler::Get(), we should wrap it
using a class that forwards to a non-owned raw pointer, i.e.:

class NonOwnedVideoCaptureObserver : public VideoCaptureObserver {
 public:
  NonOwnedVideoCaptureObserver(VideoCaptureObserver* observer)
      : observer_(observer) {}

  void OnVideoCaptureStarted(component::VideoFacingMode facing_mode) {
    observer_->OnVideoCaptureStarted(facing_mode);
  }

  void OnVideoCaptureStopped(component::VideoFacingMode facing_mode) {
    observer_->OnVideoCaptureStopped(facing_mode);
  }

 private:
  VideoCaptureObserver* const observer_;
};

[shenghao, 2017/01/24 12:52:00]

Removed all the changes in tests.

     airh_ = new AudioInputRendererHostWithInterception(
         kRenderProcessId, kRendererPid, media::AudioManager::Get(),
         media_stream_manager_.get(), AudioMirroringManager::GetInstance(),
         nullptr, &renderer_);
   }
 
   ~AudioInputRendererHostTest() override {
     airh_->OnChannelClosing();
     base::RunLoop().RunUntilIdle();
   }
@@ skipping 274 matching lines @@
   EXPECT_CALL(controller_factory_, ControllerCreated());
 
   airh_->OnMessageReceived(AudioInputHostMsg_CreateStream(
       kStreamId, kRenderFrameId, session_id, DefaultConfig()));
   base::RunLoop().RunUntilIdle();
 
   EXPECT_CALL(*controller_factory_.controller(0), Close(_));
 }
 
 }  // namespace content