GetTimeToCallback() returns estimated time to callback.

GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests

Committed: https://crrev.com/ceb6d93b695b137481f050f5b967d882a77b7398
Cr-Commit-Position: refs/heads/master@{#439174}

[The one and only Dr. Crash, 2016-12-06 06:28:58]

Description was changed from

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests
==========

to

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests
==========

[The one and only Dr. Crash, 2016-12-06 06:28:58]

drcrash@chromium.org changed reviewers:
+ mark@chromium.org

[The one and only Dr. Crash, 2016-12-06 06:29:26]

Per our discussion, added a method to return the time to the next callback.

[The one and only Dr. Crash, 2016-12-06 08:35:31]

On 2016/12/06 06:29:26, The one and only Dr. Crash wrote:
> Per our discussion, added a method to return the time to the next callback.

I am open to better names. And maybe it should at least be CallBack (as in call
back). I initially thought TimeToFire but I don't see that we use the term
"fire" anywhere, while all the doc says "... will call back."

[Mark Mentovai, 2016-12-06 18:01:16]

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h#newcode103
base/timer/timer.h:103: // Returns an estimated time to the timer calling  the
user_task_ back. If
Two spaces between “calling” and “the” should only be one. And add to this
comment that the function may return a negative TimeDelta.

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h#newcode106
base/timer/timer.h:106: virtual TimeDelta GetTimeToCallback() const;
No point in making this virtual if nobody overrides it.

[The one and only Dr. Crash, 2016-12-06 20:04:38]

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h#newcode103
base/timer/timer.h:103: // Returns an estimated time to the timer calling  the
user_task_ back. If
On 2016/12/06 18:01:16, Mark Mentovai wrote:
> Two spaces between “calling” and “the” should only be one. And add to this
> comment that the function may return a negative TimeDelta.


Can it return a negative TimeDelta? If the timer isn't running (which will
happens automatically for a one shot timer, for example), it will return
TimeDelta::Max(). If it is running, then it should always be positive.

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h#newcode106
base/timer/timer.h:106: virtual TimeDelta GetTimeToCallback() const;
On 2016/12/06 18:01:16, Mark Mentovai wrote:
> No point in making this virtual if nobody overrides it.

Ok. As far as I could tell, other virtual functions (e.g. GetCurrentDelay())
aren't overriden either.

[Mark Mentovai, 2016-12-06 20:09:54]

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h#newcode103
base/timer/timer.h:103: // Returns an estimated time to the timer calling  the
user_task_ back. If
On 2016/12/06 20:04:38, The one and only Dr. Crash wrote:
> On 2016/12/06 18:01:16, Mark Mentovai wrote:
> > Two spaces between “calling” and “the” should only be one. And add to this
> > comment that the function may return a negative TimeDelta.
> 
> 
> Can it return a negative TimeDelta? If the timer isn't running (which will
> happens automatically for a one shot timer, for example), it will return
> TimeDelta::Max(). If it is running, then it should always be positive.

If things get jammed up and the callback should have fired already but hasn’t
had a chance to run, the delta will be negative.

[The one and only Dr. Crash, 2016-12-06 20:34:08]

OK.

On Tue, Dec 6, 2016, 12:09 <mark@chromium.org> wrote:

>
> https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h
> File base/timer/timer.h (right):
>
>
>
https://codereview.chromium.org/2557663002/diff/1/base/timer/timer.h#newcode103
> base/timer/timer.h:103: // Returns an estimated time to the timer
> calling the user_task_ back. If
> On 2016/12/06 20:04:38, The one and only Dr. Crash wrote:
> > On 2016/12/06 18:01:16, Mark Mentovai wrote:
> > > Two spaces between “calling” and “the” should only be one. And add
> to this
> > > comment that the function may return a negative TimeDelta.
> >
> >
> > Can it return a negative TimeDelta? If the timer isn't running (which
> will
> > happens automatically for a one shot timer, for example), it will
> return
> > TimeDelta::Max(). If it is running, then it should always be positive.
>
> If things get jammed up and the callback should have fired already but
> hasn’t had a chance to run, the delta will be negative.
>
> https://codereview.chromium.org/2557663002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[The one and only Dr. Crash, 2016-12-06 22:33:26]

Ignore patchset 2 (wrong git branch). Patchset 3 does what you asked.

[The one and only Dr. Crash, 2016-12-06 22:33:35]

The CQ bit was checked by drcrash@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:34:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark Mentovai, 2016-12-06 22:37:52]

LGTM

[The one and only Dr. Crash, 2016-12-06 22:45:15]

The CQ bit was unchecked by drcrash@chromium.org

[The one and only Dr. Crash, 2016-12-06 23:30:40]

So I realized that if the timer has fired (or was scheduled to be fired,
unless we want more state in the Timer class to record that it had fired),
I would like GetTimeToCallback() to return a negative time (how long ago it
fired). I was going to look at the desired_/scheduled_run_time_ not being
zero, but the .h says they might actually be zero for tasks that run
immediately, though I don't really see that in the code?

On Tue, Dec 6, 2016 at 2:37 PM, <mark@chromium.org> wrote:

> LGTM
>
> https://codereview.chromium.org/2557663002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[The one and only Dr. Crash, 2016-12-14 19:51:02]

Mark, I made the changes I was talking about, do you want to have a look before
I submit?

[Mark Mentovai, 2016-12-14 20:12:40]

LGTM

[The one and only Dr. Crash, 2016-12-14 20:34:35]

The CQ bit was checked by drcrash@chromium.org

[commit-bot: I haz the power, 2016-12-14 20:36:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 21:04:27]

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

