Add "environment" parameter to spawnUri for String.fromEnvironment support.

Add "environment" parameter to spawnUri for String.fromEnvironment support.

This completes the parameters corresponding to official command line parameters
for the VM/dart2js.

R=floitsch@google.com, iposva@google.com, sigmund@google.com

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

[Lasse Reichstein Nielsen, 2015-09-17 10:55:28]

lrn@google.com changed reviewers:
+ floitsch@google.com, iposva@google.com, sigmund@google.com

[floitsch, 2015-09-17 11:33:23]

LGTM. (but wait for VM and dart2js).

[kevmoo, 2015-09-17 16:55:14]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2015-09-17 16:55:14]

DBC

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:74: if (environment != null)
throw new UnimplementedError("environment");
missing close } on 'if'

also: alignment

[Siggi Cherem (dart-lang), 2015-09-17 17:06:58]

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:74: if (environment != null)
throw new UnimplementedError("environment");
On 2015/09/17 16:55:14, kevmoo wrote:
> missing close } on 'if'
> 
> also: alignment

since the condition is repeated here too, delete line 73 entirely?

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:241: * spawned isolate uses when looking up
[String.fromEnvironment] values.
is this also meant to be used for the other `fromEnvironment` const constructors
(e.g. bool.fromEnvironment, int.fromEnvironment, etc)? Can we call them out too?

For a moment I was wondering if we wanted to switch the type to Map<String,
dynamic>, but I believe we still want to keep it as Map<String, String>, since
we don't know the type of the value until we see the const-constructor. In
particular, a flag like -Dfoo=true could be used in `const
String.fromEnvironment('foo') == 'true'` or in `const
bool.fromEnvironment('foo') == true`.

[Siggi Cherem (dart-lang), 2015-09-17 17:07:28]

... and lgtm (with the comments from the prev email)

[floitsch, 2015-09-17 18:23:10]

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:241: * spawned isolate uses when looking up
[String.fromEnvironment] values.
On 2015/09/17 17:06:57, Siggi Cherem (dart-lang) wrote:
> is this also meant to be used for the other `fromEnvironment` const
constructors
> (e.g. bool.fromEnvironment, int.fromEnvironment, etc)? Can we call them out
too?
> 
> For a moment I was wondering if we wanted to switch the type to Map<String,
> dynamic>, but I believe we still want to keep it as Map<String, String>, since
> we don't know the type of the value until we see the const-constructor. In
> particular, a flag like -Dfoo=true could be used in `const
> String.fromEnvironment('foo') == 'true'` or in `const
> bool.fromEnvironment('foo') == true`.

This Map should be used for all X.fromEnvironment constructors. As you say: the
values are transformed at use site.

[Ivan Posva, 2015-09-17 21:55:20]

LGTM

-Ivan

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:241: * spawned isolate uses when looking up
[String.fromEnvironment] values.
On 2015/09/17 18:23:10, floitsch wrote:
> On 2015/09/17 17:06:57, Siggi Cherem (dart-lang) wrote:
> > is this also meant to be used for the other `fromEnvironment` const
> constructors
> > (e.g. bool.fromEnvironment, int.fromEnvironment, etc)? Can we call them out
> too?
> > 
> > For a moment I was wondering if we wanted to switch the type to Map<String,
> > dynamic>, but I believe we still want to keep it as Map<String, String>,
since
> > we don't know the type of the value until we see the const-constructor. In
> > particular, a flag like -Dfoo=true could be used in `const
> > String.fromEnvironment('foo') == 'true'` or in `const
> > bool.fromEnvironment('foo') == true`.
> 
> This Map should be used for all X.fromEnvironment constructors. As you say:
the
> values are transformed at use site.

We do have a pretty good explanation on how the String is converted into the int
constants, but the description for bool is a bit lacking on that front. Maybe
that would lead to less user confusion and questions about the map being
String->String.

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:263: Map<String, String> environment});
I find it interesting that the new parameter is added at the end here, but in
the middle of the existing parameters for the implementations. I am fine adding
it at any position that makes sense, but please be consistent.

[Lasse Reichstein Nielsen, 2015-09-22 09:05:39]

PTAL on bool.fromEnvironment comment

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/_internal/js_runtim...
File sdk/lib/_internal/js_runtime/lib/isolate_patch.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/_internal/js_runtim...
sdk/lib/_internal/js_runtime/lib/isolate_patch.dart:74: if (environment != null)
throw new UnimplementedError("environment");
Absolutely, 'twas a bad paste error, at best.

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:241: * spawned isolate uses when looking up
[String.fromEnvironment] values.
I didn't write about int/bool.fromEnv because they are defined in terms of
String.fromEnv.

I'll add a "this is how it would be written using String.fromEnvironment" to the
bool.fromEnvironment docs as well, so it should be more clear that this is what
is happening.

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:263: Map<String, String> environment});
I've reordered them everywhere, so that the parameters corresponding to Isolate
methods are first and the ones corresponding to command line parameters are
last.

[floitsch, 2015-09-22 09:36:46]

Still LGTM.

https://codereview.chromium.org/1350053002/diff/20001/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/1350053002/diff/20001/sdk/lib/core/bool.dart#...
sdk/lib/core/bool.dart:36: * If you want to use a different truth-string than
`"true"`, you can use the
no need to codify the "true".

[Ivan Posva, 2015-09-22 09:56:59]

Still LGTM!

-Ivan

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dart
File sdk/lib/isolate/isolate.dart (right):

https://codereview.chromium.org/1350053002/diff/1/sdk/lib/isolate/isolate.dar...
sdk/lib/isolate/isolate.dart:263: Map<String, String> environment});
On 2015/09/22 09:05:39, Lasse Reichstein Nielsen wrote:
> I've reordered them everywhere, so that the parameters corresponding to
Isolate
> methods are first and the ones corresponding to command line parameters are
> last.

Thanks!

[Lasse Reichstein Nielsen, 2015-09-22 10:28:09]

https://codereview.chromium.org/1350053002/diff/20001/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/1350053002/diff/20001/sdk/lib/core/bool.dart#...
sdk/lib/core/bool.dart:36: * If you want to use a different truth-string than
`"true"`, you can use the
It's intended as code, so I prefer it codified.

[Lasse Reichstein Nielsen, 2015-09-22 10:29:44]

Committed patchset #2 (id:20001) manually as
232a9d687e9abf3a315e9b5c980334c2313f94a8 (presubmit successful).