Strip bindings before calling Uri.parse

Strip bindings before calling Uri.parse


BUG=http://dartbug.com/20286
R=sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38878

[jakemac, 2014-07-31 15:06:37]

https://codereview.chromium.org/433913002/diff/1/pkg/polymer/test/build/impor...
File pkg/polymer/test/build/import_inliner_test.dart (left):

https://codereview.chromium.org/433913002/diff/1/pkg/polymer/test/build/impor...
pkg/polymer/test/build/import_inliner_test.dart:176:
'a|web/*test_%foo_04!.html':
Barback started complaining about paths that don't start with a letter. This
must have been introduced in a newer version? Not sure why its not breaking the
bots....

[Siggi Cherem (dart-lang), 2014-07-31 21:54:22]

https://codereview.chromium.org/433913002/diff/1/pkg/polymer/test/build/impor...
File pkg/polymer/test/build/import_inliner_test.dart (left):

https://codereview.chromium.org/433913002/diff/1/pkg/polymer/test/build/impor...
pkg/polymer/test/build/import_inliner_test.dart:176:
'a|web/*test_%foo_04!.html':
On 2014/07/31 15:06:37, jakemac wrote:
> Barback started complaining about paths that don't start with a letter. This
> must have been introduced in a newer version? Not sure why its not breaking
the
> bots....

Weird, I'm surprised it didn't fail in the bots too :-/ ... did it fail in this
test always, or only if you try to listen for warnings?

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:422: href =
href.replaceAll(_BINDING_REGEX, _BINDING_PLACEHOLDER);
I wonder if we need to replace all, or if we should just find the first one. At
that point, we might be OK just splitting it and don't need a placeholder. For
example:

a/b/{{e}}/f/{{g}}/q.dart

->

prefix = a/b/
suffix = {{e}}/f/{{g}}/q.dart

Then later newPath = '${id.path}$suffix'

WDYT?

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:425: if (uri.isAbsolute) return
href;
shouldn't this return the original href without the placeholders?
(same below on most of the return statements)

[jakemac, 2014-08-01 15:20:47]

https://codereview.chromium.org/433913002/diff/1/pkg/polymer/test/build/impor...
File pkg/polymer/test/build/import_inliner_test.dart (left):

https://codereview.chromium.org/433913002/diff/1/pkg/polymer/test/build/impor...
pkg/polymer/test/build/import_inliner_test.dart:176:
'a|web/*test_%foo_04!.html':
Oddly enough this just stopped happening. I have reverted the changes... but it
seems like it might be flaky.

On 2014/07/31 21:54:22, Siggi Cherem (dart-lang) wrote:
> On 2014/07/31 15:06:37, jakemac wrote:
> > Barback started complaining about paths that don't start with a letter. This
> > must have been introduced in a newer version? Not sure why its not breaking
> the
> > bots....
> 
> Weird, I'm surprised it didn't fail in the bots too :-/ ... did it fail in
this
> test always, or only if you try to listen for warnings?

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:422: href =
href.replaceAll(_BINDING_REGEX, _BINDING_PLACEHOLDER);
This seems like a good idea, but I tried it out and there ends up being a lot of
weird edge cases. For instance, a path of just './' turns into '../web'. Also,
the trailing slashes always get removed, so if I just tack on other things I
have to add it back (but only if it had one before). Covering all of these cases
is going to overall make things more complicated unfortunately and I would
likely miss a few.

On 2014/07/31 21:54:22, Siggi Cherem (dart-lang) wrote:
> I wonder if we need to replace all, or if we should just find the first one.
At
> that point, we might be OK just splitting it and don't need a placeholder. For
> example:
> 
> a/b/{{e}}/f/{{g}}/q.dart
> 
> ->
> 
> prefix = a/b/
> suffix = {{e}}/f/{{g}}/q.dart
> 
> Then later newPath = '${id.path}$suffix'
> 
> WDYT?

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:425: if (uri.isAbsolute) return
href;
Good catch, I have updated it so I don't write over the original href varibale
any more, instead I am making a new one called uriSafeHref.

On 2014/07/31 21:54:22, Siggi Cherem (dart-lang) wrote:
> shouldn't this return the original href without the placeholders?
> (same below on most of the return statements)

[Siggi Cherem (dart-lang), 2014-08-01 16:02:51]

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:422: href =
href.replaceAll(_BINDING_REGEX, _BINDING_PLACEHOLDER);
On 2014/08/01 15:20:47, jakemac wrote:
> This seems like a good idea, but I tried it out and there ends up being a lot
of
> weird edge cases. For instance, a path of just './' turns into '../web'. Also,
> the trailing slashes always get removed, so if I just tack on other things I
> have to add it back (but only if it had one before). Covering all of these
cases
> is going to overall make things more complicated unfortunately and I would
> likely miss a few.
> 
> On 2014/07/31 21:54:22, Siggi Cherem (dart-lang) wrote:
> > I wonder if we need to replace all, or if we should just find the first one.
> At
> > that point, we might be OK just splitting it and don't need a placeholder.
For
> > example:
> > 
> > a/b/{{e}}/f/{{g}}/q.dart
> > 
> > ->
> > 
> > prefix = a/b/
> > suffix = {{e}}/f/{{g}}/q.dart
> > 
> > Then later newPath = '${id.path}$suffix'
> > 
> > WDYT?
> 

Interesting, here are a couple other ideas... it might be worth giving them a
shot.

You can do like you are doing with adding a %%BINDING%% but just for the first
one:


a/b/{{e}}/f/{{g}}/q.dart

->
prefix = a/b/%%BINDING%%
suffix = {{e}}/f/{{g}}/q.dart

what do you think?


