Add skeleton webrtc::SessionDescription and webrtc::MediaDescription classes.

Add skeleton webrtc::SessionDescription and webrtc::MediaDescription classes.

BUG=webrtc:7311
Review-Url: https://codereview.webrtc.org/2743003004
Cr-Commit-Position: refs/heads/master@{#17181}
Committed: https://chromium.googlesource.com/external/webrtc/+/55adc0e1a5f39f7b4bb706c1cbc688e75c9754ae

[Zhi Huang, 2017-03-10 23:23:55]

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

[commit-bot: I haz the power, 2017-03-10 23:24:04]

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-03-10 23:50:16]

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

[commit-bot: I haz the power, 2017-03-10 23:50:17]

Dry run: This issue passed the CQ dry run.

[Zhi Huang, 2017-03-10 23:50:55]

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

[Zhi Huang, 2017-03-10 23:50:56]

Please take a look.

[Taylor Brandstetter, 2017-03-11 00:24:29]

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
File webrtc/api/ortc/mediadescription.h (right):

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:20: // A structured representation of a media
description within an SDP message.
nit: I'd say "session description" instead of "message", here and in the other
header file. "message" could be interpreted as meaning SIP messages.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:23: explicit MediaDescription(std::string
mid) : mid_(mid) {}
Should be mid_(std::move(mid)) to ensure an extra copy isn't made.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:27: rtc::Optional<std::string> mid() const {
return mid_; }
Could add a comment that this is the "mid" attribute defined in RFC5888.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:28: void set_mid(const
rtc::Optional<std::string>& mid) { mid_ = mid; }
Same comment as in previous CL: I think this method can just take a string for
convenience, since there shouldn't be a need to "unset" the mid.

Also, maybe we don't need to use Optional at all? An empty string could be
interpreted as being "unset".

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
File webrtc/api/ortc/sessiondescription.h (right):

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
webrtc/api/ortc/sessiondescription.h:22: : session_id_(session_id),
session_version_(session_version) {}
Should be session_id_(std::move(session_id)) to ensure an extra copy isn't made.
Same deal with the setters.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
webrtc/api/ortc/sessiondescription.h:24: std::string session_id() const { return
session_id_; }
Would be nice to add comments for where these things come from. session_id is
the "sess-id" field from the o= line, and "session_version" is the
"sess-version" field.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
webrtc/api/ortc/sessiondescription.h:24: std::string session_id() const { return
session_id_; }
These could return const refs.

[Zhi Huang, 2017-03-11 01:20:39]

Please take another look. Thanks.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
File webrtc/api/ortc/mediadescription.h (right):

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:20: // A structured representation of a media
description within an SDP message.
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> nit: I'd say "session description" instead of "message", here and in the other
> header file. "message" could be interpreted as meaning SIP messages.

Done.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:23: explicit MediaDescription(std::string
mid) : mid_(mid) {}
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> Should be mid_(std::move(mid)) to ensure an extra copy isn't made.

Oh, I missed this one.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:27: rtc::Optional<std::string> mid() const {
return mid_; }
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> Could add a comment that this is the "mid" attribute defined in RFC5888.

Done.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:28: void set_mid(const
rtc::Optional<std::string>& mid) { mid_ = mid; }
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> Same comment as in previous CL: I think this method can just take a string for
> convenience, since there shouldn't be a need to "unset" the mid.
> 
> Also, maybe we don't need to use Optional at all? An empty string could be
> interpreted as being "unset".

Both ways sound good to me. Maybe it would make it more clear that this field is
optional by using rtc::Optional()?

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
File webrtc/api/ortc/sessiondescription.h (right):

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
webrtc/api/ortc/sessiondescription.h:22: : session_id_(session_id),
session_version_(session_version) {}
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> Should be session_id_(std::move(session_id)) to ensure an extra copy isn't
made.
> Same deal with the setters.

Done.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
webrtc/api/ortc/sessiondescription.h:24: std::string session_id() const { return
session_id_; }
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> These could return const refs.

Done.

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/sessiondescri...
webrtc/api/ortc/sessiondescription.h:24: std::string session_id() const { return
session_id_; }
On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> Would be nice to add comments for where these things come from. session_id is
> the "sess-id" field from the o= line, and "session_version" is the
> "sess-version" field.

Done.

[Taylor Brandstetter, 2017-03-11 01:37:07]

lgtm with minor comments

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
File webrtc/api/ortc/mediadescription.h (right):

https://codereview.webrtc.org/2743003004/diff/1/webrtc/api/ortc/mediadescript...
webrtc/api/ortc/mediadescription.h:28: void set_mid(const
rtc::Optional<std::string>& mid) { mid_ = mid; }
On 2017/03/11 01:20:38, Zhi Huang wrote:
> On 2017/03/11 00:24:29, Taylor Brandstetter wrote:
> > Same comment as in previous CL: I think this method can just take a string
for
> > convenience, since there shouldn't be a need to "unset" the mid.
> > 
> > Also, maybe we don't need to use Optional at all? An empty string could be
> > interpreted as being "unset".
> 
> Both ways sound good to me. Maybe it would make it more clear that this field
is
> optional by using rtc::Optional()?

