Added a basic WebRTC peerconnection browser test with video verification.

Added a basic WebRTC peerconnection browser test with video verification.


BUG=


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

[phoglund_chromium, 2012-12-03 14:25:08]

Here's the next test.

jam@, let me know if you want to put someone else in the line of fire for these
CLs. There will probably be more coming :)

https://chromiumcodereview.appspot.com/11428136/diff/1/content/browser/webrtc...
File content/browser/webrtc_getusermedia_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11428136/diff/1/content/browser/webrtc...
content/browser/webrtc_getusermedia_browsertest.cc:49:
IN_PROC_BROWSER_TEST_F(WebrtcGetUserMediaTest, GetVideoStreamAndStop) {
I think this way of organizing the test made a bit more sense. There's a tiny
bit more duplication but the tests are easier to understand.

[jam, 2012-12-03 15:55:54]

(I'll defer to perkj to reviewing the actual media test code, i'm just looking
at the mechanics)

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_getuse...
File content/browser/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_getuse...
content/browser/webrtc_getusermedia_browsertest.cc:29:
ASSERT_TRUE(test_server()->Stop());
test_server's destructor already calls Stop(), so no need to override this
method

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_getuse...
content/browser/webrtc_getusermedia_browsertest.cc:49:
IN_PROC_BROWSER_TEST_F(WebrtcGetUserMediaTest, GetVideoStreamAndStop) {
On 2012/12/03 14:25:08, phoglund wrote:
> I think this way of organizing the test made a bit more sense. There's a tiny
> bit more duplication but the tests are easier to understand.

to me they're both as easy to understand, but if you really like this, then ok

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
File content/browser/webrtc_peerconnection_browsertest.cc (right):

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
content/browser/webrtc_peerconnection_browsertest.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
seems that a bit of code here is duplicated in the other browser_test file. why
not put them in the same test suite file (i.e. webrtc_browsertest.cc), and have
each test calls a method in the base class with the test file location and
javascript to run?

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
content/browser/webrtc_peerconnection_browsertest.cc:15: // Note: Requires
--use-fake-device-for-media-stream (this flag is set by
nit: better than a comment, add a dcheck. that way the test will fail right away
with a clear message if someone removes that flag

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
content/browser/webrtc_peerconnection_browsertest.cc:26: virtual void TearDown()
OVERRIDE {
ditto

[phoglund_chromium, 2012-12-03 17:34:51]

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_getuse...
File content/browser/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_getuse...
content/browser/webrtc_getusermedia_browsertest.cc:29:
ASSERT_TRUE(test_server()->Stop());
On 2012/12/03 15:55:54, John Abd-El-Malek wrote:
> test_server's destructor already calls Stop(), so no need to override this
> method

Done.

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_getuse...
content/browser/webrtc_getusermedia_browsertest.cc:49:
IN_PROC_BROWSER_TEST_F(WebrtcGetUserMediaTest, GetVideoStreamAndStop) {
On 2012/12/03 15:55:54, John Abd-El-Malek wrote:
> On 2012/12/03 14:25:08, phoglund wrote:
> > I think this way of organizing the test made a bit more sense. There's a
tiny
> > bit more duplication but the tests are easier to understand.
> 
> to me they're both as easy to understand, but if you really like this, then ok
Yeah, I think we will go in a direction where we make several javascript calls
in a row soon, so I think this looks about right.

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
File content/browser/webrtc_peerconnection_browsertest.cc (right):

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
content/browser/webrtc_peerconnection_browsertest.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/12/03 15:55:54, John Abd-El-Malek wrote:
> seems that a bit of code here is duplicated in the other browser_test file.
why
> not put them in the same test suite file (i.e. webrtc_browsertest.cc), and
have
> each test calls a method in the base class with the test file location and
> javascript to run?
Good idea, I'll do that. I think I can even merge the tests since the
functionality is related anyway.

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
content/browser/webrtc_peerconnection_browsertest.cc:15: // Note: Requires
--use-fake-device-for-media-stream (this flag is set by
On 2012/12/03 15:55:54, John Abd-El-Malek wrote:
> nit: better than a comment, add a dcheck. that way the test will fail right
away
> with a clear message if someone removes that flag

Done.

https://codereview.chromium.org/11428136/diff/1/content/browser/webrtc_peerco...
content/browser/webrtc_peerconnection_browsertest.cc:26: virtual void TearDown()
OVERRIDE {
On 2012/12/03 15:55:54, John Abd-El-Malek wrote:
> ditto

Done.

[jam, 2012-12-03 21:11:53]

lgtm

[perkj_chrome, 2012-12-04 10:51:43]

https://codereview.chromium.org/11428136/diff/6001/content/browser/webrtc_bro...
File content/browser/webrtc_browsertest.cc (right):

https://codereview.chromium.org/11428136/diff/6001/content/browser/webrtc_bro...
content/browser/webrtc_browsertest.cc:49:
IN_PROC_BROWSER_TEST_F(WebrtcBrowserTest, GetVideoStreamAndStop) {
Why are these tests called stop? you never call stop in the media stream?

Can you add that?

https://codereview.chromium.org/11428136/diff/6001/content/test/data/media/pe...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/11428136/diff/6001/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:11: function call(constraints)
{
Can we instead use the example in
http://dev.w3.org/2011/webrtc/editor/webrtc.html#simple-peer-to-peer-example

https://codereview.chromium.org/11428136/diff/6001/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:100: function
isVideoPlaying(pixels, width, height) {
Why not use your existing video checker?

[phoglund_chromium, 2012-12-04 13:03:27]

Per: PTAL

https://chromiumcodereview.appspot.com/11428136/diff/6001/content/browser/web...
File content/browser/webrtc_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11428136/diff/6001/content/browser/web...
content/browser/webrtc_browsertest.cc:49:
IN_PROC_BROWSER_TEST_F(WebrtcBrowserTest, GetVideoStreamAndStop) {
On 2012/12/04 10:51:43, perkj wrote:
> Why are these tests called stop? you never call stop in the media stream?
> 
> Can you add that?
Yes, we do call stop in getusermedia_and_stop.html. So we request user media,
get it, and then stop the local stream immediately.

https://chromiumcodereview.appspot.com/11428136/diff/6001/content/test/data/m...
File content/test/data/media/peerconnection-call.html (right):

https://chromiumcodereview.appspot.com/11428136/diff/6001/content/test/data/m...
content/test/data/media/peerconnection-call.html:11: function call(constraints)
{
On 2012/12/04 10:51:43, perkj wrote:
> Can we instead use the example in
> http://dev.w3.org/2011/webrtc/editor/webrtc.html#simple-peer-to-peer-example
In that case we would need to run two tabs and implement some out-of-band
signaling mechanism. That would be an interesting test case too, but maybe
that's the next step?

https://chromiumcodereview.appspot.com/11428136/diff/6001/content/test/data/m...
content/test/data/media/peerconnection-call.html:100: function
isVideoPlaying(pixels, width, height) {
On 2012/12/04 10:51:43, perkj wrote:
> Why not use your existing video checker?
Good point. I thought about it, but then I would have to decouple it from the
PyAuto/manual test .js library. That's probably possible to do, but then I will
also have to move the code somewhere where I can depend on it. I cannot depend
on code in chrome/test/data from the content module.

Also, this detector is operating on a much more reliable and known video input,
which means it can be a bit simpler.

To summarize, we could perhaps do it, but in that case I suggest to do it as a
separate CL. I can add a TODO if that's fine.

[perkj_chrome, 2012-12-04 14:30:41]

lgtm after offline discussions.

On 2012/12/04 13:03:27, phoglund wrote:
> Per: PTAL
> 
>
https://chromiumcodereview.appspot.com/11428136/diff/6001/content/browser/web...
> File content/browser/webrtc_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11428136/diff/6001/content/browser/web...
> content/browser/webrtc_browsertest.cc:49:
> IN_PROC_BROWSER_TEST_F(WebrtcBrowserTest, GetVideoStreamAndStop) {
> On 2012/12/04 10:51:43, perkj wrote:
> > Why are these tests called stop? you never call stop in the media stream?
> > 
> > Can you add that?
> Yes, we do call stop in getusermedia_and_stop.html. So we request user media,
> get it, and then stop the local stream immediately.
> 
>
https://chromiumcodereview.appspot.com/11428136/diff/6001/content/test/data/m...
> File content/test/data/media/peerconnection-call.html (right):
> 
>
https://chromiumcodereview.appspot.com/11428136/diff/6001/content/test/data/m...
> content/test/data/media/peerconnection-call.html:11: function
call(constraints)
> {
> On 2012/12/04 10:51:43, perkj wrote:
> > Can we instead use the example in
> > http://dev.w3.org/2011/webrtc/editor/webrtc.html#simple-peer-to-peer-example
> In that case we would need to run two tabs and implement some out-of-band
> signaling mechanism. That would be an interesting test case too, but maybe
> that's the next step?
> 
>
https://chromiumcodereview.appspot.com/11428136/diff/6001/content/test/data/m...
> content/test/data/media/peerconnection-call.html:100: function
> isVideoPlaying(pixels, width, height) {
> On 2012/12/04 10:51:43, perkj wrote:
> > Why not use your existing video checker?
> Good point. I thought about it, but then I would have to decouple it from the
> PyAuto/manual test .js library. That's probably possible to do, but then I
will
> also have to move the code somewhere where I can depend on it. I cannot depend
> on code in chrome/test/data from the content module.
> 
> Also, this detector is operating on a much more reliable and known video
input,
> which means it can be a bit simpler.
> 
> To summarize, we could perhaps do it, but in that case I suggest to do it as a
> separate CL. I can add a TODO if that's fine.

[commit-bot: I haz the power, 2012-12-04 15:20:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/11428136/10002

[commit-bot: I haz the power, 2012-12-04 17:25:56]

Change committed as 170989