Fixing off-by-one error with max SCTP id.

Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
R=pthatcher@webrtc.org, skvlad@webrtc.org

Committed: https://crrev.com/1d7a637340ea097ea55061af7b566ceccc2678c8
Cr-Commit-Position: refs/heads/master@{#13906}

[Taylor Brandstetter, 2016-08-17 18:13:04]

Description was changed from

==========
Fixing off-by-one error with max SCTP id.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.
==========

to

==========
Fixing off-by-one error with max SCTP id.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
==========

[Taylor Brandstetter, 2016-08-17 18:14:36]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org, skvlad@webrtc.org

[skvlad, 2016-08-17 19:13:53]

lgtm

https://codereview.webrtc.org/2254003002/diff/1/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (right):

https://codereview.webrtc.org/2254003002/diff/1/webrtc/api/webrtcsdp.cc#newco...
webrtc/api/webrtcsdp.cc:1394: << (cricket::kMaxSctpStreams);
The parentheses aren't necessary any more.

[Taylor Brandstetter, 2016-08-17 21:32:37]

Description was changed from

==========
Fixing off-by-one error with max SCTP id.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
==========

to

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel throws an exception. But due to an off-by-one
error, creating a data channel with ID 1023 doesn't throw an exception
but instead silently fails.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
==========

[Taylor Brandstetter, 2016-08-17 21:33:25]

Description was changed from

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel throws an exception. But due to an off-by-one
error, creating a data channel with ID 1023 doesn't throw an exception
but instead silently fails.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
==========

to

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
==========

[Taylor Brandstetter, 2016-08-17 22:39:27]

https://codereview.webrtc.org/2254003002/diff/1/webrtc/api/webrtcsdp.cc
File webrtc/api/webrtcsdp.cc (right):

https://codereview.webrtc.org/2254003002/diff/1/webrtc/api/webrtcsdp.cc#newco...
webrtc/api/webrtcsdp.cc:1394: << (cricket::kMaxSctpStreams);
On 2016/08/17 19:13:53, skvlad wrote:
> The parentheses aren't necessary any more.

Done.

[pthatcher1, 2016-08-22 23:09:30]

lgtm

I'd prefer to see the following nits improved, but they are nits.

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/api/datachannel.cc
File webrtc/api/datachannel.cc (right):

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/api/datachannel.cc...
webrtc/api/datachannel.cc:34: if (potential_sid >=
static_cast<int>(cricket::kMaxSctpStreams)) {
I liked the way it read before: "if sid > maxSid { return false; }".

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/media/sctp/sctpdat...
File webrtc/media/sctp/sctpdataengine.h (right):

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/media/sctp/sctpdat...
webrtc/media/sctp/sctpdataengine.h:48: const uint16_t kMaxSctpStreams = 1024;
Why not have *both* kMaxSctpStreams and kMaxSctpSid.   Then the places where it
makes sense for one in the code can use it, and where it makes sense for other
other can use the other.

I think const expr can handle "kMaxSctpSid = kMaxSctpStreams - 1" to avoid them
getting out of sync.

[Taylor Brandstetter, 2016-08-23 00:53:14]

Please take a look again Peter. Added back kMaxSctpSid.

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/api/datachannel.cc
File webrtc/api/datachannel.cc (right):

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/api/datachannel.cc...
webrtc/api/datachannel.cc:34: if (potential_sid >=
static_cast<int>(cricket::kMaxSctpStreams)) {
On 2016/08/22 23:09:30, pthatcher1 wrote:
> I liked the way it read before: "if sid > maxSid { return false; }".  

Done.

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/media/sctp/sctpdat...
File webrtc/media/sctp/sctpdataengine.h (right):

https://codereview.webrtc.org/2254003002/diff/20001/webrtc/media/sctp/sctpdat...
webrtc/media/sctp/sctpdataengine.h:48: const uint16_t kMaxSctpStreams = 1024;
On 2016/08/22 23:09:30, pthatcher1 wrote:
> Why not have *both* kMaxSctpStreams and kMaxSctpSid.   Then the places where
it
> makes sense for one in the code can use it, and where it makes sense for other
> other can use the other.
> 
> I think const expr can handle "kMaxSctpSid = kMaxSctpStreams - 1" to avoid
them
> getting out of sync.

Ok. I thought "max streams" made it more clear that it was an inclusive maximum,
but on second thought maybe it doesn't. I'll just have both and leave a comment
to reduce future confusion. And add a "min" too for good measure.

[Taylor Brandstetter, 2016-08-24 17:44:11]

Going to go ahead and commit this since the updated patchset is essentially just
renaming.

[Taylor Brandstetter, 2016-08-24 17:44:33]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-08-24 17:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2254003002/#ps20001 (title:
"Removing unneeded parentheses.")

[commit-bot: I haz the power, 2016-08-24 17:44:43]

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, 2016-08-24 19:45:08]

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

[commit-bot: I haz the power, 2016-08-24 19:45:09]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Taylor Brandstetter, 2016-08-24 20:15:41]

Description was changed from

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
==========

to

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
R=pthatcher@webrtc.org, skvlad@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1d7a637340ea097ea55061af7...
==========

[commit-bot: I haz the power, 2016-08-24 20:15:41]

Description was changed from

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
R=pthatcher@webrtc.org, skvlad@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1d7a637340ea097ea55061af7...
==========

to

==========
Fixing off-by-one error with max SCTP id.

Normally, when creating a data channel with an out-of-range ID,
createDataChannel returns nullptr. But due to an off-by-one
error, creating a data channel with ID 1023 returns a data channel
that silently fails later.

This probably occurred because it wasn't clear whether "kMaxSctpSid" was an
inclusive or exclusive maximum, so I changed the value to
"kMaxSctpStreams". This wasn't caught by unit tests because the
off-by-one error persisted to the unit tests as well.

Also getting rid of some dead code. We were adding SCTP streams to the
ContentDescription object but they weren't being used.

BUG=619849
R=pthatcher@webrtc.org, skvlad@webrtc.org

Committed: https://crrev.com/1d7a637340ea097ea55061af7b566ceccc2678c8
Cr-Commit-Position: refs/heads/master@{#13906}
==========

[Taylor Brandstetter, 2016-08-24 20:15:43]

Committed patchset #2 (id:20001) manually as
1d7a637340ea097ea55061af7b566ceccc2678c8 (presubmit successful).

[commit-bot: I haz the power, 2016-08-24 20:15:43]

Patchset 2 (id:??) landed as
https://crrev.com/1d7a637340ea097ea55061af7b566ceccc2678c8
Cr-Commit-Position: refs/heads/master@{#13906}