From 7796b7e9af931011c07e3e431fa38a3e317d45f3 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Mon, 21 Oct 2019 17:05:41 -0700 Subject: [PATCH] Avoid use of shared_from_this in StateTarget Summary: `StateTarget` no longer uses `shared_from_this`, this allows us to remove need for `enable_shared_from_this` I decided to put `state->commit` call inside `ShadowTree.cpp` because I needed to have access to `shared_ptr` of shadow node from outside of the class itself. `state->commit` was originally designed to be only called by `ShadowNode` but this does not seem to be the case anymore since it is called from `UIManager` as well. changelog: [internal] Reviewed By: shergin Differential Revision: D18032532 fbshipit-source-id: 75c874fd04f86adc07bfddbef3a0384e17c2067b --- ReactCommon/fabric/core/shadownode/ShadowNode.cpp | 3 --- ReactCommon/fabric/core/shadownode/ShadowNode.h | 3 +-- ReactCommon/fabric/core/state/State.cpp | 2 +- ReactCommon/fabric/core/state/State.h | 9 ++------- ReactCommon/fabric/core/state/StateTarget.cpp | 4 ++-- ReactCommon/fabric/core/state/StateTarget.h | 2 +- ReactCommon/fabric/mounting/ShadowTree.cpp | 9 +++++++++ ReactCommon/fabric/uimanager/UIManager.cpp | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/ReactCommon/fabric/core/shadownode/ShadowNode.cpp b/ReactCommon/fabric/core/shadownode/ShadowNode.cpp index 7c9d552922..1a6d796e7e 100644 --- a/ReactCommon/fabric/core/shadownode/ShadowNode.cpp +++ b/ReactCommon/fabric/core/shadownode/ShadowNode.cpp @@ -224,9 +224,6 @@ void ShadowNode::cloneChildrenIfShared() { void ShadowNode::setMounted(bool mounted) const { family_->eventEmitter_->setEnabled(mounted); - if (mounted && state_) { - state_->commit(*this); - } } AncestorList ShadowNode::getAncestors( diff --git a/ReactCommon/fabric/core/shadownode/ShadowNode.h b/ReactCommon/fabric/core/shadownode/ShadowNode.h index 599bcdc411..de525f0732 100644 --- a/ReactCommon/fabric/core/shadownode/ShadowNode.h +++ b/ReactCommon/fabric/core/shadownode/ShadowNode.h @@ -40,8 +40,7 @@ using SharedShadowNodeSharedList = std::shared_ptr; using SharedShadowNodeUnsharedList = std::shared_ptr; class ShadowNode : public virtual Sealable, - public virtual DebugStringConvertible, - public std::enable_shared_from_this { + public virtual DebugStringConvertible { public: using Shared = std::shared_ptr; using Weak = std::weak_ptr; diff --git a/ReactCommon/fabric/core/state/State.cpp b/ReactCommon/fabric/core/state/State.cpp index 3e2668c905..eccb455bf5 100644 --- a/ReactCommon/fabric/core/state/State.cpp +++ b/ReactCommon/fabric/core/state/State.cpp @@ -26,7 +26,7 @@ State::State(State const &state) : stateCoordinator_(state.stateCoordinator_){}; State::State(StateCoordinator::Shared const &stateCoordinator) : stateCoordinator_(stateCoordinator){}; -void State::commit(const ShadowNode &shadowNode) const { +void State::commit(std::shared_ptr const &shadowNode) const { stateCoordinator_->setTarget(StateTarget{shadowNode}); } diff --git a/ReactCommon/fabric/core/state/State.h b/ReactCommon/fabric/core/state/State.h index 19e84bdf38..4cc26ee0e6 100644 --- a/ReactCommon/fabric/core/state/State.h +++ b/ReactCommon/fabric/core/state/State.h @@ -40,18 +40,13 @@ class State { virtual void updateState(folly::dynamic data) const; #endif + void commit(std::shared_ptr const &shadowNode) const; + protected: StateCoordinator::Shared stateCoordinator_; private: - friend class ShadowNode; friend class StateCoordinator; - friend class UIManager; - - /* - * Must be used by `ShadowNode` *only*. - */ - void commit(const ShadowNode &shadowNode) const; /* * Indicates that the state was committed once and then was replaced by a diff --git a/ReactCommon/fabric/core/state/StateTarget.cpp b/ReactCommon/fabric/core/state/StateTarget.cpp index 1e132d6e5c..a97f78556f 100644 --- a/ReactCommon/fabric/core/state/StateTarget.cpp +++ b/ReactCommon/fabric/core/state/StateTarget.cpp @@ -13,8 +13,8 @@ namespace react { StateTarget::StateTarget() : shadowNode_(nullptr) {} -StateTarget::StateTarget(const ShadowNode &shadowNode) - : shadowNode_(shadowNode.shared_from_this()) {} +StateTarget::StateTarget(std::shared_ptr shadowNode) + : shadowNode_(shadowNode) {} StateTarget::operator bool() const { return (bool)shadowNode_; diff --git a/ReactCommon/fabric/core/state/StateTarget.h b/ReactCommon/fabric/core/state/StateTarget.h index 31a0f53eb1..19b1c122ed 100644 --- a/ReactCommon/fabric/core/state/StateTarget.h +++ b/ReactCommon/fabric/core/state/StateTarget.h @@ -29,7 +29,7 @@ class StateTarget { /* * Creates a target which points to a given `ShadowNode`. */ - explicit StateTarget(const ShadowNode &shadowNode); + explicit StateTarget(std::shared_ptr shadowNode); /* * Copyable and moveable. diff --git a/ReactCommon/fabric/mounting/ShadowTree.cpp b/ReactCommon/fabric/mounting/ShadowTree.cpp index cf03be5b2b..f62c242b8d 100644 --- a/ReactCommon/fabric/mounting/ShadowTree.cpp +++ b/ReactCommon/fabric/mounting/ShadowTree.cpp @@ -21,6 +21,13 @@ namespace facebook { namespace react { +static void CommitState(ShadowNode::Shared const &shadowNode) { + auto state = shadowNode->getState(); + if (state) { + state->commit(shadowNode); + } +} + static void updateMountedFlag( const SharedShadowNodeList &oldChildren, const SharedShadowNodeList &newChildren) { @@ -58,6 +65,7 @@ static void updateMountedFlag( } newChild->setMounted(true); + CommitState(newChild); oldChild->setMounted(false); updateMountedFlag(oldChild->getChildren(), newChild->getChildren()); @@ -69,6 +77,7 @@ static void updateMountedFlag( for (index = lastIndexAfterFirstStage; index < newChildren.size(); index++) { const auto &newChild = newChildren[index]; newChild->setMounted(true); + CommitState(newChild); updateMountedFlag({}, newChild->getChildren()); } diff --git a/ReactCommon/fabric/uimanager/UIManager.cpp b/ReactCommon/fabric/uimanager/UIManager.cpp index aec8da8ce6..b7a41c9dd6 100644 --- a/ReactCommon/fabric/uimanager/UIManager.cpp +++ b/ReactCommon/fabric/uimanager/UIManager.cpp @@ -57,7 +57,7 @@ SharedShadowNode UIManager::createNode( // explicitly associate the ShadowNode with the State here so that updateState // is always safe and effectful. if (state) { - state->commit(*shadowNode); + state->commit(shadowNode); } if (delegate_) {