HTTP loading tests and package root support

HTTP loading tests and package root support

R=asiva@google.com, iposva@google.com, sgjesse@google.com

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

[Cutch, 2013-05-29 20:13:05]

Fixes breakage caused by Uri class renaming "domain" to "host".
Fixes automatic package root resolving for http: loaded scripts.
Adds a test to verify that script loading, library loading, explicit package
root and automatic package root works over http.

[Søren Gjesse, 2013-05-30 09:13:03]

dbc

https://codereview.chromium.org/16092007/diff/3001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/16092007/diff/3001/runtime/bin/builtin.dart#n...
runtime/bin/builtin.dart:191: return uri.host;
Sorry I missed this when landing the Uri change.

https://codereview.chromium.org/16092007/diff/3001/tests/standalone/http_laun...
File tests/standalone/http_launch_test.dart (right):

https://codereview.chromium.org/16092007/diff/3001/tests/standalone/http_laun...
tests/standalone/http_launch_test.dart:33: file.fullPath().then((String
fullPath) {
Why are you calling fullPath here? Seems that you are not using the result.

[Cutch, 2013-05-30 13:44:09]

https://codereview.chromium.org/16092007/diff/3001/tests/standalone/http_laun...
File tests/standalone/http_launch_test.dart (right):

https://codereview.chromium.org/16092007/diff/3001/tests/standalone/http_laun...
tests/standalone/http_launch_test.dart:33: file.fullPath().then((String
fullPath) {
On 2013/05/30 09:13:03, Søren Gjesse wrote:
> Why are you calling fullPath here? Seems that you are not using the result.

Good catch. I had copied this from an article on using HttpServer and missed
this when removing unnecessary code.

Fixed.

[siva, 2013-05-31 20:36:29]

lgtm with some comments.

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:411: }
We seem to parse the URI 3 times here once to extract the path,
once for the host and finally for the port.

Not sure if it would be more efficient to parse once and then
just do GetFields on the parsed uri object.

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:717: // Always call ResolveScriptUri so that the base
path is set.
From the comment it is not clear which base path it is referring to?

Also it could be:

Dart_Handle resolved_script_uri = ResolveScriptUri(...);

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:723: if (DartUtils::IsHttpSchemeURL(script_uri)) {
A NewString(script_uri) was done above and again inside
LoadScriptHttp we do a NewString(script_uri).

Can the signature of LoadScriptHttp be changed to
(Dart_Handle script_uri, Dart_Handle builtin_lib);

Ditto comment about the signature of ParseHttpUri a NewString
is done in LoadScriptHttp and another one is done inside
ParseHttpUri.

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
File tests/standalone/http_launch_data/http_isolate_main.dart (right):

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
tests/standalone/http_launch_data/http_isolate_main.dart:1: import
'dart:isolate';
Missing copyright stuff.

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
File tests/standalone/http_launch_data/packages/simple/simple.dart (right):

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
tests/standalone/http_launch_data/packages/simple/simple.dart:1: library simple;
Missing copyright stuff.

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
File tests/standalone/http_launch_test.dart (right):

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
tests/standalone/http_launch_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
New file should be 2013

[Cutch, 2013-05-31 22:37:19]

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:411: }
On 2013/05/31 20:36:29, siva wrote:
> We seem to parse the URI 3 times here once to extract the path,
> once for the host and finally for the port.
> 
> Not sure if it would be more efficient to parse once and then
> just do GetFields on the parsed uri object.

Done.

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:717: // Always call ResolveScriptUri so that the base
path is set.
On 2013/05/31 20:36:29, siva wrote:
> From the comment it is not clear which base path it is referring to?
> 
> Also it could be:
> 
> Dart_Handle resolved_script_uri = ResolveScriptUri(...);

Done.

https://codereview.chromium.org/16092007/diff/19001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:723: if (DartUtils::IsHttpSchemeURL(script_uri)) {
On 2013/05/31 20:36:29, siva wrote:
> A NewString(script_uri) was done above and again inside
> LoadScriptHttp we do a NewString(script_uri).
> 
> Can the signature of LoadScriptHttp be changed to
> (Dart_Handle script_uri, Dart_Handle builtin_lib);
> 
> Ditto comment about the signature of ParseHttpUri a NewString
> is done in LoadScriptHttp and another one is done inside
> ParseHttpUri.

Done.

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
File tests/standalone/http_launch_data/http_isolate_main.dart (right):

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
tests/standalone/http_launch_data/http_isolate_main.dart:1: import
'dart:isolate';
On 2013/05/31 20:36:29, siva wrote:
> Missing copyright stuff.

Done.

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
File tests/standalone/http_launch_data/packages/simple/simple.dart (right):

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
tests/standalone/http_launch_data/packages/simple/simple.dart:1: library simple;
On 2013/05/31 20:36:29, siva wrote:
> Missing copyright stuff.

Done.

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
File tests/standalone/http_launch_test.dart (right):

https://codereview.chromium.org/16092007/diff/19001/tests/standalone/http_lau...
tests/standalone/http_launch_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2013/05/31 20:36:29, siva wrote:
> New file should be 2013

Done.

[Søren Gjesse, 2013-06-03 07:12:07]

lgtm

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:76: if (scriptUri.scheme == 'http') {
How about https?

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:204: if (_entrypoint.scheme == 'http') {
Ditto.

[Ivan Posva, 2013-06-03 07:22:56]

LGTM

When we get some time we should reconsider how all of this loading is done.
There seem to be a lot of special cases coded into the resolution and handling
dart:io for example.

-Ivan

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:76: if (scriptUri.scheme == 'http') {
What if the URI we were handed on the command line is using the "file" or
"package" schemes? I am not sure where it is supported to use package based
scripts for the main entry point, but I do not see a reason not to.

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:202: path = "${_packageRoot}${uri.path}";
What is confusing in this whole scheme is that if we have a package root that
points to a http based location, then it is just using string concatenation here
and does not go through any of the proper URI handling in this file.

[Søren Gjesse, 2013-06-03 09:23:49]

https://codereview.chromium.org/16092007/diff/31001/tests/standalone/http_lau...
File tests/standalone/http_launch_test.dart (right):

https://codereview.chromium.org/16092007/diff/31001/tests/standalone/http_lau...
tests/standalone/http_launch_test.dart:4: //
With dart:io being used for the HTTP client we should add the following comments
here:

// VMOptions=
// VMOptions=--short_socket_read
// VMOptions=--short_socket_write
// VMOptions=--short_socket_read --short_socket_write

[Cutch, 2013-06-03 17:55:37]

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart
File runtime/bin/builtin.dart (right):

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:76: if (scriptUri.scheme == 'http') {
On 2013/06/03 07:12:07, Søren Gjesse wrote:
> How about https?

Now that we are using the dart:io HttpClient we can support https.

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:76: if (scriptUri.scheme == 'http') {
On 2013/06/03 07:22:56, Ivan Posva wrote:
> What if the URI we were handed on the command line is using the "file" or
> "package" schemes? I am not sure where it is supported to use package based
> scripts for the main entry point, but I do not see a reason not to.

Agreed. This code needs to be cleaned up. I'll do that as a final pass once the
tests land.

https://codereview.chromium.org/16092007/diff/31001/runtime/bin/builtin.dart#...
runtime/bin/builtin.dart:202: path = "${_packageRoot}${uri.path}";
On 2013/06/03 07:22:56, Ivan Posva wrote:
> What is confusing in this whole scheme is that if we have a package root that
> points to a http based location, then it is just using string concatenation
here
> and does not go through any of the proper URI handling in this file.

See my other comment. This function needs to be cleaned up for the reasons
you've mentioned and because it has an implicit side effect of setting
_entrypoint which should be made explicit.

https://codereview.chromium.org/16092007/diff/31001/tests/standalone/http_lau...
File tests/standalone/http_launch_test.dart (right):

https://codereview.chromium.org/16092007/diff/31001/tests/standalone/http_lau...
tests/standalone/http_launch_test.dart:4: //
On 2013/06/03 09:23:49, Søren Gjesse wrote:
> With dart:io being used for the HTTP client we should add the following
comments
> here:
> 
> // VMOptions=
> // VMOptions=--short_socket_read
> // VMOptions=--short_socket_write
> // VMOptions=--short_socket_read --short_socket_write

Done.

[Cutch, 2013-06-04 13:52:53]

Committed patchset #10 manually as r23593 (presubmit successful).