From 0f8b1d2ba3574cc48ae07cdbf3aafc05daab650f Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 18 May 2023 04:45:30 -0700 Subject: [PATCH] Resolve ReactCommon unit test issues (#37463) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/37463 Changelog: [Internal] Reviewed By: cortinico Differential Revision: D45946544 fbshipit-source-id: ecae31b0d67f1fbf92a47aebbfd111e961f2a971 --- .../tests/hermes/ReactInstanceTest.cpp | 2 +- .../renderer/mounting/Differentiator.cpp | 2 +- .../mounting/tests/StackingContextTest.cpp | 20 +++++++++++++++++-- .../react/renderer/timeline/Timeline.cpp | 3 ++- .../renderer/timeline/TimelineController.cpp | 9 ++++----- .../renderer/timeline/TimelineController.h | 3 ++- 6 files changed, 28 insertions(+), 11 deletions(-) diff --git a/packages/react-native/ReactCommon/react/bridgeless/tests/hermes/ReactInstanceTest.cpp b/packages/react-native/ReactCommon/react/bridgeless/tests/hermes/ReactInstanceTest.cpp index 63a5ccc79fc..ca3f34f14d3 100644 --- a/packages/react-native/ReactCommon/react/bridgeless/tests/hermes/ReactInstanceTest.cpp +++ b/packages/react-native/ReactCommon/react/bridgeless/tests/hermes/ReactInstanceTest.cpp @@ -12,9 +12,9 @@ #include #include -#include #include #include +#include #include using ::testing::_; diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp index 5ac18021200..79ecdc34220 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -219,7 +219,7 @@ static void sliceChildShadowNodeViewPairsRecursivelyV2( auto &childShadowNode = *sharedChildShadowNode; #ifndef ANDROID - // Temporary disabled on Android because the mounting infrastructure + // T153547836: Disabled on Android because the mounting infrastructure // is not fully ready yet. if (childShadowNode.getTraits().check(ShadowNodeTraits::Trait::Hidden)) { continue; diff --git a/packages/react-native/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp b/packages/react-native/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp index d3a3f8d129b..7ab89855bfb 100644 --- a/packages/react-native/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/mounting/tests/StackingContextTest.cpp @@ -768,10 +768,25 @@ TEST_F(StackingContextTest, zIndexAndFlattenedNodes) { }); testViewTree_([](StubViewTree const &viewTree) { - // 5 views in total. +#ifdef ANDROID + // T153547836: Android still mounts views with + // ShadowNodeTraits::Trait::Hidden + + EXPECT_EQ(viewTree.size(), 8); + + // nodeBB_ forms a stacking context + EXPECT_EQ(viewTree.getRootStubView().children.size(), 5); + + // The root view subviews are [6, 10, 9, 5]. + EXPECT_EQ(viewTree.getRootStubView().children.at(0)->tag, 6); + EXPECT_EQ(viewTree.getRootStubView().children.at(1)->tag, 10); + EXPECT_EQ(viewTree.getRootStubView().children.at(2)->tag, 9); + EXPECT_EQ(viewTree.getRootStubView().children.at(3)->tag, 5); + EXPECT_EQ(viewTree.getRootStubView().children.at(4)->tag, 3); +#else EXPECT_EQ(viewTree.size(), 5); - // The root view has all 5 subviews. + // The root view has all 4 subviews. EXPECT_EQ(viewTree.getRootStubView().children.size(), 4); // The root view subviews are [6, 10, 9, 5]. @@ -779,6 +794,7 @@ TEST_F(StackingContextTest, zIndexAndFlattenedNodes) { EXPECT_EQ(viewTree.getRootStubView().children.at(1)->tag, 9); EXPECT_EQ(viewTree.getRootStubView().children.at(2)->tag, 5); EXPECT_EQ(viewTree.getRootStubView().children.at(3)->tag, 3); +#endif }); } diff --git a/packages/react-native/ReactCommon/react/renderer/timeline/Timeline.cpp b/packages/react-native/ReactCommon/react/renderer/timeline/Timeline.cpp index 5fb3bab3948..75086235816 100644 --- a/packages/react-native/ReactCommon/react/renderer/timeline/Timeline.cpp +++ b/packages/react-native/ReactCommon/react/renderer/timeline/Timeline.cpp @@ -110,7 +110,8 @@ void Timeline::rewind(TimelineSnapshot const &snapshot) const noexcept { -> RootShadowNode::Unshared { return std::static_pointer_cast( rootShadowNode->ShadowNode::clone({})); - }); + }, + {}); assert(rewinding_ && ""); rewinding_ = false; diff --git a/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.cpp b/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.cpp index 82f31db63db..68749cc6c2e 100644 --- a/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.cpp +++ b/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.cpp @@ -15,8 +15,7 @@ namespace facebook::react { TimelineHandler TimelineController::enable(SurfaceId surfaceId) const { assert(uiManager_); - auto shadowTreePtr = (ShadowTree const *){}; - + ShadowTree const *shadowTreePtr = nullptr; uiManager_->getShadowTreeRegistry().visit( surfaceId, [&](ShadowTree const &shadowTree) { shadowTreePtr = &shadowTree; }); @@ -24,7 +23,7 @@ TimelineHandler TimelineController::enable(SurfaceId surfaceId) const { assert(shadowTreePtr); { - std::unique_lock lock(timelinesMutex_); + std::unique_lock lock(timelinesMutex_); auto timeline = std::make_unique(*shadowTreePtr); auto handler = TimelineHandler{*timeline}; @@ -34,7 +33,7 @@ TimelineHandler TimelineController::enable(SurfaceId surfaceId) const { } void TimelineController::disable(TimelineHandler &&handler) const { - std::unique_lock lock(timelinesMutex_); + std::unique_lock lock(timelinesMutex_); auto iterator = timelines_.find(handler.getSurfaceId()); assert(iterator != timelines_.end()); @@ -56,7 +55,7 @@ RootShadowNode::Unshared TimelineController::shadowTreeWillCommit( ShadowTree const &shadowTree, RootShadowNode::Shared const &oldRootShadowNode, RootShadowNode::Unshared const &newRootShadowNode) const noexcept { - std::shared_lock lock(timelinesMutex_); + std::shared_lock lock(timelinesMutex_); assert(uiManager_ && "`uiManager_` must not be `nullptr`."); diff --git a/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.h b/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.h index f328677e05d..fe09e203cc9 100644 --- a/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.h +++ b/packages/react-native/ReactCommon/react/renderer/timeline/TimelineController.h @@ -8,6 +8,7 @@ #pragma once #include +#include #include @@ -64,7 +65,7 @@ class TimelineController final : public UIManagerCommitHook { /* * Protects all the data members. */ - mutable butter::shared_mutex timelinesMutex_; + mutable std::shared_mutex timelinesMutex_; /* * Owning collection of all running `Timeline` instances.