From ed6abc4e838447bbb14a140fcee105f43c333ba0 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Thu, 18 Apr 2024 17:37:12 -0700 Subject: [PATCH] Use modern Differentiator "sliceChildShadowNodeViewPairs" in tests (#44131) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/44131 Stub tree creation is using an old differentiator path we aren't shipping today. This removes that path, so that we can unit test the new one 1. Remove `ForTesting`/`Legacy` functions 3. Update stub view tree code for API/behavior difference of new differentiator functions including unflattened views. Don't create view instructions for those, and use `mountIndex` instead of pair index 4. Remove `V2` suffix, since the old path is deleted 5. Move mounting stub test utils out of the production library Changelog: [Internal] Reviewed By: joevilches Differential Revision: D56227426 fbshipit-source-id: 0f525097cfb576e0228c9ca20a770fa41ddf1e0d --- .../renderer/mounting/Differentiator.cpp | 101 ++++-------------- .../react/renderer/mounting/Differentiator.h | 9 +- .../react/renderer/mounting/ShadowView.cpp | 10 -- .../react/renderer/mounting/ShadowView.h | 25 ----- .../mounting/{ => stubs}/StubView.cpp | 0 .../renderer/mounting/{ => stubs}/StubView.h | 0 .../mounting/{ => stubs}/StubViewTree.cpp | 0 .../mounting/{ => stubs}/StubViewTree.h | 0 .../renderer/mounting/{ => stubs}/stubs.cpp | 31 +++--- .../renderer/mounting/{ => stubs}/stubs.h | 0 10 files changed, 41 insertions(+), 135 deletions(-) rename packages/react-native/ReactCommon/react/renderer/mounting/{ => stubs}/StubView.cpp (100%) rename packages/react-native/ReactCommon/react/renderer/mounting/{ => stubs}/StubView.h (100%) rename packages/react-native/ReactCommon/react/renderer/mounting/{ => stubs}/StubViewTree.cpp (100%) rename packages/react-native/ReactCommon/react/renderer/mounting/{ => stubs}/StubViewTree.h (100%) rename packages/react-native/ReactCommon/react/renderer/mounting/{ => stubs}/stubs.cpp (75%) rename packages/react-native/ReactCommon/react/renderer/mounting/{ => stubs}/stubs.h (100%) diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp index f05a4137b90..08707a9ee9c 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -208,7 +208,7 @@ static inline bool shadowNodeIsConcrete(const ShadowNode& shadowNode) { return shadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView); } -static void sliceChildShadowNodeViewPairsRecursivelyV2( +static void sliceChildShadowNodeViewPairsRecursively( ShadowViewNodePair::NonOwningList& pairList, size_t& startOfStaticIndex, ViewNodePairScope& scope, @@ -255,21 +255,21 @@ static void sliceChildShadowNodeViewPairsRecursivelyV2( pairList.insert(it, &scope.back()); startOfStaticIndex++; if (areChildrenFlattened) { - sliceChildShadowNodeViewPairsRecursivelyV2( + sliceChildShadowNodeViewPairsRecursively( pairList, startOfStaticIndex, scope, origin, childShadowNode); } } else { pairList.push_back(&scope.back()); if (areChildrenFlattened) { size_t pairListSize = pairList.size(); - sliceChildShadowNodeViewPairsRecursivelyV2( + sliceChildShadowNodeViewPairsRecursively( pairList, pairListSize, scope, origin, childShadowNode); } } } } -ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2( +ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairs( const ShadowNode& shadowNode, ViewNodePairScope& scope, bool allowFlattened, @@ -284,7 +284,7 @@ ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2( } size_t startOfStaticIndex = 0; - sliceChildShadowNodeViewPairsRecursivelyV2( + sliceChildShadowNodeViewPairsRecursively( pairList, startOfStaticIndex, scope, layoutOffset, shadowNode); // Sorting pairs based on `orderIndex` if needed. @@ -300,7 +300,7 @@ ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2( } /** - * Prefer calling this over `sliceChildShadowNodeViewPairsV2` directly, when + * Prefer calling this over `sliceChildShadowNodeViewPairs` directly, when * possible. This can account for adding parent LayoutMetrics that are * important to take into account, but tricky, in (un)flattening cases. */ @@ -309,7 +309,7 @@ sliceChildShadowNodeViewPairsFromViewNodePair( const ShadowViewNodePair& shadowViewNodePair, ViewNodePairScope& scope, bool allowFlattened = false) { - return sliceChildShadowNodeViewPairsV2( + return sliceChildShadowNodeViewPairs( *shadowViewNodePair.shadowNode, scope, allowFlattened, @@ -346,7 +346,7 @@ static_assert( std::is_move_assignable::value, "`ShadowViewNodePair::NonOwningList` must be `move assignable`."); -static void calculateShadowViewMutationsV2( +static void calculateShadowViewMutations( ViewNodePairScope& scope, ShadowViewMutation::List& mutations, const ShadowView& parentShadowView, @@ -499,7 +499,7 @@ static void updateMatchedPairSubtrees( auto newGrandChildPairs = sliceChildShadowNodeViewPairsFromViewNodePair(newPair, innerScope); const size_t newGrandChildPairsSize = newGrandChildPairs.size(); - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, *(newGrandChildPairsSize != 0u ? &mutationContainer.downwardMutations @@ -834,7 +834,7 @@ static void calculateShadowViewMutationsFlattener( if (!oldTreeNodePair.flattened && !newTreeNodePair.flattened) { if (oldTreeNodePair.shadowNode != newTreeNodePair.shadowNode) { ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.downwardMutations, newTreeNodePair.shadowView, @@ -1010,7 +1010,7 @@ static void calculateShadowViewMutationsFlattener( if (!treeChildPair.flattened) { ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.destructiveDownwardMutations, treeChildPair.shadowView, @@ -1024,7 +1024,7 @@ static void calculateShadowViewMutationsFlattener( if (!treeChildPair.flattened) { ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.downwardMutations, treeChildPair.shadowView, @@ -1036,7 +1036,7 @@ static void calculateShadowViewMutationsFlattener( } } -static void calculateShadowViewMutationsV2( +static void calculateShadowViewMutations( ViewNodePairScope& scope, ShadowViewMutation::List& mutations, const ShadowView& parentShadowView, @@ -1131,7 +1131,7 @@ static void calculateShadowViewMutationsV2( auto newGrandChildPairs = sliceChildShadowNodeViewPairsFromViewNodePair( newChildPair, innerScope); const size_t newGrandChildPairsSize = newGrandChildPairs.size(); - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, *(newGrandChildPairsSize != 0u ? &mutationContainer.downwardMutations @@ -1197,7 +1197,7 @@ static void calculateShadowViewMutationsV2( // We also have to call the algorithm recursively to clean up the entire // subtree starting from the removed view. ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.destructiveDownwardMutations, oldChildPair.shadowView, @@ -1231,7 +1231,7 @@ static void calculateShadowViewMutationsV2( ShadowViewMutation::CreateMutation(newChildPair.shadowView)); ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.downwardMutations, newChildPair.shadowView, @@ -1468,7 +1468,7 @@ static void calculateShadowViewMutationsV2( // We also have to call the algorithm recursively to clean up the // entire subtree starting from the removed view. ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.destructiveDownwardMutations, oldChildPair.shadowView, @@ -1512,7 +1512,7 @@ static void calculateShadowViewMutationsV2( ShadowViewMutation::CreateMutation(newChildPair.shadowView)); ViewNodePairScope innerScope{}; - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerScope, mutationContainer.downwardMutations, newChildPair.shadowView, @@ -1553,65 +1553,6 @@ static void calculateShadowViewMutationsV2( std::back_inserter(mutations)); } -/** - * Only used by unit tests currently. - */ -static void sliceChildShadowNodeViewPairsRecursivelyForTesting( - ShadowViewNodePair::OwningList& pairList, - Point layoutOffset, - const ShadowNode& shadowNode) { - for (const auto& sharedChildShadowNode : shadowNode.getChildren()) { - auto& childShadowNode = *sharedChildShadowNode; - -#ifndef ANDROID - // Temporary disabled on Android because the mounting infrastructure - // is not fully ready yet. - if (childShadowNode.getTraits().check(ShadowNodeTraits::Trait::Hidden)) { - continue; - } -#endif - - auto shadowView = ShadowView(childShadowNode); - auto origin = layoutOffset; - if (shadowView.layoutMetrics != EmptyLayoutMetrics) { - origin += shadowView.layoutMetrics.frame.origin; - shadowView.layoutMetrics.frame.origin += layoutOffset; - } - - if (childShadowNode.getTraits().check( - ShadowNodeTraits::Trait::FormsStackingContext)) { - pairList.push_back({shadowView, &childShadowNode}); - } else { - if (childShadowNode.getTraits().check( - ShadowNodeTraits::Trait::FormsView)) { - pairList.push_back({shadowView, &childShadowNode}); - } - - sliceChildShadowNodeViewPairsRecursivelyForTesting( - pairList, origin, childShadowNode); - } - } -} - -/** - * Only used by unit tests currently. - */ -ShadowViewNodePair::OwningList sliceChildShadowNodeViewPairsForTesting( - const ShadowNode& shadowNode) { - auto pairList = ShadowViewNodePair::OwningList{}; - - if (!shadowNode.getTraits().check( - ShadowNodeTraits::Trait::FormsStackingContext) && - shadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView)) { - return pairList; - } - - sliceChildShadowNodeViewPairsRecursivelyForTesting( - pairList, {0, 0}, shadowNode); - - return pairList; -} - ShadowViewMutation::List calculateShadowViewMutations( const ShadowNode& oldRootShadowNode, const ShadowNode& newRootShadowNode) { @@ -1636,12 +1577,12 @@ ShadowViewMutation::List calculateShadowViewMutations( oldRootShadowView, newRootShadowView, {})); } - calculateShadowViewMutationsV2( + calculateShadowViewMutations( innerViewNodePairScope, mutations, ShadowView(oldRootShadowNode), - sliceChildShadowNodeViewPairsV2(oldRootShadowNode, viewNodePairScope), - sliceChildShadowNodeViewPairsV2(newRootShadowNode, viewNodePairScope)); + sliceChildShadowNodeViewPairs(oldRootShadowNode, viewNodePairScope), + sliceChildShadowNodeViewPairs(newRootShadowNode, viewNodePairScope)); return mutations; } diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.h b/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.h index 6215bad061c..bb5b195ffb0 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.h +++ b/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.h @@ -53,17 +53,10 @@ ShadowViewMutation::List calculateShadowViewMutations( * flattened view hierarchy. The V2 version preserves nodes even if they do * not form views and their children are flattened. */ -ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairsV2( +ShadowViewNodePair::NonOwningList sliceChildShadowNodeViewPairs( const ShadowNode& shadowNode, ViewNodePairScope& viewNodePairScope, bool allowFlattened = false, Point layoutOffset = {0, 0}); -/* - * Generates a list of `ShadowViewNodePair`s that represents a layer of a - * flattened view hierarchy. This is *only* used by unit tests currently. - */ -ShadowViewNodePair::OwningList sliceChildShadowNodeViewPairsForTesting( - const ShadowNode& shadowNode); - } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.cpp index 51b097b0b98..d1db123889b 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.cpp @@ -85,14 +85,4 @@ bool ShadowViewNodePair::operator!=(const ShadowViewNodePair& rhs) const { return !(*this == rhs); } -bool ShadowViewNodePairLegacy::operator==( - const ShadowViewNodePairLegacy& rhs) const { - return this->shadowNode == rhs.shadowNode; -} - -bool ShadowViewNodePairLegacy::operator!=( - const ShadowViewNodePairLegacy& rhs) const { - return !(*this == rhs); -} - } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.h b/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.h index 77dc6e1dcb5..e25a7133673 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.h +++ b/packages/react-native/ReactCommon/react/renderer/mounting/ShadowView.h @@ -64,7 +64,6 @@ std::vector getDebugProps( */ struct ShadowViewNodePair final { using NonOwningList = std::vector; - using OwningList = std::vector; ShadowView shadowView; const ShadowNode* shadowNode; @@ -93,30 +92,6 @@ struct ShadowViewNodePair final { } }; -/* - * Describes pair of a `ShadowView` and a `ShadowNode`. - * This is not exposed to the mounting layer. - * - */ -struct ShadowViewNodePairLegacy final { - using OwningList = std::vector; - - ShadowView shadowView; - const ShadowNode* shadowNode; - bool flattened{false}; - bool isConcreteView{true}; - - size_t mountIndex{0}; - - bool inOtherTree{false}; - - /* - * The stored pointer to `ShadowNode` represents an identity of the pair. - */ - bool operator==(const ShadowViewNodePairLegacy& rhs) const; - bool operator!=(const ShadowViewNodePairLegacy& rhs) const; -}; - } // namespace facebook::react namespace std { diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/StubView.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubView.cpp similarity index 100% rename from packages/react-native/ReactCommon/react/renderer/mounting/StubView.cpp rename to packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubView.cpp diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/StubView.h b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubView.h similarity index 100% rename from packages/react-native/ReactCommon/react/renderer/mounting/StubView.h rename to packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubView.h diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/StubViewTree.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubViewTree.cpp similarity index 100% rename from packages/react-native/ReactCommon/react/renderer/mounting/StubViewTree.cpp rename to packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubViewTree.cpp diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/StubViewTree.h b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubViewTree.h similarity index 100% rename from packages/react-native/ReactCommon/react/renderer/mounting/StubViewTree.h rename to packages/react-native/ReactCommon/react/renderer/mounting/stubs/StubViewTree.h diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/stubs.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/stubs.cpp similarity index 75% rename from packages/react-native/ReactCommon/react/renderer/mounting/stubs.cpp rename to packages/react-native/ReactCommon/react/renderer/mounting/stubs/stubs.cpp index fedd71c8d7d..bd9b7ce868a 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/stubs.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/stubs.cpp @@ -17,16 +17,16 @@ namespace facebook::react { * Sorting comparator for `reorderInPlaceIfNeeded`. */ static bool shouldFirstPairComesBeforeSecondOne( - const ShadowViewNodePair& lhs, - const ShadowViewNodePair& rhs) noexcept { - return lhs.shadowNode->getOrderIndex() < rhs.shadowNode->getOrderIndex(); + const ShadowViewNodePair* lhs, + const ShadowViewNodePair* rhs) noexcept { + return lhs->shadowNode->getOrderIndex() < rhs->shadowNode->getOrderIndex(); } /* * Reorders pairs in-place based on `orderIndex` using a stable sort algorithm. */ static void reorderInPlaceIfNeeded( - ShadowViewNodePair::OwningList& pairs) noexcept { + std::vector& pairs) noexcept { // This is a simplified version of the function intentionally copied from // `Differentiator.cpp`. std::stable_sort( @@ -41,24 +41,29 @@ static void reorderInPlaceIfNeeded( */ static void calculateShadowViewMutationsForNewTree( ShadowViewMutation::List& mutations, + ViewNodePairScope& scope, const ShadowView& parentShadowView, - ShadowViewNodePair::OwningList newChildPairs) { + std::vector newChildPairs) { // Sorting pairs based on `orderIndex` if needed. reorderInPlaceIfNeeded(newChildPairs); - for (size_t index = 0; index < newChildPairs.size(); index++) { - const auto& newChildPair = newChildPairs[index]; + for (auto newChildPair : newChildPairs) { + if (!newChildPair->isConcreteView) { + continue; + } mutations.push_back( - ShadowViewMutation::CreateMutation(newChildPair.shadowView)); + ShadowViewMutation::CreateMutation(newChildPair->shadowView)); mutations.push_back(ShadowViewMutation::InsertMutation( - parentShadowView, newChildPair.shadowView, static_cast(index))); + parentShadowView, + newChildPair->shadowView, + static_cast(newChildPair->mountIndex))); auto newGrandChildPairs = - sliceChildShadowNodeViewPairsForTesting(*newChildPair.shadowNode); + sliceChildShadowNodeViewPairs(*newChildPair->shadowNode, scope); calculateShadowViewMutationsForNewTree( - mutations, newChildPair.shadowView, newGrandChildPairs); + mutations, scope, newChildPair->shadowView, newGrandChildPairs); } } @@ -67,10 +72,12 @@ StubViewTree buildStubViewTreeWithoutUsingDifferentiator( auto mutations = ShadowViewMutation::List{}; mutations.reserve(256); + ViewNodePairScope scope; calculateShadowViewMutationsForNewTree( mutations, + scope, ShadowView(rootShadowNode), - sliceChildShadowNodeViewPairsForTesting(rootShadowNode)); + sliceChildShadowNodeViewPairs(rootShadowNode, scope)); auto emptyRootShadowNode = rootShadowNode.clone(ShadowNodeFragment{ ShadowNodeFragment::propsPlaceholder(), diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/stubs.h b/packages/react-native/ReactCommon/react/renderer/mounting/stubs/stubs.h similarity index 100% rename from packages/react-native/ReactCommon/react/renderer/mounting/stubs.h rename to packages/react-native/ReactCommon/react/renderer/mounting/stubs/stubs.h