Add Timer implementation for dart2js. Also moves tests to appropriate location.

Add Timer implementation for dart2js. Also moves tests to appropriate location.

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

[Siggi Cherem (dart-lang), 2012-08-09 00:00:58]

this has been in my backlog for quite some time...

[kasperl, 2012-08-09 06:15:33]

LGTM.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
File lib/isolate/dart2js/isolateimpl.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/isolateimpl.dart:266: if (_window != null) { // (defined in
timer_provider.dart)
Turn comment into a proper sentence and mention that it is _window that is
defined somewhere else.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
File lib/isolate/dart2js/timer_provider.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/timer_provider.dart:17: _Window get _window() native
Could this be using the JS helper rather than being a native method? Long term,
we're much more likely to be able to inline proper Dart method that use the JS
helper.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/timer_provider.dart:21: bool _once;
It seems perfectly possible to make both fields final.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/timer_provider.dart:44: if (repeating) {
Maybe use ?:.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:76: constructor_test: Fail, OK # dart2js does not
support subclassing Isolate
Do we still have this test?

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:79: constructor_test: Fail, OK # dart2js does not
support subclassing Isolate
Do we still have this test?

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:80: spawn_uri_test: Skip # loading another file is
not supported in d8
Could some of these skips be fails instead? Maybe even Fail, OK because we're
not planning on making it work.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:84: timer_cancel_test: Fail # Needs Timer to run.
Fail, OK -- it is not something we're looking to address. Alternatively, we
should try to make it possible to determine (at runtime) whether or not there is
timer support on the platform (maybe new Timer can throw if not?) and rewrite
the tests to check for that. When running on raw d8, I'd really like this to
fail in a predictable (and tested) way.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/multi...
File tests/isolate/multiple_timer_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/multi...
tests/isolate/multiple_timer_test.dart:4: #library('multiple_timer_test');
Usually we have a newline between the copyright notice and the #library tag.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_cancel1_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_cancel1_test.dart:4: #library('timer_cancel1_test');
Ditto.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_cancel2_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_cancel2_test.dart:4: #library('timer_cancel2_test');
Ditto.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_cancel_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_cancel_test.dart:4: #library('timer_cancel_test');
Ditto.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_repeat_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_repeat_test.dart:4: #library('timer_repeat_test');
Ditto.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_test.dart:4: #library('timer_test');
Ditto.

[Siggi Cherem (dart-lang), 2012-08-09 20:37:14]

Thanks Kasper!

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
File lib/isolate/dart2js/isolateimpl.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/isolateimpl.dart:266: if (_window != null) { // (defined in
timer_provider.dart)
On 2012/08/09 06:15:33, kasperl wrote:
> Turn comment into a proper sentence and mention that it is _window that is
> defined somewhere else.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
File lib/isolate/dart2js/timer_provider.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/timer_provider.dart:17: _Window get _window() native
On 2012/08/09 06:15:33, kasperl wrote:
> Could this be using the JS helper rather than being a native method? Long
term,
> we're much more likely to be able to inline proper Dart method that use the JS
> helper.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/timer_provider.dart:21: bool _once;
On 2012/08/09 06:15:33, kasperl wrote:
> It seems perfectly possible to make both fields final.

Done for '_once'.

Can't do it for _handle, though - because I need to create a closure in order to
call setTimeout. The closure creation is not allowed in the initializer block.

The vm checks that final fields are initialized in initializers (outside of the
ctor body), but it seems that dart2js is not doing so right now.

https://chromiumcodereview.appspot.com/10830230/diff/2001/lib/isolate/dart2js...
lib/isolate/dart2js/timer_provider.dart:44: if (repeating) {
On 2012/08/09 06:15:33, kasperl wrote:
> Maybe use ?:.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
File tests/isolate/isolate.status (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:76: constructor_test: Fail, OK # dart2js does not
support subclassing Isolate
On 2012/08/09 06:15:33, kasperl wrote:
> Do we still have this test?

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:79: constructor_test: Fail, OK # dart2js does not
support subclassing Isolate
On 2012/08/09 06:15:33, kasperl wrote:
> Do we still have this test?

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:80: spawn_uri_test: Skip # loading another file is
not supported in d8
On 2012/08/09 06:15:33, kasperl wrote:
> Could some of these skips be fails instead? Maybe even Fail, OK because we're
> not planning on making it work.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/isola...
tests/isolate/isolate.status:84: timer_cancel_test: Fail # Needs Timer to run.
On 2012/08/09 06:15:33, kasperl wrote:
> Fail, OK -- it is not something we're looking to address. Alternatively, we
> should try to make it possible to determine (at runtime) whether or not there
is
> timer support on the platform (maybe new Timer can throw if not?) and rewrite
> the tests to check for that. When running on raw d8, I'd really like this to
> fail in a predictable (and tested) way.

actually, that's what happens today already. I added a test for it
(timer_not_available_test), marked all these as Fail, OK.

I wonder if we should just mark them as Skip though - it doesn't seem that we
are gaining any information from running these.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/multi...
File tests/isolate/multiple_timer_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/multi...
tests/isolate/multiple_timer_test.dart:4: #library('multiple_timer_test');
On 2012/08/09 06:15:33, kasperl wrote:
> Usually we have a newline between the copyright notice and the #library tag.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_cancel1_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_cancel1_test.dart:4: #library('timer_cancel1_test');
On 2012/08/09 06:15:33, kasperl wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_cancel2_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_cancel2_test.dart:4: #library('timer_cancel2_test');
On 2012/08/09 06:15:33, kasperl wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_cancel_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_cancel_test.dart:4: #library('timer_cancel_test');
On 2012/08/09 06:15:33, kasperl wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_repeat_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_repeat_test.dart:4: #library('timer_repeat_test');
On 2012/08/09 06:15:33, kasperl wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
File tests/isolate/timer_test.dart (right):

https://chromiumcodereview.appspot.com/10830230/diff/2001/tests/isolate/timer...
tests/isolate/timer_test.dart:4: #library('timer_test');
On 2012/08/09 06:15:33, kasperl wrote:
> Ditto.

Done.

[Mads Ager (google), 2012-08-13 07:24:49]

LGTM