Allow embedders to add callbacks for responses to platform messages from the framework. by chinmaygarde · Pull Request #9655 · flutter/engine

chinmaygarde · 2019-07-02T23:43:48Z

franciscojma86 · 2019-07-03T00:04:49Z

shell/platform/embedder/embedder.cc

+    return LOG_EMBEDDER_ERROR(kInvalidArguments);
+  }
+
+  flutter::EmbedderPlatformMessageResponse::Callback response =


I think it naming this "response_callback" would make it a bit more clear since this is just the callback, not the whole response.

franciscojma86 · 2019-07-03T00:05:45Z

shell/platform/embedder/embedder.cc

+
+  auto handle = new FlutterPlatformMessageResponseHandle();
+  handle->message = fml::MakeRefCounted<flutter::PlatformMessage>(
+      "", fml::MakeRefCounted<flutter::EmbedderPlatformMessageResponse>(


Would it make sense to add a comment explaining why we are sending an empty channel, or is that obvious?

franciscojma86 · 2019-07-03T00:06:00Z

shell/platform/embedder/embedder.cc

+  return kSuccess;
+}
+
+FLUTTER_EXPORT


franciscojma86 · 2019-07-03T00:11:47Z

shell/platform/embedder/embedder_platform_message_response.h

+
+namespace flutter {
+
+class EmbedderPlatformMessageResponse : public PlatformMessageResponse {


Add class definition documentation?

franciscojma86 · 2019-07-03T00:13:04Z

shell/platform/embedder/fixtures/main.dart

+
+
+@pragma('vm:entry-point')
+void platform_messages_response() {


Should these messages have documentation relating to the tests using them?

franciscojma86 · 2019-07-03T00:15:14Z

shell/platform/embedder/tests/embedder_unittests.cc

  kill_latch.Wait();
 }

+TEST_F(EmbedderTest, ShouldBeAbleToCreateAndCollectCallbacks) {


Should this test be called CanCreateAndCollectcallbacks, for consistency with the test PlatformMessagesCanReceiveResponse below?

franciscojma86 · 2019-07-03T00:16:03Z

shell/platform/embedder/tests/embedder_unittests.cc

  kill_latch.Wait();
 }

+TEST_F(EmbedderTest, ShouldBeAbleToCreateAndCollectCallbacks) {


Suggestion for this and future tests: We should have a brief comment explaining what the test does.

stuartmorgan-g · 2019-07-03T02:51:17Z

shell/platform/embedder/embedder.cc

    return LOG_EMBEDDER_ERROR(kInvalidArguments);
  }

+  auto response_handle = SAFE_ACCESS(flutter_message, response_handle, nullptr);


Prefer auto* to auto here. (I'd actually suggest FlutterPlatformMessageResponseHandle* since I don't think the type is obvious in immediate context, and it's used in a way other than just passing off to something else, so this is a case where the guide prefers explicit type).

Fair point. Done.

stuartmorgan-g · 2019-07-03T02:54:24Z

shell/platform/embedder/embedder.h

    FlutterEngine engine,
    const FlutterPlatformMessage* message);

+// Create a platform message response handle that allows the embedder to set a


s/Create/Creates/

stuartmorgan-g · 2019-07-03T02:58:02Z

shell/platform/embedder/embedder.h


+// Create a platform message response handle that allows the embedder to set a
+// native callback for a response to a message. This handle may be set on the
+// |response_handle| field of any |FlutterPlatformMessage|. The handle must be


s/any |FlutterPlatformMessage|/any |FlutterPlatformMessage| sent to the engine/

stuartmorgan-g · 2019-07-03T02:59:45Z

shell/platform/embedder/embedder.h

+// Create a platform message response handle that allows the embedder to set a
+// native callback for a response to a message. This handle may be set on the
+// |response_handle| field of any |FlutterPlatformMessage|. The handle must be
+// collected via a call to |FlutterPlatformMessageReleaseResponseHandle|. This


It's not clear to me why we need this step; why not just do the cleanup internally, and make it an error not to send the message?

The memory management is cleaner and more explicit than deleting objects in obscure struct fields on specific calls that don't even operate on that object directly. I strongly think this makes the lifecycle of these resources easier to reason about.

shell/platform/embedder/embedder.h

stuartmorgan-g · 2019-07-03T03:04:18Z

shell/platform/embedder/embedder.h

+    void* user_data,
+    FlutterPlatformMessageResponseHandle** response_out);
+
+// Collect the handle created using


s/Collect/Collects/ (if this function is kept)

chinmaygarde

Addressed all PR comments.

chinmaygarde · 2019-07-03T03:37:57Z

shell/platform/embedder/embedder.cc

    return LOG_EMBEDDER_ERROR(kInvalidArguments);
  }

+  auto response_handle = SAFE_ACCESS(flutter_message, response_handle, nullptr);


Fair point. Done.

chinmaygarde · 2019-07-03T03:38:22Z

shell/platform/embedder/embedder.cc

+    return LOG_EMBEDDER_ERROR(kInvalidArguments);
+  }
+
+  flutter::EmbedderPlatformMessageResponse::Callback response =


