Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(931)

Issue 2770903003: Accept remote offers with current DTLS role, rather than "actpass". (Closed)

Created:
3 years, 9 months ago by Taylor Brandstetter
Modified:
3 years, 9 months ago
Reviewers:
pthatcher1
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Accept remote offers with current DTLS role, rather than "actpass". JSEP implementations are required to always generate offers with "actpass", but remote endpoints are not. So we should accept remote offers with the current negotiated DTLS role. This was recently clarified in dtls-sdp; it was somewhat ambiguous before. Also doing a bit of refactoring of JsepTransport (making a method private that should have been private, fixing unit tests that were directly calling said method). BUG=webrtc:7072 Review-Url: https://codereview.webrtc.org/2770903003 Cr-Commit-Position: refs/heads/master@{#17396} Committed: https://chromium.googlesource.com/external/webrtc/+/d8cfa1af389d815f6dd1681fcf7f168fc5e61818

Patch Set 1 #

Total comments: 3

Patch Set 2 : Adding link to spec and section number. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+254 lines, -74 lines) Patch
M webrtc/p2p/base/dtlstransportchannel_unittest.cc View 1 chunk +2 lines, -6 lines 0 comments Download
M webrtc/p2p/base/jseptransport.h View 3 chunks +11 lines, -10 lines 0 comments Download
M webrtc/p2p/base/jseptransport.cc View 1 9 chunks +29 lines, -21 lines 0 comments Download
M webrtc/p2p/base/jseptransport_unittest.cc View 7 chunks +173 lines, -31 lines 0 comments Download
M webrtc/p2p/base/transportcontroller.cc View 1 chunk +5 lines, -1 line 0 comments Download
M webrtc/p2p/base/transportcontroller_unittest.cc View 1 chunk +34 lines, -5 lines 0 comments Download

Messages

Total messages: 11 (6 generated)
Taylor Brandstetter
https://codereview.webrtc.org/2770903003/diff/1/webrtc/p2p/base/transportcontroller_unittest.cc File webrtc/p2p/base/transportcontroller_unittest.cc (left): https://codereview.webrtc.org/2770903003/diff/1/webrtc/p2p/base/transportcontroller_unittest.cc#oldcode280 webrtc/p2p/base/transportcontroller_unittest.cc:280: EXPECT_TRUE(transport_controller_->GetSslRole("audio", &role)); This test was just succeeding by accident ...
3 years, 9 months ago (2017-03-23 03:37:11 UTC) #3
pthatcher1
lgtm, with a nit https://codereview.webrtc.org/2770903003/diff/1/webrtc/p2p/base/jseptransport.cc File webrtc/p2p/base/jseptransport.cc (right): https://codereview.webrtc.org/2770903003/diff/1/webrtc/p2p/base/jseptransport.cc#newcode467 webrtc/p2p/base/jseptransport.cc:467: // current negotiated role. This ...
3 years, 9 months ago (2017-03-25 03:30:01 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2770903003/20001
3 years, 9 months ago (2017-03-27 17:08:28 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/d8cfa1af389d815f6dd1681fcf7f168fc5e61818
3 years, 9 months ago (2017-03-27 17:33:31 UTC) #10
Taylor Brandstetter
3 years, 9 months ago (2017-03-27 21:39:28 UTC) #11
Message was sent while issue was closed.
https://codereview.webrtc.org/2770903003/diff/1/webrtc/p2p/base/jseptransport.cc
File webrtc/p2p/base/jseptransport.cc (right):

https://codereview.webrtc.org/2770903003/diff/1/webrtc/p2p/base/jseptransport...
webrtc/p2p/base/jseptransport.cc:467: // current negotiated role. This is
allowed by dtls-sdp, though our
On 2017/03/25 03:30:01, pthatcher1 wrote:
> What's "dtls-sdp"?  An RFC a draft?  Being more specific would be nice,
> especially if in includes a section number.

Added link and section number.

Powered by Google App Engine
This is Rietveld 408576698