From fd99330b6caee444616329cab061473740db0d3b Mon Sep 17 00:00:00 2001 From: Theo Yaung Date: Thu, 6 Apr 2017 00:01:37 -0700 Subject: [PATCH] Refactor interfaces Reviewed By: johnislarry Differential Revision: D4840716 fbshipit-source-id: 1a82437c81ce5b767efd39ab0716998bab4f5363 --- ReactAndroid/src/main/jni/xreact/jni/BUCK | 1 - .../src/main/jni/xreact/jni/JInspector.cpp | 19 ++++--- .../src/main/jni/xreact/jni/JInspector.h | 12 ++-- .../src/main/jni/xreact/jni/OnLoad.cpp | 4 +- ReactCommon/cxxreact/BUCK | 1 - ReactCommon/cxxreact/JSCExecutor.cpp | 29 ++++++---- ReactCommon/jschelpers/BUCK | 1 + ReactCommon/jschelpers/InspectorInterfaces.h | 55 +++++++++++++++++++ ReactCommon/jschelpers/JSCWrapper.h | 28 +++++++--- ReactCommon/jschelpers/JavaScriptCore.h | 4 ++ ReactCommon/jschelpers/systemJSCWrapper.cpp | 7 +++ 11 files changed, 126 insertions(+), 35 deletions(-) create mode 100644 ReactCommon/jschelpers/InspectorInterfaces.h diff --git a/ReactAndroid/src/main/jni/xreact/jni/BUCK b/ReactAndroid/src/main/jni/xreact/jni/BUCK index 344814039..3f005f557 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/BUCK +++ b/ReactAndroid/src/main/jni/xreact/jni/BUCK @@ -39,7 +39,6 @@ cxx_library( preprocessor_flags = [ "-DLOG_TAG=\"ReactNativeJNI\"", "-DWITH_FBSYSTRACE=1", - "-DWITH_INSPECTOR=1", ], soname = "libreactnativejnifb.$(ext)", visibility = [ diff --git a/ReactAndroid/src/main/jni/xreact/jni/JInspector.cpp b/ReactAndroid/src/main/jni/xreact/jni/JInspector.cpp index cad09b1cc..5da21db08 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/JInspector.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/JInspector.cpp @@ -1,24 +1,25 @@ // Copyright 2004-present Facebook. All Rights Reserved. #include "JInspector.h" +#include -#ifdef WITH_INSPECTOR +#ifdef WITH_FBJSCEXTENSIONS namespace facebook { namespace react { namespace { -class RemoteConnection : public Inspector::RemoteConnection { +class RemoteConnection : public IRemoteConnection { public: RemoteConnection(jni::alias_ref connection) : connection_(jni::make_global(connection)) {} - void onMessage(std::string message) override { + virtual void onMessage(std::string message) override { connection_->onMessage(message); } - void onDisconnect() override { + virtual void onDisconnect() override { connection_->onDisconnect(); } private: @@ -42,7 +43,7 @@ void JRemoteConnection::onDisconnect() const { method(self()); } -JLocalConnection::JLocalConnection(std::unique_ptr connection) +JLocalConnection::JLocalConnection(std::unique_ptr connection) : connection_(std::move(connection)) {} void JLocalConnection::sendMessage(std::string message) { @@ -60,13 +61,17 @@ void JLocalConnection::registerNatives() { }); } +static IInspector* getInspectorInstance() { + return JSC_JSInspectorGetInstance(true /*useCustomJSC*/); +} + jni::global_ref JInspector::instance(jni::alias_ref) { - static auto instance = jni::make_global(newObjectCxxArgs(&Inspector::instance())); + static auto instance = jni::make_global(newObjectCxxArgs(getInspectorInstance()/*&Inspector::instance()*/)); return instance; } jni::local_ref> JInspector::getPages() { - std::vector pages = inspector_->getPages(); + std::vector pages = inspector_->getPages(); auto array = jni::JArrayClass::newArray(pages.size()); for (size_t i = 0; i < pages.size(); i++) { (*array)[i] = JPage::create(pages[i].id, pages[i].title); diff --git a/ReactAndroid/src/main/jni/xreact/jni/JInspector.h b/ReactAndroid/src/main/jni/xreact/jni/JInspector.h index 0b9db509a..d28523d09 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/JInspector.h +++ b/ReactAndroid/src/main/jni/xreact/jni/JInspector.h @@ -2,9 +2,9 @@ #pragma once -#ifdef WITH_INSPECTOR +#ifdef WITH_FBJSCEXTENSIONS -#include +#include #include #include @@ -31,14 +31,14 @@ class JLocalConnection : public jni::HybridClass { public: static constexpr auto kJavaDescriptor = "Lcom/facebook/react/bridge/Inspector$LocalConnection;"; - JLocalConnection(std::unique_ptr connection); + JLocalConnection(std::unique_ptr connection); void sendMessage(std::string message); void disconnect(); static void registerNatives(); private: - std::unique_ptr connection_; + std::unique_ptr connection_; }; class JInspector : public jni::HybridClass { @@ -54,9 +54,9 @@ public: private: friend HybridBase; - JInspector(Inspector* inspector) : inspector_(inspector) {} + JInspector(IInspector* inspector) : inspector_(inspector) {} - Inspector* inspector_; + IInspector* inspector_; }; } diff --git a/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp b/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp index 7be58ad9f..6133675df 100644 --- a/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp +++ b/ReactAndroid/src/main/jni/xreact/jni/OnLoad.cpp @@ -18,7 +18,7 @@ #include "JCallback.h" #include "JSLogging.h" -#ifdef WITH_INSPECTOR +#ifdef WITH_FBJSCEXTENSIONS #include "JInspector.h" #endif @@ -171,7 +171,7 @@ extern "C" JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void* reserved) { CxxModuleWrapperBase::registerNatives(); CxxModuleWrapper::registerNatives(); JCallbackImpl::registerNatives(); - #ifdef WITH_INSPECTOR + #ifdef WITH_FBJSCEXTENSIONS JInspector::registerNatives(); #endif diff --git a/ReactCommon/cxxreact/BUCK b/ReactCommon/cxxreact/BUCK index 65cafc150..127c5e3d6 100644 --- a/ReactCommon/cxxreact/BUCK +++ b/ReactCommon/cxxreact/BUCK @@ -39,7 +39,6 @@ if THIS_IS_FBANDROID: '-DWITH_JSC_MEMORY_PRESSURE=1', '-DWITH_REACT_INTERNAL_SETTINGS=1', '-DWITH_FB_MEMORY_PROFILING=1', - '-DWITH_INSPECTOR=1', ], deps = JSC_DEPS, visibility = [ diff --git a/ReactCommon/cxxreact/JSCExecutor.cpp b/ReactCommon/cxxreact/JSCExecutor.cpp index 810d0ecfc..3027200d9 100644 --- a/ReactCommon/cxxreact/JSCExecutor.cpp +++ b/ReactCommon/cxxreact/JSCExecutor.cpp @@ -18,10 +18,7 @@ #include #include - -#ifdef WITH_INSPECTOR -#include -#endif +#include #include "JSBundleType.h" #include "Platform.h" @@ -220,6 +217,16 @@ void JSCExecutor::setContextName(const std::string& name) { JSC_JSGlobalContextSetName(m_context, jsName); } +static bool canUseInspector(JSContextRef context) { +#if defined(__APPLE__) + return isCustomJSCPtr(context); +#elif defined(WITH_FBJSCEXTENSIONS) + return true; +#else + return false; +#endif +} + void JSCExecutor::initOnJSVMThread() throw(JSException) { SystraceSection s("JSCExecutor.initOnJSVMThread"); @@ -253,9 +260,10 @@ void JSCExecutor::initOnJSVMThread() throw(JSException) { // Add a pointer to ourselves so we can retrieve it later in our hooks Object::getGlobalObject(m_context).setPrivate(this); - #ifdef WITH_INSPECTOR - Inspector::instance().registerGlobalContext("main", m_context); - #endif + if (canUseInspector(m_context)) { + IInspector* pInspector = JSC_JSInspectorGetInstance(true); + pInspector->registerGlobalContext("main", m_context); + } installNativeHook<&JSCExecutor::nativeFlushQueueImmediate>("nativeFlushQueueImmediate"); installNativeHook<&JSCExecutor::nativeCallSyncHook>("nativeCallSyncHook"); @@ -311,9 +319,10 @@ void JSCExecutor::terminateOnJSVMThread() { m_nativeModules.reset(); - #ifdef WITH_INSPECTOR - Inspector::instance().unregisterGlobalContext(m_context); - #endif + if (canUseInspector(m_context)) { + IInspector* pInspector = JSC_JSInspectorGetInstance(true); + pInspector->unregisterGlobalContext(m_context); + } JSC_JSGlobalContextRelease(m_context); m_context = nullptr; diff --git a/ReactCommon/jschelpers/BUCK b/ReactCommon/jschelpers/BUCK index ef759f4ca..af29360f7 100644 --- a/ReactCommon/jschelpers/BUCK +++ b/ReactCommon/jschelpers/BUCK @@ -1,4 +1,5 @@ EXPORTED_HEADERS = [ + "InspectorInterfaces.h", "JavaScriptCore.h", "JSCHelpers.h", "JSCWrapper.h", diff --git a/ReactCommon/jschelpers/InspectorInterfaces.h b/ReactCommon/jschelpers/InspectorInterfaces.h new file mode 100644 index 000000000..e19eae5d9 --- /dev/null +++ b/ReactCommon/jschelpers/InspectorInterfaces.h @@ -0,0 +1,55 @@ +/** + * Copyright (c) 2016-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +#pragma once + +#include +#include +#include +#include + +namespace facebook { +namespace react { + +class IDestructible { +public: + virtual ~IDestructible() = 0; +}; + +struct InspectorPage { + const int id; + const std::string title; +}; + +class IRemoteConnection : public IDestructible { +public: + virtual ~IRemoteConnection() = 0; + virtual void onMessage(std::string message) = 0; + virtual void onDisconnect() = 0; +}; + +class ILocalConnection : public IDestructible { +public: + virtual ~ILocalConnection() = 0; + virtual void sendMessage(std::string message) = 0; + virtual void disconnect() = 0; +}; + +// Note: not destructible! +class IInspector { +public: + virtual void registerGlobalContext(std::string title, JSGlobalContextRef ctx) = 0; + virtual void unregisterGlobalContext(JSGlobalContextRef ctx) = 0; + + virtual std::vector getPages() const = 0; + virtual std::unique_ptr connect(int pageId, std::unique_ptr remote) = 0; +}; + +} +} diff --git a/ReactCommon/jschelpers/JSCWrapper.h b/ReactCommon/jschelpers/JSCWrapper.h index c1a6d0cc1..736c84070 100644 --- a/ReactCommon/jschelpers/JSCWrapper.h +++ b/ReactCommon/jschelpers/JSCWrapper.h @@ -15,19 +15,29 @@ #include #endif +#include + +#if JSCINTERNAL || (!defined(__APPLE__)) +#define JSC_IMPORT extern "C" +#else +#define JSC_IMPORT extern +#endif + +JSC_IMPORT facebook::react::IInspector* JSInspectorGetInstance(); + +// This is used to substitute an alternate JSC implementation for +// testing. These calls must all be ABI compatible with the standard JSC. +JSC_IMPORT void configureJSCForIOS(std::string); // TODO: replace with folly::dynamic once supported +JSC_IMPORT JSValueRef JSEvaluateBytecodeBundle(JSContextRef, JSObjectRef, int, JSStringRef, JSValueRef*); +JSC_IMPORT bool JSSamplingProfilerEnabled(); +JSC_IMPORT void JSStartSamplingProfilingOnMainJSCThread(JSGlobalContextRef); +JSC_IMPORT JSValueRef JSPokeSamplingProfiler(JSContextRef); + #if defined(__APPLE__) #import #import #import -// This is used to substitute an alternate JSC implementation for -// testing. These calls must all be ABI compatible with the standard JSC. -extern void configureJSCForIOS(std::string); // TODO: replace with folly::dynamic once supported -extern JSValueRef JSEvaluateBytecodeBundle(JSContextRef, JSObjectRef, int, JSStringRef, JSValueRef*); -extern bool JSSamplingProfilerEnabled(); -extern void JSStartSamplingProfilingOnMainJSCThread(JSGlobalContextRef); -extern JSValueRef JSPokeSamplingProfiler(JSContextRef); - /** * JSNoBytecodeFileFormatVersion * @@ -114,6 +124,8 @@ struct JSCWrapper { JSC_WRAPPER_METHOD(JSPokeSamplingProfiler); JSC_WRAPPER_METHOD(JSStartSamplingProfilingOnMainJSCThread); + JSC_WRAPPER_METHOD(JSInspectorGetInstance); + JSC_WRAPPER_METHOD(configureJSCForIOS); // Objective-C API diff --git a/ReactCommon/jschelpers/JavaScriptCore.h b/ReactCommon/jschelpers/JavaScriptCore.h index 4d8498834..f0e4423ff 100644 --- a/ReactCommon/jschelpers/JavaScriptCore.h +++ b/ReactCommon/jschelpers/JavaScriptCore.h @@ -182,6 +182,10 @@ jsc_poison(JSObjectMakeArrayBufferWithBytesNoCopy JSObjectMakeTypedArray jsc_poison(JSSamplingProfilerEnabled JSPokeSamplingProfiler JSStartSamplingProfilingOnMainJSCThread) +#define JSC_JSInspectorGetInstance(...) __jsc_bool_wrapper(JSInspectorGetInstance, __VA_ARGS__) +jsc_poison(JSInspectorGetInstance) + + #define JSC_configureJSCForIOS(...) __jsc_bool_wrapper(configureJSCForIOS, __VA_ARGS__) jsc_poison(configureJSCForIOS) diff --git a/ReactCommon/jschelpers/systemJSCWrapper.cpp b/ReactCommon/jschelpers/systemJSCWrapper.cpp index 80000c3fa..a36124838 100644 --- a/ReactCommon/jschelpers/systemJSCWrapper.cpp +++ b/ReactCommon/jschelpers/systemJSCWrapper.cpp @@ -28,6 +28,9 @@ UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSStringCreateWithUTF8CStringExpectAscii) #endif UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSPokeSamplingProfiler) UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSStartSamplingProfilingOnMainJSCThread) + +UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(JSInspectorGetInstance) + UNIMPLEMENTED_SYSTEM_JSC_FUNCTION(configureJSCForIOS) bool JSSamplingProfilerEnabled() { @@ -119,6 +122,10 @@ const JSCWrapper* systemJSCWrapper() { (decltype(&JSStartSamplingProfilingOnMainJSCThread)) Unimplemented_JSStartSamplingProfilingOnMainJSCThread, + .JSInspectorGetInstance = + (decltype(&JSInspectorGetInstance)) + Unimplemented_JSInspectorGetInstance, + .configureJSCForIOS = (decltype(&configureJSCForIOS))Unimplemented_configureJSCForIOS,