[Clean-up] Replace the final straggler use of MockTimeProvider with Clock.

[Clean-up] Replace the final straggler use of MockTimeProvider with Clock.

Clock is the newer interface; MockTimeProvider is deprecated.

BUG=none
TEST=unit_tests
R=pkasting@chromium.org

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

[tfarina, 2015-01-16 04:28:50]

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

[tfarina, 2015-01-16 04:28:51]

Thanks for putting this together Ilya. I gave up because I got tired of waiting
minutes and minutes for unit_tests to link after changing a single line in the
uni test.

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
File components/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:312:
scoped_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock);
Doesn't have crashed for you? I mean, not this line, but later on, scoped_ptr
will raise a check.

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:332:
AssertEquals(*cloned_url, *loaded_url);
This didn't pass for me locally.

[Ilya Sherman, 2015-01-16 04:39:18]

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
File components/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:312:
scoped_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock);
On 2015/01/16 04:28:51, tfarina wrote:
> Doesn't have crashed for you? I mean, not this line, but later on, scoped_ptr
> will raise a check.

This doesn't crash, because the ownership is transferred to the model.  I'm
guessing you had a crash because both the model and the test function tried to
maintain ownership of the clock.

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:332:
AssertEquals(*cloned_url, *loaded_url);
On 2015/01/16 04:28:51, tfarina wrote:
> This didn't pass for me locally.

Note that I modified the AssertEquals implementation.  The simpler option would
be to use a Time value that does not change when rounded to the nearest second.

[tfarina, 2015-01-16 05:01:39]

