[polymer] fix import inliner to only rewrite URL attributes once per node

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 43 matching lines @@
           JSON.encode(scriptIds, toEncodable: (id) => id.serialize())));
     }));
   }
 
   /// Visits imports in [document] and add the imported documents to documents.
   /// Documents are added in the order they appear, transitive imports are added
   /// first.
   ///
   /// Returns `true` if and only if the document was changed and should be
   /// written out.
   Future<bool> _visitImports(Document document, AssetId sourceId) {

[Siggi Cherem (dart-lang), 2014/02/27 21:14:06]

seems like sourceId is no longer needed now?

[Jennifer Messerly, 2014/02/27 21:23:18]

Oops! It was removed, and I added it back while debugging for prints, forgot to
remove it again :)

Fixed.

     bool changed = false;
 
     _moveHeadToBody(document);
 
     // Note: we need to preserve the import order in the generated output.
     return Future.forEach(document.querySelectorAll('link'), (Element tag) {
       var rel = tag.attributes['rel'];
       if (rel != 'import' && rel != 'stylesheet') return null;
 
+      // Note: URL has already been normalized so use docId.
       var href = tag.attributes['href'];
-      var id = resolve(sourceId, href, transform.logger, tag.sourceSpan,
+      var id = resolve(docId, href, transform.logger, tag.sourceSpan,
           allowAbsolute: rel == 'stylesheet');
 
       if (rel == 'import') {
         changed = true;
         if (id == null || !seen.add(id)) {
           tag.remove();
           return null;
         }
         return _inlineImport(id, tag);
 
@@ skipping 14 matching lines @@
   /// respect to eachother, because stylesheets can be pulled in transitively
   /// from imports.
   // TODO(jmesserly): vulcanizer doesn't need this because they inline JS
   // scripts, causing them to be naturally moved as part of the inlining.
   // Should we do the same? Alternatively could we inline head into head and
   // body into body and avoid this whole thing?
   void _moveHeadToBody(Document doc) {
     var insertionPoint = doc.body.firstChild;
     for (var node in doc.head.nodes.toList(growable: false)) {
       if (node is! Element) continue;
-      var tag = node.tagName;
+      var tag = node.localName;
       var type = node.attributes['type'];
       var rel = node.attributes['rel'];
       if (tag == 'style' || tag == 'script' &&
             (type == null || type == TYPE_JS || type == TYPE_DART) ||
           tag == 'link' && (rel == 'stylesheet' || rel == 'import')) {
         // Move the node into the body, where its contents will be placed.
         doc.body.insertBefore(node, insertionPoint);
       }
     }
   }
 
   // Loads an asset identified by [id], visits its imports and collects its
   // html imports. Then inlines it into the main document.
-  Future _inlineImport(AssetId id, Element link) =>
-      readAsHtml(id, transform).then((doc) => _visitImports(doc, id).then((_) {
+  Future _inlineImport(AssetId id, Element link) {
+    return readAsHtml(id, transform).then((doc) {
 

[Siggi Cherem (dart-lang), 2014/02/27 21:14:06]

nit: up to you, but it feels like this empty line is not necessary

[Jennifer Messerly, 2014/02/27 21:23:18]

yeah, personally I thought it was more readable with the newline, but that might
be a result of viewing it in the Editor which has colors .. "return new return
_extract" was somewhat hard to parse.

but anyway removed newlines.

-    new _UrlNormalizer(transform, id).visit(doc);
-    _extractScripts(doc);
+      new _UrlNormalizer(transform, id).visit(doc);

[Siggi Cherem (dart-lang), 2014/02/27 21:14:06]

I'm a bit confused. This is still applied on every call to _inlineImport, so
technically we are normalizing documents more than once? It probably happens
that subsequent times are not changing urls, but we are still traversing the
doc?

[Siggi Cherem (dart-lang), 2014/02/27 21:19:01]

just talked offline, I missed that the swap in order here fixes changes the
recursion logic. I can see now that after the change you first process the file,
then recurse (vs first recurse, then process the file).

+      return _visitImports(doc, id).then((_) {
 

[Siggi Cherem (dart-lang), 2014/02/27 21:14:06]

same here

-    // TODO(jmesserly): figure out how this is working in vulcanizer.
-    // Do they produce a <body> tag with a <head> and <body> inside?
-    var imported = new DocumentFragment();
-    imported.nodes..addAll(doc.head.nodes)..addAll(doc.body.nodes);
-    link.replaceWith(imported);
-  }));
+        _extractScripts(doc);
+
+        // TODO(jmesserly): figure out how this is working in vulcanizer.
+        // Do they produce a <body> tag with a <head> and <body> inside?
+        var imported = new DocumentFragment();
+        imported.nodes..addAll(doc.head.nodes)..addAll(doc.body.nodes);
+        link.replaceWith(imported);
+      });
+    });
+  }
 
   Future _inlineStylesheet(AssetId id, Element link) {
     return transform.readInputAsString(id).then((css) {
       var url = spanUrlFor(id, transform);
       css = new _UrlNormalizer(transform, id).visitCss(css, url);
       link.replaceWith(new Element.tag('style')..text = css);
     });
   }
 
   /// Split Dart script tags from all the other elements. Now that Dartium
@@ skipping 136 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
 ];