Add content browser tests for PeerConnection DataChannels.

Add content browser tests for PeerConnection DataChannels.

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

[perkj_chrome, 2013-01-08 15:28:12]

Time to take a look?

[phoglund_chromium, 2013-01-09 12:40:36]

https://codereview.chromium.org/11785041/diff/1/content/renderer/media/rtc_da...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/11785041/diff/1/content/renderer/media/rtc_da...
content/renderer/media/rtc_data_channel_handler.cc:96: }
Intentional?

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:13: // document title change to
OK.
I think a better name here is gNumberOfEvents and gExpectedNumberOfEvents.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:66: 
Perhaps it would be nice to number the events, like 
1. Create data channel
2. Send data
3. ...

and spread the comments out in the code when they happen. For instance, in the
dataChannel.onMessage, a comment

// 4. Received reply: done.

Basically, i'd like it to be clearer in which order all the callbacks happen.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:67: // Create a data channel on
|gFirstConnection| and sends data to
Nit: line length

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:90:
gSecondConnection.ondatachannel = function (event) {
This shadows a variable in the outer scope, which is very confusing. Either
rename the variable, or (better) extract this closure to a proper function.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:98: messageReceived = true;
Hm. Ok, so this whole messageReceived thing is used in the case when the
connection isn't open yet and we need to wait until it opens until we can send
our reply?

Could this be written as

if (dataChannel.readyState == "open") {
  dataChannel.send(sendDataString);
} else {
  sendWhenChannelOpens(sendDataString);
}

where sendWhenOpen(sendDataString) {
  dataChannel.onopen = function(event) {
    expectEquals('open', dataChannel.readyState);
      if (messageReceived) {
        dataChannel.send(sendDataString);
      }           
   }
}

Because you don't need to hook up the onopen if the connection is already open,
right? This way you get rid of the variable and it becomes a lot clearer what
the code tries to do.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:206: document.title = 'Waiting
for video...';
Use addExitConditions.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:248: function testSuccessful()
{
Why is testSuccessful an exit condition?...

[perkj_chrome, 2013-01-09 13:56:21]

PTAL

https://codereview.chromium.org/11785041/diff/1/content/renderer/media/rtc_da...
File content/renderer/media/rtc_data_channel_handler.cc (right):

https://codereview.chromium.org/11785041/diff/1/content/renderer/media/rtc_da...
content/renderer/media/rtc_data_channel_handler.cc:96: }
On 2013/01/09 12:40:36, phoglund wrote:
> Intentional?
nope

https://codereview.chromium.org/11785041/diff/1/content/renderer/media/rtc_da...
content/renderer/media/rtc_data_channel_handler.cc:96: }
On 2013/01/09 12:40:36, phoglund wrote:
> Intentional?
nope

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:13: // document title change to
OK.
On 2013/01/09 12:40:36, phoglund wrote:
> I think a better name here is gNumberOfEvents and gExpectedNumberOfEvents.

Done.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:66: 
On 2013/01/09 12:40:36, phoglund wrote:
> Perhaps it would be nice to number the events, like 
> 1. Create data channel
> 2. Send data
> 3. ...
> 
> and spread the comments out in the code when they happen. For instance, in the
> dataChannel.onMessage, a comment
> 
> // 4. Received reply: done.
> 
> Basically, i'd like it to be clearer in which order all the callbacks happen.

Done.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:67: // Create a data channel on
|gFirstConnection| and sends data to
On 2013/01/09 12:40:36, phoglund wrote:
> Nit: line length

