Rewrite WebContentsVideoCaptureDeviceTest.

Rewrite WebContentsVideoCaptureDeviceTest.

The motivation here was to support CopyFromCompositingSurfaceToVideoFrame.

What's new: we use or extend TestWebContents, TestRenderViewHost, and TestRenderWidgetHostView instead of directly overriding the impls. As a result we can use the WebContentsVideoCaptureDevice's regular constructor, and eliminate all the ForTesting setup code. The main test thread doubles as the UI thread. Instead of an event signaling a condition variable, we run the message loop until the event causes posts a Quit task.

BUG=175565, 158641, 159234
TEST=content_unittests,browser_tests
TBR=wjia@chromium.org

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

[ncarter (slow), 2013-02-15 00:38:14]

Yuri, please review.

One thing I'm not clear on, is how to get the destruction behavior
(DeAllocate()) correct for the test. Would appreciate your advice on that.

[miu, 2013-02-20 06:45:10]

You asked about how the test should handle the asynchronous DeAllocate().  The
problem is how to get a single test to block until you know it is finished with
the "UI thread."  That's what the "done_cb" in the "ForTesting()" stuff was
about.

One possible alternative that comes to mind: Instead of setting up the "UI
thread" for each test, use the static gtest methods SetUpTestCase() and
TearDownTestCase() to create the "UI thread" once, globally, to be used for all
tests.

No matter what you try, make sure and run TSAN on your solution!  ;-)

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_capture_util.h (right):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_capture_util.h:14: class
CONTENT_EXPORT WebContentsCaptureUtil {
I'm assuming this is for the unittest linkage?

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(left):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:679:
void InitializeForTesting(RenderWidgetHost* override,
For the tests, how can you be sure CaptureMachine has finished using the "UI
thread" before you destroy it in WCVCDTest::TearDown()?

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:909: //
Note: Thread joins are about to happen here (in ~CaptureThread()).
This comment should be retained (it's not related to the destroy_cb_ stuff).  It
might not be clear that thread joining will happen on the call to delete.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.h
(left):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.h:42: //
design philosophy of an asynchronous DeAllocate().  http://crbug.com/158641
Should 158641 be added to the BUG= line in the description?

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:113:
base::Lock lock_;  // Guards changes to |color_| and |copy_result_size_|
Comment should just say "guards changes to all members" or some such.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:118:
};
nit: Add DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:120:
// A stub implementation which returns solid-color bitmaps in calls to
Comment needs adjusting.  FWICT, this class simulates the GPU-accelerated path.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:136:
//
Unfinished comment.  Looks like this returns true if the accelerated path can be
used?

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:152:
CaptureTestSourceController* controller_;
nit: Type should be "CaptureTestSourceController* const" since this is only set
by the ctor.  And in the other classes below.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:153:
DISALLOW_IMPLICIT_CONSTRUCTORS(CaptureTestView);
nit: Add newline above this line.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:157:
// See comment for same warning on RenderViewHostImpl.
nit: Just so people don't have to go looking it up, maybe add a quick sentence
like "MSVC complier hates our intentional/safe diamond inheritance."

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:387:
message_loop_.RunUntilIdle();
Can you confirm whether this is still needed?  Since we're not creating a
MockRenderProcess host anymore, I think we're avoiding the
destruction-posted-to-message-loop weirdness now.

[ncarter (slow), 2013-02-22 02:16:11]

PTAL.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_capture_util.h (right):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_capture_util.h:14: class
CONTENT_EXPORT WebContentsCaptureUtil {
On 2013/02/20 06:45:10, Yuri wrote:
> I'm assuming this is for the unittest linkage?

Yes.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(left):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:679:
void InitializeForTesting(RenderWidgetHost* override,
On 2013/02/20 06:45:10, Yuri wrote:
> For the tests, how can you be sure CaptureMachine has finished using the "UI
> thread" before you destroy it in WCVCDTest::TearDown()?

This is moot now, but the old code did reliably run DoShutdownTasksOnUIThread()
(it's posted to the UI thread and thus would be there when we pumped the message
queue).  What it didn't do reliably, was to actually join the threads and free
the CaptureMachine and its components before test-case TearDown().

So I've restored the destroy_cb, though it uses a RunLoop now instead of the
ConditionVar.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:909: //
Note: Thread joins are about to happen here (in ~CaptureThread()).
On 2013/02/20 06:45:10, Yuri wrote:
> This comment should be retained (it's not related to the destroy_cb_ stuff). 
It
> might not be clear that thread joining will happen on the call to delete.

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.h
(left):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.h:42: //
design philosophy of an asynchronous DeAllocate().  http://crbug.com/158641
On 2013/02/20 06:45:10, Yuri wrote:
> Should 158641 be added to the BUG= line in the description?

Done. Also, revived the TODO because it looks like the destroy_cb is here to
stay.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
File
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:113:
base::Lock lock_;  // Guards changes to |color_| and |copy_result_size_|
On 2013/02/20 06:45:10, Yuri wrote:
> Comment should just say "guards changes to all members" or some such.

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:118:
};
On 2013/02/20 06:45:10, Yuri wrote:
> nit: Add DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:120:
// A stub implementation which returns solid-color bitmaps in calls to
On 2013/02/20 06:45:10, Yuri wrote:
> Comment needs adjusting.  FWICT, this class simulates the GPU-accelerated
path.

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:136:
//
On 2013/02/20 06:45:10, Yuri wrote:
> Unfinished comment.  Looks like this returns true if the accelerated path can
be
> used?

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:152:
CaptureTestSourceController* controller_;
On 2013/02/20 06:45:10, Yuri wrote:
> nit: Type should be "CaptureTestSourceController* const" since this is only
set
> by the ctor.  And in the other classes below.

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:153:
DISALLOW_IMPLICIT_CONSTRUCTORS(CaptureTestView);
On 2013/02/20 06:45:10, Yuri wrote:
> nit: Add newline above this line.

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:157:
// See comment for same warning on RenderViewHostImpl.
On 2013/02/20 06:45:10, Yuri wrote:
> nit: Just so people don't have to go looking it up, maybe add a quick sentence
> like "MSVC complier hates our intentional/safe diamond inheritance."

Done.

https://codereview.chromium.org/12258042/diff/15001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc:387:
message_loop_.RunUntilIdle();
On 2013/02/20 06:45:10, Yuri wrote:
> Can you confirm whether this is still needed?  Since we're not creating a
> MockRenderProcess host anymore, I think we're avoiding the
> destruction-posted-to-message-loop weirdness now.

Reworked this.

[miu, 2013-02-26 20:22:30]

lgtm

Nice work.

[miu, 2013-02-28 05:32:33]

lgtm

through patchset 12.

[ncarter (slow), 2013-02-28 06:26:06]

TBRing wjia because of one line in video_capture_controller

[commit-bot: I haz the power, 2013-02-28 06:26:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/12258042/48005

[commit-bot: I haz the power, 2013-02-28 08:44:48]

Change committed as 185181