code refactoring in import_inliner

pkg/polymer/lib/src/build/import_inliner.dart

Index: pkg/polymer/lib/src/build/import_inliner.dart
diff --git a/pkg/polymer/lib/src/build/import_inliner.dart b/pkg/polymer/lib/src/build/import_inliner.dart
index a66d1bbc84176faefcc440d0c9f6772617128ed3..285d8ada9497ac1dd10e6389615e73e3bf138392 100644
--- a/pkg/polymer/lib/src/build/import_inliner.dart
+++ b/pkg/polymer/lib/src/build/import_inliner.dart
@@ -38,120 +38,110 @@ class ImportInliner extends Transformer with PolymerTransformer {
 
   Future apply(Transform transform) {
     var logger = transform.logger;
-    var seen = new Set<AssetId>();
-    var documents = [];
     var id = transform.primaryInput.id;
-    seen.add(id);
-    return readPrimaryAsHtml(transform).then((document) {
-      var future = _visitImports(document, id, transform, seen, documents);
-      return future.then((importsFound) {
-        // We produce a secondary asset with extra information for later phases.
-        var secondaryId = id.addExtension('.scriptUrls');
-        if (!importsFound) {
-          transform.addOutput(transform.primaryInput);
-          transform.addOutput(new Asset.fromString(secondaryId, '[]'));
-          return;
-        }
-
-        // Split Dart script tags from all the other elements. Now that Dartium
-        // only allows a single script tag per page, we can't inline script
-        // tags. Instead, we collect the urls of each script tag so we import
-        // them directly from the Dart bootstrap code.
-        var scripts = [];
-
-        var fragment = new DocumentFragment();
-        for (var importedDoc in documents) {
-          bool first = true;
-          for (var e in importedDoc.queryAll('script')) {
-            if (e.attributes['type'] == 'application/dart') {
-              e.remove();
-
-              // only one Dart script per document is supported in Dartium.
-              if (first) {
-                first = false;
-                scripts.add(e);
-              } else {
-                // TODO(jmesserly): remove this when we are running linter.
-                logger.warning('more than one Dart script per HTML document is '
-                    'not supported. Script will be ignored.',
-                    span: e.sourceSpan);
-              }
-            }
-          }
-
-          // TODO(jmesserly): should we merge the head too?
-          fragment.nodes.addAll(importedDoc.body.nodes);
-        }
-
-        document.body.insertBefore(fragment, document.body.firstChild);
-
-        for (var tag in document.queryAll('link')) {
-          if (tag.attributes['rel'] == 'import') tag.remove();
-        }
-
+    var seen = new Set<AssetId>()..add(id);
+    var scriptIds = [];
+    var imported = new DocumentFragment();
+    Document document;
+
+    return readPrimaryAsHtml(transform).then((doc) {
+      document = doc;
+      return _visitImports(doc, id, transform, seen, imported, scriptIds);

[Jennifer Messerly, 2014/02/12 00:48:47]

visit imports now produces the two primary outputs directly -- the imported
content and the script IDs.

(I tried to do the content insertion inside of _visitImports too, but that
didn't work out since it's tricky to keep a pointer for where to insert into the
document. However link rel=stylesheets in the body will likely work this way.)

+    }).then((importsFound) {

[Jennifer Messerly, 2014/02/12 00:48:47]

switched to this style to reduce nesting caused by chained futures.

+
+      if (importsFound) {
+        document.body.insertBefore(imported, document.body.firstChild);
         transform.addOutput(new Asset.fromString(id, document.outerHtml));
+      } else {
+        transform.addOutput(transform.primaryInput);
+      }
 
-        var scriptIds = [];
-        for (var script in scripts) {
-          var src = script.attributes['src'];
-          if (src == null) {
-            logger.warning('unexpected script without a src url. The '
-              'ImportInliner transformer should run after running the '
-              'InlineCodeExtractor', span: script.sourceSpan);
-            continue;
-          }
-          scriptIds.add(resolve(id, src, logger, script.sourceSpan));
-        }
-        transform.addOutput(new Asset.fromString(secondaryId,
+      // We produce a secondary asset with extra information for later phases.
+      transform.addOutput(new Asset.fromString(id.addExtension('.scriptUrls'),
             JSON.encode(scriptIds, toEncodable: (id) => id.serialize())));

[Jennifer Messerly, 2014/02/12 00:48:47]

an extra JSON.encode in the !importsFound case is probably fine, given all of
the work this code is doing :)

-      });
     });
   }
 
   /**
-   * Visits imports in [document] and add their polymer-element and script tags
-   * to [elements], unless they have already been [seen]. Elements are added in
-   * the order they appear, transitive imports are added first.
+   * Visits imports in [document] and add the imported documents to [documents].
+   * Documents are added in the order they appear, transitive imports are added
+   * first.
    */
   Future<bool> _visitImports(Document document, AssetId sourceId,
-      Transform transform, Set<AssetId> seen, List<Document> documents) {
-    var importIds = [];
+      Transform transform, Set<AssetId> seen, DocumentFragment imported,
+      List<AssetId> scripts) {
+
     bool hasImports = false;
-    for (var tag in document.queryAll('link')) {
-      if (tag.attributes['rel'] != 'import') continue;
+
+    // Note: we need to preserve the import order in the generated output.
+    return Future.forEach(document.queryAll('link'), (tag) {

[Jennifer Messerly, 2014/02/12 00:48:47]

instead of two back to back for loops, it now just has one

+      if (tag.attributes['rel'] != 'import') return null;
       var href = tag.attributes['href'];
       var id = resolve(sourceId, href, transform.logger, tag.sourceSpan);
       hasImports = true;
-      if (id == null || seen.contains(id) ||
-         (id.package == 'polymer' && id.path == 'lib/init.html')) continue;
-      importIds.add(id);
-    }
-
-    if (importIds.isEmpty) return new Future.value(hasImports);
 
-    // Note: we need to preserve the import order in the generated output.
-    return Future.forEach(importIds, (id) {
-      if (seen.contains(id)) return new Future.value(null);
-      seen.add(id);
-      return _collectImportedDocuments(id, transform, seen, documents);
-    }).then((_) => true);
+      tag.remove();

[Jennifer Messerly, 2014/02/12 00:48:47]

remove the tag here, rather than another pass later

+      if (id == null || !seen.add(id) ||

[Jennifer Messerly, 2014/02/12 00:48:47]

Set.add now returns a boolean (true if it was added)

+         (id.package == 'polymer' && id.path == 'lib/init.html')) return null;
+
+      // Loads an asset identified by [id], visits its imports and collects its
+      // html imports. Then inlines it into the main document.
+      Document importedDoc;
+      return readAsHtml(id, transform).then((doc) {
+        // Visit transitive imports first.
+        importedDoc = doc;
+        return _visitImports(doc, id, transform, seen, imported, scripts);
+      }).then((_) {
+        new _UrlNormalizer(transform, id).visit(importedDoc);
+
+        _extractScripts(id, transform.logger, importedDoc, scripts);

[Jennifer Messerly, 2014/02/12 00:48:47]

FYI -- I'm changing this back to sourceId, not imported doc's ID, since we
already ran UrlNormalizer. Not sure if it matters (the tests work either way)

+
+        // TODO(jmesserly): figure out how this is working in vulcanizer.
+        // Do they produce a <body> tag with a <head> and <body> inside?
+        imported.nodes
+            ..addAll(importedDoc.head.nodes)
+            ..addAll(importedDoc.body.nodes);
+      });
+    }).then((_) => hasImports);
   }
 
   /**
-   * Loads an asset identified by [id], visits its imports and collects it's
-   * polymer-element definitions and script tags.
+   * Split Dart script tags from all the other elements. Now that Dartium
+   * only allows a single script tag per page, we can't inline script
+   * tags. Instead, we collect the urls of each script tag so we import
+   * them directly from the Dart bootstrap code.
    */
-  Future _collectImportedDocuments(AssetId id, Transform transform,
-      Set<AssetId> seen, List documents) {
-    return readAsHtml(id, transform).then((document) {
-      return _visitImports(document, id, transform, seen, documents).then((_) {
-        new _UrlNormalizer(transform, id).visit(document);
-        documents.add(document);
-      });
-    });
+  static void _extractScripts(id, logger, document, List scriptIds) {

[Jennifer Messerly, 2014/02/12 00:48:47]

this function seemed pretty self contained

+    bool first = true;
+    for (var script in document.queryAll('script')) {
+      if (script.attributes['type'] == 'application/dart') {
+        script.remove();
+
+        // only one Dart script per document is supported in Dartium.
+        if (first) {
+          first = false;
+
+          var src = script.attributes['src'];
+          if (src == null) {
+            logger.warning('unexpected script without a src url. The '
+              'ImportInliner transformer should run after running the '
+              'InlineCodeExtractor', span: script.sourceSpan);
+            continue;
+          }
+          scriptIds.add(resolve(id, src, logger, script.sourceSpan));
+
+        } else {
+          // TODO(jmesserly): remove this when we are running linter.
+          logger.warning('more than one Dart script per HTML '
+              'document is not supported. Script will be ignored.',
+              span: script.sourceSpan);
+        }
+      }
+    }
   }
 }
 
+
 /** Internally adjusts urls in the html that we are about to inline. */
 class _UrlNormalizer extends TreeVisitor {
   final Transform transform;