Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1453823004:
Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest (Closed)
DescriptionHandle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest
BUG=webrtc:5116
R=pthatcher@webrtc.org
Committed: https://crrev.com/f67c548576ad957a1e9c3196e11d45f41e320424
Cr-Commit-Position: refs/heads/master@{#10994}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : merge #
Messages
Total messages: 27 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
Description was changed from ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest ========== to ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUGS:webrtc:5116 ==========
Description was changed from ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUGS:webrtc:5116 ========== to ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUG=webrtc:5116 ==========
https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:472: void TurnPort::DestroyConnection(const rtc::SocketAddress& address) { Should it return a bool of whether or not it was destroyed? https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:720: } Should we have a TunrPort::Close method that does this? It seems like common functionality. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:986: TurnEntry* entry = FindEntry(address); Can we just make FindEntry callable by the unit test and then move this logic into the unit test? https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1208: port_->SignalTurnRefreshResult(port_, 0); Can we make a name constant for it? kTurnRefreshSuccess? https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:674: TEST_F(TurnPortTest, TestRefreshRequestGetErrorResponse) { Get => Gets https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:678: // set bad credentials. set => Set https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:683: EXPECT_TRUE_WAIT(turn_refresh_success_, kTimeout); Wait.. it's successful even though the credentials are bad? How does that work?
PTAL. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:472: void TurnPort::DestroyConnection(const rtc::SocketAddress& address) { On 2015/12/08 20:04:20, pthatcher1 wrote: > Should it return a bool of whether or not it was destroyed? Done https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:720: } On 2015/12/08 20:04:20, pthatcher1 wrote: > Should we have a TunrPort::Close method that does this? It seems like common > functionality. Done. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:986: TurnEntry* entry = FindEntry(address); On 2015/12/08 20:04:19, pthatcher1 wrote: > Can we just make FindEntry callable by the unit test and then move this logic > into the unit test? If we do that, we will need to make TurnEntry accessible outside of the TurnPort implementation. Current it is defined in turnport.cc. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:1208: port_->SignalTurnRefreshResult(port_, 0); On 2015/12/08 20:04:19, pthatcher1 wrote: > Can we make a name constant for it? kTurnRefreshSuccess? used name RESULT_SUCCESS_CODE. also used this for the permission request success. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... File webrtc/p2p/base/turnport_unittest.cc (right): https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:674: TEST_F(TurnPortTest, TestRefreshRequestGetErrorResponse) { On 2015/12/08 20:04:20, pthatcher1 wrote: > Get => Gets > Done. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:678: // set bad credentials. On 2015/12/08 20:04:20, pthatcher1 wrote: > set => Set Done. https://codereview.webrtc.org/1453823004/diff/100001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport_unittest.cc:683: EXPECT_TRUE_WAIT(turn_refresh_success_, kTimeout); On 2015/12/08 20:04:20, pthatcher1 wrote: > Wait.. it's successful even though the credentials are bad? How does that work? It is right. Because when we call FlushRequest the first time, it sends out the request in the queue which having the correct credential. Added a line of comment. Also note the comment after this line.
honghaiz@webrtc.org changed reviewers: + andresp@webrtc.org
lgtm https://codereview.webrtc.org/1453823004/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1453823004/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:41: static const int SUCCESS_RESULT_CODE = 0; TURN_SUCCESS_RESULT_CODE?
https://codereview.webrtc.org/1453823004/diff/120001/webrtc/p2p/base/turnport.cc File webrtc/p2p/base/turnport.cc (right): https://codereview.webrtc.org/1453823004/diff/120001/webrtc/p2p/base/turnport... webrtc/p2p/base/turnport.cc:41: static const int SUCCESS_RESULT_CODE = 0; On 2015/12/11 00:59:54, pthatcher1 wrote: > TURN_SUCCESS_RESULT_CODE? Done.
andresp@, Since we have agreed on the offline discussion about the turn port behavior, I am going to submit this.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1453823004/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453823004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453823004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1453823004/#ps160001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453823004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453823004/160001
Description was changed from ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUG=webrtc:5116 ========== to ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUG=webrtc:5116 R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f67c548576ad957a1e9c3196e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) manually as f67c548576ad957a1e9c3196e11d45f41e320424 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUG=webrtc:5116 R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f67c548576ad957a1e9c3196e... ========== to ========== Handle Turn error response to RefreshRequest, CreatePermissionRequest, and ChanelBindRequest BUG=webrtc:5116 R=pthatcher@webrtc.org Committed: https://crrev.com/f67c548576ad957a1e9c3196e11d45f41e320424 Cr-Commit-Position: refs/heads/master@{#10994} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f67c548576ad957a1e9c3196e11d45f41e320424 Cr-Commit-Position: refs/heads/master@{#10994} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
