Add the callback of send operation. by NathanLi · Pull Request #458 · facebookincubator/SocketRocket

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 cla@fb.com. 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


 @protocol SRWebSocketDelegate;

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


Nit: Rename to SRSendCompletionBlock.

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

+ this block will invoked with `nil`. 
+
+ @return `YES` if the string was scheduled to send, otherwise - `NO`.
+ */


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

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


Nit: Double space after complete

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


+/**
+ Send a UTF-8 String to the server.
+


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

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

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


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

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

        return NO;
    }
-
+    


Nit: Revert white space addition.

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

            return;
        }
-
+        


Nit: Remove whitespace addition.

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

-
+
+    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

+        [_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


- (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

 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

 {
    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

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


Nit: Replace with if (completion) {

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

+        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

+    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

+{
+    return [self sendDataNoCopy:data error:NULL completion:completion];
+}
+


Same as for sendString:error:completion:

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


 static const size_t SRFrameHeaderOverhead = 32;

- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data


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


		@protocol SRWebSocketDelegate;

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



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


		static const size_t SRFrameHeaderOverhead = 32;

		- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data

Conversation

NathanLi commented Aug 14, 2016

Uh oh!

ghost commented Aug 14, 2016

Uh oh!

NathanLi commented Aug 15, 2016

Uh oh!

ghost commented Aug 15, 2016

Uh oh!

nlutsenko commented Aug 23, 2016

Uh oh!

nlutsenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanLi commented Nov 3, 2016

Uh oh!

nlutsenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanLi commented Nov 9, 2016

Uh oh!

NathanLi commented Dec 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants