Made SuspendableTimer smaller (removed redudant m_active field)

Made SuspendableTimer smaller (removed redudant m_active field)

And fixed the FIXME in SuspendableTimer::resume()

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183986

[João Eiras, 2014-10-02 14:12:52]

joaoe@opera.com changed reviewers:
+ abarth@chromium.org, eseidel@chromium.org

[João Eiras, 2014-10-02 14:13:25]

Hum, do you think this small CL should have an associated issue ?

[Mike West, 2014-10-04 16:58:49]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-04 16:58:49]

Drive-by LGTM with one comment.

https://codereview.chromium.org/613163006/diff/20001/Source/core/frame/Suspen...
File Source/core/frame/SuspendableTimer.cpp (right):

https://codereview.chromium.org/613163006/diff/20001/Source/core/frame/Suspen...
Source/core/frame/SuspendableTimer.cpp:77: // start() was called before,
therefore location() is already set.
Hrm. It looks like it's safe to use m_nextFireInterval as an isActive proxy
here; it's only set in ::suspend, and only if the timer was active. Can you add
something to the comment here to that effect?

[João Eiras, 2014-10-06 12:24:51]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-10-06 12:25:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613163006/40001

[commit-bot: I haz the power, 2014-10-06 13:32:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-06 13:32:18]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[João Eiras, 2014-10-17 16:04:10]

https://codereview.chromium.org/613163006/diff/60001/Source/core/frame/Suspen...
File Source/core/frame/SuspendableTimer.cpp (right):

https://codereview.chromium.org/613163006/diff/60001/Source/core/frame/Suspen...
Source/core/frame/SuspendableTimer.cpp:35: const double kNextFireIntervalInvalid
= -DBL_MAX;
m_nextFireInterval could be zero for the case of setTimeout(...,0) (I thought
that was rounded up). So, now I'm using this as invalid value. Still lgtm ?

[Mike West, 2014-10-18 09:06:43]

https://codereview.chromium.org/613163006/diff/60001/Source/core/frame/Suspen...
File Source/core/frame/SuspendableTimer.cpp (right):

https://codereview.chromium.org/613163006/diff/60001/Source/core/frame/Suspen...
Source/core/frame/SuspendableTimer.cpp:35: const double kNextFireIntervalInvalid
= -DBL_MAX;
On 2014/10/17 16:04:10, João Eiras wrote:
> m_nextFireInterval could be zero for the case of setTimeout(...,0) (I thought
> that was rounded up). So, now I'm using this as invalid value. Still lgtm ?

Hrm. This kills some of the elegance of the previous solution. Ah well.

1. This value can't ever be negative, can it? If not, then -1 would be fine.
There's no reason to allow all sorts of negative values which are greated than
-DBL_MAX.

2. If we run with -MAX, let's spell it `std::numeric_limits<double>::min()`.

[João Eiras, 2014-10-20 09:40:23]

On 2014/10/18 09:06:43, Mike West wrote:
>
https://codereview.chromium.org/613163006/diff/60001/Source/core/frame/Suspen...
> File Source/core/frame/SuspendableTimer.cpp (right):
> 
>
https://codereview.chromium.org/613163006/diff/60001/Source/core/frame/Suspen...
> Source/core/frame/SuspendableTimer.cpp:35: const double
kNextFireIntervalInvalid
> = -DBL_MAX;
> On 2014/10/17 16:04:10, João Eiras wrote:
> > m_nextFireInterval could be zero for the case of setTimeout(...,0) (I
thought
> > that was rounded up). So, now I'm using this as invalid value. Still lgtm ?
> 
> Hrm. This kills some of the elegance of the previous solution. Ah well.
> 
> 1. This value can't ever be negative, can it? If not, then -1 would be fine.
> There's no reason to allow all sorts of negative values which are greated than
> -DBL_MAX.
> 

Doh, TimerBase::nextUnalignedFireInterval() actually ensures the lower value
returned is 0. So -1 it is.

> 2. If we run with -MAX, let's spell it `std::numeric_limits<double>::min()`.

[Mike West, 2014-10-20 10:13:52]

LGTM if you make the two changes I've suggested inline.

https://codereview.chromium.org/613163006/diff/80001/Source/core/frame/Suspen...
File Source/core/frame/SuspendableTimer.cpp (right):

https://codereview.chromium.org/613163006/diff/80001/Source/core/frame/Suspen...
Source/core/frame/SuspendableTimer.cpp:70: ASSERT(m_nextFireInterval >
kNextFireIntervalInvalid);
Can you please change this check to `m_nextFireInterval >= 0`? That's what
you're actually caring about; the invalid value just happens to be a reasonable
example of a value that doesn't fall into that category.

https://codereview.chromium.org/613163006/diff/80001/Source/core/frame/Suspen...
Source/core/frame/SuspendableTimer.cpp:82: if (m_nextFireInterval >
kNextFireIntervalInvalid) {
Here as well, please.

[João Eiras, 2014-10-20 13:31:15]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-10-20 13:31:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613163006/100001

[commit-bot: I haz the power, 2014-10-20 14:35:41]

Committed patchset #6 (id:100001) as 183986