Add fields to platform library, implement them on runtime dart.

Add fields to platform library, implement them on runtime dart.

BUG=
R=ahe@google.com, ajohnsen@google.com, floitsch@google.com, sgjesse@google.com

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

[Bill Hesse, 2013-10-23 15:54:58]

Do you agree that all the fields from Platform should move to dart:platform?

I plan to replace uses of Platform with uses of _Platform, and deprecate
Platform, in a future CL.

[Anders Johnsen, 2013-10-24 08:30:07]

Also, please add default options for dart2js/dartium.

https://codereview.chromium.org/36883005/diff/50001/sdk/lib/platform/platform...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/50001/sdk/lib/platform/platform...
sdk/lib/platform/platform.dart:42: final int numberOfProcessors =
_platform(_kNumberOfProcessors);
These should be marked as 'external', with this implementation moved to
runtime/lib/platform_patch.dart.

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/platf...
File tests/standalone/io/platform_test.dart (right):

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/platf...
tests/standalone/io/platform_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
Move test to e.g. tests/lib/platform/os_test.dart.

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/platf...
tests/standalone/io/platform_test.dart:6: import "dart:io";
Remove io import.

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/uri_p...
File tests/standalone/io/uri_platform_test.dart (right):

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/uri_p...
tests/standalone/io/uri_platform_test.dart:6: import "dart:io";
remove io import, and enable for dart2js.

[Bill Hesse, 2013-10-24 13:43:43]

OK, PTAL.

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/platf...
File tests/standalone/io/platform_test.dart (right):

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/platf...
tests/standalone/io/platform_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2013/10/24 08:30:08, Anders Johnsen wrote:
> Move test to e.g. tests/lib/platform/os_test.dart.

Done.

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/platf...
tests/standalone/io/platform_test.dart:6: import "dart:io";
On 2013/10/24 08:30:08, Anders Johnsen wrote:
> Remove io import.

Done.

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/uri_p...
File tests/standalone/io/uri_platform_test.dart (right):

https://codereview.chromium.org/36883005/diff/50001/tests/standalone/io/uri_p...
tests/standalone/io/uri_platform_test.dart:6: import "dart:io";
On 2013/10/24 08:30:08, Anders Johnsen wrote:
> remove io import, and enable for dart2js.

Done.

[Søren Gjesse, 2013-10-24 15:33:18]

lgtm

[ahe, 2013-10-24 15:38:16]

Doesn't look good for dart2js.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:10: class _Platform {
This API does not make sense for dart2js.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:36: final int numberOfProcessors =
_platform.numberOfProcessors;
This generates rather horrible code in dart2js.

Why is this not:

external int get numberOfProcessors;

[Bill Hesse, 2013-10-24 15:49:38]

