Revert "perf(view): increase view instantiation speed 40%"

chirayuk · chirayuk · commit af906c9b90d1 · 2014-08-20T13:23:33.000-07:00
This reverts commit f20eab2 as it breaks some applications. We need to write test cases for the failures and do one or both of: (1) fixup the PR 1358, (2) specify the breaking changes so that applications can be upgraded.
diff --git a/lib/core_dom/directive.dart b/lib/core_dom/directive.dart
@@ -22,7 +22,7 @@ class NodeAttrs {
 
   NodeAttrs(this.element);
 
-  operator [](String attrName) => element.getAttribute(attrName);
+  operator [](String attrName) => element.attributes[attrName];
 
   void operator []=(String attrName, String value) {
     if (_mustacheAttrs.containsKey(attrName)) {
@@ -31,7 +31,7 @@ class NodeAttrs {
     if (value == null) {
       element.attributes.remove(attrName);
     } else {
-      element.setAttribute(attrName, value);
+      element.attributes[attrName] = value;
     }
 
     if (_observers != null && _observers.containsKey(attrName)) {
@@ -86,18 +86,9 @@ class NodeAttrs {
  * ShadowRoot is ready.
  */
 class TemplateLoader {
-  async.Future<dom.Node> _template;
-  List<async.Future> _futures;
-  final dom.Node _shadowRoot;
+  final async.Future<dom.Node> template;
 
-  TemplateLoader(this._shadowRoot, this._futures);
-
-  async.Future<dom.Node> get template {
-    if (_template == null) {
-      _template = async.Future.wait(_futures).then((_) => _shadowRoot);
-    }
-    return _template;
-  }
+  TemplateLoader(this.template);
 }
 
 class _MustacheAttr {
diff --git a/lib/core_dom/shadow_dom_component_factory.dart b/lib/core_dom/shadow_dom_component_factory.dart
@@ -11,7 +11,7 @@ abstract class BoundComponentFactory {
   List<Key> get callArgs;
   Function call(dom.Element element);
 
-  static async.Future<ViewFactory> _viewFactoryFuture(
+  static async.Future<ViewFactory> _viewFuture(
         Component component, ViewCache viewCache, DirectiveMap directives) {
     if (component.template != null) {
       return new async.Future.value(viewCache.fromHtml(component.template, directives));
@@ -65,27 +65,20 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
   Component get _component => _ref.annotation as Component;
 
   String _tag;
-  async.Future<List<dom.StyleElement>> _styleElementsFuture;
-  List<dom.StyleElement> _styleElements;
-  async.Future<ViewFactory> _shadowViewFactoryFuture;
-  ViewFactory _shadowViewFactory;
+  async.Future<Iterable<dom.StyleElement>> _styleElementsFuture;
+  async.Future<ViewFactory> _viewFuture;
 
   BoundShadowDomComponentFactory(this._componentFactory, this._ref, this._directives) {
     _tag = _component.selector.toLowerCase();
-    _styleElementsFuture = async.Future.wait(_component.cssUrls.map(_urlToStyle))
-        ..then((stylesElements) => _styleElements = stylesElements);
+    _styleElementsFuture = async.Future.wait(_component.cssUrls.map(_styleFuture));
 
-    _shadowViewFactoryFuture = BoundComponentFactory._viewFactoryFuture(
+    _viewFuture = BoundComponentFactory._viewFuture(
         _component,
-        // TODO(misko): Why do we create a new one per Component. This kind of defeats the caching.
         new PlatformViewCache(_componentFactory.viewCache, _tag, _componentFactory.platform),
         _directives);
-    if (_shadowViewFactoryFuture != null) {
-      _shadowViewFactoryFuture.then((viewFactory) => _shadowViewFactory = viewFactory);
-    }
   }
 
-  async.Future<dom.StyleElement> _urlToStyle(cssUrl) {
+  async.Future<dom.StyleElement> _styleFuture(cssUrl) {
     Http http = _componentFactory.http;
     TemplateCache templateCache = _componentFactory.templateCache;
     WebPlatform platform = _componentFactory.platform;
@@ -114,7 +107,7 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
 
           // If the css shim is required, it means that scoping does not
           // work, and adding the style to the head of the document is
-          // preferable.
+          // preferrable.
           if (platform.cssShimRequired) {
             dom.document.head.append(styleElement);
             return null;
@@ -133,57 +126,47 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
             EventHandler eventHandler) {
       var s = traceEnter(View_createComponent);
       try {
-        var shadowScope = scope.createChild(new HashMap()); // Isolate
-        ComponentDirectiveInjector shadowInjector;
-        dom.ShadowRoot shadowRoot = element.createShadowRoot();
-        shadowRoot
+        var shadowDom = element.createShadowRoot()
           ..applyAuthorStyles = _component.applyAuthorStyles
           ..resetStyleInheritance = _component.resetStyleInheritance;
 
-        List<async.Future> futures = <async.Future>[];
-        TemplateLoader templateLoader = new TemplateLoader(shadowRoot, futures);
-        shadowInjector = new ShadowDomComponentDirectiveInjector(
-            injector, injector.appInjector, shadowScope, templateLoader, shadowRoot);
-        shadowInjector.bindByKey(_ref.typeKey, _ref.factory, _ref.paramKeys,
-            _ref.annotation.visibility);
-        dom.Node firstViewNode = null;
-
-        // Load ngBase CSS
-        if (_component.useNgBaseCss == true && baseCss.urls.isNotEmpty) {
-          if (baseCss.styles == null) {
-            futures.add(async.Future
-                .wait(baseCss.urls.map(_urlToStyle))
-                .then((List<dom.StyleElement> cssList) {
-                  baseCss.styles = cssList;
-                  _insertCss(cssList, shadowRoot, shadowRoot.firstChild);
-                }));
-          } else {
-            _insertCss(baseCss.styles, shadowRoot, shadowRoot.firstChild);
-          }
-        }
+        var shadowScope = scope.createChild(new HashMap()); // Isolate
 
-        if (_styleElementsFuture != null) {
-          if (_styleElements == null) {
-            futures.add(_styleElementsFuture .then((List<dom.StyleElement> styles) =>
-                _insertCss(styles, shadowRoot, firstViewNode)));
-          } else {
-            _insertCss(_styleElements, shadowRoot);
-          }
+        async.Future<Iterable<dom.StyleElement>> cssFuture;
+        if (_component.useNgBaseCss == true) {
+          cssFuture = async.Future.wait([async.Future.wait(baseCss.urls.map(_styleFuture)), _styleElementsFuture]).then((twoLists) {
+            assert(twoLists.length == 2);return []
+              ..addAll(twoLists[0])
+              ..addAll(twoLists[1]);
+          });
+        } else {
+          cssFuture = _styleElementsFuture;
         }
 
+        ComponentDirectiveInjector shadowInjector;
 
-        if (_shadowViewFactoryFuture != null) {
-          if (_shadowViewFactory == null) {
-            futures.add(_shadowViewFactoryFuture.then((ViewFactory viewFactory) =>
-                firstViewNode = _insertView(viewFactory, shadowRoot, shadowScope, shadowInjector)));
-          } else {
-            _insertView(_shadowViewFactory, shadowRoot, shadowScope, shadowInjector);
+        TemplateLoader templateLoader = new TemplateLoader(cssFuture.then((Iterable<dom.StyleElement> cssList) {
+          cssList.where((styleElement) => styleElement != null).forEach((styleElement) {
+            shadowDom.append(styleElement.clone(true));
+          });
+          if (_viewFuture != null) {
+            return _viewFuture.then((ViewFactory viewFactory) {
+              if (shadowScope.isAttached) {
+                shadowDom.nodes.addAll(viewFactory.call(shadowInjector.scope, shadowInjector).nodes);
+              }
+              return shadowDom;
+            });
           }
-        }
+          return shadowDom;
+        }));
+
+        var probe;
+        shadowInjector = new ShadowDomComponentDirectiveInjector(injector, injector.appInjector, shadowScope, templateLoader, shadowDom);
+        shadowInjector.bindByKey(_ref.typeKey, _ref.factory, _ref.paramKeys, _ref.annotation.visibility);
 
         if (_componentFactory.config.elementProbeEnabled) {
-          ElementProbe probe = _componentFactory.expando[shadowRoot] = shadowInjector.elementProbe;
-          shadowScope.on(ScopeEvent.DESTROY).listen((ScopeEvent) => _componentFactory.expando[shadowRoot] = null);
+          probe = _componentFactory.expando[shadowDom] = shadowInjector.elementProbe;
+          shadowScope.on(ScopeEvent.DESTROY).listen((ScopeEvent) => _componentFactory.expando[shadowDom] = null);
         }
 
         var controller = shadowInjector.getByKey(_ref.typeKey);
@@ -197,36 +180,6 @@ class BoundShadowDomComponentFactory implements BoundComponentFactory {
       }
     };
   }
-
-  _insertCss(List<dom.StyleElement> cssList,
-             dom.ShadowRoot shadowRoot,
-             [dom.Node insertBefore = null]) {
-    var s = traceEnter(View_styles);
-    for(int i = 0; i < cssList.length; i++) {
-      var styleElement = cssList[i];
-      if (styleElement != null) {
-        shadowRoot.insertBefore(styleElement.clone(true), insertBefore);
-      }
-    }
-    traceLeave(s);
-  }
-
-  dom.Node _insertView(ViewFactory viewFactory,
-              dom.ShadowRoot shadowRoot,
-              Scope shadowScope,
-              ShadowDomComponentDirectiveInjector shadowInjector) {
-    dom.Node first = null;
-    if (shadowScope.isAttached) {
-      View shadowView = viewFactory.call(shadowScope, shadowInjector);
-      List<dom.Node> shadowViewNodes = shadowView.nodes;
-      for (var j = 0; j < shadowViewNodes.length; j++) {
-        var node = shadowViewNodes[j];
-        if (j == 0) first = node;
-        shadowRoot.append(node);
-      }
-    }
-    return first;
-  }
 }
 
 class _ComponentAssetKey {
diff --git a/lib/core_dom/transcluding_component_factory.dart b/lib/core_dom/transcluding_component_factory.dart
@@ -87,14 +87,13 @@ class BoundTranscludingComponentFactory implements BoundComponentFactory {
   final DirectiveMap _directives;
 
   Component get _component => _ref.annotation as Component;
-  async.Future<ViewFactory> _viewFactoryFuture;
-  ViewFactory _viewFactory;
+  async.Future<ViewFactory> _viewFuture;
 
   BoundTranscludingComponentFactory(this._f, this._ref, this._directives) {
-    _viewFactoryFuture = BoundComponentFactory._viewFactoryFuture(_component, _f.viewCache, _directives);
-    if (_viewFactoryFuture != null) {
-      _viewFactoryFuture.then((viewFactory) => _viewFactory = viewFactory);
-    }
+    _viewFuture = BoundComponentFactory._viewFuture(
+        _component,
+        _f.viewCache,
+        _directives);
   }
 
   List<Key> get callArgs => _CALL_ARGS;
@@ -111,40 +110,52 @@ class BoundTranscludingComponentFactory implements BoundComponentFactory {
             ViewCache viewCache, Http http, TemplateCache templateCache,
             DirectiveMap directives, NgBaseCss baseCss, EventHandler eventHandler) {
 
-      List<async.Future> futures = [];
+      DirectiveInjector childInjector;
+      var childInjectorCompleter; // Used if the ViewFuture is available before the childInjector.
+
+      var component = _component;
       var contentPort = new ContentPort(element);
-      TemplateLoader templateLoader = new TemplateLoader(element, futures);
+
+      // Append the component's template as children
+      var elementFuture;
+
+      if (_viewFuture != null) {
+        elementFuture = _viewFuture.then((ViewFactory viewFactory) {
+          contentPort.pullNodes();
+          if (childInjector != null) {
+            element.nodes.addAll(
+                viewFactory.call(childInjector.scope, childInjector).nodes);
+            return element;
+          } else {
+            childInjectorCompleter = new async.Completer();
+            return childInjectorCompleter.future.then((childInjector) {
+              element.nodes.addAll(
+                  viewFactory.call(childInjector.scope, childInjector).nodes);
+              return element;
+            });
+          }
+        });
+      } else {
+        elementFuture = new async.Future.microtask(() => contentPort.pullNodes());
+      }
+      TemplateLoader templateLoader = new TemplateLoader(elementFuture);
+
       Scope shadowScope = scope.createChild(new HashMap());
-      DirectiveInjector childInjector = new ShadowlessComponentDirectiveInjector(
-          injector, injector.appInjector, eventHandler, shadowScope, templateLoader,
-          new ShadowlessShadowRoot(element), contentPort);
+
+      childInjector = new ShadowlessComponentDirectiveInjector(injector, injector.appInjector,
+          eventHandler, shadowScope, templateLoader, new ShadowlessShadowRoot(element),
+          contentPort);
       childInjector.bindByKey(_ref.typeKey, _ref.factory, _ref.paramKeys, _ref.annotation.visibility);
 
+      if (childInjectorCompleter != null) {
+        childInjectorCompleter.complete(childInjector);
+      }
+
       var controller = childInjector.getByKey(_ref.typeKey);
-      shadowScope.context[_component.publishAs] = controller;
+      shadowScope.context[component.publishAs] = controller;
       if (controller is ScopeAware) controller.scope = shadowScope;
       BoundComponentFactory._setupOnShadowDomAttach(controller, templateLoader, shadowScope);
-
-      if (_viewFactoryFuture != null && _viewFactory == null) {
-        futures.add(_viewFactoryFuture.then((ViewFactory viewFactory) =>
-            _insert(viewFactory, element, childInjector, contentPort)));
-      } else {
-        scope.rootScope.runAsync(() {
-          _insert(_viewFactory, element, childInjector, contentPort);
-        });
-      }
       return controller;
     };
   }
-
-  _insert(ViewFactory viewFactory, dom.Element element, DirectiveInjector childInjector,
-          ContentPort contentPort) {
-    contentPort.pullNodes();
-    if (viewFactory != null) {
-      var viewNodes = viewFactory.call(childInjector.scope, childInjector).nodes;
-      for(var i = 0; i < viewNodes.length; i++) {
-        element.append(viewNodes[i]);
-      }
-    }
-  }
 }
diff --git a/lib/core_dom/view_factory.dart b/lib/core_dom/view_factory.dart
@@ -89,12 +89,10 @@ class ViewFactory implements Function {
     }
     elementInjectors[elementBinderIndex] = elementInjector;
 
-    var textBinders = tagged.textBinders;
-    if (textBinders != null && textBinders.length > 0) {
-      var childNodes = boundNode.childNodes;
-      for (var k = 0; k < textBinders.length; k++) {
-        TaggedTextBinder taggedText = textBinders[k];
-        var childNode = childNodes[taggedText.offsetIndex];
+    if (tagged.textBinders != null) {
+      for (var k = 0; k < tagged.textBinders.length; k++) {
+        TaggedTextBinder taggedText = tagged.textBinders[k];
+        var childNode = boundNode.childNodes[taggedText.offsetIndex];
         taggedText.binder.bind(view, scope, elementInjector, childNode, eventHandler, animate);
       }
     }
@@ -110,6 +108,15 @@ class ViewFactory implements Function {
       dom.Node node = nodeList[i];
       NodeLinkingInfo linkingInfo = nodeLinkingInfos[i];
 
+      // if node isn't attached to the DOM, create a parent for it.
+      var parentNode = node.parentNode;
+      var fakeParent = false;
+      if (parentNode == null) {
+        fakeParent = true;
+        parentNode = new dom.DivElement();
+        parentNode.append(node);
+      }
+
       if (linkingInfo.isElement) {
         if (linkingInfo.containsNgBinding) {
           var tagged = elementBinders[elementBinderIndex];
@@ -135,6 +142,11 @@ class ViewFactory implements Function {
         }
         elementBinderIndex++;
       }
+
+      if (fakeParent) {
+        // extract the node from the parentNode.
+        nodeList[i] = parentNode.nodes[0];
+      }
     }
     return view;
   }
diff --git a/lib/core_dom/web_platform.dart b/lib/core_dom/web_platform.dart
@@ -42,7 +42,7 @@ class WebPlatform {
       //
       // TODO Remove the try-catch once https://github.com/angular/angular.dart/issues/1189 is fixed.
       try {
-        root.querySelectorAll("*").forEach((dom.Element n) => n.setAttribute(selector, ""));
+        root.querySelectorAll("*").forEach((n) => n.attributes[selector] = "");
       } catch (e, s) {
         print("WARNING: Failed to set up Shadow DOM shim for $selector.\n$e\n$s");
       }
@@ -69,7 +69,6 @@ class PlatformViewCache implements ViewCache {
     if (selector != null && selector != "" && platform.shadowDomShimRequired) {
       // By adding a comment with the tag name we ensure the template html is unique per selector
       // name when used as a key in the view factory cache.
-      //TODO(misko): This will always be miss, since we never put it in cache under such key.
       viewFactory = viewFactoryCache.get("<!-- Shimmed template for: <$selector> -->$html");
     } else {
       viewFactory = viewFactoryCache.get(html);
diff --git a/lib/directive/ng_base_css.dart b/lib/directive/ng_base_css.dart
@@ -12,14 +12,10 @@ part of angular.directive;
     selector: '[ng-base-css]',
     visibility: Visibility.CHILDREN)
 class NgBaseCss {
-  List<dom.StyleElement> styles;
   List<String> _urls = const [];
 
   @NgAttr('ng-base-css')
-  set urls(v) {
-    _urls = v is List ? v : [v];
-    styles = null;
-  }
+  set urls(v) => _urls = v is List ? v : [v];
 
   List<String> get urls => _urls;
 }
diff --git a/lib/ng_tracing_scopes.dart b/lib/ng_tracing_scopes.dart
diff --git a/test/core_dom/compiler_spec.dart b/test/core_dom/compiler_spec.dart
diff --git a/test/directive/ng_base_css_spec.dart b/test/directive/ng_base_css_spec.dart
