Add the callback of send operation. #458

NathanLi · 2016-08-14T13:28:17Z

Add feature:
Callback of send data.

Add feature: Callback of send data.

ghost · 2016-08-14T13:28:35Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

NathanLi · 2016-08-15T09:51:26Z

I signed in CLA. Thank you.

ghost · 2016-08-15T09:55:11Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

nlutsenko · 2016-08-23T22:04:24Z

This looks like a nice idea, will review soon and provide some feedback on naming and polish so we can get this merged in!

nlutsenko

This is a great idea, and I absolutely love it!
Let's work together to get this merged in!

There are a lot of minor nits that need to be fixed before we feel comfortable merging it in.

On top of that, please revert all whitespace additions in SRWebSocket.m lines 1398-1465

nlutsenko · 2016-10-31T17:49:39Z

SocketRocket/SRWebSocket.h

@@ -59,6 +59,8 @@ extern NSString *const SRHTTPResponseErrorKey;

 @protocol SRWebSocketDelegate;

+typedef void (^SRSendCompleteBlock)(NSError * _Nullable error);


Nit: Rename to SRSendCompletionBlock.

nlutsenko · 2016-10-31T17:50:32Z

SocketRocket/SRWebSocket.h

+
+ @return `YES` if the string was scheduled to send, otherwise - `NO`.
+ */
+- (BOOL)sendString:(NSString *)string complete:(SRSendCompleteBlock)complete;


Let's improve the naming here to the following. Also, will improve the Swift imported method.

- (BOOL)sendString:(NSString *)string completion:(nullable SRSendCompletionBlock)completion NS_SWIFT_NAME(send(string:completion:));

The above code block also ensures that everyone can pass nil to the completion argument, since it looks like you are validating it anyway.

nlutsenko · 2016-10-31T17:50:48Z

SocketRocket/SRWebSocket.h

+ Send a UTF-8 String to the server.
+
+ @param string String to send.
+ @param complete  The call back of send result.


Nit: Double space after complete

nlutsenko · 2016-10-31T17:51:08Z

SocketRocket/SRWebSocket.h

