From ae94cefdba1cac0e311c7ced8f71fcc1d4b74a8c Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Mon, 12 Jan 2026 17:02:58 -0600 Subject: [PATCH] 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 --- .../browser/electron_api_ipc_handler_impl.cc | 24 ++++----- .../electron_api_sw_ipc_handler_impl.cc | 8 ++- shell/common/gin_helper/reply_channel.cc | 52 ++++++++++++------- shell/common/gin_helper/reply_channel.h | 11 +++- 4 files changed, 55 insertions(+), 40 deletions(-) diff --git a/shell/browser/electron_api_ipc_handler_impl.cc b/shell/browser/electron_api_ipc_handler_impl.cc index 9b742e968d..8304c44a0f 100644 --- a/shell/browser/electron_api_ipc_handler_impl.cc +++ b/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 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 {}; } diff --git a/shell/browser/electron_api_sw_ipc_handler_impl.cc b/shell/browser/electron_api_sw_ipc_handler_impl.cc index 3e56307dde..9668d2a425 100644 --- a/shell/browser/electron_api_sw_ipc_handler_impl.cc +++ b/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 {}; } diff --git a/shell/common/gin_helper/reply_channel.cc b/shell/common/gin_helper/reply_channel.cc index b11cc4403d..9a4c7c4288 100644 --- a/shell/common/gin_helper/reply_channel.cc +++ b/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 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 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 = { diff --git a/shell/common/gin_helper/reply_channel.h b/shell/common/gin_helper/reply_channel.h index 2c87d769a2..861e89a7d5 100644 --- a/shell/common/gin_helper/reply_channel.h +++ b/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 + #include "shell/common/api/api.mojom.h" #include "shell/common/gin_helper/wrappable.h" @@ -38,12 +40,19 @@ class ReplyChannel : public gin_helper::DeprecatedWrappable { 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 arg); + bool SendReply(v8::Isolate* isolate, v8::Local arg); InvokeCallback callback_;