Make file-paths absolute before providing them to the CustomUrlResolver

Make file-paths absolute before providing them to the CustomUrlResolver

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/9d18982cd9a8237c425d41b35ec7740893e38d6a

[Siggi Cherem (dart-lang), 2016-04-27 23:46:02]

sigmund@google.com changed reviewers:
+ jmesserly@google.com

[Siggi Cherem (dart-lang), 2016-04-27 23:46:03]

From an early experiment, using the CustomUrlResolver seems to be good enough
for what we need in bazel - we just need to ensure the paths were absolute.

Paul - if there is a reason I should be using the ExplicitSourceResolver
instead, let me know and I'll expose it here instead. [I was just having a hard
time explaining the difference between `customUrlMapping` and
`explicitUrlMapping` when documenting the flags, and that really prompt me to
explore this direction more].

[Siggi Cherem (dart-lang), 2016-04-27 23:50:08]

FYI - exposing ExplicitSourceMapping is pretty simple too. I already have the
change available here: https://codereview.chromium.org/1928843002, I'd just like
to reduce our debt if it turns out not to be necessary.

[Jennifer Messerly, 2016-04-28 00:02:48]

LGTM :)

[Siggi Cherem (dart-lang), 2016-04-28 15:56:46]

thanks - after running the tests I saw a change in the language-all.js file,
mainly test code moved further down in the file. Is that expected and OK? All
test pass though.

[vsm, 2016-04-28 16:04:46]

On 2016/04/28 15:56:46, Siggi Cherem (dart-lang) wrote:
> thanks - after running the tests I saw a change in the language-all.js file,
> mainly test code moved further down in the file. Is that expected and OK? All
> test pass though.

Is the number of run tests the same?  Looks like some non-determinism.

LGTM - feel free to land

[Siggi Cherem (dart-lang), 2016-04-28 16:07:19]

On 2016/04/28 16:04:46, vsm wrote:
> On 2016/04/28 15:56:46, Siggi Cherem (dart-lang) wrote:
> > thanks - after running the tests I saw a change in the language-all.js file,
> > mainly test code moved further down in the file. Is that expected and OK?
All
> > test pass though.
> 
> Is the number of run tests the same?  Looks like some non-determinism.
> 
I believe so.

> LGTM - feel free to land

Thanks

[Siggi Cherem (dart-lang), 2016-04-28 16:07:33]

Description was changed from

==========
Make file-paths absolute before providing them to the CustomUrlResolver
==========

to

==========
Make file-paths absolute before providing them to the CustomUrlResolver

R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/9d18982cd9a8237c425d41b35ec7...
==========

[Siggi Cherem (dart-lang), 2016-04-28 16:07:34]

Committed patchset #2 (id:20001) manually as
9d18982cd9a8237c425d41b35ec7740893e38d6a (presubmit successful).

[Paul Berry, 2016-04-28 16:09:41]

On 2016/04/27 23:46:03, Siggi Cherem (dart-lang) wrote:
> From an early experiment, using the CustomUrlResolver seems to be good enough
> for what we need in bazel - we just need to ensure the paths were absolute.
> 
> Paul - if there is a reason I should be using the ExplicitSourceResolver
> instead, let me know and I'll expose it here instead. [I was just having a
hard
> time explaining the difference between `customUrlMapping` and
> `explicitUrlMapping` when documenting the flags, and that really prompt me to
> explore this direction more].

I see two minor reasons to prefer ExplicitSourceResolver over CustomUriResolver:

1. ExplicitSourceResolver contains a correct implementation of
`restoreAbsolute`; CustomUriResolver does not.  I don't know whether DDC will
run into trouble with this or not, but it might be better to be safe than sorry.

2. ExplicitSourceResolver returns NonExistingSource if it is asked for a source
file that does not exist, which is more consistent with the behavior of
FileUriResolver and therefore less likely to trigger bugs elsewhere in analyzer.
 Again, I don't know whether DDC will run into trouble with this or not, but it
might be better to be safe.

Your call, though.  lgtm either way.

[Jennifer Messerly, 2016-04-28 20:05:46]

On 2016/04/28 16:09:41, Paul Berry wrote:
> On 2016/04/27 23:46:03, Siggi Cherem (dart-lang) wrote:
> > From an early experiment, using the CustomUrlResolver seems to be good
enough
> > for what we need in bazel - we just need to ensure the paths were absolute.
> > 
> > Paul - if there is a reason I should be using the ExplicitSourceResolver
> > instead, let me know and I'll expose it here instead. [I was just having a
> hard
> > time explaining the difference between `customUrlMapping` and
> > `explicitUrlMapping` when documenting the flags, and that really prompt me
to
> > explore this direction more].
> 
> I see two minor reasons to prefer ExplicitSourceResolver over
CustomUriResolver:
> 
> 1. ExplicitSourceResolver contains a correct implementation of
> `restoreAbsolute`; CustomUriResolver does not.  I don't know whether DDC will
> run into trouble with this or not, but it might be better to be safe than
sorry.
> 
> 2. ExplicitSourceResolver returns NonExistingSource if it is asked for a
source
> file that does not exist, which is more consistent with the behavior of
> FileUriResolver and therefore less likely to trigger bugs elsewhere in
analyzer.
>  Again, I don't know whether DDC will run into trouble with this or not, but
it
> might be better to be safe.
> 
> Your call, though.  lgtm either way.

ExplicitSourceResolver sounds fine. Do you know why CustomUriResolver exists? I
don't even recall why we got it added in DDC, maybe something related to Flutter
"dart:sky"?

[Jennifer Messerly, 2016-04-28 20:06:09]

On 2016/04/28 20:05:46, John Messerly wrote:
> On 2016/04/28 16:09:41, Paul Berry wrote:
> > On 2016/04/27 23:46:03, Siggi Cherem (dart-lang) wrote:
> > > From an early experiment, using the CustomUrlResolver seems to be good
> enough
> > > for what we need in bazel - we just need to ensure the paths were
absolute.
> > > 
> > > Paul - if there is a reason I should be using the ExplicitSourceResolver
> > > instead, let me know and I'll expose it here instead. [I was just having a
> > hard
> > > time explaining the difference between `customUrlMapping` and
> > > `explicitUrlMapping` when documenting the flags, and that really prompt me
> to
> > > explore this direction more].
> > 
> > I see two minor reasons to prefer ExplicitSourceResolver over
> CustomUriResolver:
> > 
> > 1. ExplicitSourceResolver contains a correct implementation of
> > `restoreAbsolute`; CustomUriResolver does not.  I don't know whether DDC
will
> > run into trouble with this or not, but it might be better to be safe than
> sorry.
> > 
> > 2. ExplicitSourceResolver returns NonExistingSource if it is asked for a
> source
> > file that does not exist, which is more consistent with the behavior of
> > FileUriResolver and therefore less likely to trigger bugs elsewhere in
> analyzer.
> >  Again, I don't know whether DDC will run into trouble with this or not, but
> it
> > might be better to be safe.
> > 
> > Your call, though.  lgtm either way.
> 
> ExplicitSourceResolver sounds fine. Do you know why CustomUriResolver exists?
I
> don't even recall why we got it added in DDC, maybe something related to
Flutter
> "dart:sky"?

basically I don't want two almost identical things, one of which may or may not
be broken... :)