If that doesn't work, I think we can still get rid of the loop that restores the
bindings (which would get rid of the buildingMatches) by just replacing the
suffix in the resulting path starting from the first binding position.

var uriSafeHref = href.replaceAll(_BINDING_REGEX, _BINDING_PLACEHOLDER);
...
var newPath = id.path;
var bindingIndex = newPath.indexOf(_BINDING_PLACEHOLDER);
if (bindingIndex >= 0) {
  var prefix = newPath.substring(0, bindingIndex);
  var suffix = href.substring(href.indexOf(_BINDING_REGEX));
  var newPath = '$prefix$suffix';
}

[jakemac, 2014-08-01 22:12:36]

updated

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:422: href =
href.replaceAll(_BINDING_REGEX, _BINDING_PLACEHOLDER);
On 2014/08/01 16:02:51, Siggi Cherem (dart-lang) wrote:
> On 2014/08/01 15:20:47, jakemac wrote:
> > This seems like a good idea, but I tried it out and there ends up being a
lot
> of
> > weird edge cases. For instance, a path of just './' turns into '../web'.
Also,
> > the trailing slashes always get removed, so if I just tack on other things I
> > have to add it back (but only if it had one before). Covering all of these
> cases
> > is going to overall make things more complicated unfortunately and I would
> > likely miss a few.
> > 
> > On 2014/07/31 21:54:22, Siggi Cherem (dart-lang) wrote:
> > > I wonder if we need to replace all, or if we should just find the first
one.
> > At
> > > that point, we might be OK just splitting it and don't need a placeholder.
> For
> > > example:
> > > 
> > > a/b/{{e}}/f/{{g}}/q.dart
> > > 
> > > ->
> > > 
> > > prefix = a/b/
> > > suffix = {{e}}/f/{{g}}/q.dart
> > > 
> > > Then later newPath = '${id.path}$suffix'
> > > 
> > > WDYT?
> > 
> 
> Interesting, here are a couple other ideas... it might be worth giving them a
> shot.
> 
> You can do like you are doing with adding a %%BINDING%% but just for the first
> one:
> 
> 
> a/b/{{e}}/f/{{g}}/q.dart
> 
> ->
> prefix = a/b/%%BINDING%%
> suffix = {{e}}/f/{{g}}/q.dart
> 
> what do you think?
> 
> 
> If that doesn't work, I think we can still get rid of the loop that restores
the
> bindings (which would get rid of the buildingMatches) by just replacing the
> suffix in the resulting path starting from the first binding position.
> 
> var uriSafeHref = href.replaceAll(_BINDING_REGEX, _BINDING_PLACEHOLDER);
> ...
> var newPath = id.path;
> var bindingIndex = newPath.indexOf(_BINDING_PLACEHOLDER);
> if (bindingIndex >= 0) {
>   var prefix = newPath.substring(0, bindingIndex);
>   var suffix = href.substring(href.indexOf(_BINDING_REGEX));
>   var newPath = '$prefix$suffix';
> }

Done.

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

awesome, thanks! lgtm, just some minor suggestions below.

https://codereview.chromium.org/433913002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:448: appendToHref =
href.substring(firstBinding);
nit: since so many cases don't reach line 464, maybe we should just do this
operation when we are about to create newPath?

https://codereview.chromium.org/433913002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:465:
.replaceFirst(_BINDING_PLACEHOLDER, '');
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]

https://codereview.chromium.org/433913002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:448: appendToHref =
href.substring(firstBinding);
On 2014/08/01 22:32:32, Siggi Cherem (dart-lang) wrote:
> nit: since so many cases don't reach line 464, maybe we should just do this
> operation when we are about to create newPath?

Done.

https://codereview.chromium.org/433913002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:465:
.replaceFirst(_BINDING_PLACEHOLDER, '');
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:
> 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';

[Siggi Cherem (dart-lang), 2014-08-04 19:39:38]

lgtm

really minor minor nits below

https://codereview.chromium.org/433913002/diff/70001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/70001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:461: var prefix = (firstBinding <
1) ? id.path
nit: since we return automatically with == 0 upfront, I am debating whether it
reads slightly better to use `== -1` here (as normally it means there were no
matches).

https://codereview.chromium.org/433913002/diff/70001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:519: final _BINDING_PLACEHOLDER =
'_';
nit: maybe rename or rephrase the comment a big ("placeholder or the first
binding.. " ?)

alternatively, since we use this only in_newUrl, we could move the definition up
to the method and make it a const local variable instead, e.g.:

   const placeholder = '_';

[jakemac, 2014-08-04 19:55:01]

https://codereview.chromium.org/433913002/diff/70001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/433913002/diff/70001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:461: var prefix = (firstBinding <
1) ? id.path
On 2014/08/04 19:39:38, Siggi Cherem (dart-lang) wrote:
> nit: since we return automatically with == 0 upfront, I am debating whether it
> reads slightly better to use `== -1` here (as normally it means there were no
> matches).

Done, I think it is a bit better

https://codereview.chromium.org/433913002/diff/70001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:519: final _BINDING_PLACEHOLDER =
'_';
On 2014/08/04 19:39:38, Siggi Cherem (dart-lang) wrote:
> nit: maybe rename or rephrase the comment a big ("placeholder or the first
> binding.. " ?)
> 
> alternatively, since we use this only in_newUrl, we could move the definition
up
> to the method and make it a const local variable instead, e.g.:
> 
>    const placeholder = '_';

Done, went with the const local

[Siggi Cherem (dart-lang), 2014-08-04 20:20:34]

lgtm! :)

[jakemac, 2014-08-04 20:50:01]

Committed patchset #6 manually as 38878 (presubmit successful).