Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688943003:
Add the URL attribute to cricket::Candiate. (Objc wrapper) (Closed)
DescriptionAdd the URL attribute to cricket::Candiate. (Objc 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/2688943003
Cr-Commit-Position: refs/heads/master@{#16652}
Committed: https://chromium.googlesource.com/external/webrtc/+/d7e771da7bdde0ac41514a1c695149094abacf5e
Patch Set 1 #Patch Set 2 : Merge. #Patch Set 3 : Fix the format. #
Total comments: 6
Patch Set 4 : Keep the old constructor. #Patch Set 5 : Fix format and typo. #
Total comments: 10
Patch Set 6 : Revert unnecessary changes. #
Messages
Total messages: 46 (33 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/...
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
C++ changes 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/...
zhihuang@webrtc.org changed reviewers: + tkchin@webrtc.org
Hi, I've made some changes on Objc. Please take a look. Thanks!
zhihuang@webrtc.org changed reviewers: + tkchin@webrtc.org
Hi, I've made some changes on Objc. Please take a look. Thanks!
https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h (right): https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:46: serverUrl:(nullable NSString *)serverUrl This is a breaking API change. Can we keep the old ctor and mark it deprecated? And have the old one call into this new designated one?
https://codereview.webrtc.org/2688943003/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/RTCIceCandidate+JSON.m (right): https://codereview.webrtc.org/2688943003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/RTCIceCandidate+JSON.m:21: static NSString const *kRTCICECandidatesServerUrlKey = @"url"; Candidate server URLs don't need to be signaled, so this file doesn't need to be changed https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h (right): https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:46: serverUrl:(nullable NSString *)serverUrl On 2017/02/13 23:29:50, tkchin_webrtc wrote: > This is a breaking API change. Can we keep the old ctor and mark it deprecated? > And have the old one call into this new designated one? Also, since the URL is only something that comes out of the PeerConnection, not into it, the constructor that takes a URL should be private.
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: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
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/...
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:80001) has been deleted
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/...
Hi, Please take another look. Thanks! https://codereview.webrtc.org/2688943003/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/RTCIceCandidate+JSON.m (right): https://codereview.webrtc.org/2688943003/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/RTCIceCandidate+JSON.m:21: static NSString const *kRTCICECandidatesServerUrlKey = @"url"; On 2017/02/13 23:53:00, Taylor Brandstetter wrote: > Candidate server URLs don't need to be signaled, so this file doesn't need to be > changed Done. https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h (right): https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:46: serverUrl:(nullable NSString *)serverUrl On 2017/02/13 23:29:50, tkchin_webrtc wrote: > This is a breaking API change. Can we keep the old ctor and mark it deprecated? > And have the old one call into this new designated one? Oh, you are right. We should keep both constructors for now. Taylor: If we make the constructor with "url" private, that means we won't deprecate the other one, right? Because the constructor would be called in other places like (https://cs.corp.google.com/piper///depot/google3/third_party/objective_c/Tach...) and we need at least one public constructor.
https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h (right): https://codereview.webrtc.org/2688943003/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:46: serverUrl:(nullable NSString *)serverUrl On 2017/02/14 19:09:08, Zhi Huang wrote: > On 2017/02/13 23:29:50, tkchin_webrtc wrote: > > This is a breaking API change. Can we keep the old ctor and mark it > deprecated? > > And have the old one call into this new designated one? > > Oh, you are right. We should keep both constructors for now. > > Taylor: > If we make the constructor with "url" private, that means we won't deprecate the > other one, right? Right. An application shouldn't ever need to construct a candidate with a URL, so the existing constructor is good.
The CQ bit was checked by zhihuang@webrtc.org to run a CQ dry run
https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm (right): https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm:34: - (instancetype)initWithSdpAndServerUrl:(NSString *)sdp remove this ctor since it's only ever used privately https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm:43: _serverUrl = [serverUrl copy]; remove https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm:64: return [self initWithSdpAndServerUrl:[NSString stringForStdString:sdp] RTCIceCandidate *candidate = [self initWithSdp:sdpMLineIndex:sdpMid:]; candidate->_serverUrl = [NSString stringForStdString:candidate->server_url()]; return candidate; alternately, make the serverUrl property readwrite in the +Private file, and use candidate.serverUrl = ... https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h (right): https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:36: @property(nonatomic, readonly) NSString *serverUrl; nonatomic, readonly, nullable unless there is always a server url now? https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:41: * Deprecated. Use initWithSdpAndServerUrl instead. Revert changes in this file other than the addition of the property
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.
Patchset #6 (id:140001) has been deleted
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/...
Please take another look. Thanks. https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm (right): https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm:34: - (instancetype)initWithSdpAndServerUrl:(NSString *)sdp On 2017/02/14 21:58:55, tkchin_webrtc wrote: > remove this ctor since it's only ever used privately Done. https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm:43: _serverUrl = [serverUrl copy]; On 2017/02/14 21:58:55, tkchin_webrtc wrote: > remove Done. https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Classes/RTCIceCandidate.mm:64: return [self initWithSdpAndServerUrl:[NSString stringForStdString:sdp] On 2017/02/14 21:58:55, tkchin_webrtc wrote: > RTCIceCandidate *candidate = [self initWithSdp:sdpMLineIndex:sdpMid:]; > candidate->_serverUrl = [NSString stringForStdString:candidate->server_url()]; > return candidate; > > alternately, make the serverUrl property readwrite in the +Private file, and use > candidate.serverUrl = ... Done. https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h (right): https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:36: @property(nonatomic, readonly) NSString *serverUrl; On 2017/02/14 21:58:55, tkchin_webrtc wrote: > nonatomic, readonly, nullable > unless there is always a server url now? Done. https://codereview.webrtc.org/2688943003/diff/120001/webrtc/sdk/objc/Framewor... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceCandidate.h:41: * Deprecated. Use initWithSdpAndServerUrl instead. On 2017/02/14 21:58:55, tkchin_webrtc wrote: > Revert changes in this file other than the addition of the property Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
deadbeef@chromium.org changed reviewers: + deadbeef@chromium.org
lgtm
lgtm
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/2688943003/#ps160001 (title: "Revert unnecessary 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": 160001, "attempt_start_ts": 1487269043811920,
"parent_rev": "dbeeb701a28971c92458a2f970c523bf66f409da", "commit_rev":
"d7e771da7bdde0ac41514a1c695149094abacf5e"}
Message was sent while issue was closed.
Description was changed from ========== Add the URL attribute to cricket::Candiate. (Objc 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 cricket::Candiate. (Objc 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/2688943003 Cr-Commit-Position: refs/heads/master@{#16652} Committed: https://chromium.googlesource.com/external/webrtc/+/d7e771da7bdde0ac41514a1c6... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/d7e771da7bdde0ac41514a1c6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
