From 722bfb9032f2ccc414cf6c2694efd857f6b10138 Mon Sep 17 00:00:00 2001 From: Kazuki Sakamoto Date: Thu, 13 Oct 2016 07:52:44 -0700 Subject: [PATCH] Align C# implementation with Java Summary: - Align C# implementation with Java JNI implementation - No need to call init(reinit) - Rename Initialize and Reset - Add unittests Reviewed By: emilsjolander Differential Revision: D4009351 fbshipit-source-id: 191ddec30b0c8518eb0d76c0579afe909b673fac --- README.md | 3 - csharp/Facebook.CSSLayout/CSSNode.cs | 32 +++++- .../tests/Facebook.CSSLayout/CSSNodeTest.cs | 104 ++++++++++++++---- 3 files changed, 108 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 49a68bf0..bdd67398 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,6 @@ root.setStyleHeight(100); for (int i = 0; i < 10; i++) { CSSNode child = new CSSNode(); - child.init(); child.setStyleHeight(10); root.addChildAt(child, 0); } @@ -116,14 +115,12 @@ The full API can be found in `csharp/Facebook.CSSLayout/CSSNode.cs`. ```csharp var root = new CSSNode(); -root.Initialize(); root.StyleWidth = 100; root.StyleHeight = 100; for (var i = 0; i < 10; i++) { var child = new CSSNode(); - child.Initialize(); child.StyleHeight = 10; root.Insert(0, child); } diff --git a/csharp/Facebook.CSSLayout/CSSNode.cs b/csharp/Facebook.CSSLayout/CSSNode.cs index bb4ace99..a0dea103 100644 --- a/csharp/Facebook.CSSLayout/CSSNode.cs +++ b/csharp/Facebook.CSSLayout/CSSNode.cs @@ -30,6 +30,7 @@ namespace Facebook.CSSLayout { _measureFunc = MeasureInternal; _printFunc = PrintInternal; + Reinitialize(); } private void AssertNativeInstance() @@ -72,8 +73,27 @@ namespace Facebook.CSSLayout private void FreeManaged() { - _children = null; - _parent = null; + if (_children != null) + { + for (int i = 0; i < _children.Count; ++i) + { + var child = _children[i]; + child.AssertNativeInstance(); + child._parent = null; + Native.CSSNodeRemoveChild(_cssNode, child._cssNode); + } + _children = null; + } + + if (_parent != null) + { + var parent = _parent.Target as CSSNode; + parent.AssertNativeInstance(); + parent._children.Remove(this); + Native.CSSNodeRemoveChild(parent._cssNode, _cssNode); + _parent = null; + } + _measureFunction = null; } @@ -86,7 +106,7 @@ namespace Facebook.CSSLayout } } - public void Initialize() + public void Reinitialize() { if (_cssNode != IntPtr.Zero) { @@ -99,9 +119,13 @@ namespace Facebook.CSSLayout Native.CSSNodeSetPrintFunc(_cssNode, _printFunc); } - public void Reset() + public void Free() { AssertNativeInstance(); + if (_parent != null || (_children != null && _children.Count > 0)) + { + throw new InvalidOperationException("You should not free an attached CSSNode"); + } FreeManaged(); FreeUnmanaged(); } diff --git a/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs b/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs index f248d492..69037e36 100644 --- a/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs +++ b/csharp/tests/Facebook.CSSLayout/CSSNodeTest.cs @@ -22,9 +22,7 @@ namespace Facebook.CSSLayout public void TestAddChildGetParent() { CSSNode parent = new CSSNode(); - parent.Initialize(); CSSNode child = new CSSNode(); - child.Initialize(); Assert.IsNull(child.Parent); Assert.AreEqual(0, parent.Count); @@ -46,11 +44,8 @@ namespace Facebook.CSSLayout public void TestCannotAddChildToMultipleParents() { CSSNode parent1 = new CSSNode(); - parent1.Initialize(); CSSNode parent2 = new CSSNode(); - parent2.Initialize(); CSSNode child = new CSSNode(); - child.Initialize(); parent1.Insert(0, child); parent2.Insert(0, child); @@ -61,8 +56,7 @@ namespace Facebook.CSSLayout public void TestAlreadyInitialize() { CSSNode node = new CSSNode(); - node.Initialize(); - node.Initialize(); + node.Reinitialize(); } [Test] @@ -70,15 +64,25 @@ namespace Facebook.CSSLayout public void TestNullNativePointer() { CSSNode node = new CSSNode(); + node.Free(); node.CalculateLayout(); } [Test] [ExpectedException("System.InvalidOperationException")] - public void TestNullNativePointerWithReset() + public void TestDoubleFree() { CSSNode node = new CSSNode(); - node.Reset(); + node.Free(); + node.Free(); + } + + [Test] + public void TestReinitialize() + { + CSSNode node = new CSSNode(); + node.Free(); + node.Reinitialize(); } [Test] @@ -86,18 +90,15 @@ namespace Facebook.CSSLayout public void TestDisposed() { CSSNode node = new CSSNode(); - node.Initialize(); node.Dispose(); node.CalculateLayout(); } [Test] - public void TestReset() + public void TestFree() { CSSNode node = new CSSNode(); - node.Initialize(); - node.Reset(); - node.Initialize(); + node.Free(); } [Test] @@ -106,8 +107,6 @@ namespace Facebook.CSSLayout ForceGC(); Assert.AreEqual(0, CSSNode.GetInstanceCount()); CSSNode node = new CSSNode(); - Assert.AreEqual(0, CSSNode.GetInstanceCount()); - node.Initialize(); Assert.AreEqual(1, CSSNode.GetInstanceCount()); node.Dispose(); Assert.AreEqual(0, CSSNode.GetInstanceCount()); @@ -120,8 +119,6 @@ namespace Facebook.CSSLayout Assert.AreEqual(0, CSSNode.GetInstanceCount()); using (CSSNode node = new CSSNode()) { - Assert.AreEqual(0, CSSNode.GetInstanceCount()); - node.Initialize(); Assert.AreEqual(1, CSSNode.GetInstanceCount()); } Assert.AreEqual(0, CSSNode.GetInstanceCount()); @@ -140,8 +137,7 @@ namespace Facebook.CSSLayout private void TestDestructorForGC() { CSSNode node = new CSSNode(); - Assert.AreEqual(0, CSSNode.GetInstanceCount()); - node.Initialize(); + Assert.IsNotNull(node); Assert.AreEqual(1, CSSNode.GetInstanceCount()); node = null; } @@ -159,8 +155,6 @@ namespace Facebook.CSSLayout private void TestDestructorWithChildrenForGC1() { CSSNode node = new CSSNode(); - Assert.AreEqual(0, CSSNode.GetInstanceCount()); - node.Initialize(); Assert.AreEqual(1, CSSNode.GetInstanceCount()); TestDestructorWithChildrenForGC2(node, 1); @@ -177,14 +171,76 @@ namespace Facebook.CSSLayout private void TestDestructorWithChildrenForGC2(CSSNode parent, int count) { CSSNode child = new CSSNode(); - Assert.AreEqual(count, CSSNode.GetInstanceCount()); - child.Initialize(); Assert.AreEqual(count + 1, CSSNode.GetInstanceCount()); parent.Insert(0, child); child = null; } + [Test] + [ExpectedException("System.InvalidOperationException")] + public void TestFreeParent() + { + CSSNode parent = new CSSNode(); + CSSNode child = new CSSNode(); + parent.Insert(0, child); + parent.Free(); + } + + [Test] + [ExpectedException("System.InvalidOperationException")] + public void TestFreeChild() + { + CSSNode parent = new CSSNode(); + CSSNode child = new CSSNode(); + parent.Insert(0, child); + child.Free(); + } + + [Test] + public void TestDisposeParent() + { + ForceGC(); + Assert.AreEqual(0, CSSNode.GetInstanceCount()); + CSSNode parent = new CSSNode(); + Assert.AreEqual(1, CSSNode.GetInstanceCount()); + TestDisposeParentForGC(parent); + ForceGC(); + Assert.AreEqual(2, CSSNode.GetInstanceCount()); + parent.Dispose(); + ForceGC(); + Assert.AreEqual(0, CSSNode.GetInstanceCount()); + } + + private void TestDisposeParentForGC(CSSNode parent) + { + CSSNode child = new CSSNode(); + Assert.AreEqual(2, CSSNode.GetInstanceCount()); + parent.Insert(0, child); + child = null; + } + + [Test] + public void TestDisposeChild() + { + ForceGC(); + Assert.AreEqual(0, CSSNode.GetInstanceCount()); + CSSNode parent = new CSSNode(); + CSSNode child0 = new CSSNode(); + CSSNode child1 = new CSSNode(); + Assert.AreEqual(3, CSSNode.GetInstanceCount()); + Assert.AreEqual(0, parent.Count); + parent.Insert(0, child1); + Assert.AreEqual(0, parent.IndexOf(child1)); + parent.Insert(0, child0); + Assert.AreEqual(0, parent.IndexOf(child0)); + Assert.AreEqual(1, parent.IndexOf(child1)); + child0.Dispose(); + Assert.AreEqual(2, CSSNode.GetInstanceCount()); + Assert.AreEqual(1, parent.Count); + Assert.AreEqual(0, parent.IndexOf(child1)); + } + private void ForceGC() { GC.Collect(GC.MaxGeneration);