Ensure getInstanceModel is only called when a new instance will be created

pkg/mdv/lib/src/template_iterator.dart

Index: pkg/mdv/lib/src/template_iterator.dart
diff --git a/pkg/mdv/lib/src/template_iterator.dart b/pkg/mdv/lib/src/template_iterator.dart
index dd57cad3118bd6668e9d738cd90441277bc43790..6a39418c617e3b19af94349950f03e1b4df4a2a7 100644
--- a/pkg/mdv/lib/src/template_iterator.dart
+++ b/pkg/mdv/lib/src/template_iterator.dart
@@ -240,15 +240,28 @@ class _TemplateIterator {
     return terminator;
   }
 
-  void insertInstanceAt(int index, List<Node> instanceNodes) {
+  void insertInstanceAt(int index, DocumentFragment fragment,
+        List<Node> instanceNodes) {
+
     var previousTerminator = getTerminatorAt(index - 1);
-    var terminator = instanceNodes.length > 0 ? instanceNodes.last
-        : previousTerminator;
+    var terminator = null;
+    if (fragment != null) {
+      terminator = fragment.lastChild;
+    } else if (instanceNodes.length > 0) {
+      terminator = instanceNodes.last;
+    }
+    if (terminator == null) terminator = previousTerminator;

[justinfagnani, 2013/07/23 20:42:06]

Initialize terminator to null? I'm not sure if fragment.lastChild or
instanceNodes.last can be null, but talking to Steven, it sounded like there
were potential code size benefits to not allowing variables to be null. He
recommended making all possible instance vars final, not sure if that applies to
locals.

[Jennifer Messerly, 2013/07/23 20:49:39]

:confused: it is initialized to null ... yes, fragment.lastChild can be null,
likewise instanceNodes can be empty.

This particular case should be no problem for any competent compiler :). After
this line, "terminator" will have the same non-null properties as
previousTerminator.

 
     terminators.insert(index, terminator);
 
     var parent = _templateElement.parentNode;
     var insertBeforeNode = previousTerminator.nextNode;
+
+    if (fragment != null) {
+      parent.insertBefore(fragment, insertBeforeNode);
+      return;
+    }
+
     for (var node in instanceNodes) {
       parent.insertBefore(node, insertBeforeNode);
     }
@@ -277,16 +290,8 @@ class _TemplateIterator {
     return model;
   }
 
-  List<Node> getInstanceNodes(model, BindingDelegate delegate,
-      IdentityMap instanceCache) {
-
-    var instanceNodes = instanceCache.remove(model);
-    if (instanceNodes != null) return instanceNodes;
-
-    var fragment = _templateElement.createInstance(model, delegate);
-    instanceNodes = fragment.nodes.toList();
-    fragment.nodes.clear();
-    return instanceNodes;
+  DocumentFragment getInstanceFragment(model, BindingDelegate delegate) {
+    return _templateElement.createInstance(model, delegate);
   }
 
   void _handleChanges(Iterable<ChangeRecord> splices) {
@@ -325,10 +330,15 @@ class _TemplateIterator {
           addIndex < splice.index + splice.addedCount;
           addIndex++) {
 
-        var model = getInstanceModel(iteratedValue[addIndex], delegate);
+        var model = iteratedValue[addIndex];
+        var fragment = null;
+        var instanceNodes = instanceCache.remove(model);
+        if (instanceNodes == null) {

[justinfagnani, 2013/07/23 20:42:06]

check for empty too?

[Jennifer Messerly, 2013/07/23 20:49:39]

if we expanded to an empty set of nodes before (hence cached it) we want to
reuse the empty nodes.

+          var actualModel = getInstanceModel(model, delegate);
+          fragment = getInstanceFragment(actualModel, delegate);
+        }
 
-        var instanceNodes = getInstanceNodes(model, delegate, instanceCache);
-        insertInstanceAt(addIndex, instanceNodes);
+        insertInstanceAt(addIndex, fragment, instanceNodes);
       }
     }