Rewrote WebRTC browser tests to not use peerconnection_server.

Rewrote WebRTC browser tests to not use peerconnection_server.

This removes all the code that we used for talking to the
peerconnection_server. I also cleaned up tons of dead code from the
test javascript (most of which was used in the old manual test page,
which is now completely self-contained and doesn't use this javascript
anymore).

In place of the peerconnection_server, we'll send signaling information
through the test using ExecuteJavascript calls from the browser test.
This was a bit tricky to design since the javascript can't talk back
to the browser test without the browser test initiating the talking.
The basic design became

1. Test asks tab 1 to get user media and create an offer with audio
   and video. The offer is returned to the test as a JSON encoded
   session description.
2. The test asks tab 2 to get user media and create a peer connection.
3. The test passes the offer to tab2 and asks it to create an answer,
   and the answer is returned to the test like the offer in 1.
4. The test asks tab 1 to accept the answer.
5. Both tabs are now gathering ICE candidates.
6. The test asks tab 1 to return its ICE candidates (waiting for
   gathering to complete if necessary). The candidates are then sent
   to tab 2 which processes them. The ICE candidates are passed as
   JSON encoded RTCIceCandidate instances.
7. 6) is repeated, but from tab 2 to tab 1.
8. We wait for video / audio to start playing.

In general this is a more synchronous design compared to the
peerconnection_server design as the test used to merely arbitrate
connections to the server and then let the javascript in the two tabs
talk to each other asynchronously as far as the test was concerned.
This seems to work though, and the test has full control over what
happens. Hopefully it makes the negotiation flow clearer rather than
having this magical out-of-band signaling channel that the server
enabled previously.

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

[phoglund_chromium, 2014-05-07 11:40:22]

kjellander: main review
tommi: owner stamp

[phoglund_chromium, 2014-05-07 11:47:27]

kjellander: main review
tommi: owner stamp

[kjellander_chromium, 2014-05-08 09:11:49]

This is excellent work! What a clenaup.
lgtm with just a few nits.

I would prefer if someone with more detailed knowledge about  SDP/ICE
negotiation can take a look as well.