[commit-bot: I haz the power, 2016-12-14 21:04:29]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[The one and only Dr. Crash, 2016-12-15 20:44:14]

The CQ bit was checked by drcrash@chromium.org

[The one and only Dr. Crash, 2016-12-15 20:44:15]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2557663002/#ps80001
(title: "Removed an old file that sneaked in the patchest.")

[commit-bot: I haz the power, 2016-12-15 20:45:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 23:10:32]

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

[commit-bot: I haz the power, 2016-12-15 23:10:33]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[The one and only Dr. Crash, 2016-12-16 17:55:14]

The CQ bit was checked by drcrash@chromium.org

[commit-bot: I haz the power, 2016-12-16 17:55:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 20:10:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481910914546310,
"parent_rev": "9b3286e384f88274269262268681753ba8290bca", "commit_rev":
"ddffe90abf4e3ee57c51d7bed6d3c17e21928d7c"}

[commit-bot: I haz the power, 2016-12-16 20:10:38]

Description was changed from

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests
==========

to

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests

Review-Url: https://codereview.chromium.org/2557663002
==========

[commit-bot: I haz the power, 2016-12-16 20:10:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-16 20:14:27]

Description was changed from

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests

Review-Url: https://codereview.chromium.org/2557663002
==========

to

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests

Committed: https://crrev.com/ceb6d93b695b137481f050f5b967d882a77b7398
Cr-Commit-Position: refs/heads/master@{#439174}
==========

[commit-bot: I haz the power, 2016-12-16 20:14:28]

Patchset 5 (id:??) landed as
https://crrev.com/ceb6d93b695b137481f050f5b967d882a77b7398
Cr-Commit-Position: refs/heads/master@{#439174}

[gab, 2016-12-23 20:21:22]

What was this added for? There's not associated BUG on this CL and I don't see
any other callers still today on trunk?

This call is a pain to handle in my rewrite of base::Timer:
https://codereview.chromium.org/2491613004/

[gab, 2016-12-23 20:21:36]

Description was changed from

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests

Committed: https://crrev.com/ceb6d93b695b137481f050f5b967d882a77b7398
Cr-Commit-Position: refs/heads/master@{#439174}
==========

to

==========
GetTimeToCallback() returns estimated time to callback.

BUG=none
TEST=unit tests

Committed: https://crrev.com/ceb6d93b695b137481f050f5b967d882a77b7398
Cr-Commit-Position: refs/heads/master@{#439174}
==========

[The one and only Dr. Crash, 2016-12-28 18:04:45]

I added that to be able to handle timeouts across multiple calls in a
pending CL. I can probably deal with that in a different way, pushing the
complexity to the client. However, it's pretty useful to be enable how much
time one has left on a timer... Why is it a pain to handle?

On Fri, Dec 23, 2016 at 12:21 PM, <gab@chromium.org> wrote:

> What was this added for? There's not associated BUG on this CL and I don't
> see
> any other callers still today on trunk?
>
> This call is a pain to handle in my rewrite of base::Timer:
> https://codereview.chromium.org/2491613004/
>
> https://codereview.chromium.org/2557663002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2017-01-05 22:21:20]

On 2016/12/28 18:04:45, The one and only Dr. Crash wrote:
> I added that to be able to handle timeouts across multiple calls in a
> pending CL. I can probably deal with that in a different way, pushing the
> complexity to the client. However, it's pretty useful to be enable how much
> time one has left on a timer... Why is it a pain to handle?

Because in https://codereview.chromium.org/2491613004/, the delayed task can be
on a separate sequence when SetTaskRunner() is used (as it should currently be
but the current impl is racy). In that CL, another internal object owns "time"
and it can only be queried asynchronously if SetTaskRunner() is used.

> 
> On Fri, Dec 23, 2016 at 12:21 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> 
> > What was this added for? There's not associated BUG on this CL and I don't
> > see
> > any other callers still today on trunk?
> >
> > This call is a pain to handle in my rewrite of base::Timer:
> > https://codereview.chromium.org/2491613004/
> >
> > https://codereview.chromium.org/2557663002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

[The one and only Dr. Crash, 2017-01-06 05:36:49]

Ok, get rid of that method then and I'll do something on my side.

On Thu, Jan 5, 2017 at 2:21 PM, <gab@chromium.org> wrote:

> On 2016/12/28 18:04:45, The one and only Dr. Crash wrote:
> > I added that to be able to handle timeouts across multiple calls in a
> > pending CL. I can probably deal with that in a different way, pushing the
> > complexity to the client. However, it's pretty useful to be enable how
> much
> > time one has left on a timer... Why is it a pain to handle?
>
> Because in https://codereview.chromium.org/2491613004/, the delayed task
> can be
> on a separate sequence when SetTaskRunner() is used (as it should
> currently be
> but the current impl is racy). In that CL, another internal object owns
> "time"
> and it can only be queried asynchronously if SetTaskRunner() is used.
>
> >
> > On Fri, Dec 23, 2016 at 12:21 PM,
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org&gt;
> wrote:
> >
> > > What was this added for? There's not associated BUG on this CL and I
> don't
> > > see
> > > any other callers still today on trunk?
> > >
> > > This call is a pain to handle in my rewrite of base::Timer:
> > > https://codereview.chromium.org/2491613004/
> > >
> > > https://codereview.chromium.org/2557663002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-
> reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2557663002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2017-01-09 16:17:06]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2621663002/ by gab@chromium.org.

The reason for reverting is: Removing Timer::GetTimeToCallback() as discussed
post commit on CL introducing it..