Make chrome/browser/extensions use base::Clock instead of MockTimeProvider

Make chrome/browser/extensions use base::Clock instead of MockTimeProvider

Also replace a few instances of ad-hoc test clocks.

Improve SimpleTest{,Tick}Clock a bit.

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

[akalin, 2013-02-22 04:22:56]

+brettw for base/ stuff
+asargent for c/b/extensions stuff

[asargent_no_longer_on_chrome, 2013-02-22 19:19:45]

+cc mpcomplete who might have some thoughts on this

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarm_manager.h:62: // 'now' is usually
&base::Time::Now.
nit: pls update comment

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarms_api.h:30: base::Clock* const clock_;
Hmm, it seems a little weird for this class to have two clocks that usually are
the same, but sometimes are different. This seems like less encapsulation /
separation of concerns than the TimeProvider we had before.

I guess the advantage you were going for with the two instance variables is
letting the default_clock_ be stack-allocated in the common case, and avoiding
needing ref-counting to have a shared clock object? Had you considered just
having one clock object that is either owned outright (either via scoped_ptr, or
copy-constructed with a specialized test clock that can refer back to a shared
"actual" clock), or const ref?

[Matt Perry, 2013-02-22 19:26:24]

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarms_api.h:30: base::Clock* const clock_;
On 2013/02/22 19:19:45, Antony Sargent wrote:
> Hmm, it seems a little weird for this class to have two clocks that usually
are
> the same, but sometimes are different. This seems like less encapsulation /
> separation of concerns than the TimeProvider we had before.
> 
> I guess the advantage you were going for with the two instance variables is
> letting the default_clock_ be stack-allocated in the common case, and avoiding
> needing ref-counting to have a shared clock object? Had you considered just
> having one clock object that is either owned outright (either via scoped_ptr,
or
> copy-constructed with a specialized test clock that can refer back to a shared
> "actual" clock), or const ref?

I agree, the current pattern looks a bit weird. It would be simpler to just have
this class own the Clock reference via  a scoped_ptr (not sure if that's easy to
do with the Clock class).

[brettw, 2013-02-22 20:44:43]

base lgtm

[akalin, 2013-02-22 21:06:17]

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarms_api.h:30: base::Clock* const clock_;
On 2013/02/22 19:26:24, Matt Perry wrote:
> I agree, the current pattern looks a bit weird. It would be simpler to just
have
> this class own the Clock reference via  a scoped_ptr (not sure if that's easy
to
> do with the Clock class).

Yeah, that works.  The disadvantage is that now I have to expose an accessor to
the underlying clock, or have tests hold a raw pointer to the test clock and
assume it lasts as long as the object does.  But I guess that's not too bad. 
I'll change it.

[akalin, 2013-02-23 07:18:12]

PTAL

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarm_manager.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarm_manager.h:62: // 'now' is usually
&base::Time::Now.
On 2013/02/22 19:19:45, Antony Sargent wrote:
> nit: pls update comment

Done.

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarms_api.h:30: base::Clock* const clock_;
Ah, now I remember why I did this.  Ideally, we'd just have one constructor with
base::Clock*, and AlarmsCreateFunction wouldn't own it.  But because of how
ExtensionFunctions are used, there needs to be a default constructor.  Since we
can't use statics/globals, I decided to just store the default clock in each
instance.

However, another solution is just to have an owns_clock_ boolean.  I did that.

[akalin, 2013-02-26 00:15:13]

ping, asargent and mmenke!

On 2013/02/23 07:18:12, akalin wrote:
> PTAL
> 
>
https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
> File chrome/browser/extensions/api/alarms/alarm_manager.h (right):
> 
>
https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
> chrome/browser/extensions/api/alarms/alarm_manager.h:62: // 'now' is usually
> &base::Time::Now.
> On 2013/02/22 19:19:45, Antony Sargent wrote:
> > nit: pls update comment
> 
> Done.
> 
>
https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
> File chrome/browser/extensions/api/alarms/alarms_api.h (right):
> 
>
https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
> chrome/browser/extensions/api/alarms/alarms_api.h:30: base::Clock* const
clock_;
> Ah, now I remember why I did this.  Ideally, we'd just have one constructor
with
> base::Clock*, and AlarmsCreateFunction wouldn't own it.  But because of how
> ExtensionFunctions are used, there needs to be a default constructor.  Since
we
> can't use statics/globals, I decided to just store the default clock in each
> instance.
> 
> However, another solution is just to have an owns_clock_ boolean.  I did that.

[asargent_no_longer_on_chrome, 2013-02-26 00:34:03]

LGTM

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/alarms/alarms_api.h (right):

https://codereview.chromium.org/12314056/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/alarms/alarms_api.h:30: base::Clock* const clock_;
On 2013/02/23 07:18:12, akalin wrote:
> Ah, now I remember why I did this.  Ideally, we'd just have one constructor
with
> base::Clock*, and AlarmsCreateFunction wouldn't own it.  But because of how
> ExtensionFunctions are used, there needs to be a default constructor.  Since
we
> can't use statics/globals, I decided to just store the default clock in each
> instance.
> 
> However, another solution is just to have an owns_clock_ boolean.  I did that.

Ok, this seems simple enough I guess.

[commit-bot: I haz the power, 2013-02-26 00:46:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/12314056/6003

[commit-bot: I haz the power, 2013-02-26 01:51:20]

Change committed as 184552