Test-wise, I assume you have run all the tests we run on the ChromiumWebRTC bots
manually?
Does all tests that use this code have the JS console error checking, so we can
ensure to catch such errors?

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp#newcode607
build/all.gyp:607: 'chromium_builder_qa',  # TODO(phoglund): not sure if needed?
It's just a convenient target that is used by other targets. It's needed for the
crash_service target as we run the crash_handler on Windows builds.

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp#newcode617
build/all.gyp:617: '../chrome/chrome.gyp:crash_service',
This should be possible to remove since it's already covered in
'chromium_builder_qa', which also handles x64 (which we don't have here).

https://codereview.chromium.org/271653002/diff/20001/chrome/browser/media/web...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/271653002/diff/20001/chrome/browser/media/web...
chrome/browser/media/webrtc_browsertest_base.cc:283: ASSERT_EQ(
EXPECT_EQ?

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
File chrome/test/data/webrtc/jsep01_call.js (left):

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/jsep01_call.js:222: debug('Sending SDP message:\n' +
session_description.sdp);
Ah, I guess this will make the test logs a lot less spammy?

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
File chrome/test/data/webrtc/jsep01_call.js (right):

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/jsep01_call.js:13: // message_handling.js. The contract
for these functions is described in
I don't see the contracts described in message_handling.js except that they're
used from there. I guess this doesn't matter as the plan seems to be move the
jsep_call01.js code in there anyway.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/jsep01_call.js:48: peerConnection = new
RTCPeerConnection(servers, constraints);
Is it a good idea to make this switch in this CL? I guess we need to have
adapter.js loaded in order to use RTCPeerConnection - is that always the case?

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
File chrome/test/data/webrtc/message_handling.js (right):

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:76: * Returns a string on the format
ok-(JSON encoded session description).
nit: use @return {!string} instead (more below)

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:80: * @param {!object} constraints
Any createAnswer constraints.
nit: {!Object} (more below)

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:99: * previous offer by this page
nit: end with punctuation.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:229: iceCandidatesJson);
nit: indent.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:247: * @param {!RTCIceCandidate} The
ICE candidate to send.
@param {!RTCIceCandidate} message The ICE candidate to send.

[phoglund_chromium, 2014-05-08 13:27:42]

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp#newcode607
build/all.gyp:607: 'chromium_builder_qa',  # TODO(phoglund): not sure if needed?
On 2014/05/08 09:11:50, kjellander wrote:
> It's just a convenient target that is used by other targets. It's needed for
the
> crash_service target as we run the crash_handler on Windows builds.
All right, I'll write that in the comment then (as the pyauto comment was
obviously incorrect).

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp#newcode617
build/all.gyp:617: '../chrome/chrome.gyp:crash_service',
On 2014/05/08 09:11:50, kjellander wrote:
> This should be possible to remove since it's already covered in
> 'chromium_builder_qa', which also handles x64 (which we don't have here).

Ok, I'll remove it then while we are at it.

https://codereview.chromium.org/271653002/diff/20001/chrome/browser/media/web...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/271653002/diff/20001/chrome/browser/media/web...
chrome/browser/media/webrtc_browsertest_base.cc:283: ASSERT_EQ(
On 2014/05/08 09:11:50, kjellander wrote:
> EXPECT_EQ?

Done.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
File chrome/test/data/webrtc/jsep01_call.js (left):

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/jsep01_call.js:222: debug('Sending SDP message:\n' +
session_description.sdp);
On 2014/05/08 09:11:50, kjellander wrote:
> Ah, I guess this will make the test logs a lot less spammy?

Not really, as the blobs will show up in "returning xxxx to test". It will be
about equal to what it was.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
File chrome/test/data/webrtc/jsep01_call.js (right):

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/jsep01_call.js:13: // message_handling.js. The contract
for these functions is described in
On 2014/05/08 09:11:50, kjellander wrote:
> I don't see the contracts described in message_handling.js except that they're
> used from there. I guess this doesn't matter as the plan seems to be move the
> jsep_call01.js code in there anyway.

I think this refers to the comment at the top of message_handling.js (which is
very bare-bones). But yeah, we need to merge and reorganize these files, which I
will do in a follow-up patch.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/jsep01_call.js:48: peerConnection = new
RTCPeerConnection(servers, constraints);
On 2014/05/08 09:11:50, kjellander wrote:
> Is it a good idea to make this switch in this CL? I guess we need to have
> adapter.js loaded in order to use RTCPeerConnection - is that always the case?
This is just because Elham missed this reference when she converted these tests
to use adapter.js. The adapter will rewrite this to webkitRTCPeerConnection.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
File chrome/test/data/webrtc/message_handling.js (right):

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:76: * Returns a string on the format
ok-(JSON encoded session description).
On 2014/05/08 09:11:50, kjellander wrote:
> nit: use @return {!string} instead (more below)

The function doesn't really return on the javascript level, which is what the
@return is supposed to document. I reckoned I'd write about the return through
the DOM automation controller in free text like above. I will make the comment a
bit clearer here though.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:80: * @param {!object} constraints
Any createAnswer constraints.
On 2014/05/08 09:11:50, kjellander wrote:
> nit: {!Object} (more below)

Done.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:99: * previous offer by this page
On 2014/05/08 09:11:50, kjellander wrote:
> nit: end with punctuation.

Done.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:229: iceCandidatesJson);
On 2014/05/08 09:11:50, kjellander wrote:
> nit: indent.

Done.

https://codereview.chromium.org/271653002/diff/20001/chrome/test/data/webrtc/...
chrome/test/data/webrtc/message_handling.js:247: * @param {!RTCIceCandidate} The
ICE candidate to send.
On 2014/05/08 09:11:50, kjellander wrote:
> @param {!RTCIceCandidate} message The ICE candidate to send.

Done.

[phoglund_chromium, 2014-05-08 13:28:20]

Alright tommi, please give an owner stamp when you're ready.

[tommi (sloooow) - chröme, 2014-05-08 13:32:46]

rs lgtm

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/271653002/diff/20001/build/all.gyp#newcode607
build/all.gyp:607: 'chromium_builder_qa',  # TODO(phoglund): not sure if needed?
could you run a try job without it in?

[phoglund_chromium, 2014-05-08 14:02:19]

On 2014/05/08 13:32:46, tommi wrote:
> rs lgtm
> 
> https://codereview.chromium.org/271653002/diff/20001/build/all.gyp
> File build/all.gyp (right):
> 
> https://codereview.chromium.org/271653002/diff/20001/build/all.gyp#newcode607
> build/all.gyp:607: 'chromium_builder_qa',  # TODO(phoglund): not sure if
needed?
> could you run a try job without it in?

yep, I'll send this through the trybots.

[phoglund_chromium, 2014-05-08 14:02:33]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-05-08 14:06:51]

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

[commit-bot: I haz the power, 2014-05-08 22:14:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 15:26:33]

Change committed as 269311