From 367a93de883d6fdd896ddeae7f8418e31e43f828 Mon Sep 17 00:00:00 2001 From: David Aurelio Date: Wed, 20 Feb 2019 11:52:38 -0800 Subject: [PATCH] Move node cloning to `YGConfig` Summary: @public Encapsulates node cloning within `YGConfig`. This is necessary for allowing for context-aware cloning functions, which will ultimately allow for removal of weak global JNI references. Reviewed By: shergin Differential Revision: D14132608 fbshipit-source-id: 0dec114c8e172b1e34a4b7fd146c43f13c151ade --- tests/YGConfigTest.cpp | 60 ++++++++++++++++++++++++++++++++++++++++++ yoga/YGConfig.cpp | 13 ++++++++- yoga/YGConfig.h | 7 ++++- yoga/YGNode.cpp | 9 +------ yoga/Yoga.cpp | 13 +++------ 5 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 tests/YGConfigTest.cpp diff --git a/tests/YGConfigTest.cpp b/tests/YGConfigTest.cpp new file mode 100644 index 00000000..c8271448 --- /dev/null +++ b/tests/YGConfigTest.cpp @@ -0,0 +1,60 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the LICENSE + * file in the root directory of this source tree. + */ +// @Generated by gentest/gentest.rb from gentest/fixtures/YGDisplayTest.html + +#include +#include +#include +#include + +#include +#include + +struct ConfigCloningTest : public ::testing::Test { + std::unique_ptr> config; + void SetUp() override; + void TearDown() override; + + static YGNode clonedNode; + static YGNodeRef cloneNode(YGNodeRef, YGNodeRef, int) { + return &clonedNode; + } + static YGNodeRef doNotClone(YGNodeRef, YGNodeRef, int) { + return nullptr; + } +}; + +TEST_F(ConfigCloningTest, uses_values_provided_by_cloning_callback) { + config->setCloneNodeCallback(cloneNode); + + YGNode node{}, owner{}; + auto clone = config->cloneNode(&node, &owner, 0); + + ASSERT_EQ(clone, &clonedNode); +} + +TEST_F( + ConfigCloningTest, + falls_back_to_regular_cloning_if_callback_returns_null) { + config->setCloneNodeCallback(doNotClone); + + YGNode node{}, owner{}; + auto clone = config->cloneNode(&node, &owner, 0); + + ASSERT_NE(clone, nullptr); + YGNodeFree(clone); +} + +void ConfigCloningTest::SetUp() { + config = {YGConfigNew(), YGConfigFree}; +} + +void ConfigCloningTest::TearDown() { + config.reset(); +} + +YGNode ConfigCloningTest::clonedNode = {}; diff --git a/yoga/YGConfig.cpp b/yoga/YGConfig.cpp index ba334834..6527e587 100644 --- a/yoga/YGConfig.cpp +++ b/yoga/YGConfig.cpp @@ -6,7 +6,7 @@ */ #include "YGConfig.h" -YGConfig::YGConfig(YGLogger logger) { +YGConfig::YGConfig(YGLogger logger) : cloneNodeCallback_{nullptr} { logger_.noContext = logger; loggerUsesContext_ = false; } @@ -24,3 +24,14 @@ void YGConfig::log( logger_.noContext(config, node, logLevel, format, args); } } + +YGNodeRef YGConfig::cloneNode(YGNodeRef node, YGNodeRef owner, int childIndex) { + YGNodeRef clone = nullptr; + if (cloneNodeCallback_ != nullptr) { + clone = cloneNodeCallback_(node, owner, childIndex); + } + if (clone == nullptr) { + clone = YGNodeClone(node); + } + return clone; +} diff --git a/yoga/YGConfig.h b/yoga/YGConfig.h index d8260739..11fe678b 100644 --- a/yoga/YGConfig.h +++ b/yoga/YGConfig.h @@ -19,6 +19,7 @@ struct YGConfig { va_list args); private: + YGCloneNodeFunc cloneNodeCallback_ = nullptr; union { LogWithContextFn withContext; YGLogger noContext; @@ -31,7 +32,6 @@ public: bool shouldDiffLayoutWithoutLegacyStretchBehaviour = false; bool printTree = false; float pointScaleFactor = 1.0f; - YGCloneNodeFunc cloneNodeCallback = nullptr; std::array()> experimentalFeatures = {}; void* context = nullptr; @@ -50,4 +50,9 @@ public: void setLogger(std::nullptr_t) { setLogger(YGLogger{nullptr}); } + + YGNodeRef cloneNode(YGNodeRef node, YGNodeRef owner, int childIndex); + void setCloneNodeCallback(YGCloneNodeFunc cloneNode) { + cloneNodeCallback_ = cloneNode; + } }; diff --git a/yoga/YGNode.cpp b/yoga/YGNode.cpp index dcb1b8a2..4fcef90e 100644 --- a/yoga/YGNode.cpp +++ b/yoga/YGNode.cpp @@ -404,16 +404,9 @@ void YGNode::cloneChildrenIfNeeded() { return; } - const YGCloneNodeFunc cloneNodeCallback = config_->cloneNodeCallback; for (uint32_t i = 0; i < childCount; ++i) { const YGNodeRef oldChild = children_[i]; - YGNodeRef newChild = nullptr; - if (cloneNodeCallback) { - newChild = cloneNodeCallback(oldChild, this, i); - } - if (newChild == nullptr) { - newChild = YGNodeClone(oldChild); - } + YGNodeRef newChild = config_->cloneNode(oldChild, this, i); replaceChild(newChild, i); newChild->setOwner(this); } diff --git a/yoga/Yoga.cpp b/yoga/Yoga.cpp index 93ba9851..290e50a8 100644 --- a/yoga/Yoga.cpp +++ b/yoga/Yoga.cpp @@ -425,8 +425,6 @@ void YGNodeRemoveChild(const YGNodeRef owner, const YGNodeRef excludedChild) { // Otherwise we have to clone the node list except for the child we're trying // to delete. We don't want to simply clone all children, because then the // host will need to free the clone of the child that was just deleted. - const YGCloneNodeFunc cloneNodeCallback = - owner->getConfig()->cloneNodeCallback; uint32_t nextInsertIndex = 0; for (uint32_t i = 0; i < childCount; i++) { const YGNodeRef oldChild = owner->getChild(i); @@ -437,13 +435,8 @@ void YGNodeRemoveChild(const YGNodeRef owner, const YGNodeRef excludedChild) { owner->markDirtyAndPropogate(); continue; } - YGNodeRef newChild = nullptr; - if (cloneNodeCallback) { - newChild = cloneNodeCallback(oldChild, owner, nextInsertIndex); - } - if (newChild == nullptr) { - newChild = YGNodeClone(oldChild); - } + YGNodeRef newChild = + owner->getConfig()->cloneNode(oldChild, owner, nextInsertIndex); owner->replaceChild(newChild, nextInsertIndex); newChild->setOwner(owner); @@ -4293,7 +4286,7 @@ void* YGConfigGetContext(const YGConfigRef config) { void YGConfigSetCloneNodeFunc( const YGConfigRef config, const YGCloneNodeFunc callback) { - config->cloneNodeCallback = callback; + config->setCloneNodeCallback(callback); } static void YGTraverseChildrenPreOrder(