From 6723bfbe327da6b447f7cefed4e02bc9d7a74eea Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 21 Mar 2025 07:33:25 -0500 Subject: [PATCH] refactor: reduce coupling in `electron::api::Protocol` (#46122) * refactor: decouple api::Protocol from ElectronBrowserContext now they do not know about each other * refactor: make electron::api::ProtocolError private * refactor: remove unused isolate arg in Protocol constructor * refactor: use =default for trivial destructor --- shell/browser/api/electron_api_protocol.cc | 59 ++++++++++------------ shell/browser/api/electron_api_protocol.h | 49 +++++++++--------- shell/browser/api/electron_api_session.cc | 4 +- 3 files changed, 54 insertions(+), 58 deletions(-) diff --git a/shell/browser/api/electron_api_protocol.cc b/shell/browser/api/electron_api_protocol.cc index 9e630ba493..825b69aade 100644 --- a/shell/browser/api/electron_api_protocol.cc +++ b/shell/browser/api/electron_api_protocol.cc @@ -14,7 +14,6 @@ #include "gin/handle.h" #include "gin/object_template_builder.h" #include "shell/browser/browser.h" -#include "shell/browser/electron_browser_context.h" #include "shell/browser/protocol_registry.h" #include "shell/common/gin_converters/callback_converter.h" #include "shell/common/gin_converters/net_converter.h" @@ -194,41 +193,39 @@ const char* const kBuiltinSchemes[] = { "about", "file", "http", "https", "data", "filesystem", }; +} // namespace + +Protocol::Protocol(ProtocolRegistry* protocol_registry) + : protocol_registry_{protocol_registry} {} + // Convert error code to string. -constexpr std::string_view ErrorCodeToString(ProtocolError error) { +// static +std::string_view Protocol::ErrorCodeToString(Error error) { switch (error) { - case ProtocolError::kRegistered: + case Error::kRegistered: return "The scheme has been registered"; - case ProtocolError::kNotRegistered: + case Error::kNotRegistered: return "The scheme has not been registered"; - case ProtocolError::kIntercepted: + case Error::kIntercepted: return "The scheme has been intercepted"; - case ProtocolError::kNotIntercepted: + case Error::kNotIntercepted: return "The scheme has not been intercepted"; default: return "Unexpected error"; } } -} // namespace - -Protocol::Protocol(v8::Isolate* isolate, ProtocolRegistry* protocol_registry) - : protocol_registry_(protocol_registry) {} - -Protocol::~Protocol() = default; - -ProtocolError Protocol::RegisterProtocol(ProtocolType type, - const std::string& scheme, - const ProtocolHandler& handler) { +Protocol::Error Protocol::RegisterProtocol(ProtocolType type, + const std::string& scheme, + const ProtocolHandler& handler) { bool added = protocol_registry_->RegisterProtocol(type, scheme, handler); - return added ? ProtocolError::kOK : ProtocolError::kRegistered; + return added ? Error::kOK : Error::kRegistered; } bool Protocol::UnregisterProtocol(const std::string& scheme, gin::Arguments* args) { bool removed = protocol_registry_->UnregisterProtocol(scheme); - HandleOptionalCallback( - args, removed ? ProtocolError::kOK : ProtocolError::kNotRegistered); + HandleOptionalCallback(args, removed ? Error::kOK : Error::kNotRegistered); return removed; } @@ -236,18 +233,17 @@ bool Protocol::IsProtocolRegistered(const std::string& scheme) { return protocol_registry_->FindRegistered(scheme) != nullptr; } -ProtocolError Protocol::InterceptProtocol(ProtocolType type, - const std::string& scheme, - const ProtocolHandler& handler) { +Protocol::Error Protocol::InterceptProtocol(ProtocolType type, + const std::string& scheme, + const ProtocolHandler& handler) { bool added = protocol_registry_->InterceptProtocol(type, scheme, handler); - return added ? ProtocolError::kOK : ProtocolError::kIntercepted; + return added ? Error::kOK : Error::kIntercepted; } bool Protocol::UninterceptProtocol(const std::string& scheme, gin::Arguments* args) { bool removed = protocol_registry_->UninterceptProtocol(scheme); - HandleOptionalCallback( - args, removed ? ProtocolError::kOK : ProtocolError::kNotIntercepted); + HandleOptionalCallback(args, removed ? Error::kOK : Error::kNotIntercepted); return removed; } @@ -275,15 +271,14 @@ v8::Local Protocol::IsProtocolHandled(const std::string& scheme, base::Contains(kBuiltinSchemes, scheme)); } -void Protocol::HandleOptionalCallback(gin::Arguments* args, - ProtocolError error) { +void Protocol::HandleOptionalCallback(gin::Arguments* args, Error error) { base::RepeatingCallback)> callback; if (args->GetNext(&callback)) { util::EmitWarning( args->isolate(), "The callback argument of protocol module APIs is no longer needed.", "ProtocolDeprecateCallback"); - if (error == ProtocolError::kOK) + if (error == Error::kOK) callback.Run(v8::Null(args->isolate())); else callback.Run(v8::Exception::Error( @@ -292,11 +287,9 @@ void Protocol::HandleOptionalCallback(gin::Arguments* args, } // static -gin::Handle Protocol::Create( - v8::Isolate* isolate, - ElectronBrowserContext* browser_context) { - return gin::CreateHandle( - isolate, new Protocol(isolate, browser_context->protocol_registry())); +gin::Handle Protocol::Create(v8::Isolate* isolate, + ProtocolRegistry* protocol_registry) { + return gin::CreateHandle(isolate, new Protocol{protocol_registry}); } // static diff --git a/shell/browser/api/electron_api_protocol.h b/shell/browser/api/electron_api_protocol.h index 273efc4dea..cfb6483469 100644 --- a/shell/browser/api/electron_api_protocol.h +++ b/shell/browser/api/electron_api_protocol.h @@ -22,7 +22,6 @@ class Handle; namespace electron { -class ElectronBrowserContext; class ProtocolRegistry; namespace api { @@ -35,21 +34,12 @@ void AddServiceWorkerScheme(const std::string& scheme); void RegisterSchemesAsPrivileged(gin_helper::ErrorThrower thrower, v8::Local val); -// Possible errors. -enum class ProtocolError { - kOK, // no error - kRegistered, - kNotRegistered, - kIntercepted, - kNotIntercepted, -}; - // Protocol implementation based on network services. class Protocol final : public gin::Wrappable, public gin_helper::Constructible { public: static gin::Handle Create(v8::Isolate* isolate, - ElectronBrowserContext* browser_context); + ProtocolRegistry* protocol_registry); // gin_helper::Constructible static gin::Handle New(gin_helper::ErrorThrower thrower); @@ -63,23 +53,34 @@ class Protocol final : public gin::Wrappable, const char* GetTypeName() override; private: - Protocol(v8::Isolate* isolate, ProtocolRegistry* protocol_registry); - ~Protocol() override; + // Possible errors. + enum class Error { + kOK, // no error + kRegistered, + kNotRegistered, + kIntercepted, + kNotIntercepted, + }; // Callback types. using CompletionCallback = base::RepeatingCallback)>; + explicit Protocol(ProtocolRegistry* protocol_registry); + ~Protocol() override = default; + + [[nodiscard]] static std::string_view ErrorCodeToString(Error error); + // JS APIs. - ProtocolError RegisterProtocol(ProtocolType type, - const std::string& scheme, - const ProtocolHandler& handler); + Error RegisterProtocol(ProtocolType type, + const std::string& scheme, + const ProtocolHandler& handler); bool UnregisterProtocol(const std::string& scheme, gin::Arguments* args); bool IsProtocolRegistered(const std::string& scheme); - ProtocolError InterceptProtocol(ProtocolType type, - const std::string& scheme, - const ProtocolHandler& handler); + Error InterceptProtocol(ProtocolType type, + const std::string& scheme, + const ProtocolHandler& handler); bool UninterceptProtocol(const std::string& scheme, gin::Arguments* args); bool IsProtocolIntercepted(const std::string& scheme); @@ -92,21 +93,21 @@ class Protocol final : public gin::Wrappable, bool RegisterProtocolFor(const std::string& scheme, const ProtocolHandler& handler, gin::Arguments* args) { - auto result = RegisterProtocol(type, scheme, handler); + const auto result = RegisterProtocol(type, scheme, handler); HandleOptionalCallback(args, result); - return result == ProtocolError::kOK; + return result == Error::kOK; } template bool InterceptProtocolFor(const std::string& scheme, const ProtocolHandler& handler, gin::Arguments* args) { - auto result = InterceptProtocol(type, scheme, handler); + const auto result = InterceptProtocol(type, scheme, handler); HandleOptionalCallback(args, result); - return result == ProtocolError::kOK; + return result == Error::kOK; } // Be compatible with old interface, which accepts optional callback. - void HandleOptionalCallback(gin::Arguments* args, ProtocolError error); + void HandleOptionalCallback(gin::Arguments* args, Error error); // Weak pointer; the lifetime of the ProtocolRegistry is guaranteed to be // longer than the lifetime of this JS interface. diff --git a/shell/browser/api/electron_api_session.cc b/shell/browser/api/electron_api_session.cc index 0666476126..0e3f41e2bb 100644 --- a/shell/browser/api/electron_api_session.cc +++ b/shell/browser/api/electron_api_session.cc @@ -553,7 +553,9 @@ Session::Session(v8::Isolate* isolate, ElectronBrowserContext* browser_context) SessionPreferences::CreateForBrowserContext(browser_context); - protocol_.Reset(isolate, Protocol::Create(isolate, browser_context).ToV8()); + protocol_.Reset( + isolate, + Protocol::Create(isolate, browser_context->protocol_registry()).ToV8()); browser_context->SetUserData( kElectronApiSessionKey,