Add implementation of Isolate.resolvePackageUri for dart2js.

Add implementation of Isolate.resolvePackageUri for dart2js.

This uses 'packages/' as the default base to resolve uris from, but also exposes
a hook to allow users to overwrite it.

BUG= https://github.com/dart-lang/sdk/issues/25594
R=het@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/f460a8706d116515a921334f8bbe09542285eb77

[Siggi Cherem (dart-lang), 2016-11-14 19:35:55]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-11-14 19:36:28]

Description was changed from

==========
Add implementation of Isolate.resolvePackageUri for dart2js.

This uses 'packages/' as the default base to resolve uris from, but also exposes
a hook to allow users to overwrite it.

BUG=
==========

to

==========
Add implementation of Isolate.resolvePackageUri for dart2js.

This uses 'packages/' as the default base to resolve uris from, but also exposes
a hook to allow users to overwrite it.

BUG= https://github.com/dart-lang/sdk/issues/25594
==========

[Siggi Cherem (dart-lang), 2016-11-14 19:36:48]

sigmund@google.com changed reviewers:
+ het@google.com, lrn@google.com

[Lasse Reichstein Nielsen, 2016-11-14 20:42:35]

lgtm

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/isolate_helper.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/isolate_helper.dart:756: /// This is indirectly
used by `package:resource` to load resources. The
Maybe refer to Isolate.resolvePackageUri, which is the direct user of this
property, instead of package:resource which just uses Isolate.resolvePackageUri.

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:37: return
Uri.base.resolve('$base/${packageUri.path}');
There is no validation of the packageUri.
If the argument isn't a package: URI, it must be returned unchanged
So, start with:
  if (packageUri.scheme != "package") return packageUri;

For the resolution, I think it will work, but it would be better if you could
retain any fragment. It makes a kind of sense to be able to do:
Isolate.resolvePackageUri(Uri.parse("package:foo/asset/foo.zip#file.ext"))
(or something similar).

Maybe:
  return Uri.base.resolve(
      '$base/${packageUri.toString().subString("package:".length)}');

(I'm sure that can be made more efficient, but it's a good start).

That said, I'm not sure the VM does that either.

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
File tests/isolate/browser/package_resolve_browser_hook_test.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
tests/isolate/browser/package_resolve_browser_hook_test.dart:8: final
PACKAGE_URI = "package:foo/bar.txt";
Nitpick: Style guide says to use camelCase for constants. :)

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
tests/isolate/browser/package_resolve_browser_hook_test.dart:10: main() async {
Are you sure an async main works with the test runner? I've had problems with
that in some cases (but I never figured out exactly which cases)

[Lasse Reichstein Nielsen, 2016-11-14 20:46:23]

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:37: return
Uri.base.resolve('$base/${packageUri.path}');
Actually, it can probably just be:
  Uri.base.resolve("$base/").resolveUri(packageUri.replace(scheme: ""))
(and then it's probably worth it to cache Uri.base.resolve("$base/") instead of
computing it each time).

[Harry Terkelsen, 2016-11-14 22:27:24]

lgtm

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
File tests/isolate/browser/package_resolve_browser_hook_test.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
tests/isolate/browser/package_resolve_browser_hook_test.dart:12: expect(uri,
Uri.base.resolve('path_set_from_hook/foo/bar.txt'));
can we also test if the hook is set to a path with slashes in it?

[Siggi Cherem (dart-lang), 2016-11-14 23:06:28]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2016-11-14 23:07:15]

thanks Lasse and Harry!

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/isolate_helper.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/isolate_helper.dart:756: /// This is indirectly
used by `package:resource` to load resources. The
On 2016/11/14 20:42:34, Lasse Reichstein Nielsen wrote:
> Maybe refer to Isolate.resolvePackageUri, which is the direct user of this
> property, instead of package:resource which just uses
Isolate.resolvePackageUri.

Done.

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
File sdk/lib/_internal/js_runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:37: return
Uri.base.resolve('$base/${packageUri.path}');
On 2016/11/14 20:42:34, Lasse Reichstein Nielsen wrote:
> There is no validation of the packageUri.
> If the argument isn't a package: URI, it must be returned unchanged
> So, start with:
>   if (packageUri.scheme != "package") return packageUri;
> 
> For the resolution, I think it will work, but it would be better if you could
> retain any fragment. It makes a kind of sense to be able to do:
> Isolate.resolvePackageUri(Uri.parse("package:foo/asset/foo.zip#file.ext"))
> (or something similar).
> 
> Maybe:
>   return Uri.base.resolve(
>       '$base/${packageUri.toString().subString("package:".length)}');
> 
> (I'm sure that can be made more efficient, but it's a good start).
> 
> That said, I'm not sure the VM does that either.

Done.

https://codereview.chromium.org/2495383003/diff/20001/sdk/lib/_internal/js_ru...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:37: return
Uri.base.resolve('$base/${packageUri.path}');
On 2016/11/14 20:46:23, Lasse Reichstein Nielsen wrote:
> Actually, it can probably just be:
>   Uri.base.resolve("$base/").resolveUri(packageUri.replace(scheme: ""))
> (and then it's probably worth it to cache Uri.base.resolve("$base/") instead
of
> computing it each time).

Done. I'm checking for the package scheme first, then using this approach.

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
File tests/isolate/browser/package_resolve_browser_hook_test.dart (right):

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
tests/isolate/browser/package_resolve_browser_hook_test.dart:8: final
PACKAGE_URI = "package:foo/bar.txt";
On 2016/11/14 20:42:34, Lasse Reichstein Nielsen wrote:
> Nitpick: Style guide says to use camelCase for constants. :)

haha - good point, I just copied it from a different test. Honestly we don't
need a constant here :)

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
tests/isolate/browser/package_resolve_browser_hook_test.dart:10: main() async {
On 2016/11/14 20:42:34, Lasse Reichstein Nielsen wrote:
> Are you sure an async main works with the test runner? I've had problems with
> that in some cases (but I never figured out exactly which cases)

Good point, should be fixed now.

https://codereview.chromium.org/2495383003/diff/20001/tests/isolate/browser/p...
tests/isolate/browser/package_resolve_browser_hook_test.dart:12: expect(uri,
Uri.base.resolve('path_set_from_hook/foo/bar.txt'));
On 2016/11/14 22:27:23, Harry Terkelsen wrote:
> can we also test if the hook is set to a path with slashes in it?

Done.

[Siggi Cherem (dart-lang), 2016-11-14 23:11:55]

Description was changed from

==========
Add implementation of Isolate.resolvePackageUri for dart2js.

This uses 'packages/' as the default base to resolve uris from, but also exposes
a hook to allow users to overwrite it.

BUG= https://github.com/dart-lang/sdk/issues/25594
==========

to

==========
Add implementation of Isolate.resolvePackageUri for dart2js.

This uses 'packages/' as the default base to resolve uris from, but also exposes
a hook to allow users to overwrite it.

BUG= https://github.com/dart-lang/sdk/issues/25594
R=het@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f460a8706d116515a921334f8bbe09542285eb77
==========

[Siggi Cherem (dart-lang), 2016-11-14 23:11:56]

Committed patchset #2 (id:60001) manually as
f460a8706d116515a921334f8bbe09542285eb77 (presubmit successful).