code refactoring in import_inliner

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

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /** Transfomer that inlines polymer-element definitions from html imports. */
 library polymer.src.build.import_inliner;
 
 import 'dart:async';
 import 'dart:convert';
 
@@ skipping 20 matching lines @@
   final TransformOptions options;
 
   ImportInliner(this.options);
 
   /** Only run on entry point .html files. */
   Future<bool> isPrimary(Asset input) =>
       new Future.value(options.isHtmlEntryPoint(input.id));
 
   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;
-        }
+    var seen = new Set<AssetId>()..add(id);
+    var scriptIds = [];
+    var imported = new DocumentFragment();
+    Document document;
 
-        // 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 = [];
+    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.

 
-        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();
+      if (importsFound) {
+        document.body.insertBefore(imported, document.body.firstChild);
+        transform.addOutput(new Asset.fromString(id, document.outerHtml));
+      } else {
+        transform.addOutput(transform.primaryInput);
+      }
 
-              // 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);
-              }
-            }
-          }
+      // 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 :)

+    });
+  }
 
-          // TODO(jmesserly): should we merge the head too?
-          fragment.nodes.addAll(importedDoc.body.nodes);
-        }
+  /**
+   * 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, DocumentFragment imported,
+      List<AssetId> scripts) {
 
-        document.body.insertBefore(fragment, document.body.firstChild);
+    bool hasImports = false;
 
-        for (var tag in document.queryAll('link')) {
-          if (tag.attributes['rel'] == 'import') tag.remove();
-        }
+    // 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;
 
-        transform.addOutput(new Asset.fromString(id, document.outerHtml));
+      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;
 
-        var scriptIds = [];
-        for (var script in scripts) {
+      // 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);
+  }
+
+  /**
+   * 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.
+   */
+  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);
         }
-        transform.addOutput(new Asset.fromString(secondaryId,
-            JSON.encode(scriptIds, toEncodable: (id) => id.serialize())));
-      });
-    });
-  }
-
-  /**
-   * 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.
-   */
-  Future<bool> _visitImports(Document document, AssetId sourceId,
-      Transform transform, Set<AssetId> seen, List<Document> documents) {
-    var importIds = [];
-    bool hasImports = false;
-    for (var tag in document.queryAll('link')) {
-      if (tag.attributes['rel'] != 'import') continue;
-      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);
-  }
-
-  /**
-   * Loads an asset identified by [id], visits its imports and collects it's
-   * polymer-element definitions and script tags.
-   */
-  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);
-      });
-    });
   }
 }
 
+
 /** Internally adjusts urls in the html that we are about to inline. */
 class _UrlNormalizer extends TreeVisitor {
   final Transform transform;
 
   /** Asset where the original content (and original url) was found. */
   final AssetId sourceId;
 
   _UrlNormalizer(this.transform, this.sourceId);
 
   visitElement(Element node) {
@@ skipping 54 matching lines @@
   'cite',       // in blockquote, del, ins, q
   'data',       // in object
   'formaction', // in button, input
   'href',       // in a, area, link, base, command
   'icon',       // in command
   'manifest',   // in html
   'poster',     // in video
   'src',        // in audio, embed, iframe, img, input, script, source, track,
                 //    video
 ];