On 2013/10/24 15:38:16, ahe wrote:
> Doesn't look good for dart2js.
> 
>
https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
> File sdk/lib/platform/platform.dart (right):
> 
>
https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
> sdk/lib/platform/platform.dart:10: class _Platform {
> This API does not make sense for dart2js.
> 
>
https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
> sdk/lib/platform/platform.dart:36: final int numberOfProcessors =
> _platform.numberOfProcessors;
> This generates rather horrible code in dart2js.
> 
> Why is this not:
> 
> external int get numberOfProcessors;

We were just optimizing.  But we can change it to your way, easily.

Is

external int get numberOfProcessors;

much better than

int get numberOfProcessors => _platform.numberOfProcessors;
?

Is it just the static initialization that you want to get rid of?

If so, does
_Platform _platform
need to be an external getter, or a getter, rather than an initialized static?

We can push as much as we need to to the patch files rather than platform.dart -
just let us know.

[floitsch, 2013-10-24 15:50:57]

LGTM after Peter's concerns are addressed.

I would simply make all of the getters external.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:27: // Issue 6997: Patching top-level statics has
a problem.
TODO(6997): ...

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:33: * Get the number of processors of the
machine.
"Gets" or simply "The number of ...".

Ditto for all other comments.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:34: * Returns null if no information is
available.
New line before the "Returns".

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:46: * Get a string (`linux`, `macos`, `windows`
or `android`
missing closing parenthesis.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:86: * returned on Windows is therefore
case-insensitive and will convert
converts (no need for future)

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:102: * Returns the URI (in String form of the
script being run in this
missing closing parenthesis.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:113: * and the script name. Each fetch of
[executableArguments] returns a new
Don't overspecify. No need to specify that the result is a copy.

[Anders Johnsen, 2013-10-24 17:21:01]

LGTM, but wait for Peters comments.

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

https://codereview.chromium.org/36883005/diff/160001/runtime/bin/dartutils.cc...
runtime/bin/dartutils.cc:715: Dart_Handle package_root_string =
NewString(package_root);
Why not reuse result here?

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:47: * representing the operating system. Returns
null if not supported.
Maybe change to

  Returns `null` if the operating system could not be determined.

The current text sounds like we don't support the OS if it's `null`.

Same goes for all the other ones.

https://codereview.chromium.org/36883005/diff/160001/tests/lib/platform/scrip...
File tests/lib/platform/script_test.dart (right):

https://codereview.chromium.org/36883005/diff/160001/tests/lib/platform/scrip...
tests/lib/platform/script_test.dart:11: expect(platform.script.replaceAll('\\',
'/'),
Consider using `new Uri.file(platform.script).toString()`.

[Bill Hesse, 2013-10-25 11:06:34]

Changed according to Peter's and Ivan's suggestions.  Works on VM, dart2js,
dart2dart, dartium, dartanalyzer and dart2analyzer.

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

https://codereview.chromium.org/36883005/diff/160001/runtime/bin/dartutils.cc...
runtime/bin/dartutils.cc:715: Dart_Handle package_root_string =
NewString(package_root);
On 2013/10/24 17:21:01, Anders Johnsen wrote:
> Why not reuse result here?

It's just a matter of style.  In the rest of the function, named locals are used
for values, and result is mainly the result of an
action, testing for error, but without a value in the non-error case.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:10: class _Platform {
On 2013/10/24 15:38:16, ahe wrote:
> This API does not make sense for dart2js.

I think we would add fields that report important info about the dart2js
platform, that should be exposed.

Fields that are not applicable to dart2js will return null, as documented.

If we wanted to divide the specific info for each platform up more, we could
report a "Dart2jsPlatform" property, that would have specific details as fields,
rather than putting them at the top level.

There is top-level consensus that platform should not be in dart:io, because
some things are more general, and should be available on all platforms where
they make sense.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:27: // Issue 6997: Patching top-level statics has
a problem.
On 2013/10/24 15:50:58, floitsch wrote:
> TODO(6997): ...

Done.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:33: * Get the number of processors of the
machine.
On 2013/10/24 15:50:58, floitsch wrote:
> "Gets" or simply "The number of ...".
> 
> Ditto for all other comments.

Done.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:36: final int numberOfProcessors =
_platform.numberOfProcessors;
On 2013/10/24 15:38:16, ahe wrote:
> This generates rather horrible code in dart2js.
> 
> Why is this not:
> 
> external int get numberOfProcessors;

All of them changed to 
external String get foo, as you suggest.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:46: * Get a string (`linux`, `macos`, `windows`
or `android`
On 2013/10/24 15:50:58, floitsch wrote:
> missing closing parenthesis.

Done.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:47: * representing the operating system. Returns
null if not supported.
On 2013/10/24 17:21:01, Anders Johnsen wrote:
> Maybe change to
> 
>   Returns `null` if the operating system could not be determined.
> 
> The current text sounds like we don't support the OS if it's `null`.
> 
> Same goes for all the other ones.

Done.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:86: * returned on Windows is therefore
case-insensitive and will convert
On 2013/10/24 15:50:58, floitsch wrote:
> converts (no need for future)

Done.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:102: * Returns the URI (in String form of the
script being run in this
On 2013/10/24 15:50:58, floitsch wrote:
> missing closing parenthesis.

Done.

https://codereview.chromium.org/36883005/diff/160001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:113: * and the script name. Each fetch of
[executableArguments] returns a new
On 2013/10/24 15:50:58, floitsch wrote:
> Don't overspecify. No need to specify that the result is a copy.

Done.

https://codereview.chromium.org/36883005/diff/160001/tests/lib/platform/scrip...
File tests/lib/platform/script_test.dart (right):

https://codereview.chromium.org/36883005/diff/160001/tests/lib/platform/scrip...
tests/lib/platform/script_test.dart:11: expect(platform.script.replaceAll('\\',
'/'),
On 2013/10/24 17:21:01, Anders Johnsen wrote:
> Consider using `new Uri.file(platform.script).toString()`.

Done.

[ahe, 2013-10-25 11:34:04]

sdk/lib/_internal/lib/platform_patch.dart, LGTM

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/_internal/lib/pl...
File sdk/lib/_internal/lib/platform_patch.dart (right):

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/_internal/lib/pl...
sdk/lib/_internal/lib/platform_patch.dart:4: 
In follow-up CL:

import 'dart:_js_helper' show Primitives;

Then add additional primitives if needed.

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/_internal/lib/pl...
sdk/lib/_internal/lib/platform_patch.dart:21: patch List<String> get
executableArguments => new List<String>(0);
Add comment that this is unmodifiable.

[Bill Hesse, 2013-10-25 11:48:47]

Committed patchset #6 manually as r29244 (presubmit successful).

[Ivan Posva, 2013-10-25 17:03:47]

-Ivan

https://codereview.chromium.org/36883005/diff/160001/runtime/lib/platform_pat...
File runtime/lib/platform_patch.dart (right):

https://codereview.chromium.org/36883005/diff/160001/runtime/lib/platform_pat...
runtime/lib/platform_patch.dart:21: return "/";
null

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/platform/platfor...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:51: /**
These specific testers are problematic. Let's assume that we or a third-party
ports Dart to BSD, now they would have to change the dart:platform API.

[Ivan Posva, 2013-10-25 17:26:43]

-Ivan

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

https://codereview.chromium.org/36883005/diff/260001/runtime/bin/builtin.dart...
runtime/bin/builtin.dart:316: int get numberOfProcessors =>
Platform.numberOfProcessors;
I am assuming that we will remove the Platform from dart:io. In the meantime
please add a comment here as to what is happening because otherwise it looks
like a circularity.

[Bill Hesse, 2013-10-25 17:49:59]

https://codereview.chromium.org/36883005/diff/160001/runtime/lib/platform_pat...
File runtime/lib/platform_patch.dart (right):

https://codereview.chromium.org/36883005/diff/160001/runtime/lib/platform_pat...
runtime/lib/platform_patch.dart:21: return "/";
On 2013/10/25 17:03:48, Ivan Posva wrote:
> null

Are you ok with making this "/" on Dartium, when a Dartium implementation is
added?

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

https://codereview.chromium.org/36883005/diff/260001/runtime/bin/builtin.dart...
runtime/bin/builtin.dart:316: int get numberOfProcessors =>
Platform.numberOfProcessors;
On 2013/10/25 17:26:44, Ivan Posva wrote:
> I am assuming that we will remove the Platform from dart:io. In the meantime
> please add a comment here as to what is happening because otherwise it looks
> like a circularity.

Comment added in CL 43773004.

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/_internal/lib/pl...
File sdk/lib/_internal/lib/platform_patch.dart (right):

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/_internal/lib/pl...
sdk/lib/_internal/lib/platform_patch.dart:21: patch List<String> get
executableArguments => new List<String>(0);
On 2013/10/25 11:34:04, ahe wrote:
> Add comment that this is unmodifiable.

Done.

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/platform/platfor...
File sdk/lib/platform/platform.dart (right):

https://codereview.chromium.org/36883005/diff/260001/sdk/lib/platform/platfor...
sdk/lib/platform/platform.dart:51: /**
On 2013/10/25 17:03:48, Ivan Posva wrote:
> These specific testers are problematic. Let's assume that we or a third-party
> ports Dart to BSD, now they would have to change the dart:platform API.

Removed in CL 43773004