Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690593002:
Relanding: Add the url attribute to the IceCandidate (Java Wrapper) (Closed)
DescriptionRelanding: Add the url attribute to the IceCandidate (Java Wrapper)
The url of the ICE server is added to the IceCandiate class.
This can be used to tell which server this candidate was gathered from.
BUG=webrtc:7128
Review-Url: https://codereview.webrtc.org/2690593002
Cr-Commit-Position: refs/heads/master@{#16675}
Committed: https://chromium.googlesource.com/external/webrtc/+/8e32cd247d05b330ddf3b98d265f0fedeaa3834b
Patch Set 1 #Patch Set 2 : Fix the tests. #Patch Set 3 : Merge. #Patch Set 4 : Mark the old constructor deprecated. #
Total comments: 1
Patch Set 5 : Remove the new constructor for IceCanddiate. #Patch Set 6 : Revert unnecessay changes. #Patch Set 7 : Add new ctor back and mark it private. #
Messages
Total messages: 56 (36 generated)
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: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
Patchset #2 (id:20001) has been deleted
zhihuang@webrtc.org changed reviewers: + magjed@webrtc.org
Hi, I have some changes on the Java wrapper. Please take a look. Thanks, -Zhi
webrtc/examples/ and webrtc/sdk/android lgtm
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.
The CQ bit was checked by zhihuang@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/2690593002/#ps60001 (title: "Merge.")
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": 1487023312594620,
"parent_rev": "02c739f4ef9735fb1453a2499e74c24f3cea4453", "commit_rev":
"8586c8ee88bfbf97dd310efb2034ac4e125d4865"}
Message was sent while issue was closed.
Description was changed from ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 ========== to ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.webrtc.org/2692993002/ by deadbeef@webrtc.org. The reason for reverting is: Breaks downstream application's build.
Message was sent while issue was closed.
Description was changed from ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... ========== to ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... ==========
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by zhihuang@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/2690593002/#ps120001 (title: "Mark the old constructor deprecated.")
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
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by zhihuang@webrtc.org
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": 120001, "attempt_start_ts": 1487102067568410,
"parent_rev": "bb1de873b94bb56e5be833fd3d2b11c06e855fd7", "commit_rev":
"45efce01c76b2bd801a68ba58a4bbe85cdc2aa23"}
The CQ bit was unchecked by zhihuang@webrtc.org
Message was sent while issue was closed.
Description was changed from ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... ========== to ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Original-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16615} Committed: https://chromium.googlesource.com/external/webrtc/+/45efce01c76b2bd801a68ba58... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/45efce01c76b2bd801a68ba58...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:120001) has been created in https://codereview.webrtc.org/2699533002/ by deadbeef@webrtc.org. The reason for reverting is: Breaks AppRTCMobile interoperability. The ICE candidate URL shouldn't be signaled between endpoints, it's only there for informational purposes..
Message was sent while issue was closed.
Patchset #6 (id:160001) has been deleted
Message was sent while issue was closed.
Hi, I have updated this CL since it breaks some downstream applications. Please take another look. Thanks!
Message was sent while issue was closed.
Description was changed from ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Original-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16615} Committed: https://chromium.googlesource.com/external/webrtc/+/45efce01c76b2bd801a68ba58... ========== to ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Original-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16615} Committed: https://chromium.googlesource.com/external/webrtc/+/45efce01c76b2bd801a68ba58... ==========
On 2017/02/14 23:35:12, Zhi Huang wrote: > Hi, > I have updated this CL since it breaks some downstream applications. Please take > another look. Thanks! Please revert the revert instead using the "Revert Pachset" button, and add the update as a new Patch Set in the reland CL. That way the CL description will be generated for you in a nice uniform way, and it's easier to track what happened in git.
On 2017/02/15 12:17:17, magjed_webrtc wrote: > On 2017/02/14 23:35:12, Zhi Huang wrote: > > Hi, > > I have updated this CL since it breaks some downstream applications. Please > take > > another look. Thanks! > > Please revert the revert instead using the "Revert Pachset" button, and add the > update as a new Patch Set in the reland CL. That way the CL description will be > generated for you in a nice uniform way, and it's easier to track what happened > in git. Some people prefer using the original CL; in this situation, you can easily see just the diff from patchset #4. There's a big thread about this topic on chromium-dev, and there's no consensus, since there are different situations where each approach is preferable. Though, I would suggest that when relanding with the original CL, you modify the description to indicate that it's being relanded.
Description was changed from ========== Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Original-Commit-Position: refs/heads/master@{#16593} Committed: https://chromium.googlesource.com/external/webrtc/+/8586c8ee88bfbf97dd310efb2... Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16615} Committed: https://chromium.googlesource.com/external/webrtc/+/45efce01c76b2bd801a68ba58... ========== to ========== Relanding: Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 ==========
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
lgtm
On 2017/02/15 18:30:06, Taylor Brandstetter wrote: > On 2017/02/15 12:17:17, magjed_webrtc wrote: > > On 2017/02/14 23:35:12, Zhi Huang wrote: > > > Hi, > > > I have updated this CL since it breaks some downstream applications. Please > > take > > > another look. Thanks! > > > > Please revert the revert instead using the "Revert Pachset" button, and add > the > > update as a new Patch Set in the reland CL. That way the CL description will > be > > generated for you in a nice uniform way, and it's easier to track what > happened > > in git. > > Some people prefer using the original CL; in this situation, you can easily see > just the diff from patchset #4. There's a big thread about this topic on > chromium-dev, and there's no consensus, since there are different situations > where each approach is preferable. > > Though, I would suggest that when relanding with the original CL, you modify the > description to indicate that it's being relanded. Acknowledged.
https://codereview.webrtc.org/2690593002/diff/120001/webrtc/sdk/android/api/o... File webrtc/sdk/android/api/org/webrtc/IceCandidate.java (right): https://codereview.webrtc.org/2690593002/diff/120001/webrtc/sdk/android/api/o... webrtc/sdk/android/api/org/webrtc/IceCandidate.java:31: public IceCandidate(String sdpMid, int sdpMLineIndex, String sdp, String serverUrl) { Did this new ctor cause any problem? I prefer the old approach with a new ctor over having a final String set to "" and then overriding it anyway in jni.
On 2017/02/16 20:42:43, magjed_webrtc wrote: > https://codereview.webrtc.org/2690593002/diff/120001/webrtc/sdk/android/api/o... > File webrtc/sdk/android/api/org/webrtc/IceCandidate.java (right): > > https://codereview.webrtc.org/2690593002/diff/120001/webrtc/sdk/android/api/o... > webrtc/sdk/android/api/org/webrtc/IceCandidate.java:31: public > IceCandidate(String sdpMid, int sdpMLineIndex, String sdp, String serverUrl) { > Did this new ctor cause any problem? I prefer the old approach with a new ctor > over having a final String set to "" and then overriding it anyway in jni. Some downstream apps are using the old ctor so if we want to change the ctor, we need to keep both of them for a while. The new ctor is only used internally by the native WebRTC code and it's not expected to be called by the application. The new parameter serverUrl is collected by the WebRTC so that the app can tell which candidate comes from where. It doesn't make much sense for the app to pass the serverUrl in.
On 2017/02/16 20:55:04, Zhi Huang wrote: > On 2017/02/16 20:42:43, magjed_webrtc wrote: > > > https://codereview.webrtc.org/2690593002/diff/120001/webrtc/sdk/android/api/o... > > File webrtc/sdk/android/api/org/webrtc/IceCandidate.java (right): > > > > > https://codereview.webrtc.org/2690593002/diff/120001/webrtc/sdk/android/api/o... > > webrtc/sdk/android/api/org/webrtc/IceCandidate.java:31: public > > IceCandidate(String sdpMid, int sdpMLineIndex, String sdp, String serverUrl) { > > Did this new ctor cause any problem? I prefer the old approach with a new ctor > > over having a final String set to "" and then overriding it anyway in jni. > > Some downstream apps are using the old ctor so if we want to change the ctor, we > need to keep both of them for a while. > > The new ctor is only used internally by the native WebRTC code and it's not > expected to be called by the application. > The new parameter serverUrl is collected by the WebRTC so that the app can tell > which candidate comes from where. > It doesn't make much sense for the app to pass the serverUrl in. Ok, I think it's clearer if you add a new ctor, even if it's a couple more lines. Make the new ctor private and add a comment that it will only be called from jni, and don't mark the old ctor deprecated. lgtm if you do that.
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.
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2690593002/#ps200001 (title: "Add new ctor back and mark it private.")
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": 200001, "attempt_start_ts": 1487364078268850,
"parent_rev": "d5f2b6f7c6d1a3cf32fea6629692aec577df3372", "commit_rev":
"8e32cd247d05b330ddf3b98d265f0fedeaa3834b"}
Message was sent while issue was closed.
Description was changed from ========== Relanding: Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 ========== to ========== Relanding: Add the url attribute to the IceCandidate (Java Wrapper) The url of the ICE server is added to the IceCandiate class. This can be used to tell which server this candidate was gathered from. BUG=webrtc:7128 Review-Url: https://codereview.webrtc.org/2690593002 Cr-Commit-Position: refs/heads/master@{#16675} Committed: https://chromium.googlesource.com/external/webrtc/+/8e32cd247d05b330ddf3b98d2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001) as https://chromium.googlesource.com/external/webrtc/+/8e32cd247d05b330ddf3b98d2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
