- Move the Timer interface to the dart:isolate library so that it

- Move the Timer interface to the dart:isolate library so that it
  is accessible from both client and standalone program.
- Enable a way for the embedder to provide the implementation of
  the Timer interface.
- Update tests.
Committed: https://code.google.com/p/dart/source/detail?r=8987

[Mads Ager (google), 2012-06-13 13:07:52]

lgtm

[siva, 2012-06-13 17:05:23]

LGTM with one comment regarding LoadLibrary

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/builtin...
File runtime/bin/builtin_natives.cc (right):

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/builtin...
runtime/bin/builtin_natives.cc:142: void Builtin::SetupIOLibrary(Dart_Handle
io_lib) {
The name SetupIOLibrary seems misleading as the function is not really setting
up the IO library.

How about SetupTimerImpl(....) ?

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/main.cc...
runtime/bin/main.cc:511: Builtin::kIOLibrary);
I would prefer if we did a Dart_LookupLibrary here instead
of Builtin::LoadLibrary. We are expected to find the library here. If for some
reason there is a bug in the snapshot generation we want to assert out when the
library is not found instead of silently loading it.

[Siggi Cherem (dart-lang), 2012-06-13 17:16:39]

lgtm, a couple general comments

- rename timer_api.dart to simply timer.dart? 
(in the case of isolate_api.dart, we used the _api suffix because we don't have
an entry point file that works for the vm + dart2js, this way people are not
confused trying to find where is the #library defined. For timer.dart we don't
have this issue).

- Please add a TODO that we intend to move timer again, once we agree on a good
library to put it in, for instance 'dart:events'. (As we discussed offline, we
will move Futures out of dart:core so it can use timer. I was hoping we can put
it in a dart: library, maybe the same library as timer, and I strongly believe
it doesn't belong in dart:isolate.)

[Siggi Cherem (dart-lang), 2012-06-13 17:21:12]

https://chromiumcodereview.appspot.com/10532123/diff/1018/lib/isolate/timer_a...
File lib/isolate/timer_api.dart (right):

https://chromiumcodereview.appspot.com/10532123/diff/1018/lib/isolate/timer_a...
lib/isolate/timer_api.dart:28: class _TimerFactory {
can we split this file, moving this class and typedef to a separate file? Maybe
Kasper can comment, but I believe for dart2js we will likely provide an
implementation directly in the library code (rather than installing it lazily as
the vm does with the embedder)

[Anton Muhin, 2012-06-18 19:07:09]

lgtm

[Ivan Posva, 2012-06-21 16:17:06]

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/builtin...
File runtime/bin/builtin_natives.cc (right):

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/builtin...
runtime/bin/builtin_natives.cc:142: void Builtin::SetupIOLibrary(Dart_Handle
io_lib) {
On 2012/06/13 17:05:23, asiva wrote:
> The name SetupIOLibrary seems misleading as the function is not really setting
> up the IO library.
> 
> How about SetupTimerImpl(....) ?

I imagine that we will have more than Timer being hooked eventually.

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://chromiumcodereview.appspot.com/10532123/diff/1018/runtime/bin/main.cc...
runtime/bin/main.cc:511: Builtin::kIOLibrary);
On 2012/06/13 17:05:23, asiva wrote:
> I would prefer if we did a Dart_LookupLibrary here instead
> of Builtin::LoadLibrary. We are expected to find the library here. If for some
> reason there is a bug in the snapshot generation we want to assert out when
the
> library is not found instead of silently loading it.

Agreed, but this needs a whole restructuring of the Builtin loading code which
is not part of this change. Also you currently get an assert within
Builtin::LoadLibrary if the library is not found, even if this is not
necessarily the way you wanted to.

If we want to make this change then it should come together with a change to
reduce the standalone snapshot size and making some of the non-critical
libraries loadable from source.