Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1640993002:
Update API for Objective-C RTCPeerConnection. (Closed)
|
Created:
4 years, 10 months ago by hjon_webrtc Modified:
4 years, 10 months ago Reviewers:
tkchin_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate API for Objective-C RTCPeerConnection.
BUG=
Committed: https://crrev.com/f396f6085f9e4f16f37471a7828e3e31308c0d52
Cr-Commit-Position: refs/heads/master@{#11590}
Patch Set 1 #Patch Set 2 : Fix some style issues #
Total comments: 57
Patch Set 3 : Changes based on feedback #
Total comments: 42
Patch Set 4 : Changes based on feedback #Patch Set 5 : Update against master #
Total comments: 66
Patch Set 6 : Changes based on feedback #Patch Set 7 : Update against master #
Total comments: 8
Patch Set 8 : Changes based on feedback #
Messages
Total messages: 21 (5 generated)
hjon@webrtc.org changed reviewers: + tkchin@webrtc.org
@tkchin Could you give this a quick look as-is? Definitely need to fix some line lengths and I should give it another look after I take a break from it, so it might be a bit rough still.
At least style-wise, I think this is a better place than the first patch set.
https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:59: #"objc/RTCPeerConnectionDelegate.h", Remove Delegate/Observer. Delegate should be declared in RTCPeerConnection.h Observer should be declared in RTCPeerConnection+Private.h https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:11: #import "RTCPeerConnectionDelegate.h" Declare delegate in this class. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:37: @property(nonatomic, readonly) RTCSignalingState signalingState; missing enums defs? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:44: configuration:(nullable RTCConfiguration *)configuration doco what happens if nil configuration passed in. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:56: - (RTCDataChannel *)createDataChannelWithLabel:(NSString *)label objC naming -> dataChannelWithLabel:config: create reserved for CoreFoundation stuffs https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:59: - (void)createOfferWithConstraints:(RTCMediaConstraints *)constraints offerWithConstraints:completionHandler: answerWithConstraints:completionHandler: statsForMediaStreamTrack:outputLevel:completionHandler https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:27: NSString* const kRTCSessionDescriptionDelegateErrorDomain = @"RTCSDPError"; Should be kRTCPeerConnectionErrorDomain that's extern-ed in header. "org.webrtc.RTCPeerConnection" and then an error code for sdp error https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:32: class RTCCreateSessionDescriptionObserver following our previous precedence, let's not add RTC prefix to things already in webrtc namespace maybe CreateSessionDescriptionObserverAdapter similarly for rest of C++ interfaces here. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:39: peerConnection_ = peerConnection; c++ style: peer_connection_ (or peerconnection_), completion_handler etc. here and rest of file https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:39: peerConnection_ = peerConnection; doco why we need to keep a peer_connection strong pointer / retain cycle. suggest releasing the peerconnection and the block the moment we get OnSuccess/OnFailure and we're done with them. ditto for other observers here https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:43: void OnSuccess(SessionDescriptionInterface *desc) override { for sanity, explicitly set ivars to nil in dtor https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:49: delete desc; use a scoped_ptr instead of explicitly calling delete https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:57: userInfo:@{@"error" : str}]; use NSError standard keys here maybe you want the localized description key https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:89: userInfo:@{@"error" : str}]; ditto https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:144: configuration.nativeConfiguration; what happens here if configuration is nil? Will this be nullptr? Is that legal for CreatePeerConnection? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:164: const webrtc::SessionDescriptionInterface *sdi = nit: description not sdi https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:171: const webrtc::SessionDescriptionInterface *sdi = ditto https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:204: if (!ret) { collapse the two lines and log error if failure https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:323: return @"Stable"; nit: all caps STABLE HAVE_LOCAL_OFFER etc. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:384: return @"New"; ditto all caps https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:429: return @"New"; ditto all caps https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnectionDelegate.h (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:51: @protocol RTCPeerConnectionDelegate<NSObject> space after protocol name Delegate <NSObject> https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:53: /** Triggered when the SignalingState changed. */ nit: Triggered is a weird word. Called when the SignalingState changes? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:55: signalingStateChanged:(RTCSignalingState)stateChanged; minimum 4 spaces from left (break the : alignment) https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:78: receivedIceCandidate:(RTCIceCandidate *)candidate; ditto https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnectionObserver.mm (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionObserver.mm:22: peerConnection_ = peerConnection; peerconnection_ or peer_connection_ Any reason to wrap RTCPeerConnection instead of an id<RTCPeerConnectionDelegate>? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionObserver.mm:32: [peerConnection_.delegate peerConnection:peerConnection_ You'll want to pull out peerconnection to a local var first to make it a strong ref. Since it's weak you could theoretically end up with one call getting a peerconnection and another a nil. Here and elsewhere.
https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:59: #"objc/RTCPeerConnectionDelegate.h", On 2016/02/01 10:00:01, tkchin_webrtc wrote: > Remove Delegate/Observer. > Delegate should be declared in RTCPeerConnection.h > Observer should be declared in RTCPeerConnection+Private.h Done. Am I also understanding/guessing correctly that the Observer implementation should be in RTCPeerConnection.mm rather than in its own file? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:11: #import "RTCPeerConnectionDelegate.h" On 2016/02/01 10:00:01, tkchin_webrtc wrote: > Declare delegate in this class. Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:37: @property(nonatomic, readonly) RTCSignalingState signalingState; On 2016/02/01 10:00:01, tkchin_webrtc wrote: > missing enums defs? They were in RTCPeerConnectionDelegate.h, but I've moved them here. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:44: configuration:(nullable RTCConfiguration *)configuration On 2016/02/01 10:00:01, tkchin_webrtc wrote: > doco what happens if nil configuration passed in. > I thought a nil configuration was acceptable, but it looks like it would return a nullptr: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... Do you think we should allow a nil configuration and pass a default ourselves or just make it required? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:56: - (RTCDataChannel *)createDataChannelWithLabel:(NSString *)label On 2016/02/01 10:00:01, tkchin_webrtc wrote: > objC naming -> dataChannelWithLabel:config: > create reserved for CoreFoundation stuffs Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:59: - (void)createOfferWithConstraints:(RTCMediaConstraints *)constraints On 2016/02/01 10:00:01, tkchin_webrtc wrote: > > offerWithConstraints:completionHandler: > answerWithConstraints:completionHandler: > statsForMediaStreamTrack:outputLevel:completionHandler Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:27: NSString* const kRTCSessionDescriptionDelegateErrorDomain = @"RTCSDPError"; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > Should be kRTCPeerConnectionErrorDomain that's extern-ed in header. > "org.webrtc.RTCPeerConnection" > and then an error code for sdp error Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:32: class RTCCreateSessionDescriptionObserver On 2016/02/01 10:00:01, tkchin_webrtc wrote: > following our previous precedence, let's not add RTC prefix to things already in > webrtc namespace > > maybe CreateSessionDescriptionObserverAdapter > > similarly for rest of C++ interfaces here. Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:39: peerConnection_ = peerConnection; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > c++ style: peer_connection_ (or peerconnection_), completion_handler etc. > here and rest of file Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:39: peerConnection_ = peerConnection; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > doco why we need to keep a peer_connection strong pointer / retain cycle. > > suggest releasing the peerconnection and the block the moment we get > OnSuccess/OnFailure and we're done with them. > > ditto for other observers here Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:39: peerConnection_ = peerConnection; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > doco why we need to keep a peer_connection strong pointer / retain cycle. > > suggest releasing the peerconnection and the block the moment we get > OnSuccess/OnFailure and we're done with them. > > ditto for other observers here Whoops, looks like this is a case of me not paying enough attention when adapting the old implementation. We shouldn't need a strong pointer to the peer connection. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:43: void OnSuccess(SessionDescriptionInterface *desc) override { On 2016/02/01 10:00:01, tkchin_webrtc wrote: > for sanity, explicitly set ivars to nil in dtor Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:49: delete desc; On 2016/02/01 10:00:01, tkchin_webrtc wrote: > use a scoped_ptr instead of explicitly calling delete Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:57: userInfo:@{@"error" : str}]; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > use NSError standard keys > here maybe you want the localized description key Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:89: userInfo:@{@"error" : str}]; On 2016/02/01 10:00:01, tkchin_webrtc wrote: > ditto Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:144: configuration.nativeConfiguration; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > what happens here if configuration is nil? Will this be nullptr? Is that legal > for CreatePeerConnection? I guess I addressed this in RTCPeerConnection.h, but it looks like it's not legal. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:164: const webrtc::SessionDescriptionInterface *sdi = On 2016/02/01 10:00:02, tkchin_webrtc wrote: > nit: description not sdi Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:171: const webrtc::SessionDescriptionInterface *sdi = On 2016/02/01 10:00:01, tkchin_webrtc wrote: > ditto Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:204: if (!ret) { On 2016/02/01 10:00:01, tkchin_webrtc wrote: > collapse the two lines and log error if failure Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:323: return @"Stable"; On 2016/02/01 10:00:01, tkchin_webrtc wrote: > nit: all caps > STABLE > HAVE_LOCAL_OFFER > etc. Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:384: return @"New"; On 2016/02/01 10:00:01, tkchin_webrtc wrote: > ditto all caps Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:429: return @"New"; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > ditto all caps Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnectionDelegate.h (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:51: @protocol RTCPeerConnectionDelegate<NSObject> On 2016/02/01 10:00:02, tkchin_webrtc wrote: > space after protocol name > Delegate <NSObject> Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:53: /** Triggered when the SignalingState changed. */ On 2016/02/01 10:00:02, tkchin_webrtc wrote: > nit: Triggered is a weird word. Called when the SignalingState changes? Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:55: signalingStateChanged:(RTCSignalingState)stateChanged; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > minimum 4 spaces from left (break the : alignment) Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionDelegate.h:78: receivedIceCandidate:(RTCIceCandidate *)candidate; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > ditto Done. https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnectionObserver.mm (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionObserver.mm:22: peerConnection_ = peerConnection; On 2016/02/01 10:00:02, tkchin_webrtc wrote: > peerconnection_ or peer_connection_ > > Any reason to wrap RTCPeerConnection instead of an > id<RTCPeerConnectionDelegate>? Done. I like the idea of just wrapping id<RTCPeerConnectionDelegate>, but realized that we wouldn't have a reference to the peer connection itself, which is currently part of the delegate callback methods. I suppose the delegate interface could simply have a reference back to the peer connection, but I'm not sure that's any "cleaner". Thoughts? https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnectionObserver.mm:32: [peerConnection_.delegate peerConnection:peerConnection_ On 2016/02/01 10:00:02, tkchin_webrtc wrote: > You'll want to pull out peerconnection to a local var first to make it a strong > ref. Since it's weak you could theoretically end up with one call getting a > peerconnection and another a nil. Here and elsewhere. > Done.
https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:59: #"objc/RTCPeerConnectionDelegate.h", On 2016/02/04 00:58:37, hjon_webrtc wrote: > On 2016/02/01 10:00:01, tkchin_webrtc wrote: > > Remove Delegate/Observer. > > Delegate should be declared in RTCPeerConnection.h > > Observer should be declared in RTCPeerConnection+Private.h > > Done. > > Am I also understanding/guessing correctly that the Observer implementation > should be in RTCPeerConnection.mm rather than in its own file? Yep https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/20001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:44: configuration:(nullable RTCConfiguration *)configuration On 2016/02/04 00:58:37, hjon_webrtc wrote: > On 2016/02/01 10:00:01, tkchin_webrtc wrote: > > doco what happens if nil configuration passed in. > > > > I thought a nil configuration was acceptable, but it looks like it would return > a nullptr: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > > Do you think we should allow a nil configuration and pass a default ourselves or > just make it required? Make it req'd or provide delegating ctor. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection+Private.h (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection+Private.h:29: /** Triggered when the SignalingState changed. */ You can remove all the comments here because they're just overrides. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:13: #import "webrtc/api/objc/RTCPeerConnection.h" ? :) https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:26: extern NSString * _Nonnull const kRTCPeerConnectionErrorDomain; don't need _Nonnull here? https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:72: signalingStateChanged:(RTCSignalingState)stateChanged; didChangeSignalingState https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:74: /** Triggered when media is received on a new stream from remote peer. */ ditto nit triggered https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:83: - (void)peerConnectionNeedsRenegotiation:(RTCPeerConnection *)peerConnection; peerConnectionShouldNegotiate I don't think it's renegotation? Can you check? https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:87: iceConnectionStateChanged:(RTCIceConnectionState)newState; didChangeIceConnectionState https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:91: iceGatheringStateChanged:(RTCIceGatheringState)newState; didChangeIceGatheringState https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:95: receivedIceCandidate:(RTCIceCandidate *)candidate; didGenerateIceCandidate https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:115: /** The pending or current local description. */ ? Not pending I think. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:157: - (RTCDataChannel *)dataChannelWithLabel:(NSString *)label If possible, move this to a category below. Correspond it with the different specs. @interface RTCPeerConnection (DataChannel) @end @interface RTCPeerConnection (Stats) @end And then place the implementation methods in their own file RTCPeerConnection+DataChannel.mm RTCPeerConnection+Stats.mm https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:161: - (void)offerWithConstraints:(RTCMediaConstraints *)constraints nit: offerForConstraints / answerForConstraints https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:29: int const kRTCSessionDescriptionErrorCode = -1; kRTCPeerConnnectionSessionDescriptionError = -1; https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:54: Block_release(completion_handler_); ? ARC should be releasing for you. Set it to nil. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:66: Block_release(completion_handler_); ditto https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:74: PeerConnectionDelegateAdapter::PeerConnectionDelegateAdapter( dedent entire section? https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:86: RTCPeerConnection *strong_pc = peer_connection_; nit: just peer_connection = peer_connection_ will do. here and elsewhere https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:150: class SetSessionDescriptionObserverAdapter : Can we keep the session description bits together? (ie move this back up) https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:166: Block_release(completion_handler_); ditto. here and elsewhere https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:178: Block_release(completion_handler_); ditto
https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection+Private.h (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection+Private.h:29: /** Triggered when the SignalingState changed. */ On 2016/02/05 16:15:15, tkchin_webrtc wrote: > You can remove all the comments here because they're just overrides. Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:13: #import "webrtc/api/objc/RTCPeerConnection.h" On 2016/02/05 16:15:16, tkchin_webrtc wrote: > ? :) Oops. Fixed. :-) https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:26: extern NSString * _Nonnull const kRTCPeerConnectionErrorDomain; On 2016/02/05 16:15:16, tkchin_webrtc wrote: > don't need _Nonnull here? I didn't expect so, but I got this error without it: RTCPeerConnection.h:24:17: error: pointer is missing a nullability type specifier (_Nonnull, _Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness] Alternatively, I could make sure this line is within NS_ASSUME_NONNULL_BEGIN. Thoughts style-wise? https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:72: signalingStateChanged:(RTCSignalingState)stateChanged; On 2016/02/05 16:15:16, tkchin_webrtc wrote: > didChangeSignalingState Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:74: /** Triggered when media is received on a new stream from remote peer. */ On 2016/02/05 16:15:15, tkchin_webrtc wrote: > ditto nit triggered Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:83: - (void)peerConnectionNeedsRenegotiation:(RTCPeerConnection *)peerConnection; On 2016/02/05 16:15:15, tkchin_webrtc wrote: > peerConnectionShouldNegotiate > > I don't think it's renegotation? Can you check? You're right; the spec says negotiation, not renegotiation. Changed. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:87: iceConnectionStateChanged:(RTCIceConnectionState)newState; On 2016/02/05 16:15:16, tkchin_webrtc wrote: > didChangeIceConnectionState Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:91: iceGatheringStateChanged:(RTCIceGatheringState)newState; On 2016/02/05 16:15:15, tkchin_webrtc wrote: > didChangeIceGatheringState Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:95: receivedIceCandidate:(RTCIceCandidate *)candidate; On 2016/02/05 16:15:16, tkchin_webrtc wrote: > didGenerateIceCandidate Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:115: /** The pending or current local description. */ On 2016/02/05 16:15:16, tkchin_webrtc wrote: > ? Not pending I think. The spec mentions pending, but I'm not seeing an obvious indication that this is the case in the underlying C++, so I removed it. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:157: - (RTCDataChannel *)dataChannelWithLabel:(NSString *)label On 2016/02/05 16:15:15, tkchin_webrtc wrote: > If possible, move this to a category below. Correspond it with the different > specs. > > @interface RTCPeerConnection (DataChannel) > @end > > @interface RTCPeerConnection (Stats) > @end > > And then place the implementation methods in their own file > RTCPeerConnection+DataChannel.mm > RTCPeerConnection+Stats.mm Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:161: - (void)offerWithConstraints:(RTCMediaConstraints *)constraints On 2016/02/05 16:15:16, tkchin_webrtc wrote: > nit: offerForConstraints / answerForConstraints Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:29: int const kRTCSessionDescriptionErrorCode = -1; On 2016/02/05 16:15:16, tkchin_webrtc wrote: > kRTCPeerConnnectionSessionDescriptionError = -1; Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:54: Block_release(completion_handler_); On 2016/02/05 16:15:16, tkchin_webrtc wrote: > ? ARC should be releasing for you. Set it to nil. Done. Do you have a preference between setting to nil here or in the dtor? https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:66: Block_release(completion_handler_); On 2016/02/05 16:15:16, tkchin_webrtc wrote: > ditto Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:74: PeerConnectionDelegateAdapter::PeerConnectionDelegateAdapter( On 2016/02/05 16:15:16, tkchin_webrtc wrote: > dedent entire section? Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:86: RTCPeerConnection *strong_pc = peer_connection_; On 2016/02/05 16:15:16, tkchin_webrtc wrote: > nit: just peer_connection = peer_connection_ will do. > here and elsewhere Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:150: class SetSessionDescriptionObserverAdapter : On 2016/02/05 16:15:16, tkchin_webrtc wrote: > Can we keep the session description bits together? (ie move this back up) Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:166: Block_release(completion_handler_); On 2016/02/05 16:15:16, tkchin_webrtc wrote: > ditto. here and elsewhere Done. https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:178: Block_release(completion_handler_); On 2016/02/05 16:15:16, tkchin_webrtc wrote: > ditto Done.
https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:26: extern NSString * _Nonnull const kRTCPeerConnectionErrorDomain; On 2016/02/09 00:59:54, hjon_webrtc wrote: > On 2016/02/05 16:15:16, tkchin_webrtc wrote: > > don't need _Nonnull here? > > I didn't expect so, but I got this error without it: > > RTCPeerConnection.h:24:17: error: pointer is missing a nullability type > specifier (_Nonnull, _Nullable, or _Null_unspecified) > [-Werror,-Wnullability-completeness] > > Alternatively, I could make sure this line is within NS_ASSUME_NONNULL_BEGIN. > Thoughts style-wise? Yeah just move the NONNULL_BEGIN up https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection+Stats.mm (right): https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection+Stats.mm:36: if (completion_handler_) { in the same spirit as prior comments, assert/call/set-to-nil https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection+Stats.mm:39: Block_release(completion_handler_); Don't call Block_release, just set to nil https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:74: addedStream:(RTCMediaStream *)stream; didAddStream https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:78: removedStream:(RTCMediaStream *)stream; didRemoveStream https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:81: - (void)peerConnectionNeedsNegotiation:(RTCPeerConnection *)peerConnection; peerConnectionShouldNegotiate https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:110: /** The current remote description. */ ? Comment does not match https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:117: /** The pending or current remote description. */ What does it mean to be a pending description? I think it is okay to remove comments for things that are self-documenting. e.g. signalingState -> pretty clear what that is https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:156: completionHandler:(void (^) formatting (void (^)(RTCSessionDescription *, NSError, *))completionHandler https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:162: (RTCSessionDescription *sessionDescription, NSError *error))completionHandler; ditto https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:177: - (RTCDataChannel *)dataChannelWithLabel:(NSString *)label nit: dataChannelForLabel:configuration: https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:188: - (void)statsForMediaStreamTrack: nit: unless this is explicitly in the spec, statsForTrack:outputLevel:completionHandler seems more concise https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:192: (void (^)(NSArray<RTCStatsReport *> *stats))completionHandler; no error? https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:31: class CreateSessionDescriptionObserverAdapter I think you can move these to the +Private too https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:35: (RTCSessionDescription *sessionDescription, NSError *error)) { formatting. I can show you irl. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:46: RTCSessionDescription *session = C++ pointer style https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:50: completion_handler_(session, nil); nil out handler after calling it? It should only be called once. maybe it is better to assert at the beginning of OnSuccess/OnFailure, then call it without checking and detach it later. likewise for rest of file https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:55: NSString *str = [NSString stringForStdString:error]; ditto C++ pointer style https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:59: userInfo:@{NSLocalizedDescriptionKey : str}]; @{ NSLocal... : str } https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:61: completion_handler_(nil, err); ditto detach https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:108: PeerConnectionDelegateAdapter::~PeerConnectionDelegateAdapter() { explicitly nil out peer_connection_ https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:116: [peer_connection.delegate peerConnection:peer_connection alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:126: addedStream:mediaStream]; alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:135: removedStream:mediaStream]; alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:144: didOpenDataChannel:dataChannel]; alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:158: didChangeIceConnectionState:state]; alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:167: didChangeIceGatheringState:state]; alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:176: didGenerateIceCandidate:iceCandidate]; alignment https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:259: RTCLogError(@"Failed to create add stream: %@", stream); Failed to add stream https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:272: (void (^)(RTCSessionDescription *sessionDescription, NSError *error)) formatting https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:282: (void (^)(RTCSessionDescription *sessionDescription, NSError *error)) formatting https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:294: (completionHandler)); ( on previous line https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:302: (completionHandler)); ( on previous line https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:308: - (rtc::scoped_refptr<webrtc::PeerConnectionInterface>)nativePeerConnection { static methods first, move this after all the class methods
https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/40001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:26: extern NSString * _Nonnull const kRTCPeerConnectionErrorDomain; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > On 2016/02/09 00:59:54, hjon_webrtc wrote: > > On 2016/02/05 16:15:16, tkchin_webrtc wrote: > > > don't need _Nonnull here? > > > > I didn't expect so, but I got this error without it: > > > > RTCPeerConnection.h:24:17: error: pointer is missing a nullability type > > specifier (_Nonnull, _Nullable, or _Null_unspecified) > > [-Werror,-Wnullability-completeness] > > > > Alternatively, I could make sure this line is within NS_ASSUME_NONNULL_BEGIN. > > Thoughts style-wise? > > Yeah just move the NONNULL_BEGIN up Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection+Stats.mm (right): https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection+Stats.mm:36: if (completion_handler_) { On 2016/02/10 18:54:40, tkchin_webrtc wrote: > in the same spirit as prior comments, assert/call/set-to-nil Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection+Stats.mm:39: Block_release(completion_handler_); On 2016/02/10 18:54:40, tkchin_webrtc wrote: > Don't call Block_release, just set to nil Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:74: addedStream:(RTCMediaStream *)stream; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > didAddStream Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:78: removedStream:(RTCMediaStream *)stream; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > didRemoveStream Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:81: - (void)peerConnectionNeedsNegotiation:(RTCPeerConnection *)peerConnection; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > peerConnectionShouldNegotiate Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:110: /** The current remote description. */ On 2016/02/10 18:54:40, tkchin_webrtc wrote: > ? Comment does not match Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:117: /** The pending or current remote description. */ On 2016/02/10 18:54:40, tkchin_webrtc wrote: > What does it mean to be a pending description? > I think it is okay to remove comments for things that are self-documenting. > e.g. signalingState -> pretty clear what that is I removed most of the comments in this section, as I don't feel they provide any useful information beyond the property name. Let me know if you think any need to be added back. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:156: completionHandler:(void (^) On 2016/02/10 18:54:40, tkchin_webrtc wrote: > formatting > (void (^)(RTCSessionDescription *, > NSError, *))completionHandler Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:162: (RTCSessionDescription *sessionDescription, NSError *error))completionHandler; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > ditto Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:177: - (RTCDataChannel *)dataChannelWithLabel:(NSString *)label On 2016/02/10 18:54:40, tkchin_webrtc wrote: > nit: dataChannelForLabel:configuration: Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:188: - (void)statsForMediaStreamTrack: On 2016/02/10 18:54:40, tkchin_webrtc wrote: > nit: unless this is explicitly in the spec, > statsForTrack:outputLevel:completionHandler seems more concise Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.h:192: (void (^)(NSArray<RTCStatsReport *> *stats))completionHandler; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > no error? Feels a little surprising, but there's not an error in the spec nor in the C++. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:31: class CreateSessionDescriptionObserverAdapter On 2016/02/10 18:54:41, tkchin_webrtc wrote: > I think you can move these to the +Private too Acknowledged. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:35: (RTCSessionDescription *sessionDescription, NSError *error)) { On 2016/02/10 18:54:41, tkchin_webrtc wrote: > formatting. I can show you irl. Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:46: RTCSessionDescription *session = On 2016/02/10 18:54:40, tkchin_webrtc wrote: > C++ pointer style Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:50: completion_handler_(session, nil); On 2016/02/10 18:54:41, tkchin_webrtc wrote: > nil out handler after calling it? It should only be called once. > > maybe it is better to assert at the beginning of OnSuccess/OnFailure, then call > it without checking and detach it later. > > likewise for rest of file Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:55: NSString *str = [NSString stringForStdString:error]; On 2016/02/10 18:54:41, tkchin_webrtc wrote: > ditto C++ pointer style Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:59: userInfo:@{NSLocalizedDescriptionKey : str}]; On 2016/02/10 18:54:41, tkchin_webrtc wrote: > @{ NSLocal... : str } Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:61: completion_handler_(nil, err); On 2016/02/10 18:54:41, tkchin_webrtc wrote: > ditto detach Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:108: PeerConnectionDelegateAdapter::~PeerConnectionDelegateAdapter() { On 2016/02/10 18:54:41, tkchin_webrtc wrote: > explicitly nil out peer_connection_ Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:116: [peer_connection.delegate peerConnection:peer_connection On 2016/02/10 18:54:41, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:126: addedStream:mediaStream]; On 2016/02/10 18:54:41, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:135: removedStream:mediaStream]; On 2016/02/10 18:54:41, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:144: didOpenDataChannel:dataChannel]; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:158: didChangeIceConnectionState:state]; On 2016/02/10 18:54:41, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:167: didChangeIceGatheringState:state]; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:176: didGenerateIceCandidate:iceCandidate]; On 2016/02/10 18:54:40, tkchin_webrtc wrote: > alignment Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:259: RTCLogError(@"Failed to create add stream: %@", stream); On 2016/02/10 18:54:40, tkchin_webrtc wrote: > Failed to add stream Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:272: (void (^)(RTCSessionDescription *sessionDescription, NSError *error)) On 2016/02/10 18:54:40, tkchin_webrtc wrote: > formatting Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:282: (void (^)(RTCSessionDescription *sessionDescription, NSError *error)) On 2016/02/10 18:54:41, tkchin_webrtc wrote: > formatting Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:294: (completionHandler)); On 2016/02/10 18:54:40, tkchin_webrtc wrote: > ( on previous line Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:302: (completionHandler)); On 2016/02/10 18:54:40, tkchin_webrtc wrote: > ( on previous line Done. https://codereview.webrtc.org/1640993002/diff/80001/webrtc/api/objc/RTCPeerCo... webrtc/api/objc/RTCPeerConnection.mm:308: - (rtc::scoped_refptr<webrtc::PeerConnectionInterface>)nativePeerConnection { On 2016/02/10 18:54:41, tkchin_webrtc wrote: > static methods first, move this after all the class methods Done.
https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection.h:149: completionHandler:(void (^)(RTCSessionDescription *sessionDescription, Looks like this is actually 81 characters. I'm thinking it might be nicer to break the colon alignment so the block parameters can align, but what do you think would be best tradeoff in style?
lgrm https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... File webrtc/api/objc/RTCPeerConnection+DataChannel.mm (right): https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection+DataChannel.mm:11: #import "RTCPeerConnection+Private.h" webrtc/api https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... File webrtc/api/objc/RTCPeerConnection+Private.h (right): https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection+Private.h:11: #import "RTCPeerConnection.h" webrtc/api https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... File webrtc/api/objc/RTCPeerConnection+Stats.mm (right): https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection+Stats.mm:11: #import "RTCPeerConnection+Private.h" webrtc/api https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection+Stats.mm:20: class StatsObserverAdapter : public StatsObserver { nit: one blank line after namespace decl https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... File webrtc/api/objc/RTCPeerConnection.h (right): https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection.h:104: /** nit: move comment to first line /** <foo> * <bar> */ https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection.h:149: completionHandler:(void (^)(RTCSessionDescription *sessionDescription, On 2016/02/11 17:29:48, hjon_webrtc wrote: > Looks like this is actually 81 characters. I'm thinking it might be nicer to > break the colon alignment so the block parameters can align, but what do you > think would be best tradeoff in style? call it sdp like you have below? haha https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... File webrtc/api/objc/RTCPeerConnection.mm (right): https://codereview.webrtc.org/1640993002/diff/120001/webrtc/api/objc/RTCPeerC... webrtc/api/objc/RTCPeerConnection.mm:11: #import "RTCPeerConnection.h" webrtc/api/
The CQ bit was checked by hjon@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1640993002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1640993002/140001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
On 2016/02/11 20:43:56, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. Only full committers are accepted. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > Committers are members of the group "project-webrtc-committers". > Note that this has nothing to do with OWNERS files. lol lgtm
The CQ bit was checked by hjon@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1640993002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1640993002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Update API for Objective-C RTCPeerConnection. BUG= ========== to ========== Update API for Objective-C RTCPeerConnection. BUG= Committed: https://crrev.com/f396f6085f9e4f16f37471a7828e3e31308c0d52 Cr-Commit-Position: refs/heads/master@{#11590} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f396f6085f9e4f16f37471a7828e3e31308c0d52 Cr-Commit-Position: refs/heads/master@{#11590} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
