Rework Timer interface.

Rework Timer interface.

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

[floitsch, 2013-02-08 20:02:57]

Once you ok the change I will add one of the VM devs.

[floitsch, 2013-02-08 21:35:25]

Clarified behavior for negative durations.

[floitsch, 2013-02-08 23:30:35]

Fixed all users of the Timer class.
More comments.

[floitsch, 2013-02-08 23:46:55]

minor change to test.

[Lasse Reichstein Nielsen, 2013-02-11 13:01:21]

lgtm

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.dart
File runtime/lib/timer_patch.dart (right):

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.d...
runtime/lib/timer_patch.dart:12: if (callback is! _TimerCallback0 && callback
is! _TimerCallback1) {
Does this work in dart2js?

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.d...
runtime/lib/timer_patch.dart:15: if (duration is! Duration && duration is! int)
{
Don't check for Duration, just assume it if it isn't int.
I.e., do just what you are doing below.

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.d...
runtime/lib/timer_patch.dart:26: }
Why is this code in a patch file? Everything except _TimerFactory is generic,
and shouldn't need to be copied.

Can you abstract the creation of the timer alone, e.g., as a private external
method?

https://codereview.chromium.org/12213092/diff/12001/samples/chat/chat_server....
File samples/chat/chat_server.dart (right):

https://codereview.chromium.org/12213092/diff/12001/samples/chat/chat_server....
samples/chat/chat_server.dart:25: new Timer(const Duration(seconds: stopAfter),
serverMain.shutdown);
This can't be const. Is this code tested?

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/async_patch.dart (right):

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:26: callback is
_TimerCallback0 ? callback : () => callback(timer);
Per VM patch file: this is duplicated code. Try to abstract the things that
changes only.

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/async/timer.dart
File sdk/lib/async/timer.dart (right):

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/async/timer.dart#...
sdk/lib/async/timer.dart:14: static Timer run(void callback()) {
Static methods below constructors.

https://codereview.chromium.org/12213092/diff/12001/tests/standalone/io/secur...
File tests/standalone/io/secure_session_resume_test.dart (right):

https://codereview.chromium.org/12213092/diff/12001/tests/standalone/io/secur...
tests/standalone/io/secure_session_resume_test.dart:126: Duration delay = const
Duration();
This should not be allowed. Rally. I assume it's zero, but it's NOT obvious.

[floitsch, 2013-02-11 19:21:32]

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.dart
File runtime/lib/timer_patch.dart (right):

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.d...
runtime/lib/timer_patch.dart:12: if (callback is! _TimerCallback0 && callback
is! _TimerCallback1) {
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> Does this work in dart2js?

It should. I tested it for dart2js.

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.d...
runtime/lib/timer_patch.dart:15: if (duration is! Duration && duration is! int)
{
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> Don't check for Duration, just assume it if it isn't int.
> I.e., do just what you are doing below.

Done.

https://codereview.chromium.org/12213092/diff/12001/runtime/lib/timer_patch.d...
runtime/lib/timer_patch.dart:26: }
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> Why is this code in a patch file? Everything except _TimerFactory is generic,
> and shouldn't need to be copied.
> 
> Can you abstract the creation of the timer alone, e.g., as a private external
> method?

Filed http://dartbug.com/8467

https://codereview.chromium.org/12213092/diff/12001/samples/chat/chat_server....
File samples/chat/chat_server.dart (right):

https://codereview.chromium.org/12213092/diff/12001/samples/chat/chat_server....
samples/chat/chat_server.dart:25: new Timer(const Duration(seconds: stopAfter),
serverMain.shutdown);
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> This can't be const. Is this code tested?

strange. I will make sure to run all the tests.
done.

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/async_patch.dart (right):

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/async_patch.dart:26: callback is
_TimerCallback0 ? callback : () => callback(timer);
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> Per VM patch file: this is duplicated code. Try to abstract the things that
> changes only.

Same issue.

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/async/timer.dart
File sdk/lib/async/timer.dart (right):

https://codereview.chromium.org/12213092/diff/12001/sdk/lib/async/timer.dart#...
sdk/lib/async/timer.dart:14: static Timer run(void callback()) {
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> Static methods below constructors.

Done.

https://codereview.chromium.org/12213092/diff/12001/tests/standalone/io/secur...
File tests/standalone/io/secure_session_resume_test.dart (right):

https://codereview.chromium.org/12213092/diff/12001/tests/standalone/io/secur...
tests/standalone/io/secure_session_resume_test.dart:126: Duration delay = const
Duration();
On 2013/02/11 13:01:21, Lasse Reichstein Nielsen wrote:
> This should not be allowed. Rally. I assume it's zero, but it's NOT obvious.

We can't really avoid it.
made it clear.