From a09ab536923663224011836dc9f7dcf52a0fa8ea Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Sun, 22 Dec 2019 22:20:02 -0800 Subject: [PATCH] Fabric: Use ComponentDescriptorParameters as an actual parameter of ComponentDescriptor constructor Summary: This diff changes the signature of ComponentDescriptor constructor to make it simpler and easier to support: now all arguments are passed via struct that contains all these arguments as fields. Now the ComponentDescriptor constructor accepts three arguments one of which is optional. This causes some confusion and the possibility of bugs in all subclasses that needs to implement a custom constructor. Mostly because in every case we need to ensure that the constructor: * Accepts and pass down all parameters/arguments; * Accepts the right types of those parameters (shared vs weak pointers, references vs values). * Accepts all thee arguments and pass them (including flavor!). We failed this point several times. Overal that makes the code simpler and allows changing the set of parameters relatively easy. (There is no plan for it!) Look at the LOC balance: less code! Changelog: [INTERNAL] Reviewed By: sammy-SC Differential Revision: D18548173 fbshipit-source-id: 5d038b135e004f6c054026b3235ed57db99c086d --- .../image/ImageComponentDescriptor.h | 9 +++---- ...acyViewManagerInteropComponentDescriptor.h | 4 +-- ...cyViewManagerInteropComponentDescriptor.mm | 25 ++++++++----------- .../modal/ModalHostViewComponentDescriptor.h | 9 ------- .../slider/SliderComponentDescriptor.h | 11 +++----- .../AndroidSwitchComponentDescriptor.h | 8 +++--- .../paragraph/ParagraphComponentDescriptor.h | 12 +++------ .../AndroidTextInputComponentDescriptor.h | 11 +++----- .../ComponentDescriptor.cpp | 10 +++----- .../componentdescriptor/ComponentDescriptor.h | 17 ++++++++++--- .../ConcreteComponentDescriptor.h | 7 ++---- .../core/tests/ComponentDescriptorTest.cpp | 9 ++++--- .../fabric/core/tests/ShadowNodeTest.cpp | 18 ++++++++----- .../uimanager/ComponentDescriptorProvider.h | 16 +----------- ReactCommon/fabric/uimanager/Scheduler.cpp | 4 +-- 15 files changed, 67 insertions(+), 103 deletions(-) diff --git a/ReactCommon/fabric/components/image/ImageComponentDescriptor.h b/ReactCommon/fabric/components/image/ImageComponentDescriptor.h index a8ba3b68860..15057a0bff4 100644 --- a/ReactCommon/fabric/components/image/ImageComponentDescriptor.h +++ b/ReactCommon/fabric/components/image/ImageComponentDescriptor.h @@ -21,12 +21,9 @@ namespace react { class ImageComponentDescriptor final : public ConcreteComponentDescriptor { public: - ImageComponentDescriptor( - EventDispatcher::Weak eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor = {}) - : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor), - imageManager_(std::make_shared(contextContainer)){}; + ImageComponentDescriptor(ComponentDescriptorParameters const ¶meters) + : ConcreteComponentDescriptor(parameters), + imageManager_(std::make_shared(contextContainer_)){}; void adopt(UnsharedShadowNode shadowNode) const override { ConcreteComponentDescriptor::adopt(shadowNode); diff --git a/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.h b/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.h index c3f0051d914..8ce315f7cc9 100644 --- a/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.h +++ b/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.h @@ -19,9 +19,7 @@ class LegacyViewManagerInteropComponentDescriptor final using ConcreteComponentDescriptor::ConcreteComponentDescriptor; LegacyViewManagerInteropComponentDescriptor( - EventDispatcher::Weak const &eventDispatcher, - ContextContainer::Shared const &contextContainer = {}, - ComponentDescriptor::Flavor const &flavor = {}); + ComponentDescriptorParameters const ¶meters); /* * Returns `name` and `handle` based on a `flavor`, not on static data from * `LegacyViewManagerInteropShadowNode`. diff --git a/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.mm b/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.mm index bca28c4055c..dec8a569783 100644 --- a/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.mm +++ b/ReactCommon/fabric/components/legacyviewmanagerinterop/LegacyViewManagerInteropComponentDescriptor.mm @@ -52,32 +52,27 @@ static std::shared_ptr const constructCoordinator( } LegacyViewManagerInteropComponentDescriptor::LegacyViewManagerInteropComponentDescriptor( - EventDispatcher::Weak const &eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor) - : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor), - _coordinator(constructCoordinator(contextContainer, flavor)) + ComponentDescriptorParameters const ¶meters) + : ConcreteComponentDescriptor(parameters), _coordinator(constructCoordinator(contextContainer_, flavor_)) { } -ComponentHandle -LegacyViewManagerInteropComponentDescriptor::getComponentHandle() const { +ComponentHandle LegacyViewManagerInteropComponentDescriptor::getComponentHandle() const +{ return reinterpret_cast(getComponentName()); } -ComponentName LegacyViewManagerInteropComponentDescriptor::getComponentName() - const { +ComponentName LegacyViewManagerInteropComponentDescriptor::getComponentName() const +{ return std::static_pointer_cast(this->flavor_)->c_str(); } -void LegacyViewManagerInteropComponentDescriptor::adopt( - ShadowNode::Unshared shadowNode) const { +void LegacyViewManagerInteropComponentDescriptor::adopt(ShadowNode::Unshared shadowNode) const +{ ConcreteComponentDescriptor::adopt(shadowNode); - assert(std::dynamic_pointer_cast( - shadowNode)); - auto legacyViewManagerInteropShadowNode = - std::static_pointer_cast(shadowNode); + assert(std::dynamic_pointer_cast(shadowNode)); + auto legacyViewManagerInteropShadowNode = std::static_pointer_cast(shadowNode); auto state = LegacyViewManagerInteropState{}; state.coordinator = _coordinator; diff --git a/ReactCommon/fabric/components/modal/ModalHostViewComponentDescriptor.h b/ReactCommon/fabric/components/modal/ModalHostViewComponentDescriptor.h index 5d3d389f4f4..f1616fb7abb 100644 --- a/ReactCommon/fabric/components/modal/ModalHostViewComponentDescriptor.h +++ b/ReactCommon/fabric/components/modal/ModalHostViewComponentDescriptor.h @@ -21,16 +21,7 @@ namespace react { class ModalHostViewComponentDescriptor final : public ConcreteComponentDescriptor { public: -#ifdef ANDROID - ModalHostViewComponentDescriptor( - EventDispatcher::Weak eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor = {}) - : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor) { - } -#else using ConcreteComponentDescriptor::ConcreteComponentDescriptor; -#endif void adopt(UnsharedShadowNode shadowNode) const override { assert(std::dynamic_pointer_cast(shadowNode)); diff --git a/ReactCommon/fabric/components/slider/SliderComponentDescriptor.h b/ReactCommon/fabric/components/slider/SliderComponentDescriptor.h index deff7bc8496..30f8fd43459 100644 --- a/ReactCommon/fabric/components/slider/SliderComponentDescriptor.h +++ b/ReactCommon/fabric/components/slider/SliderComponentDescriptor.h @@ -20,15 +20,12 @@ namespace react { class SliderComponentDescriptor final : public ConcreteComponentDescriptor { public: - SliderComponentDescriptor( - EventDispatcher::Weak eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor = {}) - : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor), - imageManager_(std::make_shared(contextContainer)), + SliderComponentDescriptor(ComponentDescriptorParameters const ¶meters) + : ConcreteComponentDescriptor(parameters), + imageManager_(std::make_shared(contextContainer_)), measurementsManager_( SliderMeasurementsManager::shouldMeasureSlider() - ? std::make_shared(contextContainer) + ? std::make_shared(contextContainer_) : nullptr) {} void adopt(UnsharedShadowNode shadowNode) const override { diff --git a/ReactCommon/fabric/components/switch/androidswitch/AndroidSwitchComponentDescriptor.h b/ReactCommon/fabric/components/switch/androidswitch/AndroidSwitchComponentDescriptor.h index 3c0f07c28f9..0cc69dee54c 100644 --- a/ReactCommon/fabric/components/switch/androidswitch/AndroidSwitchComponentDescriptor.h +++ b/ReactCommon/fabric/components/switch/androidswitch/AndroidSwitchComponentDescriptor.h @@ -22,12 +22,10 @@ class AndroidSwitchComponentDescriptor final : public ConcreteComponentDescriptor { public: AndroidSwitchComponentDescriptor( - EventDispatcher::Weak eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor = {}) - : ConcreteComponentDescriptor(eventDispatcher, contextContainer, flavor), + ComponentDescriptorParameters const ¶meters) + : ConcreteComponentDescriptor(parameters), measurementsManager_(std::make_shared( - contextContainer)) {} + contextContainer_)) {} void adopt(UnsharedShadowNode shadowNode) const override { ConcreteComponentDescriptor::adopt(shadowNode); diff --git a/ReactCommon/fabric/components/text/paragraph/ParagraphComponentDescriptor.h b/ReactCommon/fabric/components/text/paragraph/ParagraphComponentDescriptor.h index 8b9fa266034..d360c93d339 100644 --- a/ReactCommon/fabric/components/text/paragraph/ParagraphComponentDescriptor.h +++ b/ReactCommon/fabric/components/text/paragraph/ParagraphComponentDescriptor.h @@ -23,17 +23,11 @@ namespace react { class ParagraphComponentDescriptor final : public ConcreteComponentDescriptor { public: - ParagraphComponentDescriptor( - EventDispatcher::Weak eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor = {}) - : ConcreteComponentDescriptor( - eventDispatcher, - contextContainer, - flavor) { + ParagraphComponentDescriptor(ComponentDescriptorParameters const ¶meters) + : ConcreteComponentDescriptor(parameters) { // Every single `ParagraphShadowNode` will have a reference to // a shared `TextLayoutManager`. - textLayoutManager_ = std::make_shared(contextContainer); + textLayoutManager_ = std::make_shared(contextContainer_); } protected: diff --git a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h index 6a1a2c5bfd4..434744d7f6e 100644 --- a/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h +++ b/ReactCommon/fabric/components/textinput/androidtextinput/AndroidTextInputComponentDescriptor.h @@ -20,16 +20,11 @@ class AndroidTextInputComponentDescriptor final : public ConcreteComponentDescriptor { public: AndroidTextInputComponentDescriptor( - EventDispatcher::Weak eventDispatcher, - const ContextContainer::Shared &contextContainer, - ComponentDescriptor::Flavor const &flavor = {}) - : ConcreteComponentDescriptor( - eventDispatcher, - contextContainer, - flavor) { + ComponentDescriptorParameters const ¶meters) + : ConcreteComponentDescriptor(parameters) { // Every single `AndroidTextInputShadowNode` will have a reference to // a shared `TextLayoutManager`. - textLayoutManager_ = std::make_shared(contextContainer); + textLayoutManager_ = std::make_shared(contextContainer_); } protected: diff --git a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.cpp b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.cpp index 67734bf93e7..dae316ad361 100644 --- a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.cpp +++ b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.cpp @@ -11,12 +11,10 @@ namespace facebook { namespace react { ComponentDescriptor::ComponentDescriptor( - EventDispatcher::Weak const &eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor) - : eventDispatcher_(eventDispatcher), - contextContainer_(contextContainer), - flavor_(flavor) {} + ComponentDescriptorParameters const ¶meters) + : eventDispatcher_(parameters.eventDispatcher), + contextContainer_(parameters.contextContainer), + flavor_(parameters.flavor) {} ContextContainer::Shared const &ComponentDescriptor::getContextContainer() const { diff --git a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h index 41a611107e0..74b24026909 100644 --- a/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h +++ b/ReactCommon/fabric/core/componentdescriptor/ComponentDescriptor.h @@ -18,6 +18,7 @@ namespace facebook { namespace react { +class ComponentDescriptorParameters; class ComponentDescriptor; using SharedComponentDescriptor = std::shared_ptr; @@ -44,10 +45,7 @@ class ComponentDescriptor { */ using Flavor = std::shared_ptr; - ComponentDescriptor( - EventDispatcher::Weak const &eventDispatcher, - ContextContainer::Shared const &contextContainer, - ComponentDescriptor::Flavor const &flavor); + ComponentDescriptor(ComponentDescriptorParameters const ¶meters); virtual ~ComponentDescriptor() = default; @@ -136,5 +134,16 @@ class ComponentDescriptor { Flavor flavor_; }; +/* + * Represents a collection of arguments that sufficient to construct a + * `ComponentDescriptor`. + */ +class ComponentDescriptorParameters { + public: + EventDispatcher::Weak eventDispatcher; + ContextContainer::Shared contextContainer; + ComponentDescriptor::Flavor flavor; +}; + } // namespace react } // namespace facebook diff --git a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h index 21670bb6b96..3b62f34d3d7 100644 --- a/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h +++ b/ReactCommon/fabric/core/componentdescriptor/ConcreteComponentDescriptor.h @@ -45,11 +45,8 @@ class ConcreteComponentDescriptor : public ComponentDescriptor { using ConcreteState = typename ShadowNodeT::ConcreteState; using ConcreteStateData = typename ShadowNodeT::ConcreteState::Data; - ConcreteComponentDescriptor( - EventDispatcher::Weak const &eventDispatcher, - ContextContainer::Shared const &contextContainer = {}, - ComponentDescriptor::Flavor const &flavor = {}) - : ComponentDescriptor(eventDispatcher, contextContainer, flavor) { + ConcreteComponentDescriptor(ComponentDescriptorParameters const ¶meters) + : ComponentDescriptor(parameters) { rawPropsParser_.prepare(); } diff --git a/ReactCommon/fabric/core/tests/ComponentDescriptorTest.cpp b/ReactCommon/fabric/core/tests/ComponentDescriptorTest.cpp index fb11a57aa0e..2a77081ffa5 100644 --- a/ReactCommon/fabric/core/tests/ComponentDescriptorTest.cpp +++ b/ReactCommon/fabric/core/tests/ComponentDescriptorTest.cpp @@ -14,7 +14,8 @@ using namespace facebook::react; TEST(ComponentDescriptorTest, createShadowNode) { auto eventDispatcher = std::shared_ptr(); SharedComponentDescriptor descriptor = - std::make_shared(eventDispatcher); + std::make_shared( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); ASSERT_EQ(descriptor->getComponentHandle(), TestShadowNode::Handle()); ASSERT_STREQ(descriptor->getComponentName(), TestShadowNode::Name()); @@ -44,7 +45,8 @@ TEST(ComponentDescriptorTest, createShadowNode) { TEST(ComponentDescriptorTest, cloneShadowNode) { auto eventDispatcher = std::shared_ptr(); SharedComponentDescriptor descriptor = - std::make_shared(eventDispatcher); + std::make_shared( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); const auto &raw = RawProps(folly::dynamic::object("nativeID", "abc")); SharedProps props = descriptor->cloneProps(nullptr, raw); @@ -68,7 +70,8 @@ TEST(ComponentDescriptorTest, cloneShadowNode) { TEST(ComponentDescriptorTest, appendChild) { auto eventDispatcher = std::shared_ptr(); SharedComponentDescriptor descriptor = - std::make_shared(eventDispatcher); + std::make_shared( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); const auto &raw = RawProps(folly::dynamic::object("nativeID", "abc")); SharedProps props = descriptor->cloneProps(nullptr, raw); diff --git a/ReactCommon/fabric/core/tests/ShadowNodeTest.cpp b/ReactCommon/fabric/core/tests/ShadowNodeTest.cpp index ffc2cf9f67b..1e33586b750 100644 --- a/ReactCommon/fabric/core/tests/ShadowNodeTest.cpp +++ b/ReactCommon/fabric/core/tests/ShadowNodeTest.cpp @@ -17,7 +17,8 @@ using namespace facebook::react; TEST(ShadowNodeTest, handleShadowNodeCreation) { auto eventDispatcher = std::shared_ptr(); - auto componentDescriptor = TestComponentDescriptor(eventDispatcher); + auto componentDescriptor = TestComponentDescriptor( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); auto family = std::make_shared( ShadowNodeFamilyFragment{ /* .tag = */ 9, @@ -47,7 +48,8 @@ TEST(ShadowNodeTest, handleShadowNodeCreation) { TEST(ShadowNodeTest, handleShadowNodeSimpleCloning) { auto eventDispatcher = std::shared_ptr(); - auto componentDescriptor = TestComponentDescriptor(eventDispatcher); + auto componentDescriptor = TestComponentDescriptor( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); auto family = std::make_shared( ShadowNodeFamilyFragment{ /* .tag = */ 9, @@ -72,7 +74,8 @@ TEST(ShadowNodeTest, handleShadowNodeSimpleCloning) { TEST(ShadowNodeTest, handleShadowNodeMutation) { auto eventDispatcher = std::shared_ptr(); - auto componentDescriptor = TestComponentDescriptor(eventDispatcher); + auto componentDescriptor = TestComponentDescriptor( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); auto family1 = std::make_shared( ShadowNodeFamilyFragment{ /* .tag = */ 1, @@ -147,7 +150,8 @@ TEST(ShadowNodeTest, handleShadowNodeMutation) { TEST(ShadowNodeTest, handleCloneFunction) { auto eventDispatcher = std::shared_ptr(); - auto componentDescriptor = TestComponentDescriptor(eventDispatcher); + auto componentDescriptor = TestComponentDescriptor( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); auto family = std::make_shared( ShadowNodeFamilyFragment{ /* .tag = */ 9, @@ -181,7 +185,8 @@ TEST(ShadowNodeTest, handleCloneFunction) { TEST(ShadowNodeTest, handleLocalData) { auto eventDispatcher = std::shared_ptr(); - auto componentDescriptor = TestComponentDescriptor(eventDispatcher); + auto componentDescriptor = TestComponentDescriptor( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); auto family = std::make_shared( ShadowNodeFamilyFragment{ /* .tag = */ 9, @@ -250,7 +255,8 @@ TEST(ShadowNodeTest, handleBacktracking) { */ auto eventDispatcher = std::shared_ptr(); - auto componentDescriptor = TestComponentDescriptor(eventDispatcher); + auto componentDescriptor = TestComponentDescriptor( + ComponentDescriptorParameters{eventDispatcher, nullptr, nullptr}); auto props = std::make_shared(); auto familyAA = std::make_shared( diff --git a/ReactCommon/fabric/uimanager/ComponentDescriptorProvider.h b/ReactCommon/fabric/uimanager/ComponentDescriptorProvider.h index 1cb39f3f2e5..a1636eba02a 100644 --- a/ReactCommon/fabric/uimanager/ComponentDescriptorProvider.h +++ b/ReactCommon/fabric/uimanager/ComponentDescriptorProvider.h @@ -14,17 +14,6 @@ namespace facebook { namespace react { -/* - * Represents a collection of arguments that sufficient to construct a - * `ComponentDescriptor`. - */ -class ComponentDescriptorParameters { - public: - EventDispatcher::Weak eventDispatcher; - ContextContainer::Shared contextContainer; - ComponentDescriptor::Flavor flavor; -}; - /* * Callable signature that represents the signature of `ComponentDescriptor` * constructor. The callable returns a unique pointer conveniently represents an @@ -61,10 +50,7 @@ ComponentDescriptor::Unique concreteComponentDescriptorConstructor( std::is_base_of::value, "ComponentDescriptorT must be a descendant of ComponentDescriptor"); - return std::make_unique( - parameters.eventDispatcher, - parameters.contextContainer, - parameters.flavor); + return std::make_unique(parameters); } /* diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index 602ef105001..da7c197fcf3 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -61,8 +61,8 @@ Scheduler::Scheduler( componentDescriptorRegistry_ = schedulerToolbox.componentRegistryFactory( eventDispatcher_, schedulerToolbox.contextContainer); - rootComponentDescriptor_ = - std::make_unique(eventDispatcher_); + rootComponentDescriptor_ = std::make_unique( + ComponentDescriptorParameters{eventDispatcher_, nullptr, nullptr}); uiManager->setDelegate(this); uiManager->setComponentDescriptorRegistry(componentDescriptorRegistry_);