chinmaygarde · 2019-07-03T03:39:48Z

shell/platform/embedder/embedder.cc

+
+  auto handle = new FlutterPlatformMessageResponseHandle();
+  handle->message = fml::MakeRefCounted<flutter::PlatformMessage>(
+      "", fml::MakeRefCounted<flutter::EmbedderPlatformMessageResponse>(


chinmaygarde · 2019-07-03T03:39:57Z

shell/platform/embedder/embedder.cc

+  return kSuccess;
+}
+
+FLUTTER_EXPORT


shell/platform/embedder/embedder.h

chinmaygarde · 2019-07-03T03:49:38Z

shell/platform/embedder/embedder.h

+// Create a platform message response handle that allows the embedder to set a
+// native callback for a response to a message. This handle may be set on the
+// |response_handle| field of any |FlutterPlatformMessage|. The handle must be
+// collected via a call to |FlutterPlatformMessageReleaseResponseHandle|. This


The memory management is cleaner and more explicit than deleting objects in obscure struct fields on specific calls that don't even operate on that object directly. I strongly think this makes the lifecycle of these resources easier to reason about.

chinmaygarde · 2019-07-03T03:50:10Z

shell/platform/embedder/embedder.h

+    void* user_data,
+    FlutterPlatformMessageResponseHandle** response_out);
+
+// Collect the handle created using


chinmaygarde · 2019-07-03T03:54:12Z

shell/platform/embedder/embedder_platform_message_response.h