Just trying to undestand what I missed and the mistakes I made in my version.

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
File components/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:231:
ASSERT_LT((expected.last_modified() - actual.last_modified()).magnitude(),
Looks like this accounts for the small difference I was seeing when this check
was failing for me.

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:642: base::Time now =
base::Time::Now();
Have you had to move this down for some reason? Or just to be closer to the
assert below? Because there is and ResetTemplateURL call above that sets
last_modifed too.

[Ilya Sherman, 2015-01-16 05:11:07]

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
File components/search_engines/template_url_service_unittest.cc (right):

https://codereview.chromium.org/855633002/diff/1/components/search_engines/te...
components/search_engines/template_url_service_unittest.cc:642: base::Time now =
base::Time::Now();
On 2015/01/16 05:01:39, tfarina wrote:
> Have you had to move this down for some reason? Or just to be closer to the
> assert below? Because there is and ResetTemplateURL call above that sets
> last_modifed too.

That's a good question.  I took your CL as a starting point, and you had moved
this down, so I left it where you'd put it.  I'm not sure why the test passes,
given the ResetTemplateURL call above, but it does pass.

[Ilya Sherman, 2015-01-21 23:46:04]

Peter, ping

[Peter Kasting, 2015-01-22 00:00:23]

Sorry, I had started looking at this when you originally sent it and somehow
lost track of it.

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
File components/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:230: // Note that
times are stored with a granularity of one second.
Doesn't this mean the assert would have to be LE instead of LT?  In case we
ticked over to the next second before this ASSERT?

Same comment below.  (I wonder if the three times you use this make it worth a
helper function.)

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:642: base::Time now =
base::Time::Now();
On 2015/01/16 05:11:07, Ilya Sherman wrote:
> On 2015/01/16 05:01:39, tfarina wrote:
> > Have you had to move this down for some reason? Or just to be closer to the
> > assert below? Because there is and ResetTemplateURL call above that sets
> > last_modifed too.
> 
> That's a good question.  I took your CL as a starting point, and you had moved
> this down, so I left it where you'd put it.  I'm not sure why the test passes,
> given the ResetTemplateURL call above, but it does pass.

Hmm.  It seems like it ought to fail to me, since I'd expect last_modified to
get updated at the ResetTemplateURL call.  Is the ResetModel call below doing
this?  If so that's probably a bug that we should fix.

[Ilya Sherman, 2015-01-22 02:38:22]

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
File components/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:230: // Note that
times are stored with a granularity of one second.
On 2015/01/22 00:00:23, Peter Kasting wrote:
> Doesn't this mean the assert would have to be LE instead of LT?  In case we
> ticked over to the next second before this ASSERT?
> 
> Same comment below.  (I wonder if the three times you use this make it worth a
> helper function.)

The issue is not that we might tick over a second.  Rather, the issue is a loss
of precision: If we started with a time that was 36.077 seconds, we end up with
a time that is 36.000 seconds.  The other option is to set expected times that
are aligned to precise seconds, as the tests previously did.

I agree that a helper function is probably more readable; done.

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:642: base::Time now =
base::Time::Now();
On 2015/01/22 00:00:23, Peter Kasting wrote:
> On 2015/01/16 05:11:07, Ilya Sherman wrote:
> > On 2015/01/16 05:01:39, tfarina wrote:
> > > Have you had to move this down for some reason? Or just to be closer to
the
> > > assert below? Because there is and ResetTemplateURL call above that sets
> > > last_modifed too.
> > 
> > That's a good question.  I took your CL as a starting point, and you had
moved
> > this down, so I left it where you'd put it.  I'm not sure why the test
passes,
> > given the ResetTemplateURL call above, but it does pass.
> 
> Hmm.  It seems like it ought to fail to me, since I'd expect last_modified to
> get updated at the ResetTemplateURL call.  Is the ResetModel call below doing
> this?  If so that's probably a bug that we should fix.

I'm not sure what's causing the test to continue to pass, but it does.  I've
restored the original position of the code for now.  If ResetModel() is causing
last_modified to get updated, then that bug is outside the intended scope of
this CL.

[Peter Kasting, 2015-01-22 17:54:22]

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
File components/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:230: // Note that
times are stored with a granularity of one second.
On 2015/01/22 02:38:22, Ilya Sherman wrote:
> On 2015/01/22 00:00:23, Peter Kasting wrote:
> > Doesn't this mean the assert would have to be LE instead of LT?  In case we
> > ticked over to the next second before this ASSERT?
> > 
> > Same comment below.  (I wonder if the three times you use this make it worth
a
> > helper function.)
> 
> The issue is not that we might tick over a second.  Rather, the issue is a
loss
> of precision: If we started with a time that was 36.077 seconds, we end up
with
> a time that is 36.000 seconds.  The other option is to set expected times that
> are aligned to precise seconds, as the tests previously did.

Right, but what I'm worried about is, if the start time is 36.499 seconds and
the end time is 36.501 seconds, we'll get 36 and 37 seconds and the total time
will be one second.  Or am I mistaken about whether this can happen?

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:642: base::Time now =
base::Time::Now();
On 2015/01/22 02:38:22, Ilya Sherman wrote:
> On 2015/01/22 00:00:23, Peter Kasting wrote:
> > On 2015/01/16 05:11:07, Ilya Sherman wrote:
> > > On 2015/01/16 05:01:39, tfarina wrote:
> > > > Have you had to move this down for some reason? Or just to be closer to
> the
> > > > assert below? Because there is and ResetTemplateURL call above that sets
> > > > last_modifed too.
> > > 
> > > That's a good question.  I took your CL as a starting point, and you had
> moved
> > > this down, so I left it where you'd put it.  I'm not sure why the test
> passes,
> > > given the ResetTemplateURL call above, but it does pass.
> > 
> > Hmm.  It seems like it ought to fail to me, since I'd expect last_modified
to
> > get updated at the ResetTemplateURL call.  Is the ResetModel call below
doing
> > this?  If so that's probably a bug that we should fix.
> 
> I'm not sure what's causing the test to continue to pass, but it does.  I've
> restored the original position of the code for now.  If ResetModel() is
causing
> last_modified to get updated, then that bug is outside the intended scope of
> this CL.

Yeah, I wasn't so much asking you to fix it as hoping you'd glance at why the
failure is occurring and file a bug against me if it looks wrong.  I guess
that's somewhat passive-aggressive/lazy :)

[Ilya Sherman, 2015-01-22 23:13:11]

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
File components/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:230: // Note that
times are stored with a granularity of one second.
On 2015/01/22 17:54:22, Peter Kasting wrote:
> On 2015/01/22 02:38:22, Ilya Sherman wrote:
> > On 2015/01/22 00:00:23, Peter Kasting wrote:
> > > Doesn't this mean the assert would have to be LE instead of LT?  In case
we
> > > ticked over to the next second before this ASSERT?
> > > 
> > > Same comment below.  (I wonder if the three times you use this make it
worth
> a
> > > helper function.)
> > 
> > The issue is not that we might tick over a second.  Rather, the issue is a
> loss
> > of precision: If we started with a time that was 36.077 seconds, we end up
> with
> > a time that is 36.000 seconds.  The other option is to set expected times
that
> > are aligned to precise seconds, as the tests previously did.
> 
> Right, but what I'm worried about is, if the start time is 36.499 seconds and
> the end time is 36.501 seconds, we'll get 36 and 37 seconds and the total time
> will be one second.  Or am I mistaken about whether this can happen?

That's not possible -- the test Clock will always return the same value for
calls to Now() (unless SetNow() is called again).

https://chromiumcodereview.appspot.com/855633002/diff/1/components/search_eng...
components/search_engines/template_url_service_unittest.cc:642: base::Time now =
base::Time::Now();
On 2015/01/22 17:54:22, Peter Kasting wrote:
> On 2015/01/22 02:38:22, Ilya Sherman wrote:
> > On 2015/01/22 00:00:23, Peter Kasting wrote:
> > > On 2015/01/16 05:11:07, Ilya Sherman wrote:
> > > > On 2015/01/16 05:01:39, tfarina wrote:
> > > > > Have you had to move this down for some reason? Or just to be closer
to
> > the
> > > > > assert below? Because there is and ResetTemplateURL call above that
sets
> > > > > last_modifed too.
> > > > 
> > > > That's a good question.  I took your CL as a starting point, and you had
> > moved
> > > > this down, so I left it where you'd put it.  I'm not sure why the test
> > passes,
> > > > given the ResetTemplateURL call above, but it does pass.
> > > 
> > > Hmm.  It seems like it ought to fail to me, since I'd expect last_modified
> to
> > > get updated at the ResetTemplateURL call.  Is the ResetModel call below
> doing
> > > this?  If so that's probably a bug that we should fix.
> > 
> > I'm not sure what's causing the test to continue to pass, but it does.  I've
> > restored the original position of the code for now.  If ResetModel() is
> causing
> > last_modified to get updated, then that bug is outside the intended scope of
> > this CL.
> 
> Yeah, I wasn't so much asking you to fix it as hoping you'd glance at why the
> failure is occurring and file a bug against me if it looks wrong.  I guess
> that's somewhat passive-aggressive/lazy :)

Fair enough.  I didn't investigate deeply enough to know how to file a
meaningful bug, sorry.  (I wrote this CL quickly over SSH, and I don't know of a
non-PITA way to run the tests over SSH, so I wanted to minimize the number of
times that I ran the tests.)

[Peter Kasting, 2015-01-22 23:16:07]

LGTM

https://chromiumcodereview.appspot.com/855633002/diff/20001/components/search...
File components/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/855633002/diff/20001/components/search...
components/search_engines/template_url_service_unittest.cc:240: void
TemplateURLServiceTest::AssertEquals(const base::Time& expected,
This won't compile -- you need AssertTimesEqual().

[Ilya Sherman, 2015-01-22 23:53:23]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-01-22 23:54:06]

https://chromiumcodereview.appspot.com/855633002/diff/20001/components/search...
File components/search_engines/template_url_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/855633002/diff/20001/components/search...
components/search_engines/template_url_service_unittest.cc:240: void
TemplateURLServiceTest::AssertEquals(const base::Time& expected,
On 2015/01/22 23:16:06, Peter Kasting wrote:
> This won't compile -- you need AssertTimesEqual().

Whoops, good catch.  Fixed.

[Ilya Sherman, 2015-01-22 23:54:13]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2015-01-22 23:54:49]

isherman@chromium.org changed reviewers:
+ thakis@chromium.org

[Ilya Sherman, 2015-01-22 23:54:50]

Nico, mind stamping for //base/* changes?

[Nico, 2015-01-23 00:00:40]

base lgtm

[Ilya Sherman, 2015-01-23 00:10:22]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-01-23 00:10:27]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2015-01-23 00:10:29]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-01-23 00:12:51]

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

[commit-bot: I haz the power, 2015-01-23 02:16:13]

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

[commit-bot: I haz the power, 2015-01-23 02:16:14]

Exceeded time limit waiting for builds to trigger.

[Ilya Sherman, 2015-01-23 02:22:04]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-01-23 02:23:09]

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

[commit-bot: I haz the power, 2015-01-23 04:25:12]

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

[commit-bot: I haz the power, 2015-01-23 04:25:14]

Exceeded time limit waiting for builds to trigger.

[Ilya Sherman, 2015-01-23 06:31:44]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-01-23 06:32:41]

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

[commit-bot: I haz the power, 2015-01-23 06:33:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-23 06:34:25]

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