From 2e3f55aced1fc4010ed2b4b4cd082f1cc93658c8 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Wed, 8 Feb 2023 15:48:17 -0800 Subject: [PATCH] Pass MountingCoordinator by value instead of reference Summary: changelog: [internal] Passing MountingCoordinator argument by value instead of reference. Using reference does not make sense since we eventually take ownership of shared_ptr anyway. This better communicates the intent. Reviewed By: christophpurrer Differential Revision: D43082955 fbshipit-source-id: 29e20abb9824c10a5f0d5e0ba1049ff6d67cee98 --- React/Fabric/Mounting/RCTMountingManager.h | 2 +- React/Fabric/Mounting/RCTMountingManager.mm | 9 ++++----- React/Fabric/RCTScheduler.mm | 4 ++-- ReactAndroid/src/main/jni/react/fabric/Binding.cpp | 4 ++-- ReactAndroid/src/main/jni/react/fabric/Binding.h | 2 +- .../src/main/jni/react/fabric/FabricMountingManager.cpp | 2 +- .../src/main/jni/react/fabric/FabricMountingManager.h | 2 +- ReactCommon/react/renderer/mounting/ShadowTree.cpp | 4 ++-- ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h | 3 +-- .../renderer/mounting/tests/StateReconciliationTest.cpp | 3 +-- ReactCommon/react/renderer/scheduler/Scheduler.cpp | 4 ++-- ReactCommon/react/renderer/scheduler/Scheduler.h | 2 +- ReactCommon/react/renderer/scheduler/SchedulerDelegate.h | 2 +- ReactCommon/react/renderer/uimanager/UIManager.cpp | 5 ++--- ReactCommon/react/renderer/uimanager/UIManager.h | 3 +-- ReactCommon/react/renderer/uimanager/UIManagerDelegate.h | 2 +- 16 files changed, 24 insertions(+), 29 deletions(-) diff --git a/React/Fabric/Mounting/RCTMountingManager.h b/React/Fabric/Mounting/RCTMountingManager.h index 31db0d49291..f33c5bcd44e 100644 --- a/React/Fabric/Mounting/RCTMountingManager.h +++ b/React/Fabric/Mounting/RCTMountingManager.h @@ -48,7 +48,7 @@ NS_ASSUME_NONNULL_BEGIN * Schedule a mounting transaction to be performed on the main thread. * Can be called from any thread. */ -- (void)scheduleTransaction:(facebook::react::MountingCoordinator::Shared const &)mountingCoordinator; +- (void)scheduleTransaction:(facebook::react::MountingCoordinator::Shared)mountingCoordinator; /** * Dispatch a command to be performed on the main thread. diff --git a/React/Fabric/Mounting/RCTMountingManager.mm b/React/Fabric/Mounting/RCTMountingManager.mm index 524492c0210..74704af2d57 100644 --- a/React/Fabric/Mounting/RCTMountingManager.mm +++ b/React/Fabric/Mounting/RCTMountingManager.mm @@ -196,21 +196,20 @@ static void RCTPerformMountInstructions( componentViewDescriptor:rootViewDescriptor]; } -- (void)scheduleTransaction:(MountingCoordinator::Shared const &)mountingCoordinator +- (void)scheduleTransaction:(MountingCoordinator::Shared)mountingCoordinator { if (RCTIsMainQueue()) { // Already on the proper thread, so: // * No need to do a thread jump; // * No need to do expensive copy of all mutations; // * No need to allocate a block. - [self initiateTransaction:mountingCoordinator]; + [self initiateTransaction:std::move(mountingCoordinator)]; return; } - auto mountingCoordinatorCopy = mountingCoordinator; RCTExecuteOnMainQueue(^{ RCTAssertMainQueue(); - [self initiateTransaction:mountingCoordinatorCopy]; + [self initiateTransaction:std::move(mountingCoordinator)]; }); } @@ -244,7 +243,7 @@ static void RCTPerformMountInstructions( }); } -- (void)initiateTransaction:(MountingCoordinator::Shared const &)mountingCoordinator +- (void)initiateTransaction:(MountingCoordinator::Shared)mountingCoordinator { SystraceSection s("-[RCTMountingManager initiateTransaction:]"); RCTAssertMainQueue(); diff --git a/React/Fabric/RCTScheduler.mm b/React/Fabric/RCTScheduler.mm index 8fc513f1f92..231adc5eb22 100644 --- a/React/Fabric/RCTScheduler.mm +++ b/React/Fabric/RCTScheduler.mm @@ -24,10 +24,10 @@ class SchedulerDelegateProxy : public SchedulerDelegate { public: SchedulerDelegateProxy(void *scheduler) : scheduler_(scheduler) {} - void schedulerDidFinishTransaction(MountingCoordinator::Shared const &mountingCoordinator) override + void schedulerDidFinishTransaction(MountingCoordinator::Shared mountingCoordinator) override { RCTScheduler *scheduler = (__bridge RCTScheduler *)scheduler_; - [scheduler.delegate schedulerDidFinishTransaction:mountingCoordinator]; + [scheduler.delegate schedulerDidFinishTransaction:std::move(mountingCoordinator)]; } void schedulerDidRequestPreliminaryViewAllocation(SurfaceId surfaceId, const ShadowNode &shadowNode) override diff --git a/ReactAndroid/src/main/jni/react/fabric/Binding.cpp b/ReactAndroid/src/main/jni/react/fabric/Binding.cpp index 487badafc1e..f5a0aa68229 100644 --- a/ReactAndroid/src/main/jni/react/fabric/Binding.cpp +++ b/ReactAndroid/src/main/jni/react/fabric/Binding.cpp @@ -484,14 +484,14 @@ std::shared_ptr Binding::verifyMountingManager( } void Binding::schedulerDidFinishTransaction( - MountingCoordinator::Shared const &mountingCoordinator) { + MountingCoordinator::Shared mountingCoordinator) { auto mountingManager = verifyMountingManager("Binding::schedulerDidFinishTransaction"); if (!mountingManager) { return; } - mountingManager->executeMount(mountingCoordinator); + mountingManager->executeMount(std::move(mountingCoordinator)); } void Binding::schedulerDidRequestPreliminaryViewAllocation( diff --git a/ReactAndroid/src/main/jni/react/fabric/Binding.h b/ReactAndroid/src/main/jni/react/fabric/Binding.h index 30c1819457f..355e616c20f 100644 --- a/ReactAndroid/src/main/jni/react/fabric/Binding.h +++ b/ReactAndroid/src/main/jni/react/fabric/Binding.h @@ -94,7 +94,7 @@ class Binding : public jni::HybridClass, void unregisterSurface(SurfaceHandlerBinding *surfaceHandler); void schedulerDidFinishTransaction( - MountingCoordinator::Shared const &mountingCoordinator) override; + MountingCoordinator::Shared mountingCoordinator) override; void schedulerDidRequestPreliminaryViewAllocation( const SurfaceId surfaceId, diff --git a/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp b/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp index 345523cf54b..54845eb97a8 100644 --- a/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp +++ b/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp @@ -263,7 +263,7 @@ local_ref FabricMountingManager::getProps( } void FabricMountingManager::executeMount( - MountingCoordinator::Shared const &mountingCoordinator) { + MountingCoordinator::Shared mountingCoordinator) { std::lock_guard lock(commitMutex_); SystraceSection s( diff --git a/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h b/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h index c098e661b66..cc970489a39 100644 --- a/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h +++ b/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h @@ -41,7 +41,7 @@ class FabricMountingManager final { void preallocateShadowView(SurfaceId surfaceId, ShadowView const &shadowView); - void executeMount(MountingCoordinator::Shared const &mountingCoordinator); + void executeMount(MountingCoordinator::Shared mountingCoordinator); void dispatchCommand( ShadowView const &shadowView, diff --git a/ReactCommon/react/renderer/mounting/ShadowTree.cpp b/ReactCommon/react/renderer/mounting/ShadowTree.cpp index c7e28fafbf2..c8624f44465 100644 --- a/ReactCommon/react/renderer/mounting/ShadowTree.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowTree.cpp @@ -415,7 +415,7 @@ ShadowTreeRevision ShadowTree::getCurrentRevision() const { void ShadowTree::mount(ShadowTreeRevision const &revision) const { mountingCoordinator_->push(revision); - delegate_.shadowTreeDidFinishTransaction(*this, mountingCoordinator_); + delegate_.shadowTreeDidFinishTransaction(mountingCoordinator_); } void ShadowTree::commitEmptyTree() const { @@ -458,7 +458,7 @@ void ShadowTree::emitLayoutEvents( } void ShadowTree::notifyDelegatesOfUpdates() const { - delegate_.shadowTreeDidFinishTransaction(*this, mountingCoordinator_); + delegate_.shadowTreeDidFinishTransaction(mountingCoordinator_); } } // namespace facebook::react diff --git a/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h b/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h index 48167330680..e20f42e1572 100644 --- a/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h +++ b/ReactCommon/react/renderer/mounting/ShadowTreeDelegate.h @@ -34,8 +34,7 @@ class ShadowTreeDelegate { * Called right after Shadow Tree commit a new state of the tree. */ virtual void shadowTreeDidFinishTransaction( - ShadowTree const &shadowTree, - MountingCoordinator::Shared const &mountingCoordinator) const = 0; + MountingCoordinator::Shared mountingCoordinator) const = 0; virtual ~ShadowTreeDelegate() noexcept = default; }; diff --git a/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp b/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp index 67069255868..f5e56c4cd15 100644 --- a/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp +++ b/ReactCommon/react/renderer/mounting/tests/StateReconciliationTest.cpp @@ -32,8 +32,7 @@ class DummyShadowTreeDelegate : public ShadowTreeDelegate { }; void shadowTreeDidFinishTransaction( - ShadowTree const &shadowTree, - MountingCoordinator::Shared const &mountingCoordinator) const override{}; + MountingCoordinator::Shared mountingCoordinator) const override{}; }; inline ShadowNode const *findDescendantNode( diff --git a/ReactCommon/react/renderer/scheduler/Scheduler.cpp b/ReactCommon/react/renderer/scheduler/Scheduler.cpp index 47368f318b8..5e316102f5a 100644 --- a/ReactCommon/react/renderer/scheduler/Scheduler.cpp +++ b/ReactCommon/react/renderer/scheduler/Scheduler.cpp @@ -299,11 +299,11 @@ void Scheduler::animationTick() const { #pragma mark - UIManagerDelegate void Scheduler::uiManagerDidFinishTransaction( - MountingCoordinator::Shared const &mountingCoordinator) { + MountingCoordinator::Shared mountingCoordinator) { SystraceSection s("Scheduler::uiManagerDidFinishTransaction"); if (delegate_ != nullptr) { - delegate_->schedulerDidFinishTransaction(mountingCoordinator); + delegate_->schedulerDidFinishTransaction(std::move(mountingCoordinator)); } } void Scheduler::uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) { diff --git a/ReactCommon/react/renderer/scheduler/Scheduler.h b/ReactCommon/react/renderer/scheduler/Scheduler.h index e47005734e3..aad360ece38 100644 --- a/ReactCommon/react/renderer/scheduler/Scheduler.h +++ b/ReactCommon/react/renderer/scheduler/Scheduler.h @@ -88,7 +88,7 @@ class Scheduler final : public UIManagerDelegate { #pragma mark - UIManagerDelegate void uiManagerDidFinishTransaction( - MountingCoordinator::Shared const &mountingCoordinator) override; + MountingCoordinator::Shared mountingCoordinator) override; void uiManagerDidCreateShadowNode(const ShadowNode &shadowNode) override; void uiManagerDidDispatchCommand( const ShadowNode::Shared &shadowNode, diff --git a/ReactCommon/react/renderer/scheduler/SchedulerDelegate.h b/ReactCommon/react/renderer/scheduler/SchedulerDelegate.h index 961d5fc1289..c0fca4a012f 100644 --- a/ReactCommon/react/renderer/scheduler/SchedulerDelegate.h +++ b/ReactCommon/react/renderer/scheduler/SchedulerDelegate.h @@ -26,7 +26,7 @@ class SchedulerDelegate { * to construct a new one. */ virtual void schedulerDidFinishTransaction( - MountingCoordinator::Shared const &mountingCoordinator) = 0; + MountingCoordinator::Shared mountingCoordinator) = 0; /* * Called right after a new ShadowNode was created. diff --git a/ReactCommon/react/renderer/uimanager/UIManager.cpp b/ReactCommon/react/renderer/uimanager/UIManager.cpp index a065c2e99e1..252cd543d8c 100644 --- a/ReactCommon/react/renderer/uimanager/UIManager.cpp +++ b/ReactCommon/react/renderer/uimanager/UIManager.cpp @@ -532,12 +532,11 @@ RootShadowNode::Unshared UIManager::shadowTreeWillCommit( } void UIManager::shadowTreeDidFinishTransaction( - ShadowTree const & /*shadowTree*/, - MountingCoordinator::Shared const &mountingCoordinator) const { + MountingCoordinator::Shared mountingCoordinator) const { SystraceSection s("UIManager::shadowTreeDidFinishTransaction"); if (delegate_ != nullptr) { - delegate_->uiManagerDidFinishTransaction(mountingCoordinator); + delegate_->uiManagerDidFinishTransaction(std::move(mountingCoordinator)); } } diff --git a/ReactCommon/react/renderer/uimanager/UIManager.h b/ReactCommon/react/renderer/uimanager/UIManager.h index 7e2d47776e0..9648cb9fb26 100644 --- a/ReactCommon/react/renderer/uimanager/UIManager.h +++ b/ReactCommon/react/renderer/uimanager/UIManager.h @@ -103,8 +103,7 @@ class UIManager final : public ShadowTreeDelegate { #pragma mark - ShadowTreeDelegate void shadowTreeDidFinishTransaction( - ShadowTree const &shadowTree, - MountingCoordinator::Shared const &mountingCoordinator) const override; + MountingCoordinator::Shared mountingCoordinator) const override; RootShadowNode::Unshared shadowTreeWillCommit( ShadowTree const &shadowTree, diff --git a/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h b/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h index 77f1e4e3b62..7891f55c011 100644 --- a/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h +++ b/ReactCommon/react/renderer/uimanager/UIManagerDelegate.h @@ -23,7 +23,7 @@ class UIManagerDelegate { * For this moment the tree is already laid out and sealed. */ virtual void uiManagerDidFinishTransaction( - MountingCoordinator::Shared const &mountingCoordinator) = 0; + MountingCoordinator::Shared mountingCoordinator) = 0; /* * Called each time when UIManager constructs a new Shadow Node. Receiver