+
+namespace flutter {
+
+class EmbedderPlatformMessageResponse : public PlatformMessageResponse {


chinmaygarde · 2019-07-03T03:54:55Z

shell/platform/embedder/tests/embedder_unittests.cc

  kill_latch.Wait();
 }

+TEST_F(EmbedderTest, ShouldBeAbleToCreateAndCollectCallbacks) {


chinmaygarde · 2019-07-03T03:56:16Z

shell/platform/embedder/tests/embedder_unittests.cc

  kill_latch.Wait();
 }

+TEST_F(EmbedderTest, ShouldBeAbleToCreateAndCollectCallbacks) {


stuartmorgan-g

LGTM with some nits and one API type question.

stuartmorgan-g · 2019-07-03T13:51:03Z

shell/platform/embedder/embedder_platform_message_response.h

+
+  //----------------------------------------------------------------------------
+  /// @brief      Destroys the message response. Can be called on any thread.
+  ///             Does no execute unfulfilled callbacks.


typo: no->not

stuartmorgan-g · 2019-07-03T13:51:18Z

shell/platform/embedder/embedder_platform_message_response.h

+  //----------------------------------------------------------------------------
+  /// @brief      Destroys the message response. Can be called on any thread.
+  ///             Does no execute unfulfilled callbacks.
+  ///


Nit: remove blank comment line.

stuartmorgan-g · 2019-07-03T13:56:32Z

shell/platform/embedder/embedder.h

+// Creates a platform message response handle that allows the embedder to set a
+// native callback for a response to a message. This handle may be set on the
+// |response_handle| field of any |FlutterPlatformMessage| sent to the engine.
+// The handle must be collected via a call to


Nit: consider adding a blank line above this, and another before the "The user data baton [...]" to break this long comment up into different conceptual sections (overview, memory management, argument details).

Done. At some point, we should move over to using doxygen style header docs too.

stuartmorgan-g · 2019-07-03T14:01:57Z

shell/platform/embedder/embedder.h

    const FlutterPlatformMessage* /* message*/,
    void* /* user data */);

+typedef void (*FlutterDataCallback)(const void* /* data */,


Missed this before: why is data a void* rather than a uint8_t* like the data in a message and the data in a response going the other way? It seems like an odd/confusing mismatch.

Agreed, it should be uint8_t*. Updated. Ditto for EmbedderPlatformMessageResponse::Callback.

…rom the framework. Fixes flutter/flutter#18852

chinmaygarde

Addressed all comments.

chinmaygarde · 2019-07-03T21:55:57Z

shell/platform/embedder/embedder.h

    const FlutterPlatformMessage* /* message*/,
    void* /* user data */);

+typedef void (*FlutterDataCallback)(const void* /* data */,


Agreed, it should be uint8_t*. Updated. Ditto for EmbedderPlatformMessageResponse::Callback.

chinmaygarde · 2019-07-03T21:58:35Z

shell/platform/embedder/embedder.h

+// Creates a platform message response handle that allows the embedder to set a
+// native callback for a response to a message. This handle may be set on the
+// |response_handle| field of any |FlutterPlatformMessage| sent to the engine.
+// The handle must be collected via a call to


Done. At some point, we should move over to using doxygen style header docs too.

chinmaygarde · 2019-07-03T21:59:09Z

shell/platform/embedder/embedder_platform_message_response.h

+
+  //----------------------------------------------------------------------------
+  /// @brief      Destroys the message response. Can be called on any thread.
+  ///             Does no execute unfulfilled callbacks.


…ssages from the framework. (flutter/engine#9655)

flutter/engine@7d3e722...3c51a7b git log 7d3e722..3c51a7b --no-merges --oneline 3c51a7b Roll src/third_party/skia 11eb847a2080..857c9f955edb (2 commits) (flutter/engine#9676) 7f828dd Raster now returns an enum rather than boolean (flutter/engine#9661) 11b6afe Roll src/third_party/dart 67ab3be10d...b5aeaa6796 (flutter/engine#9675) 3c4dbe2 Revert " Roll src/third_party/dart 67ab3be10d...43891316ca (#9670)" (flutter/engine#9673) 5e596f2 Roll src/third_party/dart 67ab3be10d...43891316ca (flutter/engine#9670) 46a2239 Roll src/third_party/skia 5b52c52141ac..11eb847a2080 (6 commits) (flutter/engine#9671) b84f89b Allow embedders to add callbacks for responses to platform messages from the framework. (flutter/engine#9655) 8dac2e9 Begin separating macOS engine from view controller (flutter/engine#9654) d3616c7 Roll src/third_party/skia 0e0113dcbd9a..5b52c52141ac (8 commits) (flutter/engine#9665) cea2c36 Mutators Stack refactoring (flutter/engine#9663) 6a8782f Roll src/third_party/skia 93eeff578b08..0e0113dcbd9a (9 commits) (flutter/engine#9662) 791143f ExternalViewEmbedder can CancelFrame after pre-roll (flutter/engine#9660) d637f29 External view embedder can tell if embedded views have mutated (flutter/engine#9653) ceee3d7 Roll src/third_party/skia 38ae3f42fec1..93eeff578b08 (1 commits) (flutter/engine#9659) d757290 Roll src/third_party/skia febc162c7898..38ae3f42fec1 (3 commits) (flutter/engine#9658) 58133ab Roll src/third_party/skia 3de5c6388142..febc162c7898 (2 commits) (flutter/engine#9657) 29342dd Roll src/third_party/skia 1e2cb444e0c1..3de5c6388142 (8 commits) (flutter/engine#9656) b547356 Pipeline allows continuations that can produce to front (flutter/engine#9652) 8306ee6 Move the mutators stack handling to preroll (flutter/engine#9651) 511b9f2 Roll src/third_party/skia 215ff3325230..1e2cb444e0c1 (12 commits) (flutter/engine#9650) 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.

PR flutter#9655 allows for providing a response handle when sending messages into the Flutter engine, but didn't check for a null response handle; this crashes all existing embedders that use FlutterEngineSendPlatformMessage.

flutter/engine@7d3e722...3c51a7b git log 7d3e722..3c51a7b --no-merges --oneline 3c51a7b Roll src/third_party/skia 11eb847a2080..857c9f955edb (2 commits) (flutter/engine#9676) 7f828dd Raster now returns an enum rather than boolean (flutter/engine#9661) 11b6afe Roll src/third_party/dart 67ab3be10d...b5aeaa6796 (flutter/engine#9675) 3c4dbe2 Revert &flutter#34; Roll src/third_party/dart 67ab3be10d...43891316ca (flutter#9670)&flutter#34; (flutter/engine#9673) 5e596f2 Roll src/third_party/dart 67ab3be10d...43891316ca (flutter/engine#9670) 46a2239 Roll src/third_party/skia 5b52c52141ac..11eb847a2080 (6 commits) (flutter/engine#9671) b84f89b Allow embedders to add callbacks for responses to platform messages from the framework. (flutter/engine#9655) 8dac2e9 Begin separating macOS engine from view controller (flutter/engine#9654) d3616c7 Roll src/third_party/skia 0e0113dcbd9a..5b52c52141ac (8 commits) (flutter/engine#9665) cea2c36 Mutators Stack refactoring (flutter/engine#9663) 6a8782f Roll src/third_party/skia 93eeff578b08..0e0113dcbd9a (9 commits) (flutter/engine#9662) 791143f ExternalViewEmbedder can CancelFrame after pre-roll (flutter/engine#9660) d637f29 External view embedder can tell if embedded views have mutated (flutter/engine#9653) ceee3d7 Roll src/third_party/skia 38ae3f42fec1..93eeff578b08 (1 commits) (flutter/engine#9659) d757290 Roll src/third_party/skia febc162c7898..38ae3f42fec1 (3 commits) (flutter/engine#9658) 58133ab Roll src/third_party/skia 3de5c6388142..febc162c7898 (2 commits) (flutter/engine#9657) 29342dd Roll src/third_party/skia 1e2cb444e0c1..3de5c6388142 (8 commits) (flutter/engine#9656) b547356 Pipeline allows continuations that can produce to front (flutter/engine#9652) 8306ee6 Move the mutators stack handling to preroll (flutter/engine#9651) 511b9f2 Roll src/third_party/skia 215ff3325230..1e2cb444e0c1 (12 commits) (flutter/engine#9650) 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.

chinmaygarde requested review from franciscojma86 and stuartmorgan-g July 2, 2019 23:43

googlebot added the cla: yes label Jul 2, 2019

franciscojma86 reviewed Jul 3, 2019

View reviewed changes

stuartmorgan-g suggested changes Jul 3, 2019

View reviewed changes

chinmaygarde commented Jul 3, 2019

View reviewed changes

stuartmorgan-g approved these changes Jul 3, 2019

View reviewed changes

franciscojma86 approved these changes Jul 3, 2019

View reviewed changes

Allow embedders to add callbacks for responses to platform messages f…

c2bfb97

…rom the framework. Fixes flutter/flutter#18852

chinmaygarde force-pushed the platform_message_response branch from a06c665 to c2bfb97 Compare July 3, 2019 22:01

chinmaygarde commented Jul 3, 2019

View reviewed changes

chinmaygarde merged commit b84f89b into flutter:master Jul 3, 2019

chinmaygarde deleted the platform_message_response branch July 3, 2019 22:44

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

b84f89b Allow embedders to add callbacks for responses to platform me…

35e740e

…ssages from the framework. (flutter/engine#9655)

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

Roll engine 7d3e722349e7..b84f89b44b75 (14 commits) flutter/flutter#35557

Closed

This was referenced Jul 4, 2019

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

Closed

Implement platform channel response handling in GLFW embedding flutter/flutter#35559

Closed

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

Roll engine 7d3e722349e7..46a2239b4240 (15 commits) flutter/flutter#35562

Closed

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

b84f89b Allow embedders to add callbacks for responses to platform me…

4b2d3e7

…ssages from the framework. (flutter/engine#9655)

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

b84f89b Allow embedders to add callbacks for responses to platform me…

791a601

…ssages from the framework. (flutter/engine#9655)

This was referenced Jul 4, 2019

Roll engine 7d3e722349e7..3c4dbe223cfe (17 commits) flutter/flutter#35565

Closed

Roll engine 7d3e722349e7..11b6afe00ca5 (18 commits) flutter/flutter#35569

Closed

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

b84f89b Allow embedders to add callbacks for responses to platform me…

a561187

…ssages from the framework. (flutter/engine#9655)

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

Roll engine 7d3e722349e7..7f828dd3e3d5 (19 commits) flutter/flutter#35571

Closed

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

b84f89b Allow embedders to add callbacks for responses to platform me…

33a1470

…ssages from the framework. (flutter/engine#9655)

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

b84f89b Allow embedders to add callbacks for responses to platform me…

4578cb9

…ssages from the framework. (flutter/engine#9655)

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

Roll engine 7d3e722349e7..3c51a7bfff77 (20 commits) flutter/flutter#35572

Merged

GeertJohan mentioned this pull request Jul 4, 2019

Implement callback on messages go-flutter-desktop/go-flutter#183

Closed

This was referenced Jul 6, 2019

Check for null response_handle in FlutterEngineSendPlatformMessage #9697

Closed

Crash in FlutterEngineSendPlatformMessage google/flutter-desktop-embedding#447

Closed

chinmaygarde mentioned this pull request Jul 6, 2019

Ensure that platform messages without response handles can be dispatched. #9698

Merged


		namespace flutter {

		class EmbedderPlatformMessageResponse : public PlatformMessageResponse {



		@pragma('vm:entry-point')
		void platform_messages_response() {

Conversation

chinmaygarde commented Jul 2, 2019

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde 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!

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!

stuartmorgan-g 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