[macos] Add reply to binary messenger #9953

franciscojma86 · 2019-07-19T21:27:51Z

No description provided.

chinmaygarde · 2019-07-19T21:37:10Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

+  captures->reply = callback;
+  auto message_reply = [](const uint8_t* data, size_t data_size, void* user_data) {
+    auto captures = reinterpret_cast<Captures*>(user_data);
+    NSData* reply_data = [NSData dataWithBytesNoCopy:(void*)data length:data_size freeWhenDone:NO];


You need to take ownership of the data here. Currently, it is owned by the engine but it may be released after this callout. The caller could then use the data past the current scope.

Changed to dataWithBytes:length: to copy the data.

chinmaygarde · 2019-07-19T21:38:52Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

+    captures->reply(reply_data);
+    delete captures;
+  };
+  FlutterPlatformMessageCreateResponseHandle(_engine, message_reply, captures, &response_handle);


You need to check if the engine could create a response handle here. This call will return kSuccess in such cases. In case of failures, you will have to remember to delete the captures yourself because you aren't going to get the callback. I suggest using a unique pointer for the captures and only release() it on kSuccess.

chinmaygarde · 2019-07-19T21:39:19Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

+  if (result != kSuccess) {
+    NSLog(@"Failed to send message to Flutter engine on channel '%@' (%d).", channel, result);
+  }
+  FlutterPlatformMessageReleaseResponseHandle(_engine, response_handle);


Maybe log in case of failure here.

chinmaygarde · 2019-07-19T22:33:16Z

shell/platform/darwin/common/framework/Source/FlutterCodecs.mm

