Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043193003:
Make the default value of rtcp-mux policy to required. (Closed)
DescriptionMake the default value of rtcp-mux policy to required.
Change the default value of rtcp-mux policy in RTCConfiguration.
Refactor the peerconnectioninterface and webrtcsession unit tests.
BUG=webrtc:6030
Committed: https://crrev.com/4dfb8cef514bf030f08d61289543e32127366f12
Cr-Commit-Position: refs/heads/master@{#15217}
Patch Set 1 #Patch Set 2 : Change the java api. #
Total comments: 5
Patch Set 3 : Refactoring the unit tests. #Patch Set 4 : Merge with new changes. #
Total comments: 1
Messages
Total messages: 34 (19 generated)
zhihuang@webrtc.org changed reviewers: + pthatcher@webrtc.org
It's a one line change. But we should do some refactoring for the unit tests. We can either change the expected value in each test case or change the configuration. I chose the second one here.
I think we should also change it here for Java apps: https://cs.chromium.org/chromium/src/third_party/webrtc/api/java/src/org/webr... and here for Chromium (in a different CL, obviously): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor... and here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor... and here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias... and here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/media... (Chromium is fun). I might have missed some places.
On 2016/06/08 17:17:14, pthatcher1 wrote: > I think we should also change it here for Java apps: > > https://cs.chromium.org/chromium/src/third_party/webrtc/api/java/src/org/webr... > > and here for Chromium (in a different CL, obviously): > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor... > > and here: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor... > > and here: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias... > > and here: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/media... > > (Chromium is fun). I might have missed some places. Sure. I will make a separate CL for chromium.
zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org, honghaiz@webrtc.org - pthatcher@webrtc.org
https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:417: } In the unit test, the default value for Init() is still Negotiate so that we do not have to modify each test cases. I think this would be fine because this unit test should have covered both Negotiate and Require no mater what the default one is.
https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:606: config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyNegotiate; Is there any reason to set kRtcpMuxPolicyNegotiate here? Do we have tests to cover the new default rtcp_mux_policy? Seems that most other methods call this method.
lgtm with one small comment https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:3291: PeerConnectionInterface::kRtcpMuxPolicyRequire; If we're passing the policy into Init, we probably don't need to set it here. Someone reading the test may be confused.
https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface_unittest.cc (right): https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface_unittest.cc:606: config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyNegotiate; On 2016/06/22 16:58:44, honghaiz3 wrote: > Is there any reason to set kRtcpMuxPolicyNegotiate here? > Do we have tests to cover the new default rtcp_mux_policy? > Seems that most other methods call this method. Some unit tests rely on the assumption that the policy is negotiate. But I think it make more sense to refactoring the test rather than setting the config since we are changing to require anyway. I will fix it in next patch. https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2043193003/diff/20001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:3291: PeerConnectionInterface::kRtcpMuxPolicyRequire; On 2016/06/22 17:01:09, Taylor Brandstetter wrote: > If we're passing the policy into Init, we probably don't need to set it here. > Someone reading the test may be confused. I agree.
lgtm
zhihuang@webrtc.org changed reviewers: + pthatcher@webrtc.org - deadbeef@webrtc.org, honghaiz@webrtc.org
Hi Peter, Can you review this? I've got two lgtms.
zhihuang@webrtc.org changed reviewers: + glaznev@webrtc.org - pthatcher@webrtc.org
Hi, I have a one-line change in Java api. Please take a look. Thanks.
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10471)
Description was changed from ========== Make the default value of rtcp-mux policy to required. Change the default value of rtcp-mux policy in RTCConfiguration. Refactor the peerconnectioninterface and webrtcsession unit tests. ========== to ========== Make the default value of rtcp-mux policy to required. Change the default value of rtcp-mux policy in RTCConfiguration. Refactor the peerconnectioninterface and webrtcsession unit tests. BUG=webrtc:6030 ==========
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hello Alex, I have a one line change in Java code. Please take a look. Thanks!
honghaiz@webrtc.org changed reviewers: + honghaiz@webrtc.org
lgtm https://codereview.webrtc.org/2043193003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnectioninterface.h (right): https://codereview.webrtc.org/2043193003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnectioninterface.h:246: rtcp_mux_policy = kRtcpMuxPolicyRequire; As this is the same as the default value, this is not needed.
The CQ bit was checked by zhihuang@webrtc.org
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/2043193003/#ps60001 (title: "Merge with new changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479925115417860,
"parent_rev": "e79ee7312784fadb9643840aebf87d8b41ecb770", "commit_rev":
"1ebeee84a13125a22f8c2a1cf7200bbfd2459f2b"}
Message was sent while issue was closed.
Description was changed from ========== Make the default value of rtcp-mux policy to required. Change the default value of rtcp-mux policy in RTCConfiguration. Refactor the peerconnectioninterface and webrtcsession unit tests. BUG=webrtc:6030 ========== to ========== Make the default value of rtcp-mux policy to required. Change the default value of rtcp-mux policy in RTCConfiguration. Refactor the peerconnectioninterface and webrtcsession unit tests. BUG=webrtc:6030 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make the default value of rtcp-mux policy to required. Change the default value of rtcp-mux policy in RTCConfiguration. Refactor the peerconnectioninterface and webrtcsession unit tests. BUG=webrtc:6030 ========== to ========== Make the default value of rtcp-mux policy to required. Change the default value of rtcp-mux policy in RTCConfiguration. Refactor the peerconnectioninterface and webrtcsession unit tests. BUG=webrtc:6030 Committed: https://crrev.com/4dfb8cef514bf030f08d61289543e32127366f12 Cr-Commit-Position: refs/heads/master@{#15217} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4dfb8cef514bf030f08d61289543e32127366f12 Cr-Commit-Position: refs/heads/master@{#15217} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
