Add support for 'dart.library.X' environment variables in dart2js.

Add support for 'dart.library.X' environment variables in dart2js.

R=johnniwinther@google.com, sigurdm@google.com

Committed: https://github.com/dart-lang/sdk/commit/369899c8ffca28f28a1af3cc323d1111c1a4a48a
Reverted: https://github.com/dart-lang/sdk/commit/b80994d6ad63ac30b400590cf51815e862c0e8a1

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

[floitsch, 2015-10-14 16:11:38]

floitsch@google.com changed reviewers:
+ johnniwinther@google.com

[floitsch, 2015-10-14 16:11:39]

I still need to write up the DEP before committing.

[Johnni Winther, 2015-10-16 09:34:31]

lgtm

https://codereview.chromium.org/1404183002/diff/40001/tests/language/library_...
File tests/language/library_env_test.dart (right):

https://codereview.chromium.org/1404183002/diff/40001/tests/language/library_...
tests/language/library_env_test.dart:1: // Copyright (c) 2015, the Dart project
authors.  Please see the AUTHORS file
Could you (additionally) make this a unittest to avoid the --categories=all
problem mentioned above?

[floitsch, 2015-10-16 19:05:07]

PTAL (just the new test).

https://codereview.chromium.org/1404183002/diff/40001/tests/language/library_...
File tests/language/library_env_test.dart (right):

https://codereview.chromium.org/1404183002/diff/40001/tests/language/library_...
tests/language/library_env_test.dart:1: // Copyright (c) 2015, the Dart project
authors.  Please see the AUTHORS file
On 2015/10/16 09:34:31, Johnni Winther wrote:
> Could you (additionally) make this a unittest to avoid the --categories=all
> problem mentioned above?

Done.

[Lasse Reichstein Nielsen, 2015-10-18 11:45:54]

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

[Lasse Reichstein Nielsen, 2015-10-18 11:45:54]

https://codereview.chromium.org/1404183002/diff/60001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:573: }
How do environment lookups at runtime ("new String.fromEnvironment(..)") work in
dart2js?
Could it include the libraries in runtime lookup as well?

[sigurdm, 2015-10-19 07:26:25]

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

[sigurdm, 2015-10-19 07:26:26]

https://codereview.chromium.org/1404183002/diff/40001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/40001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:44: /// this prefix and the name (without the
'dart:'.
DBC: missing ')'

[Johnni Winther, 2015-10-19 08:07:56]

lgtm

https://codereview.chromium.org/1404183002/diff/60001/pkg/compiler/lib/src/ap...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/60001/pkg/compiler/lib/src/ap...
pkg/compiler/lib/src/apiimpl.dart:573: }
On 2015/10/18 11:45:54, Lasse Reichstein Nielsen wrote:
> How do environment lookups at runtime ("new String.fromEnvironment(..)") work
in
> dart2js?
> Could it include the libraries in runtime lookup as well?

It's a throwing constructor so only const String.fromEnvironment(..) works.

[floitsch, 2016-01-07 19:09:03]

Merged with master and switched to using sdkLibraries.

PTAL.

[sigurdm, 2016-01-08 09:45:11]

LGTM

