- Proper URI resolution when loading scripts from the standalone binary.

- Proper URI resolution when loading scripts from the standalone binary.
Committed: https://code.google.com/p/dart/source/detail?r=5135

[Ivan Posva, 2012-03-06 01:00:53]

Peter, please review the use of the dart:uri library in builtin.dart.
Søren, this is a heads-up/plea for implementing File::GetWorkingDirectory().
Siva, we have discussed this code already, here is the actual change.

Thanks all,
-Ivan

[ahe, 2012-03-06 08:27:15]

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

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode1
bin/builtin.dart:1: // Copyright (c) 2011, the Dart project authors.  Please see
the AUTHORS file
This file LGTM, but you may want to change how you append a / to cwd.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode33
bin/builtin.dart:33: // TODO(iposva): Make these private once the Dart API is
fixed.
I'm curious, what needs to be fixed?

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode35
bin/builtin.dart:35: _logResolution("# Current working directory: $cwd");
This form of logging is expensive because you have to create the result of
string interpolation even when logging is disabled.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode37
bin/builtin.dart:37: var base = new Uri(scheme: "file", path: "$cwd/");
Appending / doesn't work for the root directory. See the code I have in
dart/frog/leg/io/io.dart.

[Ivan Posva, 2012-03-07 07:46:53]

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

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode33
bin/builtin.dart:33: // TODO(iposva): Make these private once the Dart API is
fixed.
On 2012/03/06 08:27:15, ahe wrote:
> I'm curious, what needs to be fixed?
Currently calling private methods through the Dart API is not supported.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode35
bin/builtin.dart:35: _logResolution("# Current working directory: $cwd");
On 2012/03/06 08:27:15, ahe wrote:
> This form of logging is expensive because you have to create the result of
> string interpolation even when logging is disabled.
Will be removed once I am convinced that this works. So I am willing to eat the
cost for now.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode37
bin/builtin.dart:37: var base = new Uri(scheme: "file", path: "$cwd/");
On 2012/03/06 08:27:15, ahe wrote:
> Appending / doesn't work for the root directory. See the code I have in
> dart/frog/leg/io/io.dart.
The problem with io.dart in leg is that it is currently getting the
canonicalized path, which is not what we want, but I will check for a trailing
"/".

[ahe, 2012-03-07 08:22:37]

FYI

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

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode33
bin/builtin.dart:33: // TODO(iposva): Make these private once the Dart API is
fixed.
On 2012/03/07 07:46:53, Ivan Posva wrote:
> On 2012/03/06 08:27:15, ahe wrote:
> > I'm curious, what needs to be fixed?
> Currently calling private methods through the Dart API is not supported.

I guess that by Dart API, you are referring to the native C++ API in the VM?

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/builtin.dart#newcode37
bin/builtin.dart:37: var base = new Uri(scheme: "file", path: "$cwd/");
On 2012/03/07 07:46:53, Ivan Posva wrote:
> On 2012/03/06 08:27:15, ahe wrote:
> > Appending / doesn't work for the root directory. See the code I have in
> > dart/frog/leg/io/io.dart.
> The problem with io.dart in leg is that it is currently getting the
> canonicalized path, which is not what we want, but I will check for a trailing
> "/".

I meant to refer to how I handle trailing "/" in dart/frog/leg/io/io.dart. I
think you got that point.

[siva, 2012-03-07 18:32:17]

LGTM

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/gen_snapshot.cc#new...
bin/gen_snapshot.cc:277: Dart_Handle import_map = Dart_NewList(0);
import_map = Dart_Null();

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/main.cc#newcode259
bin/main.cc:259: Dart_Handle builtin_lib =
Builtin::LoadLibrary(Builtin::kBuiltinLibrary);
Why not store builtin_lib in a persistent handle instead of calling this
everytime.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/main.cc#newcode268
bin/main.cc:268: Dart_NewString(""), Dart_NewString("resolveUri"),
Why not store

Dart_NewString("");
Dart_NewString("resolveurii");
Dart_NewString("filePathFromUri");

in persistent handles.

https://chromiumcodereview.appspot.com/9614006/diff/1/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/1/vm/dart_api_impl.cc#new...
vm/dart_api_impl.cc:2025:
Library::Handle(isolate->object_store()->builtin_library());
if (builtin.IsNull()) {
  return Api::NewError(".......");
}

https://chromiumcodereview.appspot.com/9614006/diff/9001/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/9001/bin/main.cc#newcode414
bin/main.cc:414: return true;
This was a good way of testing the script snapshot feature as it allowed for any
random script to be used.

The current unit test for script snapshot uses a pretty small script.

Any ideas on how this can be retained as a test only feature. Maybe have this in
run_vm_test ?

https://chromiumcodereview.appspot.com/9614006/diff/9001/bin/main.cc#newcode515
bin/main.cc:515: free(const_cast<char*>(original_script_name));
A free of original_working_directory and original_url_script should also be done
here and all the places where original_script_name is being freed.

[Ivan Posva, 2012-03-07 22:45:17]

https://chromiumcodereview.appspot.com/9614006/diff/9001/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/9001/bin/main.cc#newcode414
bin/main.cc:414: return true;
On 2012/03/07 18:32:17, asiva wrote:
> This was a good way of testing the script snapshot feature as it allowed for
any
> random script to be used.
> 
> The current unit test for script snapshot uses a pretty small script.
> 
> Any ideas on how this can be retained as a test only feature. Maybe have this
in
> run_vm_test ?

I am trying to avoid testing code in the main binary. Adding a proper large
scale test to the unit tests would be a nice thing, but removing this code here
does not decrease the current testing coverage.

https://chromiumcodereview.appspot.com/9614006/diff/9001/bin/main.cc#newcode515
bin/main.cc:515: free(const_cast<char*>(original_script_name));
On 2012/03/07 18:32:17, asiva wrote:
> A free of original_working_directory and original_url_script should also be
done
> here and all the places where original_script_name is being freed.

Done.

[Ivan Posva, 2012-03-08 00:49:33]

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/gen_snapshot.cc#new...
bin/gen_snapshot.cc:277: Dart_Handle import_map = Dart_NewList(0);
On 2012/03/07 18:32:17, asiva wrote:
> import_map = Dart_Null();

Done.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/main.cc#newcode259
bin/main.cc:259: Dart_Handle builtin_lib =
Builtin::LoadLibrary(Builtin::kBuiltinLibrary);
On 2012/03/07 18:32:17, asiva wrote:
> Why not store builtin_lib in a persistent handle instead of calling this
> everytime.

Because you would need a current isolate to a group of persistent handle
mapping.

https://chromiumcodereview.appspot.com/9614006/diff/1/bin/main.cc#newcode268
bin/main.cc:268: Dart_NewString(""), Dart_NewString("resolveUri"),
On 2012/03/07 18:32:17, asiva wrote:
> Why not store
> 
> Dart_NewString("");
> Dart_NewString("resolveurii");
> Dart_NewString("filePathFromUri");
> 
> in persistent handles.

ditto

https://chromiumcodereview.appspot.com/9614006/diff/1/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9614006/diff/1/vm/dart_api_impl.cc#new...
vm/dart_api_impl.cc:2025:
Library::Handle(isolate->object_store()->builtin_library());
On 2012/03/07 18:32:17, asiva wrote:
> if (builtin.IsNull()) {
>   return Api::NewError(".......");
> }

Discussed offline.