Add isActive field on Timer.

Add isActive field on Timer.

BUG= http://dartbug.com/10010
R=floitsch@google.com

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

[zarah, 2013-07-01 11:55:51]

Ptal

[floitsch, 2013-07-01 12:49:43]

FYI.

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

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/timer.dart#newc...
sdk/lib/async/timer.dart:62: * Returns true if the timer is not done, that is it
will run the callback.
Returns whether the timer is still active.

A non-periodic callback is active if the callback has not been executed, and the
timer has not been canceled.

A periodic callback is active if it has not been canceled.

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/zone.dart#newco...
sdk/lib/async/zone.dart:439: bool _isDone = false;
Remove _isDone and use _timer._isActive instead.

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/zone.dart#newco...
sdk/lib/async/zone.dart:487: bool get isActive => !_isDone;
ditto.

https://codereview.chromium.org/18325006/diff/1/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/18325006/diff/1/sdk/lib/io/timer_impl.dart#ne...
sdk/lib/io/timer_impl.dart:53: bool get _isActive => _timers.contains(this);
contains is a linear operation.
Reuse _callback instead.
It is currently not cleared for single-shot timers, but that's easy to add in
line 148.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
File tests/lib/async/timer_isActive_test.dart (right):

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:13: void checkNotActive() =>
expect(t.isActive, equals(false));
nit: if the function is that small, just use it at its use-site.