https://codereview.chromium.org/1404183002/diff/100001/pkg/compiler/lib/src/a...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/100001/pkg/compiler/lib/src/a...
pkg/compiler/lib/src/apiimpl.dart:595: if (result == null &&
!environment.containsKey(name) &&
The logic is a bit convoluted.

It might be easier to read
```
if (!environment.containsKey(name) &&
    name.startsWith(dartLibraryEnvironmentPrefix)) {
 [...]
} else {
  return environment[name];
}

[floitsch, 2016-01-08 15:51:01]

https://codereview.chromium.org/1404183002/diff/100001/pkg/compiler/lib/src/a...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/100001/pkg/compiler/lib/src/a...
pkg/compiler/lib/src/apiimpl.dart:595: if (result == null &&
!environment.containsKey(name) &&
On 2016/01/08 09:45:11, sigurdm wrote:
> The logic is a bit convoluted.
> 
> It might be easier to read
> ```
> if (!environment.containsKey(name) &&
>     name.startsWith(dartLibraryEnvironmentPrefix)) {
>  [...]
> } else {
>   return environment[name];
> }

Refactored so that we have early returns.

[Lasse Reichstein Nielsen, 2016-01-11 13:58:36]

https://codereview.chromium.org/1404183002/diff/120001/pkg/compiler/lib/src/a...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/120001/pkg/compiler/lib/src/a...
pkg/compiler/lib/src/apiimpl.dart:595: if (environment.containsKey(name)) return
environment[name];
Could this be:

String result = environment[name];
if (result != null) return result;

That is, can an "environment" contain a null value for an existing key. If not,
there is no need for doing two lookups to get to the value.

[floitsch, 2016-01-11 14:18:29]

https://codereview.chromium.org/1404183002/diff/120001/pkg/compiler/lib/src/a...
File pkg/compiler/lib/src/apiimpl.dart (right):

https://codereview.chromium.org/1404183002/diff/120001/pkg/compiler/lib/src/a...
pkg/compiler/lib/src/apiimpl.dart:595: if (environment.containsKey(name)) return
environment[name];
On 2016/01/11 13:58:35, Lasse Reichstein Nielsen wrote:
> Could this be:
> 
> String result = environment[name];
> if (result != null) return result;
> 
> That is, can an "environment" contain a null value for an existing key. If
not,
> there is no need for doing two lookups to get to the value.

Tbh I don't think it's that costly to look into the map twice.

I could change it to the following, though:
var result = env[name];
if (result != null || env.containsKey(name)) return result;

[floitsch, 2016-01-11 20:32:00]

Updated so that it avoids the second lookup if the key is found.

[floitsch, 2016-01-12 12:58:52]

Description was changed from

==========
Add support for 'dart.library.X' environment variables in dart2js.
==========

to

==========
Add support for 'dart.library.X' environment variables in dart2js.

R=johnniwinther@google.com, sigurdm@google.com

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

[floitsch, 2016-01-12 12:58:54]

Committed patchset #8 (id:140001) manually as
369899c8ffca28f28a1af3cc323d1111c1a4a48a (presubmit successful).

[floitsch, 2016-01-12 13:41:51]

Description was changed from

==========
Add support for 'dart.library.X' environment variables in dart2js.

R=johnniwinther@google.com, sigurdm@google.com

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

to

==========
Add support for 'dart.library.X' environment variables in dart2js.

R=johnniwinther@google.com, sigurdm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/369899c8ffca28f28a1af3cc323d1111c1a4a48a
Reverted:
https://github.com/dart-lang/sdk/commit/b80994d6ad63ac30b400590cf51815e862c0e8a1
==========

[floitsch, 2016-01-12 16:16:40]

I had to revert and adapt the test.
PTAL (especially at the test).

[sigurdm, 2016-01-13 13:00:48]

LGTM

https://codereview.chromium.org/1404183002/diff/180001/tests/compiler/dart2js...
File tests/compiler/dart2js/library_env_test.dart (right):

https://codereview.chromium.org/1404183002/diff/180001/tests/compiler/dart2js...
tests/compiler/dart2js/library_env_test.dart:30: DART2JS_PLATFORM,
Import no longer needed

[floitsch, 2016-01-13 15:34:33]

https://codereview.chromium.org/1404183002/diff/180001/tests/compiler/dart2js...
File tests/compiler/dart2js/library_env_test.dart (right):

https://codereview.chromium.org/1404183002/diff/180001/tests/compiler/dart2js...
tests/compiler/dart2js/library_env_test.dart:30: DART2JS_PLATFORM,
On 2016/01/13 13:00:48, sigurdm wrote:
> Import no longer needed

Thanks.
Done.

[floitsch, 2016-01-13 15:34:50]

Description was changed from

==========
Add support for 'dart.library.X' environment variables in dart2js.

R=johnniwinther@google.com, sigurdm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/369899c8ffca28f28a1af3cc323d1111c1a4a48a
Reverted:
https://github.com/dart-lang/sdk/commit/b80994d6ad63ac30b400590cf51815e862c0e8a1
==========

to

==========
Add support for 'dart.library.X' environment variables in dart2js.

R=johnniwinther@google.com, sigurdm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/369899c8ffca28f28a1af3cc323d1111c1a4a48a
Reverted:
https://github.com/dart-lang/sdk/commit/b80994d6ad63ac30b400590cf51815e862c0e8a1

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

[floitsch, 2016-01-13 15:34:51]

Committed patchset #11 (id:200001) manually as
e775fa093207913f0fea911842c3840b065ead93 (presubmit successful).