refactor: reduce coupling in electron::api::Protocol (#46183)

* refactor: decouple api::Protocol from ElectronBrowserContext

now they do not know about each other

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: make electron::api::ProtocolError private

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: remove unused isolate arg in Protocol constructor

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* refactor: use =default for trivial destructor

Co-authored-by: Charles Kerr <charles@charleskerr.com>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>

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::Error Protocol::RegisterProtocol(ProtocolType type,
-
-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) {
   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(
+  HandleOptionalCallback(args, removed ? Error::kOK : Error::kNotRegistered);
-      args, removed ? ProtocolError::kOK : ProtocolError::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,
+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(
+  HandleOptionalCallback(args, removed ? Error::kOK : Error::kNotIntercepted);
-      args, removed ? ProtocolError::kOK : ProtocolError::kNotIntercepted);
   return removed;
 }
 
@@ -275,15 +271,14 @@ v8::Local<v8::Promise> Protocol::IsProtocolHandled(const std::string& scheme,
           base::Contains(kBuiltinSchemes, scheme));
 }
 
-void Protocol::HandleOptionalCallback(gin::Arguments* args,
+void Protocol::HandleOptionalCallback(gin::Arguments* args, Error error) {
-                                      ProtocolError error) {
   base::RepeatingCallback<void(v8::Local<v8::Value>)> 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> Protocol::Create(
+gin::Handle<Protocol> Protocol::Create(v8::Isolate* isolate,
-    v8::Isolate* isolate,
+                                       ProtocolRegistry* protocol_registry) {
-    ElectronBrowserContext* browser_context) {
+  return gin::CreateHandle(isolate, new Protocol{protocol_registry});
-  return gin::CreateHandle(
-      isolate, new Protocol(isolate, browser_context->protocol_registry()));
 }
 
 // static

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<v8::Value> 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<Protocol>,
                        public gin_helper::Constructible<Protocol> {
  public:
   static gin::Handle<Protocol> Create(v8::Isolate* isolate,
-                                      ElectronBrowserContext* browser_context);
+                                      ProtocolRegistry* protocol_registry);
 
   // gin_helper::Constructible
   static gin::Handle<Protocol> New(gin_helper::ErrorThrower thrower);
@@ -63,21 +53,32 @@ class Protocol final : public gin::Wrappable<Protocol>,
   const char* GetTypeName() override;
 
  private:
-  Protocol(v8::Isolate* isolate, ProtocolRegistry* protocol_registry);
+  // Possible errors.
-  ~Protocol() override;
+  enum class Error {
+    kOK,  // no error
+    kRegistered,
+    kNotRegistered,
+    kIntercepted,
+    kNotIntercepted,
+  };
 
   // Callback types.
   using CompletionCallback =
       base::RepeatingCallback<void(v8::Local<v8::Value>)>;
 
+  explicit Protocol(ProtocolRegistry* protocol_registry);
+  ~Protocol() override = default;
+
+  [[nodiscard]] static std::string_view ErrorCodeToString(Error error);
+
   // JS APIs.
-  ProtocolError RegisterProtocol(ProtocolType type,
+  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,
+  Error InterceptProtocol(ProtocolType type,
                           const std::string& scheme,
                           const ProtocolHandler& handler);
   bool UninterceptProtocol(const std::string& scheme, gin::Arguments* args);
@@ -92,21 +93,21 @@ class Protocol final : public gin::Wrappable<Protocol>,
   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 <ProtocolType type>
   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.

shell/browser/api/electron_api_session.cc

@@ -557,7 +557,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,