Make FallbackRuntimeAgentDelegate private (#43348)

Summary: Pull Request resolved: #43348 Changelog: [Internal] Followup from D54585658. Moves the branching on `HERMES_DEBUGGER_ENABLED` into `HermesRuntimeTargetDelegate`, and correspondingly makes `FallbackRuntimeAgentDelegate` private (not exposed directly to integrators). Reviewed By: huntie Differential Revision: D54587558 fbshipit-source-id: 554b41356c1421a508c1a788d7c27f53445ecb6b
facebook · Mar 12, 2024 · 8824181 · 8824181
1 parent 59e1fc8
commit 8824181
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 86 deletions.
diff --git a/...es/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.cpp b/...es/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.cpp
@@ -5,20 +5,12 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-#include "HermesRuntimeAgentDelegate.h"
+#ifdef HERMES_ENABLE_DEBUGGER
 
-// If HERMES_ENABLE_DEBUGGER isn't defined, we can't access any Hermes
-// CDPHandler headers or types.
+#include "HermesRuntimeAgentDelegate.h"
 
-#ifdef HERMES_ENABLE_DEBUGGER
 #include <hermes/inspector/RuntimeAdapter.h>
 #include <hermes/inspector/chrome/CDPHandler.h>
-#else // HERMES_ENABLE_DEBUGGER
-// TODO(moti): FallbackRuntimeAgentDelegate should be private. We should fall
-// back at the *TargetDelegate* level, in HermesRuntimeTargetDelegate, rather
-// than within HermesRuntimeAgentDelegate.
-#include <jsinspector-modern/FallbackRuntimeAgentDelegate.h>
-#endif // HERMES_ENABLE_DEBUGGER
 
 #include <hermes/hermes.h>
 #include <jsinspector-modern/ReactCdp.h>
@@ -27,8 +19,6 @@ using namespace facebook::hermes;
 
 namespace facebook::react::jsinspector_modern {
 
-#ifdef HERMES_ENABLE_DEBUGGER
-
 namespace {
 
 /**
@@ -178,30 +168,6 @@ class HermesRuntimeAgentDelegate::Impl final : public RuntimeAgentDelegate {
   std::shared_ptr<HermesCDPHandler> hermes_;
 };
 
-#else // !HERMES_ENABLE_DEBUGGER
-
-/**
- * A stub for HermesRuntimeAgentDelegate when Hermes is compiled without
- * debugging support.
- */
-class HermesRuntimeAgentDelegate::Impl final
-    : public FallbackRuntimeAgentDelegate {
- public:
-  Impl(
-      FrontendChannel frontendChannel,
-      SessionState& sessionState,
-      std::unique_ptr<RuntimeAgentDelegate::ExportedState>,
-      const ExecutionContextDescription&,
-      std::shared_ptr<hermes::HermesRuntime> runtime,
-      RuntimeExecutor)
-      : FallbackRuntimeAgentDelegate(
-            std::move(frontendChannel),
-            sessionState,
-            runtime->description()) {}
-};
-
-#endif // HERMES_ENABLE_DEBUGGER
-
 HermesRuntimeAgentDelegate::HermesRuntimeAgentDelegate(
     FrontendChannel frontendChannel,
     SessionState& sessionState,
@@ -229,3 +195,5 @@ HermesRuntimeAgentDelegate::getExportedState() {
 }
 
 } // namespace facebook::react::jsinspector_modern
+
+#endif // HERMES_ENABLE_DEBUGGER
diff --git a/...ages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.h b/...ages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.h
@@ -7,6 +7,8 @@
 
 #pragma once
 
+#ifdef HERMES_ENABLE_DEBUGGER
+
 #include <ReactCommon/RuntimeExecutor.h>
 
 #include <hermes/hermes.h>
@@ -61,11 +63,14 @@ class HermesRuntimeAgentDelegate : public RuntimeAgentDelegate {
   virtual std::unique_ptr<ExportedState> getExportedState() override;
 
  private:
-  // We use the private implementation idiom to keep HERMES_ENABLE_DEBUGGER
-  // checks out of the header.
   class Impl;
 
   const std::unique_ptr<Impl> impl_;
 };
 
 } // namespace facebook::react::jsinspector_modern
+
+#else
+#error \
+    "HERMES_ENABLE_DEBUGGER must be enabled to use HermesRuntimeAgentDelegate."
+#endif // HERMES_ENABLE_DEBUGGER
diff --git a/...react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.cpp b/...react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.cpp
@@ -5,18 +5,16 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+#ifdef HERMES_ENABLE_DEBUGGER
+
 #include "HermesRuntimeAgentDelegateNew.h"
 
 // If HERMES_ENABLE_DEBUGGER isn't defined, we can't access any Hermes
 // CDP headers or types.
 
-#ifdef HERMES_ENABLE_DEBUGGER
 #include <hermes/AsyncDebuggerAPI.h>
 #include <hermes/cdp/CDPAgent.h>
 #include <hermes/inspector/RuntimeAdapter.h>
-#else // HERMES_ENABLE_DEBUGGER
-#include <jsinspector-modern/FallbackRuntimeAgentDelegate.h>
-#endif // HERMES_ENABLE_DEBUGGER
 
 #include <hermes/hermes.h>
 #include <jsinspector-modern/ReactCdp.h>
@@ -25,8 +23,6 @@ using namespace facebook::hermes;
 
 namespace facebook::react::jsinspector_modern {
 
-#ifdef HERMES_ENABLE_DEBUGGER
-
 class HermesRuntimeAgentDelegateNew::Impl final : public RuntimeAgentDelegate {
  public:
   Impl(
@@ -81,31 +77,6 @@ class HermesRuntimeAgentDelegateNew::Impl final : public RuntimeAgentDelegate {
   std::unique_ptr<hermes::cdp::CDPAgent> hermes_;
 };
 
-#else // !HERMES_ENABLE_DEBUGGER
-
-/**
- * A stub for HermesRuntimeAgentDelegateNew when Hermes is compiled without
- * debugging support.
- */
-class HermesRuntimeAgentDelegateNew::Impl final
-    : public FallbackRuntimeAgentDelegate {
- public:
-  Impl(
-      FrontendChannel frontendChannel,
-      SessionState& sessionState,
-      std::unique_ptr<RuntimeAgentDelegate::ExportedState>,
-      const ExecutionContextDescription&,
-      HermesRuntime& runtime,
-      HermesRuntimeTargetDelegate& runtimeTargetDelegate,
-      RuntimeExecutor)
-      : FallbackRuntimeAgentDelegate(
-            std::move(frontendChannel),
-            sessionState,
-            runtime.description()) {}
-};
-
-#endif // HERMES_ENABLE_DEBUGGER
-
 HermesRuntimeAgentDelegateNew::HermesRuntimeAgentDelegateNew(
     FrontendChannel frontendChannel,
     SessionState& sessionState,
@@ -130,3 +101,5 @@ bool HermesRuntimeAgentDelegateNew::handleRequest(
 }
 
 } // namespace facebook::react::jsinspector_modern
+
+#endif // HERMES_ENABLE_DEBUGGER
diff --git a/...s/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.h b/...s/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.h
@@ -7,6 +7,8 @@
 
 #pragma once
 
+#ifdef HERMES_ENABLE_DEBUGGER
+
 #include "HermesRuntimeTargetDelegate.h"
 
 #include <ReactCommon/RuntimeExecutor.h>
@@ -65,11 +67,14 @@ class HermesRuntimeAgentDelegateNew : public RuntimeAgentDelegate {
   bool handleRequest(const cdp::PreparsedRequest& req) override;
 
  private:
-  // We use the private implementation idiom to keep HERMES_ENABLE_DEBUGGER
-  // checks out of the header.
   class Impl;
 
   const std::unique_ptr<Impl> impl_;
 };
 
 } // namespace facebook::react::jsinspector_modern
+
+#else
+#error \
+    "HERMES_ENABLE_DEBUGGER must be enabled to use HermesRuntimeAgentDelegateNew."
+#endif // HERMES_ENABLE_DEBUGGER
diff --git a/...s/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp b/...s/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp
@@ -8,14 +8,19 @@
 #include <jsinspector-modern/InspectorFlags.h>
 #include <jsinspector-modern/RuntimeTarget.h>
 
-#include "HermesRuntimeAgentDelegate.h"
-#include "HermesRuntimeAgentDelegateNew.h"
 #include "HermesRuntimeTargetDelegate.h"
 
+// If HERMES_ENABLE_DEBUGGER isn't defined, we can't access any Hermes
+// CDPHandler headers or types.
 #ifdef HERMES_ENABLE_DEBUGGER
+#include "HermesRuntimeAgentDelegate.h"
+#include "HermesRuntimeAgentDelegateNew.h"
+
 #include <hermes/cdp/CDPDebugAPI.h>
 
 using namespace facebook::hermes::cdp;
+#else
+#include <jsinspector-modern/FallbackRuntimeTargetDelegate.h>
 #endif // HERMES_ENABLE_DEBUGGER
 
 #include <utility>
@@ -24,9 +29,9 @@ using namespace facebook::hermes;
 
 namespace facebook::react::jsinspector_modern {
 
-class HermesRuntimeTargetDelegate::Impl : public RuntimeTargetDelegate {
- public:
 #ifdef HERMES_ENABLE_DEBUGGER
+class HermesRuntimeTargetDelegate::Impl final : public RuntimeTargetDelegate {
+ public:
   explicit Impl(
       HermesRuntimeTargetDelegate& delegate,
       std::shared_ptr<HermesRuntime> hermesRuntime)
@@ -37,12 +42,6 @@ class HermesRuntimeTargetDelegate::Impl : public RuntimeTargetDelegate {
   CDPDebugAPI& getCDPDebugAPI() {
     return *cdpDebugAPI_;
   }
-#else
-  explicit Impl(
-      HermesRuntimeTargetDelegate& delegate,
-      std::shared_ptr<HermesRuntime> hermesRuntime)
-      : delegate_(delegate), runtime_(std::move(hermesRuntime)) {}
-#endif
 
   // RuntimeTargetDelegate methods
 
@@ -77,12 +76,26 @@ class HermesRuntimeTargetDelegate::Impl : public RuntimeTargetDelegate {
  private:
   HermesRuntimeTargetDelegate& delegate_;
   std::shared_ptr<HermesRuntime> runtime_;
-
-#ifdef HERMES_ENABLE_DEBUGGER
   const std::unique_ptr<CDPDebugAPI> cdpDebugAPI_;
-#endif
 };
 
+#else
+
+/**
+ * A stub for HermesRuntimeTargetDelegate when Hermes is compiled without
+ * debugging support.
+ */
+class HermesRuntimeTargetDelegate::Impl final
+    : public FallbackRuntimeTargetDelegate {
+ public:
+  explicit Impl(
+      HermesRuntimeTargetDelegate&,
+      std::shared_ptr<HermesRuntime> hermesRuntime)
+      : FallbackRuntimeTargetDelegate{hermesRuntime->description()} {}
+};
+
+#endif // HERMES_ENABLE_DEBUGGER
+
 HermesRuntimeTargetDelegate::HermesRuntimeTargetDelegate(
     std::shared_ptr<HermesRuntime> hermesRuntime)
     : impl_(std::make_unique<Impl>(*this, std::move(hermesRuntime))) {}

diff --git a/...e/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestGenericEngineAdapter.cpp b/...e/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestGenericEngineAdapter.cpp
@@ -5,8 +5,6 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-#include <jsinspector-modern/FallbackRuntimeAgentDelegate.h>
-
 #include <folly/executors/QueuedImmediateExecutor.h>
 #include <hermes/hermes.h>