Timer timer;
timer = new Timer(const Duration(seconds: 1),
                  expectAsync0(() => expect(timer.isActive, equals(false)));

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:34: });
check that timer is active outside callback.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:47: expect(timer.isActive, equals(t));
t?

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:50: timer = new Timer(new
Duration(milliseconds: 1),
nits:
no need to assign it if you don't use it later.

Use Timer.run if you just want to yield before doing something:
Timer.run(expectAsync0(cancelOtherTimer));

I would assign the other timer first, so that the test becomes easier to read:

Timer timer = new Timer(const Duration(seconds: 1),
                        () => fail("...");
Timer.run(expectAsync0(() {
  expect(timer.isActive, equals(true));
  timer.cancel();
  expect(timer.isActive, equals(false));
});

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:52: timer = new Timer(new
Duration(milliseconds: 1000),
seconds: 1

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:53: expectAsync0(unreachable, count:
0));
this line should not be reached. -> don't expectAsync0 it.

https://codereview.chromium.org/18325006/diff/1/tools/dom/src/native_DOMImple...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/18325006/diff/1/tools/dom/src/native_DOMImple...
tools/dom/src/native_DOMImplementation.dart:226: final int id = maker(() {
callback(timer); }, milliSeconds);
This callback invocation will not set the _isDone in the _Timer.

I would prefer not to change a private field in a different class. So try to
restructure the code so that the callback (or the whole setup) goes through the
_Timer class.

CC @blois for these changes.

[zarah, 2013-07-01 16:15:35]

Ptal

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

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/timer.dart#newc...
sdk/lib/async/timer.dart:62: * Returns true if the timer is not done, that is it
will run the callback.
On 2013/07/01 12:49:43, floitsch wrote:
> Returns whether the timer is still active.
> 
> A non-periodic callback is active if the callback has not been executed, and
the
> timer has not been canceled.
> 
> A periodic callback is active if it has not been canceled.

Done.

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/zone.dart#newco...
sdk/lib/async/zone.dart:439: bool _isDone = false;
On 2013/07/01 12:49:43, floitsch wrote:
> Remove _isDone and use _timer._isActive instead.

Done.

https://codereview.chromium.org/18325006/diff/1/sdk/lib/async/zone.dart#newco...
sdk/lib/async/zone.dart:487: bool get isActive => !_isDone;
On 2013/07/01 12:49:43, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18325006/diff/1/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/18325006/diff/1/sdk/lib/io/timer_impl.dart#ne...
sdk/lib/io/timer_impl.dart:53: bool get _isActive => _timers.contains(this);
On 2013/07/01 12:49:43, floitsch wrote:
> contains is a linear operation.
> Reuse _callback instead.
> It is currently not cleared for single-shot timers, but that's easy to add in
> line 148.

Done.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
File tests/lib/async/timer_isActive_test.dart (right):

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:13: void checkNotActive() =>
expect(t.isActive, equals(false));
On 2013/07/01 12:49:43, floitsch wrote:
> nit: if the function is that small, just use it at its use-site.
> 
> Timer timer;
> timer = new Timer(const Duration(seconds: 1),
>                   expectAsync0(() => expect(timer.isActive, equals(false)));

Done.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:34: });
On 2013/07/01 12:49:43, floitsch wrote:
> check that timer is active outside callback.

Done.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:47: expect(timer.isActive, equals(t));
On 2013/07/01 12:49:43, floitsch wrote:
> t?

t-> true

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:50: timer = new Timer(new
Duration(milliseconds: 1),
On 2013/07/01 12:49:43, floitsch wrote:
> nits:
> no need to assign it if you don't use it later.
> 
> Use Timer.run if you just want to yield before doing something:
> Timer.run(expectAsync0(cancelOtherTimer));
> 
> I would assign the other timer first, so that the test becomes easier to read:
> 
> Timer timer = new Timer(const Duration(seconds: 1),
>                         () => fail("...");
> Timer.run(expectAsync0(() {
>   expect(timer.isActive, equals(true));
>   timer.cancel();
>   expect(timer.isActive, equals(false));
> });

Done.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:52: timer = new Timer(new
Duration(milliseconds: 1000),
On 2013/07/01 12:49:43, floitsch wrote:
> seconds: 1

Done.

https://codereview.chromium.org/18325006/diff/1/tests/lib/async/timer_isActiv...
tests/lib/async/timer_isActive_test.dart:53: expectAsync0(unreachable, count:
0));
On 2013/07/01 12:49:43, floitsch wrote:
> this line should not be reached. -> don't expectAsync0 it.

Done.

https://codereview.chromium.org/18325006/diff/1/tools/dom/src/native_DOMImple...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/18325006/diff/1/tools/dom/src/native_DOMImple...
tools/dom/src/native_DOMImplementation.dart:226: final int id = maker(() {
callback(timer); }, milliSeconds);
On 2013/07/01 12:49:43, floitsch wrote:
> This callback invocation will not set the _isDone in the _Timer.
> 
> I would prefer not to change a private field in a different class. So try to
> restructure the code so that the callback (or the whole setup) goes through
the
> _Timer class.
> 
> CC @blois for these changes.

Done.

[floitsch, 2013-07-01 17:50:56]

LGTM.

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/_internal/lib/isol...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/_internal/lib/isol...
sdk/lib/_internal/lib/isolate_helper.dart:1432: bool get isActive => (_handle !=
null);
no need for the parenthesis.

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

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/async/timer.dart#n...
sdk/lib/async/timer.dart:64: * A non-periodic callback is active if the callback
has not been executed,
My fault should have been:
"A non-periodic timer is active ..."
"A periodic timer ..."

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/io/timer_impl.dart...
sdk/lib/io/timer_impl.dart:150: timer._callback = null;
Add comment: // Mark timer as inactive.

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
File tests/lib/async/timer_isActive_test.dart (right):

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:14: expectAsync0( () =>
expect(t.isActive, equals(false))));
no need for space.

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:39: () => fail("Should not be
reached."));
if possible indent with other argument. If not, move first argument to next line
too, or move the whole 'new Timer' to the next line.
Alternatively:
Timer timer = new Timer(const Duration(seconds: 1), () {
  fail("Should not be reached");
});

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:44: }));
indentation (should be 2 spaces to the left).

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:45: });
ditto.

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
tools/dom/src/native_DOMImplementation.dart:207: factory
_Timer.timerFactoryClosure(int milliSeconds,
Make this the default constructor. If you use the `canceller` field as indicator
if the timer is active (see below) then you can make this a non-factory
constructor.

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
tools/dom/src/native_DOMImplementation.dart:220: final int id = maker(() {
_isDone = true; callback(timer);}, milliSeconds);
Don't put more than one statement on the same line.

You could also use the _canceller field (which should be private and called
"canceler") as indication instead of having an extra boolean field.

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
tools/dom/src/native_DOMImplementation.dart:221: timer = new _Timer(() {
canceller(id); });
Nit: it is annoying to me that we wrap the canceler in two closures.
Make `id` non-final, move it above the if and create the full canceler directly:
canceler = () => window._clearInterval(id);

[zarah, 2013-07-02 11:22:01]

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/_internal/lib/isol...
File sdk/lib/_internal/lib/isolate_helper.dart (right):

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/_internal/lib/isol...
sdk/lib/_internal/lib/isolate_helper.dart:1432: bool get isActive => (_handle !=
null);
On 2013/07/01 17:50:56, floitsch wrote:
> no need for the parenthesis.

Done.

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

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/async/timer.dart#n...
sdk/lib/async/timer.dart:64: * A non-periodic callback is active if the callback
has not been executed,
On 2013/07/01 17:50:56, floitsch wrote:
> My fault should have been:
> "A non-periodic timer is active ..."
> "A periodic timer ..."

Done.

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/18325006/diff/9001/sdk/lib/io/timer_impl.dart...
sdk/lib/io/timer_impl.dart:150: timer._callback = null;
On 2013/07/01 17:50:56, floitsch wrote:
> Add comment: // Mark timer as inactive.

Done.

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
File tests/lib/async/timer_isActive_test.dart (right):

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:14: expectAsync0( () =>
expect(t.isActive, equals(false))));
On 2013/07/01 17:50:56, floitsch wrote:
> no need for space.

