Add tests for legacy SDP features.

Add tests for legacy SDP features.

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

[bemasc, 2013-10-24 22:59:12]

Oops, trybots found a bug.  Investigating...

[bemasc, 2013-10-24 23:12:39]

On 2013/10/24 22:59:12, bemasc wrote:
> Oops, trybots found a bug.  Investigating...

Fixed.

[phoglund_chromium, 2013-10-25 00:45:05]

https://codereview.chromium.org/39033006/diff/260002/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/39033006/diff/260002/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:246: #endif
Nit: insert blank line here.

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

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:11: var gTestWithoutMsid =
false;
This growing amount of global state is a bit unfortunate. What if we replace all
these with one function transformSdp(sdp) that by default does returns
unmodified SDP, but is overridden in the run-without-msid-and-bundle and your
new case? You can keep the small helpers for stripping out various things of the
SDP and use those to compose the transform functions.

Possibly except gTestWithGice, which is used in the iceCallbacks, but if can
avoid that too that's great.

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:22: var externalSdesLines = {
Nit: I guess the external* variables are constants, so they should be EXTERNAL_*
according to the style guide.

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:508: offer.sdp =
mungeSdp(offer.sdp);
Like I said above, try replacing this with the new overridable transformSdp
function.

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:515: if (gTestWithoutMsid) {
This should be able to move into mungeSdp, or the new transform funciton, right?
Why do it outside?

[bemasc, 2013-10-25 00:53:52]

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

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:515: if (gTestWithoutMsid) {
On 2013/10/25 00:45:05, phoglund wrote:
> This should be able to move into mungeSdp, or the new transform funciton,
right?
> Why do it outside?

This modification has to be performed after setLocalDescription, but before
setRemoteDescription, because it produces SDP that is only valid as a remote
description.

[phoglund_chromium, 2013-10-25 01:15:59]

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

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:515: if (gTestWithoutMsid) {
Oh, ok. Perhaps we can consider two SDP transform functions then: transformSdp
and transformSdpRemoteOnly? Or since the latter will only replace one global
variable with another, maybe it's not worth it.

[bemasc, 2013-10-25 17:39:55]

https://codereview.chromium.org/39033006/diff/260002/content/browser/media/we...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/39033006/diff/260002/content/browser/media/we...
content/browser/media/webrtc_browsertest.cc:246: #endif
On 2013/10/25 00:45:05, phoglund wrote:
> Nit: insert blank line here.

Done.

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

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:11: var gTestWithoutMsid =
false;
On 2013/10/25 00:45:05, phoglund wrote:
> This growing amount of global state is a bit unfortunate. What if we replace
all
> these with one function transformSdp(sdp) that by default does returns
> unmodified SDP, but is overridden in the run-without-msid-and-bundle and your
> new case? You can keep the small helpers for stripping out various things of
the
> SDP and use those to compose the transform functions.
> 
> Possibly except gTestWithGice, which is used in the iceCallbacks, but if can
> avoid that too that's great.

Done.

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:22: var externalSdesLines = {
On 2013/10/25 00:45:05, phoglund wrote:
> Nit: I guess the external* variables are constants, so they should be
EXTERNAL_*
> according to the style guide.

Done.

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:508: offer.sdp =
mungeSdp(offer.sdp);
On 2013/10/25 00:45:05, phoglund wrote:
> Like I said above, try replacing this with the new overridable transformSdp
> function.

Done.

https://codereview.chromium.org/39033006/diff/260002/content/test/data/media/...
content/test/data/media/peerconnection-call.html:515: if (gTestWithoutMsid) {
On 2013/10/25 01:15:59, phoglund wrote:
> Oh, ok. Perhaps we can consider two SDP transform functions then: transformSdp
> and transformSdpRemoteOnly? Or since the latter will only replace one global
> variable with another, maybe it's not worth it.

Done.

[phoglund_chromium, 2013-10-25 20:39:45]

lgtm

[phoglund_chromium, 2013-10-31 16:29:21]

Oh, and I'm not an owner so you need an owner stamp from xians@ or perhaps
perkj@. Shijing, can you stamp this?

[no longer working on chromium, 2013-10-31 16:51:59]

rubber stamp lgtm

[commit-bot: I haz the power, 2013-10-31 17:59:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemasc@chromium.org/39033006/350001

[commit-bot: I haz the power, 2013-10-31 23:16:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemasc@chromium.org/39033006/350001

[commit-bot: I haz the power, 2013-11-01 01:11:07]

Change committed as 232259