components: Introduce AlarmTimer class and use it for GCM heartbeat

components: Introduce AlarmTimer class and use it for GCM heartbeat

This adds the AlarmTimer class to components, which is capable of waking
up the system from suspend on platforms that support this operation
(currently only Chrome OS with linux version 3.11 or higher).  On all
other platforms, the AlarmTimer behaves exactly the same as a regular
Timer.

BUG=crosbug.com/p/32272

Committed: https://crrev.com/26436e402e6e1b780f6565539ae0acb3e2976fb9
Cr-Commit-Position: refs/heads/master@{#301175}

[Chirantan Ekbote, 2014-10-09 01:36:18]

chirantan@chromium.org changed reviewers:
+ darin@chromium.org, zea@chromium.org

[Chirantan Ekbote, 2014-10-09 01:36:18]

Hi Darin,

Apologies for the noise but I wasn't sure who was the right person to review
this.  This CL adds a new timer class called AlarmTimer that is capable of
waking the system up from suspend in order to run some task.  We need this for
lucid sleep so that we can keep the connection between the GCM client and the
server alive by regularly waking up to send heartbeat messages.  We'd also like
to have it for the chrome os network manager (shill) so that it can wake up to
send NAT keepalive messages and the power manager (powerd) so that it can
periodically check the battery status.  It seemed like base/ was the best place
to put this so that 1) gcm wouldn't have to depend on chromeos/ and 2) powerd
and shill could also use the same implementation since they link against base/.

Hi Nicolas, please review the gcm bits as owner.

Thank you.

[darin (slow to review), 2014-10-09 03:41:36]

Why does this make sense as a feature of base/? Who are the intended consumers?

[Chirantan Ekbote, 2014-10-09 04:01:42]

On 2014/10/09 03:41:36, darin wrote:
> Why does this make sense as a feature of base/? Who are the intended
consumers?

The intended consumers are gcm, shill, and powerd.  Putting it in base/ allows
all three to share the same implementation since shill and powerd are chrome os
system daemons that link against base/.

[Nicolas Zea, 2014-10-09 04:01:43]

Agree that this isn't necessarily appropriate for base/. I think it might make
more sense to just put it next to the heartbeat_manager.h in
google_apis/gcm/engine, at least until something else in the chrome codebase
depends on it too.

That said, fixing the heartbeat to wake up the system seems like I good idea on
systems where we're also waking up when network messages are received.

One question I have: I've been thinking about switching to an alarm
implementation that doesn't use TimeTicks, as that value can stop increasing
while suspended. Does it make sense to still be using TimeTicks here?

[Chirantan Ekbote, 2014-10-09 18:12:17]

On 2014/10/09 04:01:43, Nicolas Zea wrote:
> Agree that this isn't necessarily appropriate for base/. I think it might make
> more sense to just put it next to the heartbeat_manager.h in
> google_apis/gcm/engine, at least until something else in the chrome codebase
> depends on it too.
> 

As I mentioned earlier, I don't agree with this.  If we move this out of base/
then we have to re-implement it on the chrome os side and that implementation
will be slightly different than the one that ends up in google_apis/gcm/engine. 
Any bugs that come up will manifest in slightly different ways in the two
versions and will require slightly different fixes.  I guess I just don't see
the benefit of keeping this out of base/ when compared to the cost of having to
maintain two slightly different versions of essentially the same thing.

> That said, fixing the heartbeat to wake up the system seems like I good idea
on
> systems where we're also waking up when network messages are received.
> 
> One question I have: I've been thinking about switching to an alarm
> implementation that doesn't use TimeTicks, as that value can stop increasing
> while suspended. Does it make sense to still be using TimeTicks here?

