Strip bindings before calling Uri.parse

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 be0410c532526798484eb1001d0ef71032375de1..47a0f3941e197bb95edc456b4433c3876f74e616 100644
--- a/pkg/polymer/lib/src/build/import_inliner.dart
+++ b/pkg/polymer/lib/src/build/import_inliner.dart
@@ -268,8 +268,8 @@ String _libraryNameFor(AssetId id, int suffix) {
       '${path.extension(id.path).substring(1)}';
   if (name.startsWith('lib/')) name = name.substring(4);
   name = name.split('/').map((part) {
-    part = part.replaceAll(INVALID_LIB_CHARS_REGEX, '_');
-    if (part.startsWith(NUM_REGEX)) part = '_${part}';
+    part = part.replaceAll(_INVALID_LIB_CHARS_REGEX, '_');
+    if (part.startsWith(_NUM_REGEX)) part = '_${part}';
     return part;
   }).join(".");
   return '${id.package}.${name}_$suffix';
@@ -343,19 +343,19 @@ class _UrlNormalizer extends TreeVisitor {
     if (!isCustomTagName(node.localName)) {
       node.attributes.forEach((name, value) {
         if (_urlAttributes.contains(name)) {
-          if (!name.startsWith('_') && value.contains(_BINDINGS)) {
+          if (!name.startsWith('_') && value.contains(_BINDING_REGEX)) {
             transform.logger.warning(
                 'When using bindings with the "$name" attribute you may '
                 'experience errors in certain browsers. Please use the '
                 '"_$name" attribute instead. For more information, see '
                 'http://goo.gl/5av8cU', span: node.sourceSpan, asset: sourceId);
-          } else if (name.startsWith('_') && !value.contains(_BINDINGS)) {
+          } else if (name.startsWith('_') && !value.contains(_BINDING_REGEX)) {
             transform.logger.warning(
                 'The "$name" attribute is only supported when using bindings. '
                 'Please change to the "${name.substring(1)}" attribute.',
                 span: node.sourceSpan, asset: sourceId);
           }
-          if (value != '' && !value.trim().startsWith(_BINDINGS)) {
+          if (value != '' && !value.trim().startsWith(_BINDING_REGEX)) {
             node.attributes[name] = _newUrl(value, node.sourceSpan);
             changed = changed || value != node.attributes[name];
           }
@@ -378,7 +378,6 @@ class _UrlNormalizer extends TreeVisitor {
 
   static final _URL = new RegExp(r'url\(([^)]*)\)', multiLine: true);
   static final _QUOTE = new RegExp('["\']', multiLine: true);
-  static final _BINDINGS = new RegExp(r'({{)|(\[\[)');
 
   /// Visit the CSS text and replace any relative URLs so we can inline it.
   // Ported from:
@@ -435,26 +434,42 @@ class _UrlNormalizer extends TreeVisitor {
   }
 
   String _newUrl(String href, SourceSpan span) {
-    // Uri.parse blows up on invalid characters (like {{). Encoding the uri
-    // allows it to be parsed, which does the correct thing in the general case.
-    // This uri not used to build the new uri, so it never needs to be decoded.
-    var uri = Uri.parse(Uri.encodeFull(href));
+    // We only want to parse the part of the href leading up to the first
+    // binding, anything after that is not informative.
+    var hrefToParse;
+    var appendToHref = '';
+    var firstBinding = href.indexOf(_BINDING_REGEX);
+    if (firstBinding == -1) {
+      hrefToParse = href;
+    } else if (firstBinding == 0) {
+      return href;
+    } else {
+      hrefToParse = '${href.substring(0, firstBinding)}$_BINDING_PLACEHOLDER';
+      appendToHref = href.substring(firstBinding);

[Siggi Cherem (dart-lang), 2014/08/01 22:32:32]

nit: since so many cases don't reach line 464, maybe we should just do this
operation when we are about to create newPath?

[jakemac, 2014/08/04 14:33:15]

Done.

+    }
+
+    var uri = Uri.parse(hrefToParse);
     if (uri.isAbsolute) return href;
     if (!uri.scheme.isEmpty) return href;
     if (!uri.host.isEmpty) return href;
     if (uri.path.isEmpty) return href;  // Implies standalone ? or # in URI.
     if (path.isAbsolute(href)) return href;
 
-    var id = uriToAssetId(sourceId, href, transform.logger, span);
+    var id = uriToAssetId(sourceId, hrefToParse, transform.logger, span);
     if (id == null) return href;
     var primaryId = transform.primaryInput.id;
 
-    if (id.path.startsWith('lib/')) {
-      return '${topLevelPath}packages/${id.package}/${id.path.substring(4)}';
+    // Put the original suffix back onto the path now that we have parsed it,
+    // and remove the placeholder.
+    var newPath = '${id.path}${appendToHref}'
+        .replaceFirst(_BINDING_PLACEHOLDER, '');

[Siggi Cherem (dart-lang), 2014/08/01 22:32:32]

nit: it's probably slightly cheaper to do the replace first, or use substring
(since we know that we are removing chars at the end of id.path

One last idea, I promise :) - now that the placeholder is only used once at the
end of the path, I wonder if we could just use a simple character like '_' and
do:

var prefix = id.path.substring(0, id.path.length - 1);
var suffix = href.substring(firstBinding);
var newPath = '$prefix$suffix';

[jakemac, 2014/08/04 14:33:15]

I changed it to a version of this, the issue is that I need to do different
things based on whether or not there was a binding found (and replaced) earlier
on so its not quite this straightforward, by using replaceFirst I was able to
ignore this but its true that it was a bit more expensive.

On 2014/08/01 22:32:32, Siggi Cherem (dart-lang) wrote:

+
+    if (newPath.startsWith('lib/')) {
+      return '${topLevelPath}packages/${id.package}/${newPath.substring(4)}';
     }
 
-    if (id.path.startsWith('asset/')) {
-      return '${topLevelPath}assets/${id.package}/${id.path.substring(6)}';
+    if (newPath.startsWith('asset/')) {
+      return '${topLevelPath}assets/${id.package}/${newPath.substring(6)}';
     }
 
     if (primaryId.package != id.package) {
@@ -465,7 +480,7 @@ class _UrlNormalizer extends TreeVisitor {
     }
 
     var builder = path.url;
-    return builder.relative(builder.join('/', id.path),
+    return builder.relative(builder.join('/', newPath),
         from: builder.join('/', builder.dirname(primaryId.path)));
   }
 }
@@ -496,8 +511,12 @@ const _urlAttributes = const [
 const IGNORED_LINKED_STYLE_ATTRS =
     const ['charset', 'href', 'href-lang', 'rel', 'rev'];
 
-/// Global RegExp objects for validating generated library names.
-final INVALID_LIB_CHARS_REGEX = new RegExp('[^a-z0-9_]');
-final NUM_REGEX = new RegExp('[0-9]');
+/// Global RegExp objects.
+final _INVALID_LIB_CHARS_REGEX = new RegExp('[^a-z0-9_]');
+final _NUM_REGEX = new RegExp('[0-9]');
+final _BINDING_REGEX = new RegExp(r'(({{.*}})|(\[\[.*\]\]))');
+
+/// Placeholder for bindings in urls for [Uri.parse].
+final _BINDING_PLACEHOLDER = '%BINDING%';
 
 _getSpan(SourceFile file, AstNode node) => file.span(node.offset, node.end);