base: add MockTimer

base: add MockTimer

MockTimer is a subclass of Timer that will never fire on its own, but can be
manually fired by a test with the Fire() method.

BUG=131227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256942

[Elly Fong-Jones, 2014-03-13 14:40:02]

PTAL? trying to avoid adding the same code in chrome/ for
<https://codereview.chromium.org/136203009/>.

[Mark Mentovai, 2014-03-13 15:17:34]

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

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer.h#newc...
base/timer/mock_timer.h:31: virtual void Fire();
Doesn’t need to be virtual.

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer.h#newc...
base/timer/mock_timer.h:32: protected:
This should be private, not protected. Also, a nit: put a blank line before it
to set it off from the Fire method above.

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer.h#newc...
base/timer/mock_timer.h:33: base::Closure user_task_;
All of these fields duplicate fields in the base class. Is the duplication
really needed? Can most or all of it be avoided by accessing the fields of the
base?

You may need to add accessors to the base to make this work. My rule of thumb
would be to add accessors for anything that you can, but not to add any mutators
to the base class just to make this test class work.

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer_unitte...
File base/timer/mock_timer_unittest.cc (right):

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer_unitte...
base/timer/mock_timer_unittest.cc:11: void CallMeMaybe(int *number) {
:)

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

https://codereview.chromium.org/197243003/diff/1/base/timer/timer.h#newcode85
base/timer/timer.h:85: virtual bool IsRunning() const {
Do all of these need to become virtual? What about if we do more sharing of the
fields as I suggested in my comments in mock_timer.h?

[Elly Fong-Jones, 2014-03-13 15:30:09]

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

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer.h#newc...
base/timer/mock_timer.h:31: virtual void Fire();
On 2014/03/13 15:17:34, Mark Mentovai wrote:
> Doesn’t need to be virtual.

Done.

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer.h#newc...
base/timer/mock_timer.h:32: protected:
On 2014/03/13 15:17:34, Mark Mentovai wrote:
> This should be private, not protected. Also, a nit: put a blank line before it
> to set it off from the Fire method above.

Done.

https://codereview.chromium.org/197243003/diff/1/base/timer/mock_timer.h#newc...
base/timer/mock_timer.h:33: base::Closure user_task_;
On 2014/03/13 15:17:34, Mark Mentovai wrote:
> All of these fields duplicate fields in the base class. Is the duplication
> really needed? Can most or all of it be avoided by accessing the fields of the
> base?
> 
> You may need to add accessors to the base to make this work. My rule of thumb
> would be to add accessors for anything that you can, but not to add any
mutators
> to the base class just to make this test class work.

Okay. I moved retain_user_task_ and is_repeating_ into the parent class and
added accessors, but I can't move delay_, is_running_ or user_task_ because I'd
need to add test-only accessors for them.

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

https://codereview.chromium.org/197243003/diff/1/base/timer/timer.h#newcode85
base/timer/timer.h:85: virtual bool IsRunning() const {
On 2014/03/13 15:17:34, Mark Mentovai wrote:
> Do all of these need to become virtual? What about if we do more sharing of
the
> fields as I suggested in my comments in mock_timer.h?

I could make is_running_ and delay_ protected, maybe? Otherwise since there are
no setters for them the child class needs to be able to return its own values.

[Mark Mentovai, 2014-03-13 15:48:55]

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

https://codereview.chromium.org/197243003/diff/1/base/timer/timer.h#newcode85
base/timer/timer.h:85: virtual bool IsRunning() const {
Elly Jones wrote:
> I could make is_running_ and delay_ protected, maybe? Otherwise since there
are
> no setters for them the child class needs to be able to return its own values.

We don’t make data members protected, ever. But you can have protected helper
methods that operate on private data.

It looks like you can get rid of the mock class’ copies of delay_ and user_task_
altogether. Just remove GetCurrentDelay, Start, and Stop. The base class’
implementations are appropriate as-is, and you can remove the “virtual” from
both of those. In Fire(), you can access the base user_task_ via the public
user_task() getter.

is_running_ is a bit trickier. I think that you can share the base class’ copy
of is_running_ if you gave the base a protected method to deal with is_running_.
But it’s probably not worth perverting the base class in this way for just the
mock class, so I’d be inclined to leave it alone.

[Mark Mentovai, 2014-03-13 15:49:52]

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h#newco...
base/timer/timer.h:110: bool retain_user_task() const { return
retain_user_task_; }
If nobody has needed these outside of a Timer subclass yet, they probably won’t.
Can you move them into the protected section?

[Elly Fong-Jones, 2014-03-13 15:56:29]

On 2014/03/13 15:48:55, Mark Mentovai wrote:
> https://codereview.chromium.org/197243003/diff/1/base/timer/timer.h
> File base/timer/timer.h (right):
> 
> https://codereview.chromium.org/197243003/diff/1/base/timer/timer.h#newcode85
> base/timer/timer.h:85: virtual bool IsRunning() const {
> Elly Jones wrote:
> > I could make is_running_ and delay_ protected, maybe? Otherwise since there
> are
> > no setters for them the child class needs to be able to return its own
values.
> 
> We don’t make data members protected, ever. But you can have protected helper
> methods that operate on private data.
> 
> It looks like you can get rid of the mock class’ copies of delay_ and
user_task_
> altogether. Just remove GetCurrentDelay, Start, and Stop. The base class’
> implementations are appropriate as-is, and you can remove the
“virtual” from
> both of those. In Fire(), you can access the base user_task_ via the public
> user_task() getter.
> 
> is_running_ is a bit trickier. I think that you can share the base
class’ copy
> of is_running_ if you gave the base a protected method to deal with
is_running_.
> But it’s probably not worth perverting the base class in this way for just the
> mock class, so I’d be inclined to leave it alone.

Unfortunately, if I don't add a protected accessor for is_running_ so I can
reuse it, I can't reuse Stop() or Start() either, since they'll be setting the
parent's is_running_, and I have my own copy. I think I'm going to add and use a
protected accessor, sadly; otherwise I'll have to duplicate Stop() and Start().

[Mark Mentovai, 2014-03-13 15:59:59]

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h#newco...
base/timer/timer.h:85: virtual bool IsRunning() const {
The (public, even) accessor already exists here.

[Elly Fong-Jones, 2014-03-13 16:00:57]

On 2014/03/13 15:59:59, Mark Mentovai wrote:
> https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h
> File base/timer/timer.h (right):
> 
>
https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h#newco...
> base/timer/timer.h:85: virtual bool IsRunning() const {
> The (public, even) accessor already exists here.

Sorry, I meant setter.

[Elly Fong-Jones, 2014-03-13 16:02:43]

On 2014/03/13 16:00:57, Elly Jones wrote:
> On 2014/03/13 15:59:59, Mark Mentovai wrote:
> > https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h
> > File base/timer/timer.h (right):
> > 
> >
>
https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h#newco...
> > base/timer/timer.h:85: virtual bool IsRunning() const {
> > The (public, even) accessor already exists here.
> 
> Sorry, I meant setter.

In particular, Reset() needs to change what IsRunning() returns.

[Mark Mentovai, 2014-03-13 16:03:55]

Ah, right, I see.

OK, judgement call. Given the need for a protected setter, if you’d rather stick
with what you have now, I’m fine with it.

[Elly Fong-Jones, 2014-03-13 16:07:16]

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h#newco...
base/timer/timer.h:85: virtual bool IsRunning() const {
On 2014/03/13 16:00:00, Mark Mentovai wrote:
> The (public, even) accessor already exists here.

Done.

https://codereview.chromium.org/197243003/diff/20001/base/timer/timer.h#newco...
base/timer/timer.h:110: bool retain_user_task() const { return
retain_user_task_; }
On 2014/03/13 15:49:52, Mark Mentovai wrote:
> If nobody has needed these outside of a Timer subclass yet, they probably
won’t.
> Can you move them into the protected section?

Done.

[Mark Mentovai, 2014-03-13 16:22:09]

LGTM

[Elly Fong-Jones, 2014-03-13 16:52:40]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-03-13 16:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/197243003/40001

[commit-bot: I haz the power, 2014-03-13 17:09:08]

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

[commit-bot: I haz the power, 2014-03-13 17:09:09]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg
  tryserver.chromium on mac_chromium_rel

[Elly Fong-Jones, 2014-03-13 17:19:39]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-03-13 17:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/197243003/60001

[Elly Fong-Jones, 2014-03-13 18:26:48]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2014-03-13 18:41:02]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-03-13 18:41:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ellyjones@chromium.org/197243003/80001

[commit-bot: I haz the power, 2014-03-13 22:04:41]

Change committed as 256942