Use the same draft version in SDP data channel answers as used in the offer.

Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback before continuing.

BUG=chromium:686212
Review-Url: https://codereview.webrtc.org/2690943011
Cr-Commit-Position: refs/heads/master@{#16686}
Committed: https://chromium.googlesource.com/external/webrtc/+/4b2e0829caf40dd3e1bd28107b6f54002d73c2c8

[Zach Stein, 2017-02-16 19:06:34]

Description was changed from

==========
Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The
deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated
them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered
DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback
before continuing.

BUG=webrtc686212
==========

to

==========
Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The
deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated
them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered
DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback
before continuing.

BUG=webrtc:686212
==========

[Zach Stein, 2017-02-16 19:40:44]

The CQ bit was checked by zstein@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 19:40:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Zach Stein, 2017-02-16 19:41:04]

The CQ bit was unchecked by zstein@webrtc.org

[Zach Stein, 2017-02-16 23:42:02]

The CQ bit was checked by zstein@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 23:42:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Zach Stein, 2017-02-16 23:44:16]

zstein@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Zach Stein, 2017-02-16 23:44:17]

Not quite done yet, but I thought this would be a good time for some feedback.

[commit-bot: I haz the power, 2017-02-16 23:49:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 23:49:44]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-02-17 02:10:57]

