[Mojo Video Capture] Hook up video capture service behind a feature flag

content/browser/renderer_host/media/video_capture_browsertest.cc

Index: content/browser/renderer_host/media/video_capture_browsertest.cc
diff --git a/content/browser/renderer_host/media/video_capture_browsertest.cc b/content/browser/renderer_host/media/video_capture_browsertest.cc
index 19ce4c692abc2ee6e47fa775240c1da15b0af1df..a4113524a279d695242ca5322656a8ab6678d5e8 100644
--- a/content/browser/renderer_host/media/video_capture_browsertest.cc
+++ b/content/browser/renderer_host/media/video_capture_browsertest.cc
@@ -14,6 +14,7 @@
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
 #include "media/capture/video_capture_types.h"
+#include "services/video_capture/public/cpp/constants.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 using testing::_;
@@ -66,6 +67,7 @@ struct TestParams {
   gfx::Size resolution_to_use;
   float frame_rate_to_use;
   bool exercise_accelerated_jpeg_decoding;
+  bool use_mojo_service;
 };
 
 struct FrameInfo {
@@ -75,6 +77,8 @@ struct FrameInfo {
   base::TimeDelta timestamp;
 };
 
+// Integration test that exercises the VideoCaptureManager instance running in
+// the Browser process.
 class VideoCaptureBrowserTest
     : public ContentBrowserTest,
       public ::testing::WithParamInterface<TestParams> {
@@ -90,7 +94,7 @@ class VideoCaptureBrowserTest
 
   void TearDownCaptureDeviceOnIOThread(base::Closure continuation,
                                        bool post_to_end_of_message_queue) {
-    // StopCaptureForClient must not be called synchronously from either the
+    // DisconnectClient() must not be called synchronously from either the
     // |done_cb| passed to StartCaptureForClient() nor any callback made to a
     // VideoCaptureControllerEventHandler. To satisfy this, we have to post our
     // invocation to the end of the IO message queue.
@@ -125,6 +129,10 @@ class VideoCaptureBrowserTest
       base::CommandLine::ForCurrentProcess()->AppendSwitch(
           switches::kDisableAcceleratedMjpegDecode);
     }
+    if (GetParam().use_mojo_service) {
+      base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+          switches::kEnableFeatures, video_capture::kMojoVideoCapture.name);
+    }
   }
 
   // This cannot be part of an override of SetUp(), because at the time when
@@ -177,6 +185,12 @@ class VideoCaptureBrowserTest
 };
 
 IN_PROC_BROWSER_TEST_P(VideoCaptureBrowserTest, StartAndImmediatelyStop) {
+#if defined(OS_ANDROID)
+  // Mojo video capture is currently not supported on Android

[mcasas, 2017/05/10 02:33:32]

This sounds pretty bad: at least a TODO() and a bug should
be available.  Same in l.216.

[chfremer, 2017/05/10 17:59:39]

Done.

+  if (GetParam().use_mojo_service)
+    return;
+#endif
+
   SetUpRequiringBrowserMainLoopOnMainThread();
   base::RunLoop run_loop;
   auto quit_run_loop_on_current_thread_cb =
@@ -194,11 +208,14 @@ IN_PROC_BROWSER_TEST_P(VideoCaptureBrowserTest, StartAndImmediatelyStop) {
 
 IN_PROC_BROWSER_TEST_P(VideoCaptureBrowserTest,
                        ReceiveFramesFromFakeCaptureDevice) {
-// TODO(chfremer): This test case is flaky on Android. Find out cause of
-// flakiness and then re-enable. See crbug.com/709039.
 #if defined(OS_ANDROID)
+  // TODO(chfremer): This test case is flaky on Android. Find out cause of
+  // flakiness and then re-enable. See crbug.com/709039.
   if (GetParam().exercise_accelerated_jpeg_decoding)
     return;
+  // Mojo video capture is currently not supported on Android
+  if (GetParam().use_mojo_service)
+    return;
 #endif
 
   SetUpRequiringBrowserMainLoopOnMainThread();
@@ -281,16 +298,22 @@ INSTANTIATE_TEST_CASE_P(
     ,
     VideoCaptureBrowserTest,
     Values(TestParams{"fps=25,device-count=2", 0, media::PIXEL_FORMAT_I420,
-                      gfx::Size(1280, 720), 25.0f, false},
+                      gfx::Size(1280, 720), 25.0f, false, false},
+           TestParams{"fps=25,device-count=2", 0, media::PIXEL_FORMAT_I420,
+                      gfx::Size(1280, 720), 25.0f, false, true},
            // The 2nd device outputs Y16
            TestParams{"fps=25,device-count=2", 1, media::PIXEL_FORMAT_Y16,
-                      gfx::Size(1280, 720), 25.0f, false},
+                      gfx::Size(1280, 720), 25.0f, false, false},
+           TestParams{"fps=25,device-count=2", 1, media::PIXEL_FORMAT_Y16,
+                      gfx::Size(1280, 720), 25.0f, false, true},
            TestParams{"fps=15,device-count=2", 1, media::PIXEL_FORMAT_Y16,
-                      gfx::Size(640, 480), 15.0f, false},
+                      gfx::Size(640, 480), 15.0f, false, false},
            // The 3rd device outputs MJPEG, which is converted to I420.
            TestParams{"fps=15,device-count=3", 2, media::PIXEL_FORMAT_I420,
-                      gfx::Size(640, 480), 25.0f, false},
+                      gfx::Size(640, 480), 25.0f, false, false},
+           TestParams{"fps=15,device-count=3", 2, media::PIXEL_FORMAT_I420,
+                      gfx::Size(640, 480), 25.0f, false, true},
            TestParams{"fps=6,device-count=3", 2, media::PIXEL_FORMAT_I420,
-                      gfx::Size(640, 480), 6.0f, true}));
+                      gfx::Size(640, 480), 6.0f, true, false}));

[mcasas, 2017/05/10 02:33:33]

Consider using ::testing::Combine to simplify all this cases

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
(search for "Combine" in the link, plenty of examples in
Cr codebase too).

[chfremer, 2017/05/10 17:59:39]

I prefer to not use ::testing::Combine() here, because we do not need to test
all combinations. 
I agree that this hand-select list is quite verbose, but I think it is worth the
build/bot time savings of not running test cases that are not needed, especially
since these tests are fairly slow (can be > 1s per test case).

[mcasas, 2017/05/10 18:15:43]

Why not?  It should be made explicit here why not,
otherwise Combine() seems to be easier to read, and
the time spent in bots should not be a concern: we want
more testing, not less; and also, needed infra would be 
provisioned only when there's load for it.

[chfremer, 2017/05/10 21:08:05]

Done.

 
 }  // namespace content