Done.

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:39: () => fail("Should not be
reached."));
On 2013/07/01 17:50:56, floitsch wrote:
> if possible indent with other argument. If not, move first argument to next
line
> too, or move the whole 'new Timer' to the next line.
> Alternatively:
> Timer timer = new Timer(const Duration(seconds: 1), () {
>   fail("Should not be reached");
> });

Done.

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:44: }));
On 2013/07/01 17:50:56, floitsch wrote:
> indentation (should be 2 spaces to the left).

Done.

https://codereview.chromium.org/18325006/diff/9001/tests/lib/async/timer_isAc...
tests/lib/async/timer_isActive_test.dart:45: });
On 2013/07/01 17:50:56, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
tools/dom/src/native_DOMImplementation.dart:207: factory
_Timer.timerFactoryClosure(int milliSeconds,
On 2013/07/01 17:50:56, floitsch wrote:
> Make this the default constructor. If you use the `canceller` field as
indicator
> if the timer is active (see below) then you can make this a non-factory
> constructor.

Done.

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
tools/dom/src/native_DOMImplementation.dart:220: final int id = maker(() {
_isDone = true; callback(timer);}, milliSeconds);
On 2013/07/01 17:50:56, floitsch wrote:
> Don't put more than one statement on the same line.
> 
> You could also use the _canceller field (which should be private and called
> "canceler") as indication instead of having an extra boolean field.

Done.

https://codereview.chromium.org/18325006/diff/9001/tools/dom/src/native_DOMIm...
tools/dom/src/native_DOMImplementation.dart:221: timer = new _Timer(() {
canceller(id); });
On 2013/07/01 17:50:56, floitsch wrote:
> Nit: it is annoying to me that we wrap the canceler in two closures.
> Make `id` non-final, move it above the if and create the full canceler
directly:
> canceler = () => window._clearInterval(id);

Done.

[floitsch, 2013-07-02 11:27:19]

Almost there.
LGTM once comments are addressed.

https://codereview.chromium.org/18325006/diff/16001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/18325006/diff/16001/sdk/lib/io/timer_impl.dar...
sdk/lib/io/timer_impl.dart:53: bool get isActive => (_callback != null);
Remove parenthesis.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:201: class _Timer implements Timer {
Remove leading space.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:202: final _canceler;
Can not be final, since you change it.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:208: _canceler = null;
No. Don't remove the canceler for repeating timers.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:217: _canceler =
window._clearTimeout(id);
This must be a closure.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:238: bool _isDone = false;
Rename _isDone to _isActive

[zarah, 2013-07-02 11:43:12]

https://codereview.chromium.org/18325006/diff/16001/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/18325006/diff/16001/sdk/lib/io/timer_impl.dar...
sdk/lib/io/timer_impl.dart:53: bool get isActive => (_callback != null);
On 2013/07/02 11:27:19, floitsch wrote:
> Remove parenthesis.

Done.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:201: class _Timer implements Timer {
On 2013/07/02 11:27:19, floitsch wrote:
> Remove leading space.

Done.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:202: final _canceler;
On 2013/07/02 11:27:19, floitsch wrote:
> Can not be final, since you change it.

Done, of course!

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:208: _canceler = null;
On 2013/07/02 11:27:19, floitsch wrote:
> No. Don't remove the canceler for repeating timers.

Done.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:217: _canceler =
window._clearTimeout(id);
On 2013/07/02 11:27:19, floitsch wrote:
> This must be a closure.

Done.

https://codereview.chromium.org/18325006/diff/16001/tools/dom/src/native_DOMI...
tools/dom/src/native_DOMImplementation.dart:238: bool _isDone = false;
On 2013/07/02 11:27:19, floitsch wrote:
> Rename _isDone to _isActive

Done.

[zarah, 2013-07-02 12:01:21]

Committed patchset #4 manually as r24678 (presubmit successful).