Looks good; just needs the mediasession test. I'd suggest manually constructing
a SessionDescription object representing an offer that uses the new format, then
test the output of "CreateAnswer". The "MediaSessionOptions" will need
data_channel_type set to cricket::DCT_SCTP I think.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1999: // Respond with sctmap if the offer uses sctmap.
Missing a 'p'

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.h
File webrtc/pc/mediasession.h (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.h#...
webrtc/pc/mediasession.h:404: DataContentDescription() : use_sctpmap_(true) {}
nit: We generally prefer providing default values where the variable is declared
when it makes sense. So, "bool use_sctpmap_ = true;"

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.h#...
webrtc/pc/mediasession.h:411: // Maybe it belongs in MediaContentDescription
instead
I think it makes sense here, since it's the only place it's relevant.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp.cc#ne...
webrtc/pc/webrtcsdp.cc:1407: bool use_sctpmap = data_desc != nullptr ?
data_desc->use_sctpmap() : true;
It won't be possible for data_desc to be nullptr here

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp_unitt...
File webrtc/pc/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp_unitt...
webrtc/pc/webrtcsdp_unittest.cc:2706: AddSctpDataChannel(true);
nit: In situations where you may wonder "what does this parameter mean", and
it's important to the test, we sometimes do:

bool use_sctpmap = true;
AddSctpDataChannel(use_sctpmap);

Or, define "const bool kUseSctpmap = true;" at the top of the file.

Elsewhere you may see this:

AddSctpDataChannel(true /* use_sctpmap */);

But that's generally not our style.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp_unitt...
webrtc/pc/webrtcsdp_unittest.cc:2724: TEST_F(WebRtcSdpTest,
DeserializeSdpWithSctpDataChannelAndNewUnusualPort) {
nit: "AndNew" doesn't quite make it clear what's being tested. A different name
or a comment above the test would be good.

[Zach Stein, 2017-02-17 21:29:17]

Fixed up the style issues and added mediasession tests.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.cc
File webrtc/pc/mediasession.cc (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.cc...
webrtc/pc/mediasession.cc:1999: // Respond with sctmap if the offer uses sctmap.
On 2017/02/17 02:10:56, Taylor Brandstetter wrote:
> Missing a 'p'

Done.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.h
File webrtc/pc/mediasession.h (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.h#...
webrtc/pc/mediasession.h:404: DataContentDescription() : use_sctpmap_(true) {}
On 2017/02/17 02:10:56, Taylor Brandstetter wrote:
> nit: We generally prefer providing default values where the variable is
declared
> when it makes sense. So, "bool use_sctpmap_ = true;"

Done.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/mediasession.h#...
webrtc/pc/mediasession.h:411: // Maybe it belongs in MediaContentDescription
instead
On 2017/02/17 02:10:56, Taylor Brandstetter wrote:
> I think it makes sense here, since it's the only place it's relevant.

I was thinking about moving it to MediaContentDescription because that would
remove the need to downcast media_desc to data_desc in BuildMediaDescription.
Since I now know that SCTP is only relevant to data channels, I think moving the
call to BuildSctpContentAttributes (and possibly BuildRtpContentAttributes) to
the if (media_type ...) branches of BuildMediaDescription would be better.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp.cc
File webrtc/pc/webrtcsdp.cc (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp.cc#ne...
webrtc/pc/webrtcsdp.cc:1407: bool use_sctpmap = data_desc != nullptr ?
data_desc->use_sctpmap() : true;
On 2017/02/17 02:10:56, Taylor Brandstetter wrote:
> It won't be possible for data_desc to be nullptr here

Done. This is the block I would like to try and move up to the if
(media_type...) part of this method (~L1274).

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp_unitt...
File webrtc/pc/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp_unitt...
webrtc/pc/webrtcsdp_unittest.cc:2706: AddSctpDataChannel(true);
On 2017/02/17 02:10:57, Taylor Brandstetter wrote:
> nit: In situations where you may wonder "what does this parameter mean", and
> it's important to the test, we sometimes do:
> 
> bool use_sctpmap = true;
> AddSctpDataChannel(use_sctpmap);
> 
> Or, define "const bool kUseSctpmap = true;" at the top of the file.
> 
> Elsewhere you may see this:
> 
> AddSctpDataChannel(true /* use_sctpmap */);
> 
> But that's generally not our style.

Done.

https://codereview.webrtc.org/2690943011/diff/20001/webrtc/pc/webrtcsdp_unitt...
webrtc/pc/webrtcsdp_unittest.cc:2724: TEST_F(WebRtcSdpTest,
DeserializeSdpWithSctpDataChannelAndNewUnusualPort) {
On 2017/02/17 02:10:56, Taylor Brandstetter wrote:
> nit: "AndNew" doesn't quite make it clear what's being tested. A different
name
> or a comment above the test would be good.

Done.

[Zach Stein, 2017-02-17 21:32:32]

The CQ bit was checked by zstein@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 21:32:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-17 21:57:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 21:57:34]

Dry run: Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23242)

[Taylor Brandstetter, 2017-02-17 22:15:35]

lgtm with minor comments

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
File webrtc/pc/mediasession_unittest.cc (right):

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
webrtc/pc/mediasession_unittest.cc:972:
TEST_F(MediaSessionDescriptionFactoryTest, TestCreateDataAnswerUsesSctpmap) {
Could you leave a comment above these two tests describing what they're testing
from a high level? For example, "Test that the use_sctpmap flag in an answer,
representing whether or not SDP will use legacy 'a=sctpmap' attributes, matches
the flag in the offer."

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
webrtc/pc/mediasession_unittest.cc:975: std::unique_ptr<SessionDescription>
offer(f1_.CreateOffer(opts, NULL));
We use C++11 "nullptr" instead of "NULL". Though don't worry about changing it
in this CL; I'm going to run a script to convert every "NULL" to "nullptr" some
time soon. https://codereview.webrtc.org/2685783014/

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
webrtc/pc/mediasession_unittest.cc:977: ContentInfo* offer_dv =
offer->GetContentByName("data");
What does "dv" stand for?

I'd suggest more descriptive names, such as "offer_data_content",
"offer_data_description", though it matters less in a unit test.

[Zach Stein, 2017-02-17 22:55:29]

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
File webrtc/pc/mediasession_unittest.cc (right):

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
webrtc/pc/mediasession_unittest.cc:972:
TEST_F(MediaSessionDescriptionFactoryTest, TestCreateDataAnswerUsesSctpmap) {
On 2017/02/17 22:15:35, Taylor Brandstetter wrote:
> Could you leave a comment above these two tests describing what they're
testing
> from a high level? For example, "Test that the use_sctpmap flag in an answer,
> representing whether or not SDP will use legacy 'a=sctpmap' attributes,
matches
> the flag in the offer."

Done.

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
webrtc/pc/mediasession_unittest.cc:975: std::unique_ptr<SessionDescription>
offer(f1_.CreateOffer(opts, NULL));
On 2017/02/17 22:15:35, Taylor Brandstetter wrote:
> We use C++11 "nullptr" instead of "NULL". Though don't worry about changing it
> in this CL; I'm going to run a script to convert every "NULL" to "nullptr"
some
> time soon. https://codereview.webrtc.org/2685783014/

I'll match the rest of the file for now.

https://codereview.webrtc.org/2690943011/diff/40001/webrtc/pc/mediasession_un...
webrtc/pc/mediasession_unittest.cc:977: ContentInfo* offer_dv =
offer->GetContentByName("data");
On 2017/02/17 22:15:35, Taylor Brandstetter wrote:
> What does "dv" stand for?
> 
> I'd suggest more descriptive names, such as "offer_data_content",
> "offer_data_description", though it matters less in a unit test.

I meant for this to be dc for data content - I'll change it to that. This
matches the rest of the file. The test above uses vc instead of dc - I'll fix
that one too. I would rather not change the entire file here though, so I'll
stick with dc instead of data_content for now.

[Zach Stein, 2017-02-17 23:23:49]

The CQ bit was checked by zstein@webrtc.org

[Zach Stein, 2017-02-17 23:23:49]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2690943011/#ps60001 (title:
"Fix variable names in media session tests and add some comments.")

[commit-bot: I haz the power, 2017-02-17 23:24:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Zach Stein, 2017-02-17 23:27:06]

Description was changed from

==========
Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The
deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated
them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered
DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback
before continuing.

BUG=webrtc:686212
==========

to

==========
Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The
deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated
them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered
DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback
before continuing.

BUG=chromium:686212
==========

[commit-bot: I haz the power, 2017-02-17 23:43:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:43:59]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)

[Zach Stein, 2017-02-18 03:42:55]

The CQ bit was checked by zstein@webrtc.org

[commit-bot: I haz the power, 2017-02-18 03:43:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-18 03:48:39]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487389375913040,
"parent_rev": "2c546da52981af00301e50321bce7d76bc32aa6c", "commit_rev":
"4b2e0829caf40dd3e1bd28107b6f54002d73c2c8"}

[commit-bot: I haz the power, 2017-02-18 03:48:42]

Description was changed from

==========
Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The
deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated
them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered
DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback
before continuing.

BUG=chromium:686212
==========

to

==========
Use the same draft version in SDP data channel answers as used in the offer.

This change adds a flag, use_sctpmap, to DataContentDescription. The
deserialization code sets the flag based on the format of the m= line.
There were already unit tests using SDP in the new format, so I just updated
them to check use_sctpmap was set as expected.

The change to mediasession copies use_sctpmap from the offered
DataContentDescription to the answer.
I haven't figured out how to test this change yet, but wanted to get feedback
before continuing.

BUG=chromium:686212

Review-Url: https://codereview.webrtc.org/2690943011
Cr-Commit-Position: refs/heads/master@{#16686}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4b2e0829caf40dd3e1bd28107...
==========

[commit-bot: I haz the power, 2017-02-18 03:48:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/4b2e0829caf40dd3e1bd28107...