From 205de0538cd16943bfe1cb8c35bb8005310e1600 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Fri, 7 Jun 2019 12:00:01 -0700 Subject: [PATCH] Fabric: New names for ContextContainer methods Summary: ... and slighly new behaviour for one of them. The method does nothing if given `key` already exists in the container. This diff finishes the transition of ContextContainer from an internal bag of things with unclear yet ownership into a legit dedicated dependency injection container for the product code. The original names of methods imply that the container can have only one object of a given type which is no longer true. The new API is much more generic and idiomatic to C++, it mimics `std:map` API which is intuitive to anyone who familiar with C++ containers. Besides the naming, `insert` method changed the semantic a bit; now it does nothing in case of inserting an object with a key that already exists. That might seem counterintuitive for "normal" people, but C++ has some wired reasons for that and, hopefully, it's expected behavior in the C++ community. Fun fact: We need this to fix hot-reload. Reviewed By: sahrens Differential Revision: D15681736 fbshipit-source-id: 194f342528446a911eaf072ba3a94a5d8af3cb52 --- React/Fabric/RCTSurfacePresenter.mm | 4 ++-- .../java/com/facebook/react/fabric/jni/Binding.cpp | 4 ++-- .../platform/android/SliderMeasurementsManager.cpp | 3 +-- .../fabric/imagemanager/platform/ios/ImageManager.mm | 2 +- .../platform/android/TextLayoutManager.cpp | 3 +-- ReactCommon/fabric/uimanager/Scheduler.cpp | 10 +++++----- ReactCommon/utils/ContextContainer.h | 10 ++++------ 7 files changed, 16 insertions(+), 20 deletions(-) diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm index 0892963a39b..edb6972ea2d 100644 --- a/React/Fabric/RCTSurfacePresenter.mm +++ b/React/Fabric/RCTSurfacePresenter.mm @@ -260,8 +260,8 @@ using namespace facebook::react; // Those two pieces eventually should be moved out there: // * `RCTImageLoader` should be moved to `RNImageComponentView`. // * `ReactNativeConfig` should be set by outside product code. - _contextContainer->registerInstance(_reactNativeConfig, "ReactNativeConfig"); - _contextContainer->registerInstance(wrapManagedObject([_bridge imageLoader]), "RCTImageLoader"); + _contextContainer->insert("ReactNativeConfig", _reactNativeConfig); + _contextContainer->insert("RCTImageLoader", wrapManagedObject([_bridge imageLoader])); return _contextContainer; } diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp index 9d2a6c12cac..80295048094 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp +++ b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp @@ -136,8 +136,8 @@ void Binding::installFabricUIManager( std::shared_ptr config = std::make_shared(reactNativeConfig); - contextContainer->registerInstance(config, "ReactNativeConfig"); - contextContainer->registerInstance(javaUIManager_, "FabricUIManager"); + contextContainer->insert("ReactNativeConfig", config); + contextContainer->insert("FabricUIManager", javaUIManager_); auto toolbox = SchedulerToolbox{}; toolbox.contextContainer = contextContainer; diff --git a/ReactCommon/fabric/components/slider/platform/android/SliderMeasurementsManager.cpp b/ReactCommon/fabric/components/slider/platform/android/SliderMeasurementsManager.cpp index ca6e6808f2c..c44288caefc 100644 --- a/ReactCommon/fabric/components/slider/platform/android/SliderMeasurementsManager.cpp +++ b/ReactCommon/fabric/components/slider/platform/android/SliderMeasurementsManager.cpp @@ -26,8 +26,7 @@ Size SliderMeasurementsManager::measure( } const jni::global_ref &fabricUIManager = - contextContainer_->getInstance>( - "FabricUIManager"); + contextContainer_->at>("FabricUIManager"); static auto measure = jni::findClassStatic("com/facebook/react/fabric/FabricUIManager") diff --git a/ReactCommon/fabric/imagemanager/platform/ios/ImageManager.mm b/ReactCommon/fabric/imagemanager/platform/ios/ImageManager.mm index d5c64f5f06b..e3b4074eb24 100644 --- a/ReactCommon/fabric/imagemanager/platform/ios/ImageManager.mm +++ b/ReactCommon/fabric/imagemanager/platform/ios/ImageManager.mm @@ -18,7 +18,7 @@ namespace react { ImageManager::ImageManager(ContextContainer::Shared const &contextContainer) { RCTImageLoader *imageLoader = - (RCTImageLoader *)unwrapManagedObject(contextContainer->getInstance>("RCTImageLoader")); + (RCTImageLoader *)unwrapManagedObject(contextContainer->at>("RCTImageLoader")); self_ = (__bridge_retained void *)[[RCTImageManager alloc] initWithImageLoader:imageLoader]; } diff --git a/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp b/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp index b6e04fe311e..46e9fb6e68f 100644 --- a/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp +++ b/ReactCommon/fabric/textlayoutmanager/platform/android/TextLayoutManager.cpp @@ -27,8 +27,7 @@ Size TextLayoutManager::measure( ParagraphAttributes paragraphAttributes, LayoutConstraints layoutConstraints) const { const jni::global_ref &fabricUIManager = - contextContainer_->getInstance>( - "FabricUIManager"); + contextContainer_->at>("FabricUIManager"); static auto measure = jni::findClassStatic("com/facebook/react/fabric/FabricUIManager") diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index 1eb3fcd969f..9b78bdc649e 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -22,8 +22,8 @@ Scheduler::Scheduler(SchedulerToolbox schedulerToolbox) { runtimeExecutor_ = schedulerToolbox.runtimeExecutor; reactNativeConfig_ = - schedulerToolbox.contextContainer->getInstance>( - "ReactNativeConfig"); + schedulerToolbox.contextContainer + ->at>("ReactNativeConfig"); auto uiManager = std::make_unique(); auto &uiManagerRef = *uiManager; @@ -64,10 +64,10 @@ Scheduler::Scheduler(SchedulerToolbox schedulerToolbox) { UIManagerBinding::install(runtime, uiManagerBinding_); }); - schedulerToolbox.contextContainer->registerInstance( + schedulerToolbox.contextContainer->insert( + "ComponentDescriptorRegistry_DO_NOT_USE_PRETTY_PLEASE", std::weak_ptr( - componentDescriptorRegistry_), - "ComponentDescriptorRegistry_DO_NOT_USE_PRETTY_PLEASE"); + componentDescriptorRegistry_)); } Scheduler::~Scheduler() { diff --git a/ReactCommon/utils/ContextContainer.h b/ReactCommon/utils/ContextContainer.h index 4e5a653607b..4889a43bc7b 100644 --- a/ReactCommon/utils/ContextContainer.h +++ b/ReactCommon/utils/ContextContainer.h @@ -27,6 +27,7 @@ class ContextContainer final { /* * Registers an instance of the particular type `T` in the container * using the provided `key`. Only one instance can be registered per key. + * The method does nothing if given `key` already exists in the container. * * Convention is to use the plain base class name for the key, so for * example if the type `T` is `std::shared_ptr`, @@ -35,12 +36,9 @@ class ContextContainer final { *`EmptyReactNativeConfig`. */ template - void registerInstance(T const &instance, std::string const &key) const { + void insert(std::string const &key, T const &instance) const { std::unique_lock lock(mutex_); - assert( - instances_.find(key) == instances_.end() && - "ContextContainer already had instance for given key."); instances_.insert({key, std::make_shared(instance)}); #ifndef NDEBUG @@ -54,7 +52,7 @@ class ContextContainer final { * Throws an exception if the instance could not be found. */ template - T getInstance(std::string const &key) const { + T at(std::string const &key) const { std::shared_lock lock(mutex_); assert( @@ -72,7 +70,7 @@ class ContextContainer final { * Returns an empty optional if the instance could not be found. */ template - better::optional findInstance(std::string const &key) const { + better::optional find(std::string const &key) const { std::shared_lock lock(mutex_); auto iterator = instances_.find(key);