Refactor Effen to make explicit the distinction between render & non-render nodes.

sky/framework/fn.dart

Index: sky/framework/fn.dart
diff --git a/sky/framework/fn.dart b/sky/framework/fn.dart
index 974b7f5b277e6f76a1e8e01cb737735c7b3c8b1b..5e6ccba2df635db484e75957f29f499b0b3e7277 100644
--- a/sky/framework/fn.dart
+++ b/sky/framework/fn.dart
@@ -81,6 +81,10 @@ void _parentInsertBefore(sky.ParentNode parent,
   }
 }
 
+/*
+ * All Effen nodes derive from Node. All nodes have a _parent, a _key and
+ * can be sync'd.
+ */
 abstract class Node {
   String _key;
   Node _parent;
@@ -95,52 +99,125 @@ abstract class Node {
     _key = key == null ? "$runtimeType" : "$runtimeType-$key";
   }
 
-  Node get _emptyNode;
+  bool _willSync(Node old) => false;
 
-  sky.Node _createNode();
+  void _sync(Node old, sky.ParentNode host, sky.Node insertBefore);
 
-  void _mount(Node parent, sky.ParentNode host, sky.Node insertBefore) {
-    var node = _emptyNode;
-    node._parent = parent;
+  void _remove() {
+    _defunct = true;
+    _root = null;
+  }
 
-    node._root = _createNode();
-    assert(node._root != null);
+  Node _syncChild(Node node, Node oldNode, sky.ParentNode host,
+      sky.Node insertBefore) {
+    if (node == oldNode)
+      return node; // Nothing to do. Subtrees must be identical.
 
-    _parentInsertBefore(host, node._root, insertBefore);
+    // TODO(rafaelw): This eagerly removes the old DOM. It may be that a
+    // new component was built that could re-use some of it. Consider
+    // syncing the new VDOM against the old one.
+    if (oldNode != null && node._key != oldNode._key) {
+      oldNode._remove();
+    }
+
+    if (node._willSync(oldNode)) {
+      oldNode._sync(node, host, insertBefore);
+      node._defunct = true;
+      assert(oldNode._root is sky.Node);
+      return oldNode;
+    }
 
-    _syncNode(node);
+    node._parent = this;
+    node._sync(oldNode, host, insertBefore);
+    if (oldNode != null)
+      oldNode._defunct = true;
+
+    assert(node._root is sky.Node);
+    return node;
   }
 
-  bool _sync(Node old, Node parent, sky.ParentNode host,
-      sky.Node insertBefore) {
+  void _syncEvents(EventMap oldEventMap) {
+    List<EventHandler> newHandlers = events._handlers;
+    int newStartIndex = 0;
+    int newEndIndex = newHandlers.length;
 
-    if (old == null) {
-      _mount(parent, host, insertBefore);
-      return false;
+    List<EventHandler> oldHandlers = oldEventMap._handlers;
+    int oldStartIndex = 0;
+    int oldEndIndex = oldHandlers.length;
+
+    // Skip over leading handlers that match.
+    while (newStartIndex < newEndIndex && oldStartIndex < oldEndIndex) {
+      EventHandler newHandler = newHandlers[newStartIndex];
+      EventHandler oldHandler = oldHandlers[oldStartIndex];
+      if (newHandler.type != oldHandler.type
+          || newHandler.listener != oldHandler.listener)
+        break;
+      ++newStartIndex;
+      ++oldStartIndex;
+    }
+
+    // Skip over trailing handlers that match.
+    while (newStartIndex < newEndIndex && oldStartIndex < oldEndIndex) {
+      EventHandler newHandler = newHandlers[newEndIndex - 1];
+      EventHandler oldHandler = oldHandlers[oldEndIndex - 1];
+      if (newHandler.type != oldHandler.type
+          || newHandler.listener != oldHandler.listener)
+        break;
+      --newEndIndex;
+      --oldEndIndex;
     }
 
-    return _syncNode(old);
+    sky.Element root = _root as sky.Element;
+
+    for (int i = oldStartIndex; i < oldEndIndex; ++i) {
+      EventHandler oldHandler = oldHandlers[i];
+      root.removeEventListener(oldHandler.type, oldHandler.listener);
+    }
+
+    for (int i = newStartIndex; i < newEndIndex; ++i) {
+      EventHandler newHandler = newHandlers[i];
+      root.addEventListener(newHandler.type, newHandler.listener);
+    }
   }
 
-  // Return true IFF the old node has *become* the new node (should be
-  // retained because it is stateful)
-  bool _syncNode(Node old) {
-    assert(!old._defunct);
-    _root = old._root;
-    _parent = old._parent;
-    return false;
+}
+
+/*
+ * RenderNodes correspond to a desired state of a sky.Node. They are fully
+ * immutable, with one exception: A Node which is a Component which lives within
+ * an Element's children list, may be replaced with the "old" instance if it
+ * has become stateful.
+ */
+abstract class RenderNode extends Node {
+
+  RenderNode({ Object key }) : super(key: key);
+
+  RenderNode get _emptyNode;
+
+  sky.Node _createNode();
+
+  void _sync(Node old, sky.ParentNode host, sky.Node insertBefore) {
+    if (old == null) {
+      _root = _createNode();
+      _parentInsertBefore(host, _root, insertBefore);
+      old = _emptyNode;
+    } else {
+      _root = old._root;
+    }
+
+    _syncNode(old);
   }
 
-  void _unmount() {
+  void _syncNode(RenderNode old);
+
+  void _remove() {
     assert(_root != null);
-    _parent = null;
     _root.remove();
-    _defunct = true;
-    _root = null;
+    super._remove();
   }
 }
 
-class Text extends Node {
+class Text extends RenderNode {
   final String data;
 
   // Text nodes are special cases of having non-unique keys (which don't need
@@ -149,27 +226,25 @@ class Text extends Node {
   // the data.
   Text(this.data) : super(key:'*text*');
 
-  static Text _emptyText = new Text(null);
+  static final Text _emptyText = new Text(null);
 
-  Node get _emptyNode => _emptyText;
+  RenderNode get _emptyNode => _emptyText;
 
   sky.Node _createNode() {
     return new sky.Text(data);
   }
 
-  bool _syncNode(Node old) {
-    super._syncNode(old);
+  void _syncNode(RenderNode old) {
     if (old == _emptyText)
-      return false; // we set inside _createNode();
+      return; // we set inside _createNode();
 
     (_root as sky.Text).data = data;
-    return false;
   }
 }
 
 final List<Node> _emptyList = new List<Node>();
 
-abstract class Element extends Node {
+abstract class Element extends RenderNode {
 
   String get _tagName;
 
@@ -194,11 +269,11 @@ abstract class Element extends Node {
     }
   }
 
-  void _unmount() {
-    super._unmount();
+  void _remove() {
+    super._remove();
     if (_children != null) {
       for (var child in _children) {
-        child._unmount();
+        child._remove();
       }
     }
   }
@@ -217,57 +292,11 @@ abstract class Element extends Node {
     }
   }
 
-  void _syncEvents([Element old]) {
-    List<EventHandler> newHandlers = events._handlers;
-    int newStartIndex = 0;
-    int newEndIndex = newHandlers.length;
-
-    List<EventHandler> oldHandlers = old.events._handlers;
-    int oldStartIndex = 0;
-    int oldEndIndex = oldHandlers.length;
-
-    // Skip over leading handlers that match.
-    while (newStartIndex < newEndIndex && oldStartIndex < oldEndIndex) {
-      EventHandler newHandler = newHandlers[newStartIndex];
-      EventHandler oldHandler = oldHandlers[oldStartIndex];
-      if (newHandler.type != oldHandler.type
-          || newHandler.listener != oldHandler.listener)
-        break;
-      ++newStartIndex;
-      ++oldStartIndex;
-    }
-
-    // Skip over trailing handlers that match.
-    while (newStartIndex < newEndIndex && oldStartIndex < oldEndIndex) {
-      EventHandler newHandler = newHandlers[newEndIndex - 1];
-      EventHandler oldHandler = oldHandlers[oldEndIndex - 1];
-      if (newHandler.type != oldHandler.type
-          || newHandler.listener != oldHandler.listener)
-        break;
-      --newEndIndex;
-      --oldEndIndex;
-    }
-
-    sky.Element root = _root as sky.Element;
-
-    for (int i = oldStartIndex; i < oldEndIndex; ++i) {
-      EventHandler oldHandler = oldHandlers[i];
-      root.removeEventListener(oldHandler.type, oldHandler.listener);
-    }
-
-    for (int i = newStartIndex; i < newEndIndex; ++i) {
-      EventHandler newHandler = newHandlers[i];
-      root.addEventListener(newHandler.type, newHandler.listener);
-    }
-  }
-
-  bool _syncNode(Node old) {
-    super._syncNode(old);
-
+  void _syncNode(RenderNode old) {
     Element oldElement = old as Element;
     sky.Element root = _root as sky.Element;
 
-    _syncEvents(oldElement);
+    _syncEvents(oldElement.events);
 
     if (_class != oldElement._class)
       root.setAttribute('class', _class);
@@ -276,12 +305,9 @@ abstract class Element extends Node {
       root.setAttribute('style', inlineStyle);
 
     _syncChildren(oldElement);
-
-    return false;
   }
 
   void _syncChildren(Element oldElement) {
-    // print("---Syncing children of $_key");
     sky.Element root = _root as sky.Element;
     assert(root != null);
 
@@ -297,18 +323,11 @@ abstract class Element extends Node {
     Node oldNode = null;
 
     void sync(int atIndex) {
-      if (currentNode._sync(oldNode, this, _root, nextSibling)) {
-        // oldNode was stateful and must be retained.
-        currentNode = oldNode;
-        _children[atIndex] = currentNode;
-      }
-
-      assert(currentNode._root is sky.Node);
+      _children[atIndex] = _syncChild(currentNode, oldNode, _root, nextSibling);
     }
 
     // Scan backwards from end of list while nodes can be directly synced
     // without reordering.
-    // print("...scanning backwards");
     while (endIndex > startIndex && oldEndIndex > oldStartIndex) {
       currentNode = _children[endIndex - 1];
       oldNode = oldChildren[oldEndIndex - 1];
@@ -317,7 +336,6 @@ abstract class Element extends Node {
         break;
       }
 
-      // print('> syncing matched at: $endIndex : $oldEndIndex');
       endIndex--;
       oldEndIndex--;
       sync(endIndex);
@@ -363,20 +381,17 @@ abstract class Element extends Node {
         return false;
 
       oldNodeIdMap[currentNode._key] = null; // mark it reordered.
-      // print("Reparenting ${currentNode._key}");
       _parentInsertBefore(root, oldNode._root, nextSibling);
       return true;
     }
 
     // Scan forwards, this time we may re-order;
-    // print("...scanning forward");
     nextSibling = root.firstChild;
     while (startIndex < endIndex && oldStartIndex < oldEndIndex) {
       currentNode = _children[startIndex];
       oldNode = oldChildren[oldStartIndex];
 
       if (currentNode._key == oldNode._key) {
-        // print('> syncing matched at: $startIndex : $oldStartIndex');
         assert(currentNode.runtimeType == oldNode.runtimeType);
         nextSibling = nextSibling.nextSibling;
         sync(startIndex);
@@ -386,33 +401,24 @@ abstract class Element extends Node {
       }
 
       oldNode = null;
-      if (searchForOldNode()) {
-        // print('> reordered to $startIndex');
-      } else {
-        // print('> inserting at $startIndex');
-      }
-
+      searchForOldNode();
       sync(startIndex);
       startIndex++;
     }
 
     // New insertions
     oldNode = null;
-    // print('...processing remaining insertions');
     while (startIndex < endIndex) {
-      // print('> inserting at $startIndex');
       currentNode = _children[startIndex];
       sync(startIndex);
       startIndex++;
     }
 
     // Removals
-    // print('...processing remaining removals');
     currentNode = null;
     while (oldStartIndex < oldEndIndex) {
       oldNode = oldChildren[oldStartIndex];
-      // print('> ${oldNode._key} removing from $oldEndIndex');
-      oldNode._unmount();
+      oldNode._remove();
       advanceOldStartIndex();
     }
   }
@@ -424,7 +430,7 @@ class Container extends Element {
 
   static final Container _emptyContainer = new Container();
 
-  Node get _emptyNode => _emptyContainer;
+  RenderNode get _emptyNode => _emptyContainer;
 
   Container({
     Object key,
@@ -445,11 +451,11 @@ class Image extends Element {
 
   static final Image _emptyImage = new Image();
 
-  Node get _emptyNode => _emptyImage;
+  RenderNode get _emptyNode => _emptyImage;
 
-  String src;
-  int width;
-  int height;
+  final String src;
+  final int width;
+  final int height;
 
   Image({
     Object key,
@@ -466,7 +472,7 @@ class Image extends Element {
     inlineStyle: inlineStyle
   );
 
-  bool _syncNode(Node old) {
+  void _syncNode(Node old) {
     super._syncNode(old);
 
     Image oldImage = old as Image;
@@ -480,8 +486,6 @@ class Image extends Element {
 
     if (height != oldImage.height)
       skyImage.style['height'] = '${height}px';
-
-    return false;
   }
 }
 
@@ -493,9 +497,9 @@ class Anchor extends Element {
 
   Node get _emptyNode => _emptyAnchor;
 
-  String href;
-  int width;
-  int height;
+  final String href;
+  final int width;
+  final int height;
 
   Anchor({
     Object key,
@@ -512,7 +516,7 @@ class Anchor extends Element {
     inlineStyle: inlineStyle
   );
 
-  bool _syncNode(Node old) {
+  void _syncNode(Node old) {
     super._syncNode(old);
 
     Anchor oldAnchor = old as Anchor;
@@ -520,19 +524,30 @@ class Anchor extends Element {
 
     if (href != oldAnchor.href)
       skyAnchor.href = href;
-
-    return false;
   }
 }
 
 List<Component> _dirtyComponents = new List<Component>();
+Set<Component> _mountedComponents = new HashSet<Component>();
+Set<Component> _unmountedComponents = new HashSet<Component>();
+
 bool _buildScheduled = false;
 bool _inRenderDirtyComponents = false;
 
+void _notifyMountStatusChanged() {
+  _unmountedComponents.forEach((c) => c.didUnmount());
+  _mountedComponents.forEach((c) => c.didMount());
+  _mountedComponents.clear();
+  _unmountedComponents.clear();

[abarth-chromium, 2015/03/18 19:26:43]

Should we move these collections into local variables before iterating them?

[rafaelw, 2015/03/18 20:00:02]

You can check my math here, but the intent of these collections was to make
building & signalling mount/unmount entirely disjoint. Built cannot be trigged
from mount/unmount and unmount/mount cannot be  fired during build.

Thus, there should be no danger that these collections are mutated while they
are iterating.

+}
+
 void _buildDirtyComponents() {
+  Stopwatch sw;
+  if (_shouldLogRenderDuration)
+    sw = new Stopwatch()..start();
+
   try {
     _inRenderDirtyComponents = true;
-    Stopwatch sw = new Stopwatch()..start();
 
     _dirtyComponents.sort((a, b) => a._order - b._order);
     for (var comp in _dirtyComponents) {
@@ -541,13 +556,16 @@ void _buildDirtyComponents() {
 
     _dirtyComponents.clear();
     _buildScheduled = false;
-
-    sw.stop();
-    if (_shouldLogRenderDuration)
-      print("Render took ${sw.elapsedMicroseconds} microseconds");
   } finally {
     _inRenderDirtyComponents = false;
   }
+
+  _notifyMountStatusChanged();
+
+  if (_shouldLogRenderDuration) {
+    sw.stop();
+    print("Render took ${sw.elapsedMicroseconds} microseconds");
+  }
 }
 
 void _scheduleComponentForRender(Component c) {
@@ -560,13 +578,17 @@ void _scheduleComponentForRender(Component c) {
   }
 }
 
+EventMap _emptyEventMap = new EventMap();
+
 abstract class Component extends Node {
-  bool _dirty = true; // components begin dirty because they haven't built.
-  Node _vdom = null;
+  bool get _isBuilding => _currentlyBuilding == this;
+  bool _dirty = true;
+
+  Node _built;
   final int _order;
   static int _currentOrder = 0;
   bool _stateful;
-  static Component _currentlyRendering;
+  static Component _currentlyBuilding;
 
   Component({ Object key, bool stateful })
       : _stateful = stateful != null ? stateful : false,
@@ -580,103 +602,78 @@ abstract class Component extends Node {
   // needed to get sizing info.
   sky.Node getRoot() => _root;
 
-  void _mount(Node parent, sky.ParentNode host, sky.Node insertBefore) {
-    _parent = parent;
-
-    _syncInternal(host, insertBefore);
-
-    didMount();
-  }
-
-  void _unmount() {
-    assert(_vdom != null);
+  void _remove() {
+    assert(_built != null);
     assert(_root != null);
-    _vdom._unmount();
-    _vdom = null;
-    _root = null;
-    _parent = null;
-    _defunct = true;
-    didUnmount();
+    _built._remove();
+    _built = null;
+    _unmountedComponents.add(this);
+    super._remove();
   }
 
-  bool _syncNode(Node old) {
+  bool _willSync(Node old) {

[abarth-chromium, 2015/03/18 19:26:43]

Can you add a comment to the declaration of _willSync in the base class explain
what it's return value means?

[rafaelw, 2015/03/18 20:00:02]

Done. Also added comment to  _syncChild

     Component oldComponent = old as Component;
-
-    if (!oldComponent._stateful) {
-      _vdom = oldComponent._vdom;
-      _syncInternal();
-
+    if (oldComponent == null || !oldComponent._stateful)
       return false;
-    }
 
-    _stateful = false; // prevent iloop from _syncInternal below.
+    // Make |this| the "old" Component
+    _stateful = false;
+    _built = oldComponent._built;
+    assert(_built != null);
 
+    // Make |oldComponent| the "new" component
     reflect.copyPublicFields(this, oldComponent);
-
+    oldComponent._built = null;
     oldComponent._dirty = true;
-    _dirty = false;
-
-    oldComponent._syncInternal();
-    return true;  // Retain old component
+    return true;
   }
 
-  void _syncInternal([sky.Node host, sky.Node insertBefore]) {
-    if (!_dirty) {
-      assert(_vdom != null);
-      return;
+  /* There are three cases here:
+   * 1) Building for the first time:
+   *      assert(_built == null && old == null)
+   * 2) Re-building (because a dirty flag got set):
+   *      assert(_built != null && old == null)
+   * 3) Syncing against an old version
+   *      assert(_built == null && old != null)
+   */
+  void _sync(Node old, sky.ParentNode host, sky.Node insertBefore) {
+    assert(!_defunct);
+    assert(_built == null || old == null);
+
+    Component oldComponent = old as Component;
+
+    var oldBuilt;
+    if (oldComponent == null) {
+      oldBuilt = _built;
+    } else {
+      assert(_built == null);
+      oldBuilt = oldComponent._built;
     }
 
-    var oldRendered = _vdom;
-    bool mounting = oldRendered == null;
+    if (oldBuilt == null)
+      _mountedComponents.add(this);
 
     int lastOrder = _currentOrder;
     _currentOrder = _order;
-    _currentlyRendering = this;
-    _vdom = build();
-    _currentlyRendering = null;
+    _currentlyBuilding = this;
+    _built = build();
+    _currentlyBuilding = null;
     _currentOrder = lastOrder;
 
-    _vdom.events.addAll(events);
-
+    _built = _syncChild(_built, oldBuilt, host, insertBefore);
     _dirty = false;
+    _root = _built._root;
 
-    // TODO(rafaelw): This eagerly removes the old DOM. It may be that a
-    // new component was built that could re-use some of it. Consider
-    // syncing the new VDOM against the old one.
-    if (oldRendered != null &&
-        _vdom.runtimeType != oldRendered.runtimeType) {
-      var oldRoot = oldRendered._root;
-      host = oldRoot.parentNode;
-      insertBefore = oldRoot.nextSibling;
-      oldRendered._unmount();
-      oldRendered = null;
-    }
-
-    if (_vdom._sync(oldRendered, this, host, insertBefore)) {
-      _vdom = oldRendered; // retain stateful component
-    }
-
-    _root = _vdom._root;
-    assert(_vdom._root is sky.Node);
-
-    if (mounting) {
-      didMount();
-    }
+    _built.events.addAll(events);
+    _syncEvents(oldComponent != null ? oldComponent.events : _emptyEventMap);
   }
 
   void _buildIfDirty() {
-    if (_defunct)
+    if (!_dirty || _defunct)
       return;
 
-    assert(_vdom != null);
-
-    var vdom = _vdom;
-    while (vdom is Component) {
-      vdom = vdom._vdom;
-    }
-
-    assert(vdom._root != null);
-    _syncInternal();
+    assert(_root != null);
+    _sync(null, _root.parentNode, _root.nextSibling);
   }
 
   void scheduleBuild() {
@@ -684,13 +681,13 @@ abstract class Component extends Node {
   }
 
   void setState(Function fn()) {
-    assert(_vdom != null || _defunct); // cannot setState before mounting.
     _stateful = true;
     fn();
-    if (!_defunct && _currentlyRendering != this) {
-      _dirty = true;
-      _scheduleComponentForRender(this);
-    }
+    if (_isBuilding || _dirty || _defunct)
+      return;
+
+    _dirty = true;
+    _scheduleComponentForRender(this);
   }
 
   Node build();
@@ -705,7 +702,7 @@ abstract class App extends Component {
     new Future.microtask(() {
       Stopwatch sw = new Stopwatch()..start();
 
-      _mount(null, _host, null);
+      _sync(null, _host, null);
       assert(_root is sky.Node);
 
       sw.stop();