Refactor cloning of YogaNode

Summary: see Test Plan

Reviewed By: priteshrnandgaonkar

Differential Revision: D7339832

fbshipit-source-id: 2de6f47ae7601ac083d3b9fbe10ffaf6307ae760

java/com/facebook/yoga/YogaConfig.java

@@ -25,7 +25,7 @@ public class YogaConfig {
 
   long mNativePointer;
   private YogaLogger mLogger;
-  private YogaNodeClonedFunction mNodeClonedFunction;
+  private YogaNodeCloneFunction mYogaNodeCloneFunction;
 
   private native long jni_YGConfigNew();
   public YogaConfig() {
@@ -97,16 +97,15 @@ public class YogaConfig {
     return mLogger;
   }
 
-  private native void jni_YGConfigSetHasNodeClonedFunc(long nativePointer, boolean hasClonedFunc);
+  private native void jni_YGConfigSetHasCloneNodeFunc(long nativePointer, boolean hasClonedFunc);
 
-  public void setOnNodeCloned(YogaNodeClonedFunction nodeClonedFunction) {
+  public void setOnCloneNode(YogaNodeCloneFunction cloneYogaNodeFunction) {
-    mNodeClonedFunction = nodeClonedFunction;
+    mYogaNodeCloneFunction = cloneYogaNodeFunction;
-    jni_YGConfigSetHasNodeClonedFunc(mNativePointer, nodeClonedFunction != null);
+    jni_YGConfigSetHasCloneNodeFunc(mNativePointer, cloneYogaNodeFunction != null);
   }
 
   @DoNotStrip
-  public final void onNodeCloned(
+  private final YogaNode cloneNode(YogaNode oldNode, YogaNode parent, int childIndex) {
-      YogaNode oldNode, YogaNode newNode, YogaNode parent, int childIndex) {
+    return mYogaNodeCloneFunction.cloneNode(oldNode, parent, childIndex);
-    mNodeClonedFunction.onNodeCloned(oldNode, newNode, parent, childIndex);
   }
 }

java/com/facebook/yoga/YogaNode.java

@@ -697,4 +697,18 @@ public class YogaNode implements Cloneable {
   public void print() {
     jni_YGNodePrint(mNativePointer);
   }
+
+  /**
+   * This method replaces the child at childIndex position with the newNode received by parameter.
+   * This is different than calling removeChildAt and addChildAt because this method ONLY replaces
+   * the child in the mChildren datastructure. @DoNotStrip: called from JNI
+   *
+   * @return the nativePointer of the newNode {@linl YogaNode}
+   */
+  @DoNotStrip
+  private final long replaceChild(YogaNode newNode, int childIndex) {
+    mChildren.remove(childIndex);
+    mChildren.add(childIndex, newNode);
+    return newNode.mNativePointer;
+  }
 }

java/com/facebook/yoga/YogaNodeCloneFunction.java

@@ -10,8 +10,8 @@ package com.facebook.yoga;
 import com.facebook.proguard.annotations.DoNotStrip;
 
 @DoNotStrip
-public interface YogaNodeClonedFunction {
+public interface YogaNodeCloneFunction {
 
   @DoNotStrip
-  void onNodeCloned(YogaNode oldNode, YogaNode newNode, YogaNode parent, int childIndex);
+  YogaNode cloneNode(YogaNode oldNode, YogaNode parent, int childIndex);
 }

java/jni/YGJNI.cpp

@@ -142,31 +142,49 @@ static float YGJNIBaselineFunc(YGNodeRef node, float width, float height) {
   }
 }
 
-static void YGJNIOnNodeClonedFunc(
+static inline YGNodeRef _jlong2YGNodeRef(jlong addr) {
+  return reinterpret_cast<YGNodeRef>(static_cast<intptr_t>(addr));
+}
+
+static inline YGConfigRef _jlong2YGConfigRef(jlong addr) {
+  return reinterpret_cast<YGConfigRef>(static_cast<intptr_t>(addr));
+}
+
+static YGNodeRef YGJNIOnNodeClonedFunc(
     YGNodeRef oldNode,
-    YGNodeRef newNode,
     YGNodeRef parent,
     int childIndex) {
   auto config = oldNode->getConfig();
   if (!config) {
-    return;
+    return nullptr;
   }
+
   static auto onNodeClonedFunc = findClassStatic("com/facebook/yoga/YogaConfig")
-                                     ->getMethod<void(
+                                     ->getMethod<alias_ref<JYogaNode>(
                                          local_ref<JYogaNode>,
                                          local_ref<JYogaNode>,
-                                         local_ref<JYogaNode>,
+                                         jint)>("cloneNode");
-                                         jint)>("onNodeCloned");
 
   auto context = reinterpret_cast<YGConfigContext*>(YGConfigGetContext(config));
   auto javaConfig = context->config;
 
-  onNodeClonedFunc(
+  auto newNode = onNodeClonedFunc(
       javaConfig->get(),
       YGNodeJobject(oldNode)->lockLocal(),
-      YGNodeJobject(newNode)->lockLocal(),
       YGNodeJobject(parent)->lockLocal(),
       childIndex);
+
+  static auto replaceChild = findClassStatic("com/facebook/yoga/YogaNode")
+                                     ->getMethod<jlong(
+                                         local_ref<JYogaNode>,
+                                         jint)>("replaceChild");
+
+  jlong newNodeNativePointer = replaceChild(
+      YGNodeJobject(parent)->lockLocal(),
+      newNode,
+      childIndex);
+
+  return _jlong2YGNodeRef(newNodeNativePointer);
 }
 
 static YGSize YGJNIMeasureFunc(
@@ -234,14 +252,6 @@ static int YGJNILogFunc(const YGConfigRef config,
   return result;
 }
 
-static inline YGNodeRef _jlong2YGNodeRef(jlong addr) {
-  return reinterpret_cast<YGNodeRef>(static_cast<intptr_t>(addr));
-}
-
-static inline YGConfigRef _jlong2YGConfigRef(jlong addr) {
-  return reinterpret_cast<YGConfigRef>(static_cast<intptr_t>(addr));
-}
-
 jlong jni_YGNodeNew(alias_ref<jobject> thiz) {
   const YGNodeRef node = YGNodeNew();
   node->setContext(new weak_ref<jobject>(make_weak(thiz)));
@@ -506,10 +516,10 @@ void jni_YGConfigSetUseLegacyStretchBehaviour(alias_ref<jobject>,
   YGConfigSetUseLegacyStretchBehaviour(config, useLegacyStretchBehaviour);
 }
 
-void jni_YGConfigSetHasNodeClonedFunc(
+void jni_YGConfigSetHasCloneNodeFunc(
     alias_ref<jobject> thiz,
     jlong nativePointer,
-    jboolean hasNodeClonedFunc) {
+    jboolean hasCloneNodeFunc) {
   const YGConfigRef config = _jlong2YGConfigRef(nativePointer);
   auto context = reinterpret_cast<YGConfigContext*>(YGConfigGetContext(config));
   if (context && context->config) {
@@ -517,15 +527,15 @@ void jni_YGConfigSetHasNodeClonedFunc(
     context->config = nullptr;
   }
 
-  if (hasNodeClonedFunc) {
+  if (hasCloneNodeFunc) {
     if (!context) {
       context = new YGConfigContext();
       YGConfigSetContext(config, context);
     }
     context->config = new global_ref<jobject>(make_global(thiz));
-    YGConfigSetNodeClonedFunc(config, YGJNIOnNodeClonedFunc);
+    YGConfigSetCloneNodeFunc(config, YGJNIOnNodeClonedFunc);
   } else {
-    YGConfigSetNodeClonedFunc(config, nullptr);
+    YGConfigSetCloneNodeFunc(config, nullptr);
   }
 }
 
@@ -652,7 +662,7 @@ jint JNI_OnLoad(JavaVM *vm, void *) {
             YGMakeNativeMethod(jni_YGConfigSetPointScaleFactor),
             YGMakeNativeMethod(jni_YGConfigSetUseLegacyStretchBehaviour),
             YGMakeNativeMethod(jni_YGConfigSetLogger),
-            YGMakeNativeMethod(jni_YGConfigSetHasNodeClonedFunc),
+            YGMakeNativeMethod(jni_YGConfigSetHasCloneNodeFunc),
             YGMakeNativeMethod(
                 jni_YGConfigSetShouldDiffLayoutWithoutLegacyStretchBehaviour),
         });

java/tests/com/facebook/yoga/YogaNodeTest.java

@@ -255,12 +255,17 @@ public class YogaNodeTest {
   public void testCloneNodeListener() throws Exception {
     final AtomicBoolean onNodeClonedExecuted = new AtomicBoolean(false);
     YogaConfig config = new YogaConfig();
-    config.setOnNodeCloned(
+    config.setOnCloneNode(
-        new YogaNodeClonedFunction() {
+        new YogaNodeCloneFunction() {
           @Override
-          public void onNodeCloned(
+          public YogaNode cloneNode(YogaNode oldNode, YogaNode parent, int childIndex) {
-              YogaNode oldNode, YogaNode newNode, YogaNode parent, int childIndex) {
+            try {
               onNodeClonedExecuted.set(true);
+              return oldNode.clone();
+            } catch (CloneNotSupportedException ex) {
+              // DO nothing
+              return null;
+            }
           }
         });
     YogaNode root = new YogaNode(config);
@@ -268,6 +273,7 @@ public class YogaNodeTest {
     root.setHeight(100f);
     YogaNode child0 = new YogaNode(config);
     root.addChildAt(child0, 0);
+    child0.setWidth(50f);
     root.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED);
 
     // Force a clone to happen.
@@ -276,20 +282,29 @@ public class YogaNodeTest {
     root2.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED);
 
     assertTrue(onNodeClonedExecuted.get());
+    assertEquals(1, root2.getChildCount());
+    YogaNode clonedNode = root2.getChildAt(0);
+    assertNotSame(child0, clonedNode);
+    assertEquals(child0.getWidth(), clonedNode.getWidth());
+    assertEquals(50f, clonedNode.getWidth().value, 0.01f);
   }
 
   @Test
   public void testOnNodeClonedLeak() throws Exception {
     YogaConfig config = new YogaConfig();
-    config.setOnNodeCloned(
+    config.setOnCloneNode(
-        new YogaNodeClonedFunction() {
+        new YogaNodeCloneFunction() {
           @Override
-          public void onNodeCloned(
+          public YogaNode cloneNode(YogaNode oldNode, YogaNode parent, int childIndex) {
-              YogaNode oldNode, YogaNode newNode, YogaNode parent, int childIndex) {
+            try {
-            // Do nothing
+              return oldNode.clone();
+            } catch (CloneNotSupportedException ex) {
+              // DO nothing
+              return null;
+            }
           }
         });
-    config.setOnNodeCloned(null);
+    config.setOnCloneNode(null);
     WeakReference<Object> ref = new WeakReference<Object>(config);
     // noinspection UnusedAssignment
     config = null;

yoga/YGNode.cpp

@@ -558,15 +558,18 @@ void YGNode::cloneChildrenIfNeeded() {
     return;
   }
 
-  const YGNodeClonedFunc cloneNodeCallback = config_->cloneNodeCallback;
+  const YGCloneNodeFunc cloneNodeCallback = config_->cloneNodeCallback;
   for (uint32_t i = 0; i < childCount; ++i) {
     const YGNodeRef oldChild = children_[i];
-    const YGNodeRef newChild = YGNodeClone(oldChild);
+    YGNodeRef newChild = nullptr;
+    if (cloneNodeCallback) {
+      newChild = cloneNodeCallback(oldChild, this, i);
+    }
+    if (newChild == nullptr) {
+      newChild = YGNodeClone(oldChild);
+    }
     replaceChild(newChild, i);
     newChild->setParent(this);
-    if (cloneNodeCallback) {
-      cloneNodeCallback(oldChild, newChild, this, i);
-    }
   }
 }
 

yoga/Yoga-internal.h

@@ -94,7 +94,7 @@ struct YGConfig {
   bool shouldDiffLayoutWithoutLegacyStretchBehaviour;
   float pointScaleFactor;
   YGLogger logger;
-  YGNodeClonedFunc cloneNodeCallback;
+  YGCloneNodeFunc cloneNodeCallback;
   void* context;
 };
 

yoga/Yoga.cpp

@@ -428,7 +428,7 @@ void YGNodeRemoveChild(const YGNodeRef parent, 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 YGNodeClonedFunc cloneNodeCallback =
+  const YGCloneNodeFunc cloneNodeCallback =
       parent->getConfig()->cloneNodeCallback;
   uint32_t nextInsertIndex = 0;
   for (uint32_t i = 0; i < childCount; i++) {
@@ -440,12 +440,16 @@ void YGNodeRemoveChild(const YGNodeRef parent, const YGNodeRef excludedChild) {
       parent->markDirtyAndPropogate();
       continue;
     }
-    const YGNodeRef newChild = YGNodeClone(oldChild);
+    YGNodeRef newChild = nullptr;
+    if (cloneNodeCallback) {
+      newChild = cloneNodeCallback(oldChild, parent, nextInsertIndex);
+    }
+    if (newChild == nullptr) {
+      newChild = YGNodeClone(oldChild);
+    }
     parent->replaceChild(newChild, nextInsertIndex);
     newChild->setParent(parent);
-    if (cloneNodeCallback) {
+
-      cloneNodeCallback(oldChild, newChild, parent, nextInsertIndex);
-    }
     nextInsertIndex++;
   }
   while (nextInsertIndex < childCount) {
@@ -3964,7 +3968,7 @@ void *YGConfigGetContext(const YGConfigRef config) {
   return config->context;
 }
 
-void YGConfigSetNodeClonedFunc(const YGConfigRef config, const YGNodeClonedFunc callback) {
+void YGConfigSetCloneNodeFunc(const YGConfigRef config, const YGCloneNodeFunc callback) {
   config->cloneNodeCallback = callback;
 }
 

yoga/Yoga.h

@@ -62,8 +62,7 @@ typedef int (*YGLogger)(const YGConfigRef config,
                         YGLogLevel level,
                         const char *format,
                         va_list args);
-typedef void (*YGNodeClonedFunc)(YGNodeRef oldNode,
+typedef YGNodeRef (*YGCloneNodeFunc)(YGNodeRef oldNode,
-                                 YGNodeRef newNode,
                                  YGNodeRef parent,
                                  int childIndex);
 
@@ -283,8 +282,8 @@ WIN_EXPORT bool YGConfigIsExperimentalFeatureEnabled(const YGConfigRef config,
 WIN_EXPORT void YGConfigSetUseWebDefaults(const YGConfigRef config, const bool enabled);
 WIN_EXPORT bool YGConfigGetUseWebDefaults(const YGConfigRef config);
 
-WIN_EXPORT void YGConfigSetNodeClonedFunc(const YGConfigRef config,
+WIN_EXPORT void YGConfigSetCloneNodeFunc(const YGConfigRef config,
-                                          const YGNodeClonedFunc callback);
+                                          const YGCloneNodeFunc callback);
 
 // Export only for C#
 WIN_EXPORT YGConfigRef YGConfigGetDefault(void);