From 88241812b7102b62c218086e5068835e7b188cb7 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Mon, 11 Mar 2024 09:30:53 -0700 Subject: [PATCH] Make FallbackRuntimeAgentDelegate private (#43348) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/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 --- .../chrome/HermesRuntimeAgentDelegate.cpp | 40 ++----------------- .../chrome/HermesRuntimeAgentDelegate.h | 9 ++++- .../chrome/HermesRuntimeAgentDelegateNew.cpp | 35 ++-------------- .../chrome/HermesRuntimeAgentDelegateNew.h | 9 ++++- .../chrome/HermesRuntimeTargetDelegate.cpp | 39 ++++++++++++------ ...JsiIntegrationTestGenericEngineAdapter.cpp | 2 - 6 files changed, 48 insertions(+), 86 deletions(-) diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.cpp index 3b1d1caa11c1e0..2641d55c009830 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.cpp +++ b/packages/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 #include -#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 -#endif // HERMES_ENABLE_DEBUGGER #include #include @@ -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 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, - const ExecutionContextDescription&, - std::shared_ptr 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/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.h b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.h index 52614c9474a8fa..22bf33bf86272c 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.h +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegate.h @@ -7,6 +7,8 @@ #pragma once +#ifdef HERMES_ENABLE_DEBUGGER + #include #include @@ -61,11 +63,14 @@ class HermesRuntimeAgentDelegate : public RuntimeAgentDelegate { virtual std::unique_ptr 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_; }; } // namespace facebook::react::jsinspector_modern + +#else +#error \ + "HERMES_ENABLE_DEBUGGER must be enabled to use HermesRuntimeAgentDelegate." +#endif // HERMES_ENABLE_DEBUGGER diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.cpp index 429b7e69132edd..2f628aef9d1540 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.cpp +++ b/packages/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 #include #include -#else // HERMES_ENABLE_DEBUGGER -#include -#endif // HERMES_ENABLE_DEBUGGER #include #include @@ -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_; }; -#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, - 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/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.h b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.h index 2505451eb27deb..3a8e959b23fc39 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.h +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeAgentDelegateNew.h @@ -7,6 +7,8 @@ #pragma once +#ifdef HERMES_ENABLE_DEBUGGER + #include "HermesRuntimeTargetDelegate.h" #include @@ -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_; }; } // namespace facebook::react::jsinspector_modern + +#else +#error \ + "HERMES_ENABLE_DEBUGGER must be enabled to use HermesRuntimeAgentDelegateNew." +#endif // HERMES_ENABLE_DEBUGGER diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp index fbbb1a51c37964..9c5542b08ebe78 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/HermesRuntimeTargetDelegate.cpp @@ -8,14 +8,19 @@ #include #include -#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 using namespace facebook::hermes::cdp; +#else +#include #endif // HERMES_ENABLE_DEBUGGER #include @@ -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) @@ -37,12 +42,6 @@ class HermesRuntimeTargetDelegate::Impl : public RuntimeTargetDelegate { CDPDebugAPI& getCDPDebugAPI() { return *cdpDebugAPI_; } -#else - explicit Impl( - HermesRuntimeTargetDelegate& delegate, - std::shared_ptr 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 runtime_; - -#ifdef HERMES_ENABLE_DEBUGGER const std::unique_ptr 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) + : FallbackRuntimeTargetDelegate{hermesRuntime->description()} {} +}; + +#endif // HERMES_ENABLE_DEBUGGER + HermesRuntimeTargetDelegate::HermesRuntimeTargetDelegate( std::shared_ptr hermesRuntime) : impl_(std::make_unique(*this, std::move(hermesRuntime))) {} diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestGenericEngineAdapter.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestGenericEngineAdapter.cpp index e7afe3e74667bd..db0e3693787729 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestGenericEngineAdapter.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/engines/JsiIntegrationTestGenericEngineAdapter.cpp @@ -5,8 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -#include - #include #include