That's a good argument. I could go either way. Might as well go with "Optional"
since that's what we already have.

https://codereview.webrtc.org/2743003004/diff/20001/webrtc/api/ortc/mediadesc...
File webrtc/api/ortc/mediadescription.h (right):

https://codereview.webrtc.org/2743003004/diff/20001/webrtc/api/ortc/mediadesc...
webrtc/api/ortc/mediadescription.h:30: void set_mid(std::string mid) {
mid_.emplace(mid); }
std::move(mid) here too.

https://codereview.webrtc.org/2743003004/diff/20001/webrtc/api/ortc/mediadesc...
webrtc/api/ortc/mediadescription.h:35: //
https://tools.ietf.org/html/rfc5888#section-6
Can you move this comment to above the "mid()" method, so it's in the public
part of the class?

https://codereview.webrtc.org/2743003004/diff/20001/webrtc/api/ortc/sessionde...
File webrtc/api/ortc/sessiondescription.h (right):

https://codereview.webrtc.org/2743003004/diff/20001/webrtc/api/ortc/sessionde...
webrtc/api/ortc/sessiondescription.h:41: // session_version_ is the
"sess-version" field.
Same with this comment

[Zhi Huang, 2017-03-11 02:13:59]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2017-03-11 02:14:00]

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/2743003004/#ps40001 (title:
"Make the comments public.")

[commit-bot: I haz the power, 2017-03-11 02:14:02]

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

[Taylor Brandstetter, 2017-03-11 02:19:57]

https://codereview.webrtc.org/2743003004/diff/40001/webrtc/api/ortc/sessionde...
File webrtc/api/ortc/sessiondescription.h (right):

https://codereview.webrtc.org/2743003004/diff/40001/webrtc/api/ortc/sessionde...
webrtc/api/ortc/sessiondescription.h:31: const std::string& session_id() const {
return session_id_; }
Something I realized a little late: The session id is guaranteed to be
representable as a 64-bit unsigned integer. So we should change this to an
int64_t. It would be reasonable for an application to want to make integer
comparisons, since JSEP says the ID should be incremented by one for each new
offer.

[Taylor Brandstetter, 2017-03-11 02:20:59]

https://codereview.webrtc.org/2743003004/diff/40001/webrtc/api/ortc/sessionde...
File webrtc/api/ortc/sessiondescription.h (right):

https://codereview.webrtc.org/2743003004/diff/40001/webrtc/api/ortc/sessionde...
webrtc/api/ortc/sessiondescription.h:31: const std::string& session_id() const {
return session_id_; }
On 2017/03/11 02:19:56, Taylor Brandstetter wrote:
> Something I realized a little late: The session id is guaranteed to be
> representable as a 64-bit unsigned integer. So we should change this to an
> int64_t. It would be reasonable for an application to want to make integer
> comparisons, since JSEP says the ID should be incremented by one for each new
> offer.

Meant to say "64-bit signed integer".

[commit-bot: I haz the power, 2017-03-11 02:33:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489198439898840,
"parent_rev": "3a72c228a164845a66e8f336757d9b617ea89c48", "commit_rev":
"55adc0e1a5f39f7b4bb706c1cbc688e75c9754ae"}

[commit-bot: I haz the power, 2017-03-11 02:33:48]

Description was changed from

==========
Add skeleton webrtc::SessionDescription and webrtc::MediaDescription classes.

BUG=webrtc:7311
==========

to

==========
Add skeleton webrtc::SessionDescription and webrtc::MediaDescription classes.

BUG=webrtc:7311

Review-Url: https://codereview.webrtc.org/2743003004
Cr-Commit-Position: refs/heads/master@{#17181}
Committed:
https://chromium.googlesource.com/external/webrtc/+/55adc0e1a5f39f7b4bb706c1c...
==========

[commit-bot: I haz the power, 2017-03-11 02:33:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/55adc0e1a5f39f7b4bb706c1c...

[Zhi Huang, 2017-03-13 03:36:24]

On 2017/03/11 02:20:59, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2743003004/diff/40001/webrtc/api/ortc/sessionde...
> File webrtc/api/ortc/sessiondescription.h (right):
> 
>
https://codereview.webrtc.org/2743003004/diff/40001/webrtc/api/ortc/sessionde...
> webrtc/api/ortc/sessiondescription.h:31: const std::string& session_id() const
{
> return session_id_; }
> On 2017/03/11 02:19:56, Taylor Brandstetter wrote:
> > Something I realized a little late: The session id is guaranteed to be
> > representable as a 64-bit unsigned integer. So we should change this to an
> > int64_t. It would be reasonable for an application to want to make integer
> > comparisons, since JSEP says the ID should be incremented by one for each
new
> > offer.
> 
> Meant to say "64-bit signed integer".

I'll have a follow up cl for this.