refactor: add static ReplyChannel::SendError() helper (#49338)

* refactor: add static void ReplyChannel::SendError()

* refactor: use static SendError() instead of instantiating a temporary

* refactor: remove non-static version of SendError()

* refactor: remove redundant callback-is-non-null checks

shell/browser/electron_api_ipc_handler_impl.cc

@@ -140,31 +140,25 @@ gin_helper::internal::Event* ElectronApiIPCHandlerImpl::MakeIPCEvent(
     bool internal,
     electron::mojom::ElectronApiIPC::InvokeCallback callback) {
   if (!session) {
-    if (callback) {
-      // We must always invoke the callback if present.
-      gin_helper::internal::ReplyChannel::Create(isolate, std::move(callback))
-          ->SendError("Session does not exist");
-    }
+    // We must always invoke the callback if present.
+    gin_helper::internal::ReplyChannel::SendError(isolate, std::move(callback),
+                                                  "Session does not exist");
     return {};
   }
 
   api::WebContents* api_web_contents = api::WebContents::From(web_contents());
   if (!api_web_contents) {
-    if (callback) {
-      // We must always invoke the callback if present.
-      gin_helper::internal::ReplyChannel::Create(isolate, std::move(callback))
-          ->SendError("WebContents does not exist");
-    }
+    // We must always invoke the callback if present.
+    gin_helper::internal::ReplyChannel::SendError(isolate, std::move(callback),
+                                                  "WebContents does not exist");
     return {};
   }
 
   v8::Local<v8::Object> wrapper;
   if (!api_web_contents->GetWrapper(isolate).ToLocal(&wrapper)) {
-    if (callback) {
-      // We must always invoke the callback if present.
-      gin_helper::internal::ReplyChannel::Create(isolate, std::move(callback))
-          ->SendError("WebContents was destroyed");
-    }
+    // We must always invoke the callback if present.
+    gin_helper::internal::ReplyChannel::SendError(isolate, std::move(callback),
+                                                  "WebContents was destroyed");
     return {};
   }
 

shell/browser/electron_api_sw_ipc_handler_impl.cc

@@ -159,11 +159,9 @@ gin_helper::internal::Event* ElectronApiSWIPCHandlerImpl::MakeIPCEvent(
     bool internal,
     electron::mojom::ElectronApiIPC::InvokeCallback callback) {
   if (!session) {
-    if (callback) {
-      // We must always invoke the callback if present.
-      gin_helper::internal::ReplyChannel::Create(isolate, std::move(callback))
-          ->SendError("Session does not exist");
-    }
+    // We must always invoke the callback if present.
+    gin_helper::internal::ReplyChannel::SendError(isolate, std::move(callback),
+                                                  "Session does not exist");
     return {};
   }
 

shell/common/gin_helper/reply_channel.cc

@@ -37,30 +37,44 @@ ReplyChannel::ReplyChannel(InvokeCallback callback)
 
 ReplyChannel::~ReplyChannel() {
   if (callback_)
-    SendError("reply was never sent");
+    SendError(electron::JavascriptEnvironment::GetIsolate(),
+              std::move(callback_), "reply was never sent");
 }
 
-void ReplyChannel::SendError(const std::string& msg) {
-  v8::Isolate* isolate = electron::JavascriptEnvironment::GetIsolate();
-  // If there's no current context, it means we're shutting down, so we
-  // don't need to send an event.
-  v8::HandleScope scope(isolate);
-  if (!isolate->GetCurrentContext().IsEmpty()) {
-    auto message = gin::DataObjectBuilder(isolate).Set("error", msg).Build();
-    SendReply(isolate, message);
-  }
+// static
+void ReplyChannel::SendError(v8::Isolate* isolate,
+                             InvokeCallback callback,
+                             std::string_view const errmsg) {
+  if (!callback)
+    return;
+
+  // If there's no current context, it means we're shutting down,
+  // so we don't need to send an event.
+  v8::HandleScope scope{isolate};
+  if (isolate->GetCurrentContext().IsEmpty())
+    return;
+
+  SendReplyImpl(isolate, std::move(callback),
+                gin::DataObjectBuilder(isolate).Set("error", errmsg).Build());
+}
+
+// static
+bool ReplyChannel::SendReplyImpl(v8::Isolate* isolate,
+                                 InvokeCallback callback,
+                                 v8::Local<v8::Value> arg) {
+  if (!callback)
+    return false;
+
+  blink::CloneableMessage msg;
+  if (!gin::ConvertFromV8(isolate, arg, &msg))
+    return false;
+
+  std::move(callback).Run(std::move(msg));
+  return true;
 }
 
 bool ReplyChannel::SendReply(v8::Isolate* isolate, v8::Local<v8::Value> arg) {
-  if (!callback_)
-    return false;
-  blink::CloneableMessage message;
-  if (!gin::ConvertFromV8(isolate, arg, &message)) {
-    return false;
-  }
-
-  std::move(callback_).Run(std::move(message));
-  return true;
+  return SendReplyImpl(isolate, std::move(callback_), std::move(arg));
 }
 
 gin::DeprecatedWrapperInfo ReplyChannel::kWrapperInfo = {

shell/common/gin_helper/reply_channel.h

@@ -5,6 +5,8 @@
 #ifndef ELECTRON_SHELL_COMMON_GIN_HELPER_REPLY_CHANNEL_H_
 #define ELECTRON_SHELL_COMMON_GIN_HELPER_REPLY_CHANNEL_H_
 
+#include <string_view>
+
 #include "shell/common/api/api.mojom.h"
 #include "shell/common/gin_helper/wrappable.h"
 
@@ -38,12 +40,19 @@ class ReplyChannel : public gin_helper::DeprecatedWrappable<ReplyChannel> {
       v8::Isolate* isolate) override;
   const char* GetTypeName() override;
 
-  void SendError(const std::string& msg);
+  // Invokes `callback` (if it's non-null) with `errmsg` as an arg.
+  static void SendError(v8::Isolate* isolate,
+                        InvokeCallback callback,
+                        std::string_view errmsg);
 
  private:
   explicit ReplyChannel(InvokeCallback callback);
   ~ReplyChannel() override;
 
+  static bool SendReplyImpl(v8::Isolate* isolate,
+                            InvokeCallback callback,
+                            v8::Local<v8::Value> arg);
+
   bool SendReply(v8::Isolate* isolate, v8::Local<v8::Value> arg);
 
   InvokeCallback callback_;