My suggestion would be to fix TimeTicks instead to include time spent in
suspend.  It looks like we already handle this on mac and windows and we can fix
it at least on linux by changing the clock_gettime syscall to use CLOCK_BOOTTIME
instead of CLOCK_MONOTONIC (https://lwn.net/Articles/428176/).

[Chirantan Ekbote, 2014-10-14 21:39:06]

chirantan@chromium.org changed reviewers:
+ brettw@chromium.org
- darin@chromium.org

[Chirantan Ekbote, 2014-10-14 21:41:51]

Hi Brett,

Please take a look at this CL as owner of base/.  While the AlarmTimer class
only has one consumer within chrome (GCM), it has two consumers in chrome os
(shill and powerd) and we would really like all three to share the same code.

Thank you.

[Chirantan Ekbote, 2014-10-14 21:45:51]

chirantan@chromium.org changed reviewers:
+ thestig@chromium.org
- brettw@chromium.org

[Chirantan Ekbote, 2014-10-14 21:47:27]

Sigh... I just noticed that Brett doesn't appear to be an owner of base/
anymore.  Lei, please take a look as owner of base/.

[Lei Zhang, 2014-10-15 00:03:09]

So shill and powerd are both CrOS projects that use chromeos's libbase right?

What are the costs of having two copies? How much of a maintenance burden is it?
This looks like code that once you have working, probably won't change for a
long time.

Brett's advise for new base/ OWNERS is to essentially keep new things out of
base/ unless there's a great need for it.

[Chirantan Ekbote, 2014-10-15 01:40:56]

On 2014/10/15 00:03:09, Lei Zhang wrote:
> So shill and powerd are both CrOS projects that use chromeos's libbase right?
> 

Yes that's correct.

> What are the costs of having two copies? How much of a maintenance burden is
it?
> This looks like code that once you have working, probably won't change for a
> long time.
> 

I can't quantify the maintenance burden since this is pretty new code.  I will
say though that this touches the suspend/resume code path, which tends to be one
of the most buggy areas that we deal with on the kernel team.  Code that works
now may become flaky in the future because of some seemingly unrelated change
and having only one copy of this code would give us one less thing to worry
about.

> Brett's advise for new base/ OWNERS is to essentially keep new things out of
> base/ unless there's a great need for it.

I understand the need to keep the amount of churn in base/ minimal. 
Unfortunately, base/ is the only thing that's shared between chrome and chrome
os, even though we're all really part of the same team.  I would gladly stick
this somewhere else where it could still be shared.

[Lei Zhang, 2014-10-15 06:36:12]

Let's get another opinion from derat@, who has more chrome + chromeos
experience.

[Daniel Erat, 2014-10-15 15:03:51]

On 2014/10/15 06:36:12, Lei Zhang wrote:
> Let's get another opinion from derat@, who has more chrome + chromeos
> experience.

i'd rather have this in base than be duplicated across multiple places, but i'm
not a base owner. :-P

re earlier discussion: i'd be very careful if switching TimeTicks from
CLOCK_MONOTONIC to CLOCK_BOOTTIME, as there may be code that depends on the
existing behavior. (with that said, i think that there's also some
power-management-related code, both in chrome and in powerd, that uses Time
instead of TimeTicks because it explicitly wants to know how much time elapsed
while suspended. that code would benefit from switching to TimeTicks if it used
CLOCK_BOOTTIME.)

[Daniel Erat, 2014-10-15 15:04:20]

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

[Daniel Erat, 2014-10-15 15:04:22]

https://codereview.chromium.org/641943002/diff/50001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/641943002/diff/50001/base/base.gypi#newcode667
base/base.gypi:667: 'timer/rtc_alarm_chromeos.cc',
nit: fix alphabetization

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.cc
File base/timer/alarm_timer.cc (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:62: #if defined(OS_CHROMEOS)
i don't have strong feelings here, but it might be nice to all of the post-Timer
initialization into a separate method so it isn't duplicated between the two
c'tors.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:78: if (!can_wake_from_suspend_)
nit:

  return can_wake_from_suspend_ ? is_running_ : Timer::IsRunning();

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:85: if (!can_wake_from_suspend_)
nit:

  return can_wake_from_suspend_ ? delay_ : Timer::GetCurrentDelay();

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:100: // one anyway for clarity.
not sure i understand this. if this method is identical to Timer::Start(), why
is it being overridden?

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:182: // Now lets run the user task.
nit: s/lets/let's/ or just delete that word

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h
File base/timer/alarm_timer.h (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h...
base/timer/alarm_timer.h:40: // Stop the currently running timer.  It should be
safe to call this more
nit: s/Stop/Stops/

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h...
base/timer/alarm_timer.h:44: // Reset the timer to fire after |delay| amount of
time has passed.  Cancels
nit: s/Reset/Resets/; can also remove "amount of time" here

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h...
base/timer/alarm_timer.h:103: }  // namespace base
nit: add blank line after this one

https://codereview.chromium.org/641943002/diff/50001/base/timer/rtc_alarm_chr...
File base/timer/rtc_alarm_chromeos.cc (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/rtc_alarm_chr...
base/timer/rtc_alarm_chromeos.cc:28: CHECK(ret);
nit: just put the CHECK() around the call above?

https://codereview.chromium.org/641943002/diff/50001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/timer.h#newco...
base/timer/timer.h:137: // If true, hold on to the user_task_ closure object for
reuse.
nit: s/the user_task_/|user_task_|/

https://codereview.chromium.org/641943002/diff/50001/base/timer/timer.h#newco...
base/timer/timer.h:140: // If true, user_task_ is scheduled to run sometime in
the future.
nit: |user_task_|

https://codereview.chromium.org/641943002/diff/50001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/50001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.h:69: base::Timer* heartbeat_timer_;
please document the ownership of this pointer

[Nicolas Zea, 2014-10-15 18:00:52]

do the message pump implementations themselves need to be modified to wake up?
Its not clear to me the timers they use will take into account the time spent
while suspended.

I'm wondering if we need something similar to
https://codereview.chromium.org/342011 for all platforms that support the alarm
timer.

[Lei Zhang, 2014-10-15 18:41:00]

On 2014/10/15 15:03:51, Daniel Erat wrote:
> On 2014/10/15 06:36:12, Lei Zhang wrote:
> > Let's get another opinion from derat@, who has more chrome + chromeos
> > experience.
> 
> i'd rather have this in base than be duplicated across multiple places, but
i'm
> not a base owner. :-P

Is there no other place where chrome and chromeos can share code?

[Daniel Erat, 2014-10-15 20:11:01]

On 2014/10/15 18:41:00, Lei Zhang wrote:
> On 2014/10/15 15:03:51, Daniel Erat wrote:
> > On 2014/10/15 06:36:12, Lei Zhang wrote:
> > > Let's get another opinion from derat@, who has more chrome + chromeos
> > > experience.
> > 
> > i'd rather have this in base than be duplicated across multiple places, but
> i'm
> > not a base owner. :-P
> 
> Is there no other place where chrome and chromeos can share code?

there's system_api, but i think we only use that to share headers and .proto
files currently.

[Chirantan Ekbote, 2014-10-16 21:26:39]

ptal.  When I removed the timer.h include from heartbeat_manager.h and forward
declared it instead, I uncovered some missing includes in other files (mainly
bind.h and bind_helpers.h) so I've fixed those here as well.

Re:CLOCK_BOOTTIME vs CLOCK_MONOTONIC
This probably deserves its own separate discussion but the TimeTicks
implementations for mac, ios, and windows all include the time spent in suspend.
 So it would be really surprising if something depended on that time *not* being
included.  Unless there's something in Chrome OS that depends on that behavior.

https://codereview.chromium.org/641943002/diff/50001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/641943002/diff/50001/base/base.gypi#newcode667
base/base.gypi:667: 'timer/rtc_alarm_chromeos.cc',
On 2014/10/15 15:04:21, Daniel Erat wrote:
> nit: fix alphabetization

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.cc
File base/timer/alarm_timer.cc (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:62: #if defined(OS_CHROMEOS)
On 2014/10/15 15:04:22, Daniel Erat wrote:
> i don't have strong feelings here, but it might be nice to all of the
post-Timer
> initialization into a separate method so it isn't duplicated between the two
> c'tors.

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:100: // one anyway for clarity.
On 2014/10/15 15:04:21, Daniel Erat wrote:
> not sure i understand this. if this method is identical to Timer::Start(), why
> is it being overridden?

It's being overridden so that someone reading the code doesn't have to look
through two files to see the whole implementation.  If it's ok to not override
these and just re-use the Timer implementation, then we can also get rid of
IsRunning() and GetCurrentDelay(), which is what I've done in the latest patch
set.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.c...
base/timer/alarm_timer.cc:182: // Now lets run the user task.
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: s/lets/let's/ or just delete that word

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h
File base/timer/alarm_timer.h (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h...
base/timer/alarm_timer.h:40: // Stop the currently running timer.  It should be
safe to call this more
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: s/Stop/Stops/

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h...
base/timer/alarm_timer.h:44: // Reset the timer to fire after |delay| amount of
time has passed.  Cancels
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: s/Reset/Resets/; can also remove "amount of time" here

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/alarm_timer.h...
base/timer/alarm_timer.h:103: }  // namespace base
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: add blank line after this one

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/rtc_alarm_chr...
File base/timer/rtc_alarm_chromeos.cc (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/rtc_alarm_chr...
base/timer/rtc_alarm_chromeos.cc:28: CHECK(ret);
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: just put the CHECK() around the call above?

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/50001/base/timer/timer.h#newco...
base/timer/timer.h:137: // If true, hold on to the user_task_ closure object for
reuse.
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: s/the user_task_/|user_task_|/

Done.

https://codereview.chromium.org/641943002/diff/50001/base/timer/timer.h#newco...
base/timer/timer.h:140: // If true, user_task_ is scheduled to run sometime in
the future.
On 2014/10/15 15:04:22, Daniel Erat wrote:
> nit: |user_task_|

Done.

https://codereview.chromium.org/641943002/diff/50001/google_apis/gcm/engine/h...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/50001/google_apis/gcm/engine/h...
google_apis/gcm/engine/heartbeat_manager.h:69: base::Timer* heartbeat_timer_;
On 2014/10/15 15:04:22, Daniel Erat wrote:
> please document the ownership of this pointer

Done.

[Lei Zhang, 2014-10-16 21:38:36]

On 2014/10/15 20:11:01, Daniel Erat wrote:
> On 2014/10/15 18:41:00, Lei Zhang wrote:
> > On 2014/10/15 15:03:51, Daniel Erat wrote:
> > > On 2014/10/15 06:36:12, Lei Zhang wrote:
> > > > Let's get another opinion from derat@, who has more chrome + chromeos
> > > > experience.
> > > 
> > > i'd rather have this in base than be duplicated across multiple places,
but
> > i'm
> > > not a base owner. :-P
> > 
> > Is there no other place where chrome and chromeos can share code?
> 
> there's system_api, but i think we only use that to share headers and .proto
> files currently.

So should there be some other place aside from base/ where CrOS daemons and CrOS
in Chrome can share code? There's the top-level chromeos/ directory, but that's
chrome-only right? As is, we'd have to put code in base/ or have separate
copies, neither of which makes everyone happy.

Let's say we put it in base/, then developers may react as follows: "Oh look, a
fancy AlarmTimer class in base/, let me try using it in my code. Oh wait, it
only works on CrOS."

[Chirantan Ekbote, 2014-10-16 21:49:55]

On 2014/10/15 18:00:52, Nicolas Zea wrote:
> do the message pump implementations themselves need to be modified to wake up?
> Its not clear to me the timers they use will take into account the time spent
> while suspended.
> 
> I'm wondering if we need something similar to
> https://codereview.chromium.org/342011 for all platforms that support the
alarm
> timer.

On linux, we use libevent to implement the message pump.  I haven't found any
documentation that says what it does during system suspend but I'm guessing that
it does include the time somehow or we would have had the same problem on linux
that that CL was trying to fix.

[Chirantan Ekbote, 2014-10-16 21:54:48]

On 2014/10/16 21:38:36, Lei Zhang wrote:
> On 2014/10/15 20:11:01, Daniel Erat wrote:
> > On 2014/10/15 18:41:00, Lei Zhang wrote:
> > > On 2014/10/15 15:03:51, Daniel Erat wrote:
> > > > On 2014/10/15 06:36:12, Lei Zhang wrote:
> > > > > Let's get another opinion from derat@, who has more chrome + chromeos
> > > > > experience.
> > > > 
> > > > i'd rather have this in base than be duplicated across multiple places,
> but
> > > i'm
> > > > not a base owner. :-P
> > > 
> > > Is there no other place where chrome and chromeos can share code?
> > 
> > there's system_api, but i think we only use that to share headers and .proto
> > files currently.
> 
> So should there be some other place aside from base/ where CrOS daemons and
CrOS
> in Chrome can share code? There's the top-level chromeos/ directory, but
that's
> chrome-only right? As is, we'd have to put code in base/ or have separate
> copies, neither of which makes everyone happy.
> 

+1.  I'd really like to have somewhere else to put common code.  I can start a
discussion about what to do on the mailing lists.  In the meanwhile, maybe we
can land this in base/ for now with the understanding that it will move out into
whatever this other common place is going to be.

> Let's say we put it in base/, then developers may react as follows: "Oh look,
a
> fancy AlarmTimer class in base/, let me try using it in my code. Oh wait, it
> only works on CrOS."

FWIW, it only works on CrOS because we only need it for CrOS :-)  We could do
the same thing for windows with a "Waitable Timer Object"
(http://msdn.microsoft.com/en-us/library/windows/desktop/aa373235(v=vs.85).aspx)
and I'm sure there's something similar for other platforms.

[Nicolas Zea, 2014-10-16 21:59:13]

On 2014/10/16 21:49:55, chirantan wrote:
> On 2014/10/15 18:00:52, Nicolas Zea wrote:
> > do the message pump implementations themselves need to be modified to wake
up?
> > Its not clear to me the timers they use will take into account the time
spent
> > while suspended.
> > 
> > I'm wondering if we need something similar to
> > https://codereview.chromium.org/342011 for all platforms that support the
> alarm
> > timer.
> 
> On linux, we use libevent to implement the message pump.  I haven't found any
> documentation that says what it does during system suspend but I'm guessing
that
> it does include the time somehow or we would have had the same problem on
linux
> that that CL was trying to fix.

My understanding is that all platforms have that problem, it's just that most
applications don't care about it. FYI, that specific patch was reverted because
it doesn't help since TimeTicks themselves aren't suspend-resistant (see
https://codereview.chromium.org/6831009). From what I've found, there are few
applications that care about events happening based on wall-clock time (GCM
being one of them). Here are two relevant bugs that seem to be implicating this
behavior as well: crbug.com/172754 and crbug.com/172754

Feel free to start a separate thread if you want to move this discussion out of
this CL though.

[Chirantan Ekbote, 2014-10-16 22:23:35]

On 2014/10/16 21:59:13, Nicolas Zea wrote:
> On 2014/10/16 21:49:55, chirantan wrote:
> > On 2014/10/15 18:00:52, Nicolas Zea wrote:
> > > do the message pump implementations themselves need to be modified to wake
> up?
> > > Its not clear to me the timers they use will take into account the time
> spent
> > > while suspended.
> > > 
> > > I'm wondering if we need something similar to
> > > https://codereview.chromium.org/342011 for all platforms that support the
> > alarm
> > > timer.
> > 
> > On linux, we use libevent to implement the message pump.  I haven't found
any
> > documentation that says what it does during system suspend but I'm guessing
> that
> > it does include the time somehow or we would have had the same problem on
> linux
> > that that CL was trying to fix.
> 
> My understanding is that all platforms have that problem, it's just that most
> applications don't care about it. FYI, that specific patch was reverted
because
> it doesn't help since TimeTicks themselves aren't suspend-resistant (see
> https://codereview.chromium.org/6831009). From what I've found, there are few
> applications that care about events happening based on wall-clock time (GCM
> being one of them). Here are two relevant bugs that seem to be implicating
this
> behavior as well: crbug.com/172754 and crbug.com/172754
>

I don't think all platforms have this problem, I think it's only posix.  We use
mach_absolute_time() on mac and an offset from KERN_BOOTTIME on iOS, both of
which are based on the absolute time elapsed since system boot.  Similarly on
windows we use timeGetTime(), which returns the time elapsed since windows was
started.  So it's really only posix that has this problem because we use
CLOCK_MONOTONIC.

Btw, while researching this I found that the python folks have a excellent
document that describes all the different system time functions here:
http://legacy.python.org/dev/peps/pep-0418/#operating-system-time-functions

[Daniel Erat, 2014-10-16 23:25:44]

one highish-level question: do you think that there will be implementations of
this class for non-chrome-os platforms in the near future? if not, it may be
cleaner to just make this class be chrome-os-only for now.

[Nicolas Zea, 2014-10-16 23:34:15]

On 2014/10/16 22:23:35, chirantan wrote:
> On 2014/10/16 21:59:13, Nicolas Zea wrote:
> > On 2014/10/16 21:49:55, chirantan wrote:
> > > On 2014/10/15 18:00:52, Nicolas Zea wrote:
> > > > do the message pump implementations themselves need to be modified to
wake
> > up?
> > > > Its not clear to me the timers they use will take into account the time
> > spent
> > > > while suspended.
> > > > 
> > > > I'm wondering if we need something similar to
> > > > https://codereview.chromium.org/342011 for all platforms that support
the
> > > alarm
> > > > timer.
> > > 
> > > On linux, we use libevent to implement the message pump.  I haven't found
> any
> > > documentation that says what it does during system suspend but I'm
guessing
> > that
> > > it does include the time somehow or we would have had the same problem on
> > linux
> > > that that CL was trying to fix.
> > 
> > My understanding is that all platforms have that problem, it's just that
most
> > applications don't care about it. FYI, that specific patch was reverted
> because
> > it doesn't help since TimeTicks themselves aren't suspend-resistant (see
> > https://codereview.chromium.org/6831009). From what I've found, there are
few
> > applications that care about events happening based on wall-clock time (GCM
> > being one of them). Here are two relevant bugs that seem to be implicating
> this
> > behavior as well: crbug.com/172754 and crbug.com/172754
> >
> 
> I don't think all platforms have this problem, I think it's only posix.  We
use
> mach_absolute_time() on mac and an offset from KERN_BOOTTIME on iOS, both of
> which are based on the absolute time elapsed since system boot.  Similarly on
> windows we use timeGetTime(), which returns the time elapsed since windows was
> started.  So it's really only posix that has this problem because we use
> CLOCK_MONOTONIC.
> 
> Btw, while researching this I found that the python folks have a excellent
> document that describes all the different system time functions here:
> http://legacy.python.org/dev/peps/pep-0418/#operating-system-time-functions

Are you sure you're looking at the right column in that table?
mach_absolute_time does not record suspended time according to it (which matches
my own investigations), just sleep time (which is process sleep, not machine
sleep). Notice that even CLOCK_MONOTONIC records during "sleep".

Either way though, my point is more that this isn't a POSIX specific thing, and
that to get messages to execute in a timely manner likely involves modifying the
message pump as well to wake it up. My understanding is that Libevent uses
CLOCK_MONOTONIC as well, rather than CLOCK_BOOTTIME. See for example
https://github.com/libevent/libevent/blob/master/whatsnew-2.1.txt

[Chirantan Ekbote, 2014-10-17 02:13:54]

On 2014/10/16 23:34:15, Nicolas Zea wrote:
> 
> Are you sure you're looking at the right column in that table?
> mach_absolute_time does not record suspended time according to it (which
matches
> my own investigations), just sleep time (which is process sleep, not machine
> sleep). Notice that even CLOCK_MONOTONIC records during "sleep".
> 
> Either way though, my point is more that this isn't a POSIX specific thing,
and
> that to get messages to execute in a timely manner likely involves modifying
the
> message pump as well to wake it up. My understanding is that Libevent uses
> CLOCK_MONOTONIC as well, rather than CLOCK_BOOTTIME. See for example
> https://github.com/libevent/libevent/blob/master/whatsnew-2.1.txt

You're right.  I didn't look at the table, just the descriptions of the
functions but even there it says that mach_absolute_time "is based on the
absolute elapsed time" rather than "is the absolute elapsed time".  /me should
read more carefully (=_=).

So I guess fixing TimeTicks is more messy than I originally thought, especially
if libevent is also using CLOCK_MONOTONIC or CLOCK_MONOTONIC_RAW.  Bringing
things back to this CL, the only place TimeTicks is used is for
desired_run_time_.  Does it make sense then to switch it to Time? Especially
since this timer explicitly deals with suspends.  We're still going a
bookkeeping issue because PendingTask uses TimeTicks so tasks run by this timer
are going to appear to run earlier than they were supposed to.

[Nicolas Zea, 2014-10-17 21:36:33]

On 2014/10/17 02:13:54, chirantan wrote:
> On 2014/10/16 23:34:15, Nicolas Zea wrote:
> > 
> > Are you sure you're looking at the right column in that table?
> > mach_absolute_time does not record suspended time according to it (which
> matches
> > my own investigations), just sleep time (which is process sleep, not machine
> > sleep). Notice that even CLOCK_MONOTONIC records during "sleep".
> > 
> > Either way though, my point is more that this isn't a POSIX specific thing,
> and
> > that to get messages to execute in a timely manner likely involves modifying
> the
> > message pump as well to wake it up. My understanding is that Libevent uses
> > CLOCK_MONOTONIC as well, rather than CLOCK_BOOTTIME. See for example
> > https://github.com/libevent/libevent/blob/master/whatsnew-2.1.txt
> 
> You're right.  I didn't look at the table, just the descriptions of the
> functions but even there it says that mach_absolute_time "is based on the
> absolute elapsed time" rather than "is the absolute elapsed time".  /me should
> read more carefully (=_=).
> 
> So I guess fixing TimeTicks is more messy than I originally thought,
especially
> if libevent is also using CLOCK_MONOTONIC or CLOCK_MONOTONIC_RAW.  Bringing
> things back to this CL, the only place TimeTicks is used is for
> desired_run_time_.  Does it make sense then to switch it to Time? Especially
> since this timer explicitly deals with suspends.  We're still going a
> bookkeeping issue because PendingTask uses TimeTicks so tasks run by this
timer
> are going to appear to run earlier than they were supposed to.

Well, I'd argue if you're opting to use this timer it means that they'll run
when they're supposed to on CrOS, but may be delayed on non-CrOS. But yeah, I
think using base::Time might be the safest approach for now, despite the
possible disparity in how things work.

[Chirantan Ekbote, 2014-10-20 22:19:06]

chirantan@chromium.org changed reviewers:
+ blundell@chromium.org
- thestig@chromium.org

[Chirantan Ekbote, 2014-10-20 22:19:06]

ptal

[Chirantan Ekbote, 2014-10-20 22:19:59]

chirantan@chromium.org changed reviewers:
+ thestig@chromium.org

[blundell, 2014-10-21 05:53:25]

The dependencies here are wrong unfortunately; I wasn't aware (but should have
asked) what code in Chromium was going to depend on the new timer code.

Nicolas, does //google_apis/gcm already have a mechanism in place for the
embedder to inject information? It looks like //google_apis/gcm should be
calling out to the embedder to create the timer on its behalf.

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/DEPS
File google_apis/gcm/DEPS (right):

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/DEPS#ne...
google_apis/gcm/DEPS:13: "+components/timer",  # For AlarmTimer class on Chrome
OS
Uh-oh. //google_apis sits below //components in the dependency hierarchy.

[Nicolas Zea, 2014-10-21 17:57:54]

HeartbeatManager is owned by google_apis/gcm/mcs_client.h, which is instantiated
by components/gcm_driver/gcm_client_impl.h, which I believe should be able to
depend on components/timers.

We could change that so the GCMClientImpl instantiates the heartbeat manager
itself by injecting an AlarmTimer via its constructor, and then passes the
HeartbeatManager as a constructor to the MCSClient.

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:32:
heartbeat_timer_(make_scoped_ptr(
do you need to call make_scoped_ptr here?

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.h:72: // Timer for triggering
heartbeats.  Owned by HeartbeatManager.
nit: Since you're using a scoped_ptr now, ownership is implied. No need for the
comment.

[Chirantan Ekbote, 2014-10-21 21:13:48]

Please take another look.

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/DEPS
File google_apis/gcm/DEPS (right):

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/DEPS#ne...
google_apis/gcm/DEPS:13: "+components/timer",  # For AlarmTimer class on Chrome
OS
On 2014/10/21 05:53:25, blundell wrote:
> Uh-oh. //google_apis sits below //components in the dependency hierarchy.

Fixed in the new patch set.

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.cc (right):

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.cc:32:
heartbeat_timer_(make_scoped_ptr(
On 2014/10/21 17:57:54, Nicolas Zea wrote:
> do you need to call make_scoped_ptr here?

I remember reading somewhere that you couldn't construct a scoped_ptr<parent>
with just a child* but I can't find it now and it looks like it actually works
just fine.  I ended up having to change this section anyway to deal with the
DEPS problem.

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/340001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.h:72: // Timer for triggering
heartbeats.  Owned by HeartbeatManager.
On 2014/10/21 17:57:54, Nicolas Zea wrote:
> nit: Since you're using a scoped_ptr now, ownership is implied. No need for
the
> comment.

Done.

[Lei Zhang, 2014-10-21 22:34:12]

Chirantan, do you want to also own the new component you are adding?

Is "timer" a good component name?

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
base/timer/timer.h:135: const bool is_repeating_;
You can leave it in private: and call is_repeating() instead. Same for
|retain_user_task_|

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer.cc:82: if (delay_ == base::TimeDelta()) {
timer.cc uses "delay_ > TimeDelta::FromMicroseconds(0)" BTW.

[Chirantan Ekbote, 2014-10-21 22:51:43]

On 2014/10/21 22:34:12, Lei Zhang wrote:
> Chirantan, do you want to also own the new component you are adding?
> 

Sure :-) but I'm not sure that I'm qualified to be an owner yet.

> Is "timer" a good component name?
> 

I tried to stick with the convention from base/.  I was also considering
"timers", which sounds a little better.  Or did you have something else in mind?

> https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
> File base/timer/timer.h (right):
> 
>
https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
> base/timer/timer.h:135: const bool is_repeating_;
> You can leave it in private: and call is_repeating() instead. Same for
> |retain_user_task_|
> 
>
https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
> File components/timer/alarm_timer.cc (right):
> 
>
https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
> components/timer/alarm_timer.cc:82: if (delay_ == base::TimeDelta()) {
> timer.cc uses "delay_ > TimeDelta::FromMicroseconds(0)" BTW.

[Chirantan Ekbote, 2014-10-21 22:52:21]

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
base/timer/timer.h:135: const bool is_repeating_;
On 2014/10/21 22:34:11, Lei Zhang wrote:
> You can leave it in private: and call is_repeating() instead. Same for
> |retain_user_task_|

Acknowledged.

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer.cc:82: if (delay_ == base::TimeDelta()) {
On 2014/10/21 22:34:11, Lei Zhang wrote:
> timer.cc uses "delay_ > TimeDelta::FromMicroseconds(0)" BTW.

Ah ok.  Will fix in the next patch set.

[blundell, 2014-10-22 13:46:24]

//components LGTM, but I didn't look at the code under //components/timer so
please get derat's LGTM on that

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
base/timer/timer.h:141: bool is_running_;
To follow the pattern, I would make a getter for is_running_.

https://codereview.chromium.org/641943002/diff/380001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/641943002/diff/380001/components/OWNERS#newco...
components/OWNERS:200: per-file timer*=derat@chromium.org
Yes, let's add at least 1 more owner here, whether you or someone else.

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS
File components/timer/DEPS (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS#n...
components/timer/DEPS:4: "+base",
Everything can depend on //base, so this shouldn't be necessary.

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS#n...
components/timer/DEPS:5: "-chrome",
This isn't necessary as no component can depend on //chrome (i.e., this
dependency would never creep in).

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS#n...
components/timer/DEPS:6: "+components/timer",
This shouldn't be necessary.

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS#n...
components/timer/DEPS:9: "-third_party",
You might want to look at the DEPS that are allowed in //src/DEPS to see if
there are other things you want to disallow here.

[Nicolas Zea, 2014-10-22 17:32:16]

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.h:57: void
SetHeartbeatTimer(scoped_ptr<base::Timer> timer);
Why not just pass it in at construction time?

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:109: void
SetHeartbeatTimer(scoped_ptr<base::Timer> timer);
I think you can pass this in at construction time too.

[Chirantan Ekbote, 2014-10-22 18:47:14]

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
base/timer/timer.h:141: bool is_running_;
On 2014/10/22 13:46:24, blundell wrote:
> To follow the pattern, I would make a getter for is_running_.

But we also need to set |is_running_|.  So I guess my question is:  Is it better
to have a getter/setter for common variables or is it better to make them
protected?  In the case of |is_repeating_| and |retain_user_task_|, those
variables are const so there's no real reason for them to be in protected.

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS
File components/timer/DEPS (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS#n...
components/timer/DEPS:9: "-third_party",
On 2014/10/22 13:46:24, blundell wrote:
> You might want to look at the DEPS that are allowed in //src/DEPS to see if
> there are other things you want to disallow here.

Acknowledged.

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.h:57: void
SetHeartbeatTimer(scoped_ptr<base::Timer> timer);
On 2014/10/22 17:32:16, Nicolas Zea wrote:
> Why not just pass it in at construction time?

I originally tried that.  But to pass it in at construction time, we have to
pass it into the MCSClient constructor and then we need to change every location
that constructs one to also pass in a timer (there are a few such locations,
including the constructors for FakeMCSClient and TestMCSClient).  The timers
would also have to be constructed exactly the way the HeartbeatManager expects
so we would have to do

    new base::Timer(true, false)

or

    new timer::AlarmTimer(true, false)

which is really just an implementation detail of HeartbeatManager and it didn't
seem right to have that leaking everywhere.

[Daniel Erat, 2014-10-23 03:44:54]

lgtm for components/timer

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer.cc:82: if (delay_ == base::TimeDelta()) {
On 2014/10/21 22:52:21, chirantan wrote:
> On 2014/10/21 22:34:11, Lei Zhang wrote:
> > timer.cc uses "delay_ > TimeDelta::FromMicroseconds(0)" BTW.
> 
> Ah ok.  Will fix in the next patch set.

TimeDelta() and TimeDelta::FromMicroseconds(0) are equivalent, right? is the
point that negative durations should be handled by this block as well?

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer.h:83: // every timer Reset() is called.
nit: s/timer/time/

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer_unittest.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer_unittest.cc:18: // regular Timer so it should pass
the same tests as the Timer class.
i don't love the duplication here but don't have any better suggestions (unless
there's some way to refactor the testing code so it can be shared).

[blundell, 2014-10-23 05:54:20]

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
base/timer/timer.h:141: bool is_running_;
On 2014/10/22 18:47:14, chirantan wrote:
> On 2014/10/22 13:46:24, blundell wrote:
> > To follow the pattern, I would make a getter for is_running_.
> 
> But we also need to set |is_running_|.  So I guess my question is:  Is it
better
> to have a getter/setter for common variables or is it better to make them
> protected?  In the case of |is_repeating_| and |retain_user_task_|, those
> variables are const so there's no real reason for them to be in protected.

Google style is actually definitive on this point:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer_unittest.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer_unittest.cc:18: // regular Timer so it should pass
the same tests as the Timer class.
On 2014/10/23 03:44:54, Daniel Erat wrote:
> i don't love the duplication here but don't have any better suggestions
(unless
> there's some way to refactor the testing code so it can be shared).

This duplication seems pretty bad. Could you reach out to the person with the
most expertise in base/timer to get some ideas on refactoring? Doesn't need to
block this CL.

[Nicolas Zea, 2014-10-23 17:49:03]

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.h:57: void
SetHeartbeatTimer(scoped_ptr<base::Timer> timer);
On 2014/10/22 18:47:14, chirantan wrote:
> On 2014/10/22 17:32:16, Nicolas Zea wrote:
> > Why not just pass it in at construction time?
> 
> I originally tried that.  But to pass it in at construction time, we have to
> pass it into the MCSClient constructor and then we need to change every
location
> that constructs one to also pass in a timer (there are a few such locations,
> including the constructors for FakeMCSClient and TestMCSClient).  The timers
> would also have to be constructed exactly the way the HeartbeatManager expects
> so we would have to do
> 
>     new base::Timer(true, false)
> 
> or
> 
>     new timer::AlarmTimer(true, false)
> 
> which is really just an implementation detail of HeartbeatManager and it
didn't
> seem right to have that leaking everywhere.

IMO its worse to have the possibility of someone changing the heartbeat timer at
runtime (e.g. you had to add that started_ boolean, which I'd be concerned about
not being updated properly). I'd rather keep the actual logic simple, and just
pass it in via the constructor, even if it means modifying all the places it
might be instantiated.

[Chirantan Ekbote, 2014-10-23 23:53:30]

Please take another look.

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/base/timer/timer.h#newc...
base/timer/timer.h:141: bool is_running_;
On 2014/10/23 05:54:20, blundell wrote:
> On 2014/10/22 18:47:14, chirantan wrote:
> > On 2014/10/22 13:46:24, blundell wrote:
> > > To follow the pattern, I would make a getter for is_running_.
> > 
> > But we also need to set |is_running_|.  So I guess my question is:  Is it
> better
> > to have a getter/setter for common variables or is it better to make them
> > protected?  In the case of |is_repeating_| and |retain_user_task_|, those
> > variables are const so there's no real reason for them to be in protected.
> 
> Google style is actually definitive on this point:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Access_Control

Done.

https://codereview.chromium.org/641943002/diff/380001/components/OWNERS
File components/OWNERS (right):

https://codereview.chromium.org/641943002/diff/380001/components/OWNERS#newco...
components/OWNERS:200: per-file timer*=derat@chromium.org
On 2014/10/22 13:46:24, blundell wrote:
> Yes, let's add at least 1 more owner here, whether you or someone else.

Done.

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS
File components/timer/DEPS (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/DEPS#n...
components/timer/DEPS:6: "+components/timer",
On 2014/10/22 13:46:24, blundell wrote:
> This shouldn't be necessary.

Done.

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer.cc:82: if (delay_ == base::TimeDelta()) {
On 2014/10/23 03:44:54, Daniel Erat wrote:
> On 2014/10/21 22:52:21, chirantan wrote:
> > On 2014/10/21 22:34:11, Lei Zhang wrote:
> > > timer.cc uses "delay_ > TimeDelta::FromMicroseconds(0)" BTW.
> > 
> > Ah ok.  Will fix in the next patch set.
> 
> TimeDelta() and TimeDelta::FromMicroseconds(0) are equivalent, right? is the
> point that negative durations should be handled by this block as well?

The documentation for timerfd_settime doesn't say how it handles negative
durations (it just says a non-zero duration will arm the timer) so I think it's
better to deal with it here.

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer.h (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer.h:83: // every timer Reset() is called.
On 2014/10/23 03:44:54, Daniel Erat wrote:
> nit: s/timer/time/

Done.

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
File components/timer/alarm_timer_unittest.cc (right):

https://codereview.chromium.org/641943002/diff/380001/components/timer/alarm_...
components/timer/alarm_timer_unittest.cc:18: // regular Timer so it should pass
the same tests as the Timer class.
On 2014/10/23 05:54:20, blundell wrote:
> On 2014/10/23 03:44:54, Daniel Erat wrote:
> > i don't love the duplication here but don't have any better suggestions
> (unless
> > there's some way to refactor the testing code so it can be shared).
> 
> This duplication seems pretty bad. Could you reach out to the person with the
> most expertise in base/timer to get some ideas on refactoring? Doesn't need to
> block this CL.

I've filed crbug.com/426647.  We'll land this for now and then figure out how to
refactor it.

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
File google_apis/gcm/engine/heartbeat_manager.h (right):

https://codereview.chromium.org/641943002/diff/380001/google_apis/gcm/engine/...
google_apis/gcm/engine/heartbeat_manager.h:57: void
SetHeartbeatTimer(scoped_ptr<base::Timer> timer);
On 2014/10/23 17:49:02, Nicolas Zea wrote:
> On 2014/10/22 18:47:14, chirantan wrote:
> > On 2014/10/22 17:32:16, Nicolas Zea wrote:
> > > Why not just pass it in at construction time?
> > 
> > I originally tried that.  But to pass it in at construction time, we have to
> > pass it into the MCSClient constructor and then we need to change every
> location
> > that constructs one to also pass in a timer (there are a few such locations,
> > including the constructors for FakeMCSClient and TestMCSClient).  The timers
> > would also have to be constructed exactly the way the HeartbeatManager
expects
> > so we would have to do
> > 
> >     new base::Timer(true, false)
> > 
> > or
> > 
> >     new timer::AlarmTimer(true, false)
> > 
> > which is really just an implementation detail of HeartbeatManager and it
> didn't
> > seem right to have that leaking everywhere.
> 
> IMO its worse to have the possibility of someone changing the heartbeat timer
at
> runtime (e.g. you had to add that started_ boolean, which I'd be concerned
about
> not being updated properly). I'd rather keep the actual logic simple, and just
> pass it in via the constructor, even if it means modifying all the places it
> might be instantiated.

Done.

[Nicolas Zea, 2014-10-24 00:28:19]

gcm LGTM! Thanks for cleaning that up

https://codereview.chromium.org/641943002/diff/400001/components/gcm_driver/DEPS
File components/gcm_driver/DEPS (right):

https://codereview.chromium.org/641943002/diff/400001/components/gcm_driver/D...
components/gcm_driver/DEPS:4: "+components/timers",
nit: comment that this is only for cros

https://codereview.chromium.org/641943002/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/641943002/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:104: scoped_ptr<base::Timer> timer);
nit: prefer naming this heartbeat_timer (here and in the cc file, and the
heartbeat_manager's definition)

[Lei Zhang, 2014-10-24 00:44:50]

base/ lgtm with some nits.

https://codereview.chromium.org/641943002/diff/400001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/400001/base/timer/timer.h#newc...
base/timer/timer.h:118: const tracked_objects::Location& posted_from() { return
posted_from_; }
this can be a const method, same for is_running().

https://codereview.chromium.org/641943002/diff/400001/base/timer/timer.h#newc...
base/timer/timer.h:123: bool is_running() { return is_running_; }
nit: group with the other getters, and put blank line above private:

[Chirantan Ekbote, 2014-10-24 01:04:13]

Thanks for the reviews!

https://codereview.chromium.org/641943002/diff/400001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/641943002/diff/400001/base/timer/timer.h#newc...
base/timer/timer.h:118: const tracked_objects::Location& posted_from() { return
posted_from_; }
On 2014/10/24 00:44:49, Lei Zhang wrote:
> this can be a const method, same for is_running().

Done.

https://codereview.chromium.org/641943002/diff/400001/base/timer/timer.h#newc...
base/timer/timer.h:123: bool is_running() { return is_running_; }
On 2014/10/24 00:44:49, Lei Zhang wrote:
> nit: group with the other getters, and put blank line above private:

Done.

https://codereview.chromium.org/641943002/diff/400001/components/gcm_driver/DEPS
File components/gcm_driver/DEPS (right):

https://codereview.chromium.org/641943002/diff/400001/components/gcm_driver/D...
components/gcm_driver/DEPS:4: "+components/timers",
On 2014/10/24 00:28:18, Nicolas Zea wrote:
> nit: comment that this is only for cros

Done.

https://codereview.chromium.org/641943002/diff/400001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/641943002/diff/400001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:104: scoped_ptr<base::Timer> timer);
On 2014/10/24 00:28:18, Nicolas Zea wrote:
> nit: prefer naming this heartbeat_timer (here and in the cc file, and the
> heartbeat_manager's definition)

Done.

[Chirantan Ekbote, 2014-10-24 01:04:22]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-10-24 01:08:57]

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

[commit-bot: I haz the power, 2014-10-24 01:29:46]

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

[commit-bot: I haz the power, 2014-10-24 01:29:47]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Chirantan Ekbote, 2014-10-24 01:46:53]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-10-24 01:48:12]

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

[commit-bot: I haz the power, 2014-10-24 02:09:08]

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

[commit-bot: I haz the power, 2014-10-24 02:09:10]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Chirantan Ekbote, 2014-10-24 18:27:53]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2014-10-24 18:29:43]

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

[commit-bot: I haz the power, 2014-10-24 19:44:57]

Committed patchset #10 (id:440001)

[commit-bot: I haz the power, 2014-10-24 19:45:38]

Patchset 10 (id:??) landed as
https://crrev.com/26436e402e6e1b780f6565539ae0acb3e2976fb9
Cr-Commit-Position: refs/heads/master@{#301175}