Add support for a timeout flag.

Add support for a timeout flag.

Closes #279

R=lrn@google.com, kevmoo@google.com

Committed: https://github.com/dart-lang/test/commit/f610d7b17a4270a352be9f96e0c78839950e36a1

[Lasse Reichstein Nielsen, 2016-01-26 09:58:58]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2016-01-26 09:58:59]

lgtm

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
File lib/src/frontend/timeout.dart (right):

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:46: const Timeout(this.duration)
Nothing here prevents you from passing "null" as argument, creating an object
that's like "none", but not identical to it.

I guess nothing can be done in a const constructor. Maybe this doesn't need to
be a const constructor, and then you would be able to enforce that duration is
non-null (until we get non-null types).

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:71: factory Timeout.parse(String timeout) {
We generally make "parse" a static function, not a constructor
(see DateTime, int, double, Uri).

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:110: case "d": return number * 24 * 60 * 60 *
1000000;
Maybe parenthesize (24 * 60 * 60 * 1000000).

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:116: default: throw "Unknown unit $unit.";
Consider making this an error instead of just a string.

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:124: Timeout merge(Timeout other) {
"merge" sounds symmetrical, but this seems like it applies "other" on top of
"this" (with an other.scale applying to this.duration but an other.duration
replacing this.scale).

Also, the documentation doesn't say that either being none means that the result
is none (so "none is contagious" :)

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:139: int get hashCode => duration.hashCode ^ 4 *
scaleFactor.hashCode;
Any particular reason for the 4*? Consider something like 5* (or 31*) to keep
some of the bits of the scale factor in the lower bits.

[nweiz, 2016-01-26 23:13:14]

Code review changes

[nweiz, 2016-01-26 23:13:44]

Description was changed from

==========
Add support for a timeout flag.

Closes #279

R=kevmoo@google.com
==========

to

==========
Add support for a timeout flag.

Closes #279

R=lrn@google.com, kevmoo@google.com

Committed:
https://github.com/dart-lang/test/commit/f610d7b17a4270a352be9f96e0c78839950e...
==========

[nweiz, 2016-01-26 23:13:46]

Committed patchset #3 (id:40001) manually as
f610d7b17a4270a352be9f96e0c78839950e36a1 (presubmit successful).

[nweiz, 2016-01-26 23:15:40]

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
File lib/src/frontend/timeout.dart (right):

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:46: const Timeout(this.duration)
On 2016/01/26 09:58:58, Lasse Reichstein Nielsen wrote:
> Nothing here prevents you from passing "null" as argument, creating an object
> that's like "none", but not identical to it.
> 
> I guess nothing can be done in a const constructor. Maybe this doesn't need to
> be a const constructor, and then you would be able to enforce that duration is
> non-null (until we get non-null types).

It needs to be const because it's used as an annotation.

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:71: factory Timeout.parse(String timeout) {
On 2016/01/26 09:58:59, Lasse Reichstein Nielsen wrote:
> We generally make "parse" a static function, not a constructor
> (see DateTime, int, double, Uri).

This is true in the SDK, but externally pretty much all parse functions are
factory constructors (which I personally prefer since it means *all* APIs that
synchronously create an object from arguments are constructor-like).

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:110: case "d": return number * 24 * 60 * 60 *
1000000;
On 2016/01/26 09:58:59, Lasse Reichstein Nielsen wrote:
> Maybe parenthesize (24 * 60 * 60 * 1000000).

I don't think that adds much.

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:116: default: throw "Unknown unit $unit.";
On 2016/01/26 09:58:58, Lasse Reichstein Nielsen wrote:
> Consider making this an error instead of just a string.

Done.

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:124: Timeout merge(Timeout other) {
On 2016/01/26 09:58:58, Lasse Reichstein Nielsen wrote:
> "merge" sounds symmetrical, but this seems like it applies "other" on top of
> "this" (with an other.scale applying to this.duration but an other.duration
> replacing this.scale).

I guess "compose" might be a more accurate name, but it's inherited from
Metadata.merge which is much more commonly used to merge unrelated metadata than
to compose different versions of the same sort.

> Also, the documentation doesn't say that either being none means that the
result
> is none (so "none is contagious" :)

Done.

https://codereview.chromium.org/1604043003/diff/20001/lib/src/frontend/timeou...
lib/src/frontend/timeout.dart:139: int get hashCode => duration.hashCode ^ 4 *
scaleFactor.hashCode;
On 2016/01/26 09:58:58, Lasse Reichstein Nielsen wrote:
> Any particular reason for the 4*? Consider something like 5* (or 31*) to keep
> some of the bits of the scale factor in the lower bits.

Done.