Refactor URI processing of native extensions directive #import("dart-ext:foo").

Refactor URI processing of native extensions directive #import("dart-ext:foo").

BUG=
TEST=standalone/io/TestExtensionsTest

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

[Ivan Posva, 2012-04-17 18:50:06]

LGTM with comment.

-Ivan

https://chromiumcodereview.appspot.com/10103031/diff/1/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://chromiumcodereview.appspot.com/10103031/diff/1/runtime/bin/builtin.da...
runtime/bin/builtin.dart:77: // resolved.scheme = 'foo';  // Causes a segfault. 
Verify this is expected.
Since the bug is filed now, you can remove this commented out code.

https://chromiumcodereview.appspot.com/10103031/diff/1/runtime/bin/builtin.da...
runtime/bin/builtin.dart:93: case 'dart-ext': path = _filePathFromFileUri(uri);
break;
Since dart-ext is not a file URI I am expecting this code to break once we
introduce an assert inside _filePathFromFileUri.

How about a remapping step before getting into this code converting the dart-ext
URI into the switch, such as this?

if (uri.scheme == "dart-ext") {
  uri = new Uri(scheme: "file", path: uri.path);
}

I can imagine that we could add other file-like URIs in the future.

[Bill Hesse, 2012-04-18 09:56:39]

https://chromiumcodereview.appspot.com/10103031/diff/1/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://chromiumcodereview.appspot.com/10103031/diff/1/runtime/bin/builtin.da...
runtime/bin/builtin.dart:77: // resolved.scheme = 'foo';  // Causes a segfault. 
Verify this is expected.
This line will be removed.  A separate bug and reproduction
(http://code.google.com/p/dart/issues/detail?id=2621) has been filed.

https://chromiumcodereview.appspot.com/10103031/diff/1/runtime/bin/builtin.da...
runtime/bin/builtin.dart:93: case 'dart-ext': path = _filePathFromFileUri(uri);
break;
Fixed by calling _filePathFromOtherUri.

I wrote code that did such a remapping, but replaced it because "Get a file path
from a URI, ignoring the scheme" is a clear, consistent functionality that is
needed, and implementing it by faking a file: scheme just so an added check can
pass seems like the wrong solution.

Adding the check would catch places where we accidentally call it with a
non-file: scheme, but we know that we are intentionally calling it with a
non-file: scheme in places.

As a temporary fix, I cloned the function _filePathFromFileUri, and called it
_filePathFromOtherUri.
I think a good refactoring in the future would be that the common code could
actually go into _filePathFromUri, and only package: would call an external
helper.

On 2012/04/17 18:50:06, Ivan Posva wrote:
> Since dart-ext is not a file URI I am expecting this code to break once we
> introduce an assert inside _filePathFromFileUri.
> 
> How about a remapping step before getting into this code converting the
dart-ext
> URI into the switch, such as this?
> 
> if (uri.scheme == "dart-ext") {
>   uri = new Uri(scheme: "file", path: uri.path);
> }
> 
> I can imagine that we could add other file-like URIs in the future.