@@ -279,6 +281,18 @@ extern NSString *const SRHTTPResponseErrorKey;
 - (BOOL)sendString:(NSString *)string error:(NSError **)error NS_SWIFT_NAME(send(string:));

 /**
+ Send a UTF-8 String to the server.
+
+ @param string String to send.


Nit: Please align String to send with the next parameter description for better readability.

nlutsenko · 2016-10-31T17:51:51Z

SocketRocket/SRWebSocket.h

+
+ @param string String to send.
+ @param complete  The call back of send result.
+ If an error occurs, this block will invoked with an `NSerror` object cantaining information about the error; Otherwise


Nit: NSError
Nit: containing
Nit: , otherwise this block will be invoked

nlutsenko · 2016-10-31T18:07:42Z

SocketRocket/SRWebSocket.m

        }
        SRDebugLog(message);
        return NO;
    }
-
+    


Nit: Revert white space addition.

nlutsenko · 2016-10-31T18:07:53Z

SocketRocket/SRWebSocket.m

@@ -1051,10 +1111,29 @@ - (void)_pumpWriting;
            [self _failWithError:error];
            return;
        }
-
+        


Nit: Remove whitespace addition.

nlutsenko · 2016-10-31T18:10:56Z

SocketRocket/SRWebSocket.m

-
+
+    SRDataCallback *record = [[SRDataCallback alloc] init];
+    record.completeBlock = complete;


This completion can be nil and we are going to waste precious (on iOS at least) CPU cycles for running all this math later.
What do you think if we don't add the record to the _sendCallbacks at all if completion is nil?
This is going to ease the work later as well.

nlutsenko · 2016-10-31T18:11:39Z

SocketRocket/SRWebSocket.m

+        [_sendCallbacks enumerateKeysAndObjectsUsingBlock:^(NSNumber * _Nonnull key, SRDataCallback * _Nonnull obj, BOOL * _Nonnull stop) {
+            if (NSMaxRange(obj.range) <= _outputBufferOffset) {
+                [removeKeys addObject:key];
+                if (obj.completeBlock) {


This can go away once we can ensure that _sendCallbacks only contain objects with non-nil completion.

nlutsenko · 2016-10-31T18:12:00Z

SocketRocket/SRWebSocket.m

@@ -1314,74 +1393,85 @@ -(void)_pumpScanner;

 static const size_t SRFrameHeaderOverhead = 32;

- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data
-{
+- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data complete:(SRSendCompleteBlock)complete {


Nit: Move the opening bracket to the next line.

merge from facebook

NathanLi · 2016-11-03T15:40:51Z

@nlutsenko Thank you for taking the time to review my code. I have changed you mentioned, please review again.

nlutsenko

Almost there...
Few nits, but most importantly:

Using NSValue valueWithRange: for the key
Improving the method chains and signatures for error:completion: style methods

nlutsenko · 2016-11-07T17:39:26Z

SocketRocket/SRWebSocket.m

@@ -69,6 +69,31 @@
 NSString *const SRWebSocketErrorDomain = @"SRWebSocketErrorDomain";
 NSString *const SRHTTPResponseErrorKey = @"HTTPResponseStatusCode";

+@interface SRDataCallback : NSObject
+@property (nonatomic, assign) NSRange   range;


Nit: Remove extra space aka make it look like NSRange range;

nlutsenko · 2016-11-07T17:39:51Z

SocketRocket/SRWebSocket.m

@@ -548,6 +577,13 @@ - (void)_closeWithProtocolError:(NSString *)message;
 - (void)_failWithError:(NSError *)error;
 {
    dispatch_async(_workQueue, ^{
+        [_sendCallbacks enumerateKeysAndObjectsUsingBlock:^(NSNumber * _Nonnull key, SRDataCallback * _Nonnull obj, BOOL * _Nonnull stop) {
+            if (obj.completion) {


Since completion is never nil - you don't need this check.

nlutsenko · 2016-11-07T17:40:16Z

SocketRocket/SRWebSocket.m

        return;
    }
-
+
+    if (completion != nil) {


Nit: Replace with if (completion) {

nlutsenko · 2016-11-07T17:40:58Z

SocketRocket/SRWebSocket.m

+        SRDataCallback *record =  [[SRDataCallback alloc] initWithRange:dataRange completion:completion];
+
+        static NSUInteger keyCount = 0;
+        _sendCallbacks[@(keyCount++)] = record;


Not sure if this is the best way to use a key here.
Can we use a [NSValue valueWithRange:dataRange] as a key?

nlutsenko · 2016-11-07T17:42:30Z

SocketRocket/SRWebSocket.m

+    return [self sendString:string error:NULL completion:completion];
+}
+
+- (BOOL)sendString:(NSString *)string error:(NSError **)error completion:(nullable SRSendCompletionBlock)completion {


What do you think if we would remove error argument from this method and move it only to the method that needs it? Thus we can eliminate the need for weird method with synchronous and asynchronous signatures?

nlutsenko · 2016-11-07T17:42:52Z

SocketRocket/SRWebSocket.m

+    return [self sendDataNoCopy:data error:NULL completion:completion];
+}
+
+- (BOOL)sendDataNoCopy:(nullable NSData *)data error:(NSError **)error completion:(SRSendCompletionBlock)completion


Same as for sendString:error:completion:

nlutsenko · 2016-11-07T17:43:37Z

SocketRocket/SRWebSocket.m

@@ -1320,19 +1418,26 @@ -(void)_pumpScanner;

 static const size_t SRFrameHeaderOverhead = 32;

- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data
+- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data completion:(SRSendCompletionBlock)completion


completion argument needs proper nullability annotation aka completion:(nullable SRSendCompletionBlock)completion

…e methods

NathanLi · 2016-11-09T01:12:16Z

Yes, you are right. Use dataRange as a key is correct.

NathanLi · 2016-12-29T07:57:15Z

@nlutsenko Can you review the code?

Add the callback of send operation.

f1e1cee

Add feature: Callback of send data.

ghost added the GH Review: review-needed label Aug 14, 2016

ghost added the CLA Signed label Aug 15, 2016

nlutsenko self-assigned this Aug 23, 2016

ghost added the CLA Signed label Aug 23, 2016

nlutsenko requested changes Oct 31, 2016

View reviewed changes

nlutsenko removed the GH Review: review-needed label Oct 31, 2016

nlutsenko removed their assignment Oct 31, 2016

Merge remote-tracking branch 'facebook/master'

c7b8f8d

merge from facebook

facebook-github-bot added the GH Review: review-needed label Nov 3, 2016

NathanLi added 5 commits November 3, 2016 19:47

Rename SRSendCompleteBlock to SRSendCompletionBlock

296c151

Fixed naming problems

f599d0c

Change the call back key for uniqune.

ac1c632

Remove useless white space.

399e25b

Remove useless white space.

00545e5

nlutsenko requested changes Nov 7, 2016

View reviewed changes

Repository owner removed the GH Review: review-needed label Nov 7, 2016

NathanLi added 2 commits November 8, 2016 23:15

Improving the method chains and signatures for error:completion: styl…

b313fe8

…e methods

Use [NSValue valueWithRange:dataRange] as a key of _sendCallbacks.

706623c

Change the error message.

f630b95

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the callback of send operation. #458

Add the callback of send operation. #458

NathanLi commented Aug 14, 2016

ghost commented Aug 14, 2016

NathanLi commented Aug 15, 2016

ghost commented Aug 15, 2016

nlutsenko commented Aug 23, 2016

nlutsenko left a comment

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

nlutsenko Oct 31, 2016

NathanLi commented Nov 3, 2016

nlutsenko left a comment

nlutsenko Nov 7, 2016

nlutsenko Nov 7, 2016

nlutsenko Nov 7, 2016

nlutsenko Nov 7, 2016

nlutsenko Nov 7, 2016

nlutsenko Nov 7, 2016

nlutsenko Nov 7, 2016

NathanLi commented Nov 9, 2016

NathanLi commented Dec 29, 2016

		@@ -59,6 +59,8 @@ extern NSString *const SRHTTPResponseErrorKey;

		@protocol SRWebSocketDelegate;

		typedef void (^SRSendCompleteBlock)(NSError * _Nullable error);



		SRDataCallback *record = [[SRDataCallback alloc] init];
		record.completeBlock = complete;

Add the callback of send operation. #458

Are you sure you want to change the base?

Add the callback of send operation. #458

Conversation

NathanLi commented Aug 14, 2016

ghost commented Aug 14, 2016

NathanLi commented Aug 15, 2016

ghost commented Aug 15, 2016

nlutsenko commented Aug 23, 2016

nlutsenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanLi commented Nov 3, 2016

nlutsenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanLi commented Nov 9, 2016

NathanLi commented Dec 29, 2016