@@ -73,7 +73,7 @@ - (NSData*)encode:(id)message {
 }

 - (id)decode:(NSData*)message {
-  if (message == nil)
+  if (message == nil || message.length == 0)


You cal get rid of the message==nil check. message.length sends an Objective C message of length to message. Messages to nil objects return zero.

chinmaygarde · 2019-07-19T22:34:15Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

+- (void)sendOnChannel:(NSString*)channel
+              message:(NSData* _Nullable)message
+          binaryReply:(FlutterBinaryReply _Nullable)callback {
+  FlutterPlatformMessageResponseHandle* response_handle = nullptr;


Nit: Please define this nearer to where it is used. Right above the FlutterPlatformMessageCreateResponseHandle works better.

chinmaygarde · 2019-07-19T22:35:58Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

+  }
+
+  result = FlutterPlatformMessageReleaseResponseHandle(_engine, response_handle);
+    if (result != kSuccess) {


Please run clang-format over this. I believe the presubmits will block this patch anyway because of the formatting. It is a good idea to clang-format on save of each engine file. You can setup your editor to do this so you don't forget.

You fixed this right before I sent my review :) Thanks.

stuartmorgan · 2019-07-22T18:11:24Z

shell/platform/darwin/common/framework/Source/FlutterCodecs.mm

@@ -73,7 +73,7 @@ - (NSData*)encode:(id)message {
 }

 - (id)decode:(NSData*)message {
-  if (message == nil)
+  if (message.length == 0)


What's the reason for this change? It's actually changing the behavior of this API; before this change it's an assertion failure to send a non-nil but zero-length message, and now it's not.

(And if this is a change we want to make, then the now-redundant if below it should be removed.)

I was getting an assertion failure since the data I was getting from the response was not null, but length 0. I now think that was WAI here, but added a length > 0 check in the channel to avoid hitting this assertion

stuartmorgan · 2019-07-22T18:13:26Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

@@ -345,6 +345,49 @@ - (void)sendOnChannel:(nonnull NSString*)channel message:(nullable NSData*)messa
  }


I would expect sendOnChannel:message: to be implemented by calling sendOnChannel:message:binaryReply: with a nil reply callback, so that we don't have duplicate codepaths.

stuartmorgan · 2019-07-22T18:14:48Z

shell/platform/darwin/macos/framework/Source/FLEEngine.mm

+  auto message_reply = [](const uint8_t* data, size_t data_size, void* user_data) {
+    auto captures = reinterpret_cast<Captures*>(user_data);
+    NSData* reply_data = [NSData dataWithBytes:(void*)data length:data_size];
+    captures->reply(reply_data);


If |callback| is nil, which the API allows, this will crash. The callback-handling code needs to be wrapped in a nil check.

stuartmorgan · 2019-07-22T18:17:31Z

I didn't notice this was merged while I was writing my review until after submitting. It's up to you whether you want to revert and reland with the comments addressed, or do a follow-up ASAP.

franciscojma86 · 2019-07-22T21:26:38Z

Addressed comments in new PR #10009

flutter/engine@9e9da56...d6970bf git log 9e9da56..d6970bf --no-merges --oneline d6970bf Roll src/third_party/skia ebab03ffbffb..6dc14ded91ea (6 commits) (flutter/engine#10005) 6f3a2eb [macos] Add reply to binary messenger (flutter/engine#9953) a7ef508 Clean up cirrus.yml file a little (flutter/engine#9958) 68d269e Roll src/third_party/dart a089199b93..fedd74669a (8 commits) (flutter/engine#10001) a004a6e Roll fuchsia/sdk/core/linux-amd64 from FBDZ1EXEOcpLsJY4a5JRok5wPvXm5SURbJw1V7lpfKUC to tY_fod_tTCLft3FAUUxCP_HdLIahaJJK4WCXOA7nNGQC (flutter/engine#10000) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

Add reply to binary messenger

f81bfb9

franciscojma86 requested review from chinmaygarde and stuartmorgan July 19, 2019 21:27

googlebot added the cla: yes label Jul 19, 2019

chinmaygarde requested changes Jul 19, 2019

View reviewed changes

franciscojma86 added 2 commits July 19, 2019 15:24

Address comments

fbf828e

Formatting

3de5ff6

chinmaygarde approved these changes Jul 19, 2019

View reviewed changes

Address comments

1d418c2

franciscojma86 merged commit 6f3a2eb into flutter:master Jul 22, 2019

franciscojma86 deleted the response-macos branch July 22, 2019 18:08

stuartmorgan requested changes Jul 22, 2019

View reviewed changes

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 22, 2019

6f3a2eb [macos] Add reply to binary messenger (flutter/engine#9953)

775539e

engine-flutter-autoroll mentioned this pull request Jul 22, 2019

Roll engine 9e9da56b8923..d6970bf36a1e (5 commits) flutter/flutter#36702

Merged

stuartmorgan mentioned this pull request Aug 13, 2019

Implement platform channel response handling in macOS embedding flutter/flutter#35558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[macos] Add reply to binary messenger #9953

[macos] Add reply to binary messenger #9953

franciscojma86 commented Jul 19, 2019

chinmaygarde Jul 19, 2019

franciscojma86 Jul 19, 2019

chinmaygarde Jul 19, 2019

franciscojma86 Jul 19, 2019

chinmaygarde Jul 19, 2019

franciscojma86 Jul 19, 2019

chinmaygarde Jul 19, 2019

franciscojma86 Jul 19, 2019

chinmaygarde Jul 19, 2019

franciscojma86 Jul 19, 2019

chinmaygarde Jul 19, 2019

chinmaygarde Jul 19, 2019

franciscojma86 Jul 19, 2019

stuartmorgan Jul 22, 2019

franciscojma86 Jul 22, 2019

stuartmorgan Jul 22, 2019

franciscojma86 Jul 22, 2019

stuartmorgan Jul 22, 2019

franciscojma86 Jul 22, 2019

stuartmorgan commented Jul 22, 2019

franciscojma86 commented Jul 22, 2019

		@@ -345,6 +345,49 @@ - (void)sendOnChannel:(nonnull NSString)channel message:(nullable NSData)messa
		}

[macos] Add reply to binary messenger #9953

[macos] Add reply to binary messenger #9953

Conversation

franciscojma86 commented Jul 19, 2019

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

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

stuartmorgan commented Jul 22, 2019

franciscojma86 commented Jul 22, 2019