Done.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:90:
gSecondConnection.ondatachannel = function (event) {
renamed. I would prefer to not break out anyting since I think this the test is
more readable when the behavior is all defined in this functions.

On 2013/01/09 12:40:36, phoglund wrote:
> This shadows a variable in the outer scope, which is very confusing. Either
> rename the variable, or (better) extract this closure to a proper function.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:98: messageReceived = true;
On 2013/01/09 12:40:36, phoglund wrote:
> Hm. Ok, so this whole messageReceived thing is used in the case when the
> connection isn't open yet and we need to wait until it opens until we can send
> our reply?
> 
> Could this be written as
> 
> if (dataChannel.readyState == "open") {
>   dataChannel.send(sendDataString);
> } else {
>   sendWhenChannelOpens(sendDataString);
> }
> 
> where sendWhenOpen(sendDataString) {
>   dataChannel.onopen = function(event) {
>     expectEquals('open', dataChannel.readyState);
>       if (messageReceived) {
>         dataChannel.send(sendDataString);
>       }           
>    }
> }
> 
> Because you don't need to hook up the onopen if the connection is already
open,
> right? This way you get rid of the variable and it becomes a lot clearer what
> the code tries to do.

Done.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:206: document.title = 'Waiting
for video...';
On 2013/01/09 12:40:36, phoglund wrote:
> Use addExitConditions.

Done.

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:248: function testSuccessful()
{
Renamed to exitConditionMet. This is called when an exit event occur.

On 2013/01/09 12:40:36, phoglund wrote:
> Why is testSuccessful an exit condition?...

[phoglund_chromium, 2013-01-09 16:36:45]

lgtm, some nits

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/11785041/diff/1/content/test/data/media/peerc...
content/test/data/media/peerconnection-call.html:90:
gSecondConnection.ondatachannel = function (event) {
On 2013/01/09 13:56:21, perkj wrote:
> renamed. I would prefer to not break out anyting since I think this the test
is
> more readable when the behavior is all defined in this functions.
> 
> On 2013/01/09 12:40:36, phoglund wrote:
> > This shadows a variable in the outer scope, which is very confusing. Either
> > rename the variable, or (better) extract this closure to a proper function.
> 
Ok, fair enough.

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

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:67: // This function is used
for setting up a test that:
Nit: Creates

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:72: // 3. When data is received
on |gFirstConnection|, the data
Nit: passes

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:79: 
Nit: line length

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:100: 
Nit: line length

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:248: 
Nit: rename to eventOccurred or something more suitable.

[perkj_chrome, 2013-01-10 09:53:39]

John, can you rubber stamp or review? 

Thanks

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

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:67: // This function is used
for setting up a test that:
On 2013/01/09 16:36:45, phoglund wrote:
> Nit: Creates

Done.

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:72: // 3. When data is received
on |gFirstConnection|, the data
On 2013/01/09 16:36:45, phoglund wrote:
> Nit: passes

Done.

https://codereview.chromium.org/11785041/diff/7002/content/test/data/media/pe...
content/test/data/media/peerconnection-call.html:248: 
On 2013/01/09 16:36:45, phoglund wrote:
> Nit: rename to eventOccurred or something more suitable.

addExpectedEvent and eventOccured ?

[jam, 2013-01-10 17:18:00]

On 2013/01/10 09:53:39, perkj wrote:
> John, can you rubber stamp or review? 

content/browser/media got added yesterday by jiayl. can you move this test to
that file? also add an OWNERS file with the relevant media people.

[perkj_chrome, 2013-01-11 08:36:47]

On 2013/01/10 17:18:00, John Abd-El-Malek wrote:
> On 2013/01/10 09:53:39, perkj wrote:
> > John, can you rubber stamp or review? 
> 
> content/browser/media got added yesterday by jiayl. can you move this test to
> that file? also add an OWNERS file with the relevant media people.

ok I:
1. Moved webrtc_browsertest.cc to content/browser/media/
2. Copied the OWNERS file from content/browser/renderer_host/media to
content/browser/media and content/tests/data/media.
3. Added phoglund as owner in content/tests/data/media.

[jam, 2013-01-11 23:43:09]

lgtm, thanks

[commit-bot: I haz the power, 2013-01-14 08:13:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/11785041/17001

[commit-bot: I haz the power, 2013-01-14 10:57:26]

Change committed as 176655