Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3012843002:
android: add IceServer.urls field (Closed)
|
Created:
3 years, 3 months ago by korniltsev Modified:
3 years, 3 months ago Reviewers:
sakal CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, diogor Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
Descriptionandroid: add IceServer.urls field
This makes api more consistent with ios and native library
BUG=None
Review-Url: https://codereview.webrtc.org/3012843002
Cr-Commit-Position: refs/heads/master@{#19770}
Committed: https://chromium.googlesource.com/external/webrtc/+/0ea0310b89a35e20072b3cad6712c46eba869228
Patch Set 1 #
Total comments: 16
Patch Set 2 : urls.isEmpty check inside builder's constructor #
Total comments: 3
Patch Set 3 : urls nulcheck in builer #Patch Set 4 : android: add IceServer.urls field. squash. rebase on master. #
Total comments: 1
Patch Set 5 : jstring -> jobject #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== android: add IceServer.urls field This makes api more consistent with ios and native library BUG=None ========== to ========== android: add IceServer.urls field This makes api more consistent with ios and native library BUG=None ==========
korniltsev.anatoly@gmail.com changed reviewers: + sakal@webrtc.org
https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:136: this(uri, null, username, password, tlsCertPolicy, hostname, null); Collections.singletonList(uri) instead of null https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:141: if (uri == null && urls == null) { I don't think either of them should be null, use ||. Also add the check for username, password and hostname. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:142: throw new NullPointerException("uri == null && urls == null"); IllegalArgumentException instead of NullPointerException https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:154: return uri != null ? uri Ignore uri here and just print urls. Ensure that List.toString returns a sensible presentation of the urls. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:164: return new Builder(urls); Throw IllegalArgumentException if urls doesn't contain at least one url or it is null. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:206: null, urls, username, password, tlsCertPolicy, hostname, tlsAlpnProtocols); Pass in urls.get(0) instead of null. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/src/jni/pc... File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/src/jni/pc... webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:387: server.uri = JavaToStdString(jni, uri); Just ignore the uri paramater and set server.uri to the first of the urls.
https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:136: this(uri, null, username, password, tlsCertPolicy, hostname, null); On 2017/09/07 07:46:36, sakal wrote: > Collections.singletonList(uri) instead of null Done. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:141: if (uri == null && urls == null) { On 2017/09/07 07:46:36, sakal wrote: > I don't think either of them should be null, use ||. Also add the check for > username, password and hostname. Done. Also added urls.isEmpty() check. Also checked urls items are not null. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:142: throw new NullPointerException("uri == null && urls == null"); On 2017/09/07 07:46:36, sakal wrote: > IllegalArgumentException instead of NullPointerException Done. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:154: return uri != null ? uri On 2017/09/07 07:46:36, sakal wrote: > Ignore uri here and just print urls. Ensure that List.toString returns a > sensible presentation of the urls. Not quite sure about the second part. I know ArrayList and LinkedList return proper representation, but there are probably other implementations which don't and I don't understand how can I ensure it. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:164: return new Builder(urls); On 2017/09/07 07:46:36, sakal wrote: > Throw IllegalArgumentException if urls doesn't contain at least one url or it is > null. The check is inside IceServer's constructor. Should I duplicate it so it fails faster? https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:206: null, urls, username, password, tlsCertPolicy, hostname, tlsAlpnProtocols); On 2017/09/07 07:46:36, sakal wrote: > Pass in urls.get(0) instead of null. Done. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/src/jni/pc... File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/src/jni/pc... webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:387: server.uri = JavaToStdString(jni, uri); On 2017/09/07 07:46:36, sakal wrote: > Just ignore the uri paramater and set server.uri to the first of the urls. Done.
https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (right): https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:154: return uri != null ? uri On 2017/09/07 20:33:46, korniltsev wrote: > On 2017/09/07 07:46:36, sakal wrote: > > Ignore uri here and just print urls. Ensure that List.toString returns a > > sensible presentation of the urls. > > Not quite sure about the second part. I know ArrayList and LinkedList return > proper representation, but there are probably other implementations which > don't and I don't understand how can I ensure it. It's enough as long as this works in practice with implementation that we use. https://codereview.webrtc.org/3012843002/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/PeerConnection.java:164: return new Builder(urls); On 2017/09/07 20:33:46, korniltsev wrote: > On 2017/09/07 07:46:36, sakal wrote: > > Throw IllegalArgumentException if urls doesn't contain at least one url or it > is > > null. > > The check is inside IceServer's constructor. Should I duplicate it so it fails > faster? urls.isEmpty will throw a null pointer exception now if urls is null. We need to check it here because otherwise urls.get(0) will throw. Can you add a check for urls == null || urls.isEmpty() https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:380: server.urls = JavaToStdVectorStrings(jni, urls); Is it safe to not set server.uri at all or should we set it to the first of the urls?
On 2017/09/08 08:22:55, sakal wrote: > urls.isEmpty will throw a null pointer exception now if urls is null. We need to > check it here because otherwise urls.get(0) will throw. Can you add a check for > urls == null || urls.isEmpty() Done
https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:380: server.urls = JavaToStdVectorStrings(jni, urls); On 2017/09/08 08:22:55, sakal wrote: > Is it safe to not set server.uri at all or should we set it to the first of the > urls? At first we try to parse urls, if it is empty then we try uri. See iceserverparsing.cc https://chromium.googlesource.com/external/webrtc/trunk/webrtc/+/refs/heads/m... Also objc sdk does exactly the same. https://chromium.googlesource.com/external/webrtc/+/master/webrtc/sdk/objc/Fr...
On 2017/09/08 16:20:20, korniltsev wrote: > https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... > File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): > > https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... > webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:380: server.urls = > JavaToStdVectorStrings(jni, urls); > On 2017/09/08 08:22:55, sakal wrote: > > Is it safe to not set server.uri at all or should we set it to the first of > the > > urls? > > At first we try to parse urls, if it is empty then we try uri. See > iceserverparsing.cc > https://chromium.googlesource.com/external/webrtc/trunk/webrtc/+/refs/heads/m... > Also objc sdk does exactly the same. > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/sdk/objc/Fr... lgtm I think this needs to be rebased though.
rebased on master
https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): https://codereview.webrtc.org/3012843002/diff/20001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:380: server.urls = JavaToStdVectorStrings(jni, urls); On 2017/09/08 16:20:20, korniltsev wrote: > On 2017/09/08 08:22:55, sakal wrote: > > Is it safe to not set server.uri at all or should we set it to the first of > the > > urls? > > At first we try to parse urls, if it is empty then we try uri. See > iceserverparsing.cc > https://chromium.googlesource.com/external/webrtc/trunk/webrtc/+/refs/heads/m... > Also objc sdk does exactly the same. > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/sdk/objc/Fr... Acknowledged. https://codereview.webrtc.org/3012843002/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/pc/java_native_conversion.cc (right): https://codereview.webrtc.org/3012843002/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:369: jstring urls = reinterpret_cast<jstring>( Remove reinterpret_cast here. This is no longer jstring but a general jobject.
On 2017/09/11 11:13:08, sakal wrote: > > https://codereview.webrtc.org/3012843002/diff/60001/webrtc/sdk/android/src/jn... > webrtc/sdk/android/src/jni/pc/java_native_conversion.cc:369: jstring urls = > reinterpret_cast<jstring>( > Remove reinterpret_cast here. This is no longer jstring but a general jobject. Oops. Done.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/3012843002/#ps80001 (title: "jstring -> jobject")
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": 80001, "attempt_start_ts": 1505135297154590,
"parent_rev": "e0406fd955239769c02a16f13e5739ae6b9d4bb8", "commit_rev":
"0ea0310b89a35e20072b3cad6712c46eba869228"}
Message was sent while issue was closed.
Description was changed from ========== android: add IceServer.urls field This makes api more consistent with ios and native library BUG=None ========== to ========== android: add IceServer.urls field This makes api more consistent with ios and native library BUG=None Review-Url: https://codereview.webrtc.org/3012843002 Cr-Commit-Position: refs/heads/master@{#19770} Committed: https://chromium.googlesource.com/external/webrtc/+/0ea0310b89a35e20072b3cad6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/0ea0310b89a35e20072b3cad6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
