From e8770d7bb243647c93b1cc2c63b9783ce3b34ddb Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Mon, 7 Dec 2020 13:34:48 -0800 Subject: [PATCH] Code quality: Refactor ShadowViewMutation::UpdateMutation to remove `index`, `parentShadowView` parameters Summary: The `index` parameter for UpdateMutation is optional, and is normally just -1. It's not useful, so remove it. `parentShadowView` is also not relevant and is not used; in some existing use-cases the actual parent view of the updated view is available, and in some contexts the parent view is not set. The function now will always set the index to -1 for UpdateMutations, and `{}` for ParentShadowView. This should have no impact on iOS or Android, as this parameter is not used. It could theoretically have an impact on lifetimes of objects retained (now not retained) by not passing parentShadowView into the mutation. For example, any shared props or state associated with the parent will not be retained in the Update mutation now. Changelog: [Internal] Reviewed By: shergin Differential Revision: D25342943 fbshipit-source-id: 0ddbef76a6e2eefc2629c9729f721d8674d7737e --- .../animations/LayoutAnimationDriver.cpp | 2 +- .../LayoutAnimationKeyFrameManager.cpp | 14 ++++-------- .../renderer/mounting/Differentiator.cpp | 22 +++++-------------- .../renderer/mounting/ShadowViewMutation.cpp | 8 +++---- .../renderer/mounting/ShadowViewMutation.h | 6 ++--- 5 files changed, 15 insertions(+), 37 deletions(-) diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp index 5386b237a0e..b981d1aa5c9 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp @@ -74,7 +74,7 @@ void LayoutAnimationDriver::animationMutationsForFrame( // Create the mutation instruction auto updateMutation = ShadowViewMutation::UpdateMutation( - keyframe.parentView, keyframe.viewPrev, mutatedShadowView, -1); + keyframe.viewPrev, mutatedShadowView); // All generated Update mutations must have an "old" and "new" // ShadowView. Checking for nonzero tag doesn't guarantee that the views diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp index 24a8fb9296c..48d652328d8 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp @@ -1252,10 +1252,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( 1, keyFrame.viewStart, keyFrame.viewEnd); auto generatedPenultimateMutation = ShadowViewMutation::UpdateMutation( - keyFrame.parentView, - keyFrame.viewPrev, - mutatedShadowView, - -1); + keyFrame.viewPrev, mutatedShadowView); assert(generatedPenultimateMutation.oldChildShadowView.tag != 0); assert(generatedPenultimateMutation.newChildShadowView.tag != 0); PrintMutationInstruction( @@ -1264,7 +1261,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( finalConflictingMutations.push_back(generatedPenultimateMutation); auto generatedMutation = ShadowViewMutation::UpdateMutation( - keyFrame.parentView, mutatedShadowView, keyFrame.viewEnd, -1); + mutatedShadowView, keyFrame.viewEnd); assert(generatedMutation.oldChildShadowView.tag != 0); assert(generatedMutation.newChildShadowView.tag != 0); PrintMutationInstruction( @@ -1437,10 +1434,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( 1, keyFrame.viewStart, keyFrame.viewEnd); auto generatedPenultimateMutation = ShadowViewMutation::UpdateMutation( - keyFrame.parentView, - keyFrame.viewPrev, - mutatedShadowView, - -1); + keyFrame.viewPrev, mutatedShadowView); assert(generatedPenultimateMutation.oldChildShadowView.tag != 0); assert(generatedPenultimateMutation.newChildShadowView.tag != 0); PrintMutationInstruction( @@ -1450,7 +1444,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( generatedPenultimateMutation); auto generatedMutation = ShadowViewMutation::UpdateMutation( - keyFrame.parentView, mutatedShadowView, keyFrame.viewEnd, -1); + mutatedShadowView, keyFrame.viewEnd); assert(generatedMutation.oldChildShadowView.tag != 0); assert(generatedMutation.newChildShadowView.tag != 0); PrintMutationInstruction( diff --git a/ReactCommon/react/renderer/mounting/Differentiator.cpp b/ReactCommon/react/renderer/mounting/Differentiator.cpp index eb2fa09a3e8..f4e798b31eb 100644 --- a/ReactCommon/react/renderer/mounting/Differentiator.cpp +++ b/ReactCommon/react/renderer/mounting/Differentiator.cpp @@ -486,10 +486,7 @@ static void calculateShadowViewMutationsFlattener( newTreeNodePair.isConcreteView && oldTreeNodePair.isConcreteView) { mutationInstructionContainer.updateMutations.push_back( ShadowViewMutation::UpdateMutation( - parentShadowView, - oldTreeNodePair.shadowView, - newTreeNodePair.shadowView, - newTreeNodePair.mountIndex)); + oldTreeNodePair.shadowView, newTreeNodePair.shadowView)); } // Update children if appropriate. @@ -850,10 +847,7 @@ static void calculateShadowViewMutationsV2( if (newChildPair.isConcreteView && oldChildPair.shadowView != newChildPair.shadowView) { updateMutations.push_back(ShadowViewMutation::UpdateMutation( - parentShadowView, - oldChildPair.shadowView, - newChildPair.shadowView, - newChildPair.mountIndex)); + oldChildPair.shadowView, newChildPair.shadowView)); } // Recursively update tree if ShadowNode pointers are not equal @@ -995,10 +989,7 @@ static void calculateShadowViewMutationsV2( // concrete view. The case where they're different is handled above. if (oldChildPair.shadowView != newChildPair.shadowView) { updateMutations.push_back(ShadowViewMutation::UpdateMutation( - parentShadowView, - oldChildPair.shadowView, - newChildPair.shadowView, - newChildPair.mountIndex)); + oldChildPair.shadowView, newChildPair.shadowView)); } // Remove from newRemainingPairs @@ -1251,10 +1242,7 @@ static void calculateShadowViewMutationsV2( if (oldChildPair.shadowView != newChildPair.shadowView) { updateMutations.push_back(ShadowViewMutation::UpdateMutation( - parentShadowView, - oldChildPair.shadowView, - newChildPair.shadowView, - newChildPair.mountIndex)); + oldChildPair.shadowView, newChildPair.shadowView)); } } if (!oldChildPair.flattened && @@ -1516,7 +1504,7 @@ ShadowViewMutation::List calculateShadowViewMutations( if (oldRootShadowView != newRootShadowView) { mutations.push_back(ShadowViewMutation::UpdateMutation( - ShadowView(), oldRootShadowView, newRootShadowView, -1)); + oldRootShadowView, newRootShadowView)); } calculateShadowViewMutationsV2( diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp index 9b8d966a3c1..dbf4afd227c 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.cpp @@ -57,16 +57,14 @@ ShadowViewMutation ShadowViewMutation::RemoveMutation( } ShadowViewMutation ShadowViewMutation::UpdateMutation( - ShadowView parentShadowView, ShadowView oldChildShadowView, - ShadowView newChildShadowView, - int index) { + ShadowView newChildShadowView) { return { /* .type = */ Update, - /* .parentShadowView = */ parentShadowView, + /* .parentShadowView = */ {}, /* .oldChildShadowView = */ oldChildShadowView, /* .newChildShadowView = */ newChildShadowView, - /* .index = */ index, + /* .index = */ -1, }; } diff --git a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h index 07ee6bcaf7e..3bd0067d5fd 100644 --- a/ReactCommon/react/renderer/mounting/ShadowViewMutation.h +++ b/ReactCommon/react/renderer/mounting/ShadowViewMutation.h @@ -55,10 +55,8 @@ struct ShadowViewMutation final { * Creates and returns an `Update` mutation. */ static ShadowViewMutation UpdateMutation( - ShadowView parentShadowView, ShadowView oldChildShadowView, - ShadowView newChildShadowView, - int index); + ShadowView newChildShadowView); #pragma mark - Type @@ -70,7 +68,7 @@ struct ShadowViewMutation final { ShadowView parentShadowView = {}; ShadowView oldChildShadowView = {}; ShadowView newChildShadowView = {}; - int index = {}; + int index = -1; }; using ShadowViewMutationList = std::vector;