Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1354913002:
Make PeerConnectionTest.doTest wait for ice candidates (Closed)
|
Created:
5 years, 3 months ago by perkj_webrtc Modified:
5 years, 3 months ago Reviewers:
magjed_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake PeerConnectionTest.doTest wait for ice candidates
This change the PeerConnectionTest.doTest wait for at least one ice candidate and also make sure the list of candidates in gotIceCandidates is synchronized.
BUG=webrtc:5010
Committed: https://crrev.com/780be751902e38687e578b4429995d783dcbae76
Cr-Commit-Position: refs/heads/master@{#9997}
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Addressed comments #
Total comments: 9
Patch Set 3 : Addressed comments.- #
Total comments: 2
Patch Set 4 : Added import List #Messages
Total messages: 20 (8 generated)
perkj@webrtc.org changed reviewers: + magjed@webrtc.org
Please?
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
https://codereview.webrtc.org/1354913002/diff/40001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1354913002/diff/40001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:384: public void waitForAtLeastOneIceCandidate() { I would prefer gotIceCandidates.wait() instead of Thread.sleep(). You can also return a list of candidates directly here.
ptal
https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:385: public LinkedList<IceCandidate> getAtLeastOneIceCandidate() throws InterruptedException { The return type should be List<IceCandidate>. https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:387: while(gotIceCandidates.isEmpty()) Braces should be used even for single statements according to the Java style guide. Also, s/while(/while (/g. https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:534: ObserverExpectations candidateExpectations = This new variable is not used? https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:654: LinkedList<IceCandidate> offeringCandidates = offeringExpectations.getAtLeastOneIceCandidate(); ditto: s/LinkedList/List/g, or remove |offeringCandidates| and inline call in for loop. https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:661: LinkedList<IceCandidate> answeringCandidates = ditto: s/LinkedList/List/g, or remove |answeringCandidates| and inline call in for loop.
https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:385: public LinkedList<IceCandidate> getAtLeastOneIceCandidate() throws InterruptedException { On 2015/09/18 16:20:56, magjed_webrtc wrote: > The return type should be List<IceCandidate>. Done. https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:387: while(gotIceCandidates.isEmpty()) On 2015/09/18 16:20:57, magjed_webrtc wrote: > Braces should be used even for single statements according to the Java style > guide. Also, s/while(/while (/g. Done. https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:654: LinkedList<IceCandidate> offeringCandidates = offeringExpectations.getAtLeastOneIceCandidate(); On 2015/09/18 16:20:56, magjed_webrtc wrote: > ditto: s/LinkedList/List/g, or remove |offeringCandidates| and inline call in > for loop. Done. https://codereview.webrtc.org/1354913002/diff/60001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:661: LinkedList<IceCandidate> answeringCandidates = On 2015/09/18 16:20:57, magjed_webrtc wrote: > ditto: s/LinkedList/List/g, or remove |answeringCandidates| and inline call in > for loop. Done.
lgtm https://codereview.webrtc.org/1354913002/diff/80001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1354913002/diff/80001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:75: private final LinkedList<IceCandidate> gotIceCandidates = nit: the type of |gotIceCandidates| should be List<IceCandidate>
https://codereview.webrtc.org/1354913002/diff/80001/talk/app/webrtc/java/test... File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1354913002/diff/80001/talk/app/webrtc/java/test... talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:75: private final LinkedList<IceCandidate> gotIceCandidates = On 2015/09/21 08:15:29, magjed_webrtc wrote: > nit: the type of |gotIceCandidates| should be List<IceCandidate> Sure, but this is the same as all others. I will ignore this comment.
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354913002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354913002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/6829)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by perkj@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/1354913002/#ps120001 (title: "Added import List")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1354913002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1354913002/120001
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/780be751902e38687e578b4429995d783dcbae76 Cr-Commit-Position: refs/heads/master@{#9997} |
