Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664233002:
Implement new PeerConnection certificate policy API in ObjC API (Closed)
DescriptionImplement new PeerConnection certificate policy API in ObjC API
BUG=webrtc:7088
Review-Url: https://codereview.webrtc.org/2664233002
Cr-Commit-Position: refs/heads/master@{#16424}
Committed: https://chromium.googlesource.com/external/webrtc/+/6741516e185f82a6a0e524179b5be5fde06f9d20
Patch Set 1 #Patch Set 2 : Fix bug where username/credential was not forwarded in constructor. #
Total comments: 6
Patch Set 3 : Remove unused branches and improve doc. #
Total comments: 8
Patch Set 4 : Review fixes. #
Total comments: 6
Patch Set 5 : Review private fix, rename to stringForTlsCertPolicy and style fixes. #
Total comments: 2
Patch Set 6 : Remove another default. #
Messages
Total messages: 45 (30 generated)
The CQ bit was checked by hnsl@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/...
hnsl@webrtc.org changed reviewers: + haysc@webrtc.org, leosha@webrtc.org, tkchin@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22250)
The CQ bit was checked by hnsl@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.
Description was changed from ========== Implement new PeerConnection certificate policy API in ObjC API BUG=webrtc:7088 ========== to ========== Implement new PeerConnection certificate policy API in ObjC API BUG=webrtc:7088 ==========
hnsl@webrtc.org changed reviewers: + denicija@webrtc.org
lgtm with few small remarks. https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:79: } else { I don't think this else will ever be executed. https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:103: } else { Same here https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceServer.h (right): https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceServer.h:34: /** TLS certificate policy to use if this RTCIceServer object is a TURN server. Move this to the next line to formulate the comment like so /** * TLS certificate policy to use... */
The CQ bit was checked by hnsl@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.
https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:79: } else { On 2017/02/01 14:26:53, denicija-webrtc wrote: > I don't think this else will ever be executed. Done. https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:103: } else { On 2017/02/01 14:26:53, denicija-webrtc wrote: > Same here Done. https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceServer.h (right): https://codereview.webrtc.org/2664233002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceServer.h:34: /** TLS certificate policy to use if this RTCIceServer object is a TURN server. On 2017/02/01 14:26:53, denicija-webrtc wrote: > Move this to the next line to formulate the comment like so > /** > * TLS certificate policy to use... > */ Done.
The CQ bit was checked by hnsl@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/2664233002/#ps40001 (title: "Remove unused branches and improve doc.")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12921)
haysc@, tkchin@: Your LGTM is still required.
hnsl@webrtc.org changed reviewers: + magjed@webrtc.org
magjed@: Hi, can you LGTM this?
https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:32: NSParameterAssert(urlStrings.count); you can remove this assert since it is checked in the designated initializer https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:39: - (instancetype)initWithURLStrings:(NSArray<NSString*>*)urlStrings ditto fix * https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:56: (unsigned long)_tlsCertPolicy]; would be nice to have this as a string so we don't have to look up the number in the header https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:73: iceServer.tls_cert_policy = recommend using a switch on _tlsCertPolicy and assigning it https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:92: if (nativeServer.tls_cert_policy == ditto switch https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceServer.h (right): https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCIceServer.h:48: - (instancetype)initWithURLStrings:(NSArray<NSString*>*)urlStrings fix * position to be what it was doco that it's a convenience ctor with a secure tlscertpolicy
The CQ bit was checked by hnsl@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/...
On 2017/02/01 17:50:55, hnsl1 wrote: > magjed@: Hi, can you LGTM this? Daniela and Zeke are already reviewing, I'll leave it to them.
magjed@webrtc.org changed reviewers: - magjed@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:51: - (NSString*)describeTlsCertPolicy:(RTCTlsCertPolicy)tlsCertPolicy { nit: (NSString *) to follow other places in the source: - (NSString *)stringForTlsCertPolicy; and place in private section https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:92: default: remove default case so that compiler will complain when new enum is added https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:118: default: ditto
https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:32: NSParameterAssert(urlStrings.count); On 2017/02/01 17:57:40, tkchin_webrtc wrote: > you can remove this assert since it is checked in the designated initializer Done. https://codereview.webrtc.org/2664233002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:39: - (instancetype)initWithURLStrings:(NSArray<NSString*>*)urlStrings On 2017/02/01 17:57:40, tkchin_webrtc wrote: > ditto fix * Done. https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:51: - (NSString*)describeTlsCertPolicy:(RTCTlsCertPolicy)tlsCertPolicy { On 2017/02/02 17:54:00, tkchin_webrtc wrote: > nit: (NSString *) > > to follow other places in the source: > - (NSString *)stringForTlsCertPolicy; > > and place in private section Done. https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:92: default: On 2017/02/02 17:54:00, tkchin_webrtc wrote: > remove default case so that compiler will complain when new enum is added Done. https://codereview.webrtc.org/2664233002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:118: default: On 2017/02/02 17:54:00, tkchin_webrtc wrote: > ditto Done.
The CQ bit was checked by hnsl@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/...
lgtm https://codereview.webrtc.org/2664233002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:66: default: remove default
https://codereview.webrtc.org/2664233002/diff/80001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm (right): https://codereview.webrtc.org/2664233002/diff/80001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCIceServer.mm:66: default: On 2017/02/02 19:51:12, tkchin_webrtc wrote: > remove default Done.
The CQ bit was checked by hnsl@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from denicija@webrtc.org, tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2664233002/#ps100001 (title: "Remove another default.")
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": 100001, "attempt_start_ts": 1486065248620950,
"parent_rev": "a5d94fff99815ef9a7e9a35fc5bc6bf17ae14f78", "commit_rev":
"6741516e185f82a6a0e524179b5be5fde06f9d20"}
Message was sent while issue was closed.
Description was changed from ========== Implement new PeerConnection certificate policy API in ObjC API BUG=webrtc:7088 ========== to ========== Implement new PeerConnection certificate policy API in ObjC API BUG=webrtc:7088 Review-Url: https://codereview.webrtc.org/2664233002 Cr-Commit-Position: refs/heads/master@{#16424} Committed: https://chromium.googlesource.com/external/webrtc/+/6741516e185f82a6a0e524179... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/6741516e185f82a6a0e524179... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
