stuart feedback 3

Author: gaaclarke

shell/platform/common/client_wrapper/core_implementations.cc

@@ -26,6 +26,11 @@ namespace flutter {
 // ========== binary_messenger_impl.h ==========
 
 namespace {
+
+using FlutterDesktopMessengerScopedLock =
+    std::unique_ptr<FlutterDesktopMessenger,
+                    decltype(&FlutterDesktopMessengerUnlock)>;
+
 // Passes |message| to |user_data|, which must be a BinaryMessageHandler, along
 // with a BinaryReply that will send a response on |message|'s response handle.
 //
@@ -42,9 +47,8 @@ void ForwardToHandler(FlutterDesktopMessengerRef messenger,
   BinaryReply reply_handler = [messenger_ptr, response_handle](
                                   const uint8_t* reply,
                                   size_t reply_size) mutable {
-    // Note: This can be called on any thread.
-    auto lock = std::unique_ptr<FlutterDesktopMessenger,
-                                decltype(&FlutterDesktopMessengerUnlock)>(
+    // Note: This lambda can be called on any thread.
+    auto lock = FlutterDesktopMessengerScopedLock(
         FlutterDesktopMessengerLock(messenger_ptr.get()),
         &FlutterDesktopMessengerUnlock);
     if (!FlutterDesktopMessengerIsAvailable(messenger_ptr.get())) {

shell/platform/embedder/embedder.cc

@@ -2242,12 +2242,12 @@ FlutterEngineResult FlutterPlatformMessageReleaseResponseHandle(
   return kSuccess;
 }
 
+// Note: This can execute on any thread.
 FlutterEngineResult FlutterEngineSendPlatformMessageResponse(
     FLUTTER_API_SYMBOL(FlutterEngine) engine,
     const FlutterPlatformMessageResponseHandle* handle,
     const uint8_t* data,
     size_t data_length) {
-  // Note: This can execute on any thread.
   if (data_length != 0 && data == nullptr) {
     return LOG_EMBEDDER_ERROR(
         kInvalidArguments,

shell/platform/glfw/flutter_glfw.cc

@@ -156,27 +156,43 @@ struct FlutterDesktopPluginRegistrar {
 struct FlutterDesktopMessenger {
   FlutterDesktopMessenger() = default;
 
+  /// Increments the reference count.
+  ///
+  /// Thread-safe.
   void AddRef() { ref_count_.fetch_add(1); }
 
+  /// Decrements the reference count and deletes the object if the count has
+  /// gone to zero.
+  ///
+  /// Thread-safe.
   void Release() {
     int32_t old_count = ref_count_.fetch_sub(1);
     if (old_count <= 1) {
       delete this;
     }
   }
 
+  /// Getter for the engine field.
   FlutterDesktopEngineState* GetEngine() const { return engine_; }
+
+  /// Setter for the engine field.
+  /// Thread-safe.
   void SetEngine(FlutterDesktopEngineState* engine) {
     std::scoped_lock lock(mutex_);
     engine_ = engine;
   }
 
+  /// Returns the mutex associated with the |FlutterDesktopMessenger|.
+  ///
+  /// This mutex is used to synchronize reading or writing state inside the
+  /// |FlutterDesktopMessenger| (ie |engine_|).
   std::mutex& GetMutex() { return mutex_; }
 
- private:
   FlutterDesktopMessenger(const FlutterDesktopMessenger& value) = delete;
   FlutterDesktopMessenger& operator=(const FlutterDesktopMessenger& value) =
       delete;
+
+ private:
   // The engine that backs this messenger.
   FlutterDesktopEngineState* engine_;
   std::atomic<int32_t> ref_count_ = 0;

shell/platform/windows/public/flutter_windows.h

@@ -182,7 +182,11 @@ FlutterDesktopEngineGetPluginRegistrar(FlutterDesktopEngineRef engine,
 
 // Returns the messenger associated with the engine.
 //
-// This does not effect the reference count of the FlutterDesktopMessenger.
+// This does not provide an owning reference, so should *not* be balanced with a
+// call to |FlutterDesktopMessengerRelease|.
+//
+// Callers should use |FlutterDesktopMessengerAddRef| if the returned pointer
+// will potentially outlive 'engine', such as when passing it to another thread.
 FLUTTER_EXPORT FlutterDesktopMessengerRef
 FlutterDesktopEngineGetMessenger(FlutterDesktopEngineRef engine);
 

shell/platform/windows/window_state.h

@@ -40,25 +40,47 @@ struct FlutterDesktopPluginRegistrar {
 struct FlutterDesktopMessenger {
  public:
   FlutterDesktopMessenger() = default;
+
+  /// Getter for the engine field.
   flutter::FlutterWindowsEngine* GetEngine() const { return engine; }
+
+  /// Setter for the engine field.
+  /// Thread-safe.
   void SetEngine(flutter::FlutterWindowsEngine* arg_engine) {
     std::scoped_lock lock(mutex_);
     engine = arg_engine;
   }
+
+  /// Increments the reference count.
+  ///
+  /// Thread-safe.
   FlutterDesktopMessenger* AddRef() {
     ref_count_.fetch_add(1);
     return this;
   }
+
+  /// Decrements the reference count and deletes the object if the count has
+  /// gone to zero.
+  ///
+  /// Thread-safe.
   void Release() {
     int32_t old_count = ref_count_.fetch_sub(1);
     if (old_count <= 1) {
       delete this;
     }
   }
+
+  /// Returns the mutex associated with the |FlutterDesktopMessenger|.
+  ///
+  /// This mutex is used to synchronize reading or writing state inside the
+  /// |FlutterDesktopMessenger| (ie |engine|).
   std::mutex& GetMutex() { return mutex_; }
 
+  FlutterDesktopMessenger(const FlutterDesktopMessenger& value) = delete;
+  FlutterDesktopMessenger& operator=(const FlutterDesktopMessenger& value) =
+      delete;
+
  private:
-  FML_DISALLOW_COPY_AND_ASSIGN(FlutterDesktopMessenger);
   // The engine that owns this state object.
   flutter::FlutterWindowsEngine* engine = nullptr;
   std::mutex mutex_;