SuggestionsStore unittests not using real time.

SuggestionsStore unittests not using real time for checking suggestions expiration.

BUG=440252
Committed: https://crrev.com/cd12be4441a6a95fe86396b7e450968264625eba
Cr-Commit-Position: refs/heads/master@{#309434}

[gayane -on leave until 09-2017, 2014-12-18 16:49:12]

gayane@chromium.org changed reviewers:
+ mathp@chromium.org, mgiuca@chromium.org

[gayane -on leave until 09-2017, 2014-12-18 16:49:29]

Please have a look.

[Mathieu, 2014-12-18 20:05:28]

lgtm, thanks!

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
File components/suggestions/suggestions_store.cc (right):

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
components/suggestions/suggestions_store.cc:11: #include "base/time/clock.h"
don't need to include clock.h here, it's in the .h

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
File components/suggestions/suggestions_store_unittest.cc (right):

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
components/suggestions/suggestions_store_unittest.cc:75: : pref_service_(new
user_prefs::TestingPrefServiceSyncable) {
nit: what this change done by a formatting tool?

[gayane -on leave until 09-2017, 2014-12-18 21:06:49]

Done.

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
File components/suggestions/suggestions_store.cc (right):

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
components/suggestions/suggestions_store.cc:11: #include "base/time/clock.h"
On 2014/12/18 20:05:28, Mathieu Perreault wrote:
> don't need to include clock.h here, it's in the .h

Done.

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
File components/suggestions/suggestions_store_unittest.cc (right):

https://codereview.chromium.org/816693003/diff/1/components/suggestions/sugge...
components/suggestions/suggestions_store_unittest.cc:75: : pref_service_(new
user_prefs::TestingPrefServiceSyncable) {
On 2014/12/18 20:05:28, Mathieu Perreault wrote:
> nit: what this change done by a formatting tool?

No, I added a testing code there and then removed, but forgot to fix the
newline. Done.

[Matt Giuca, 2014-12-18 22:51:14]

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_store.cc (right):

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store.cc:20: clock_(new base::DefaultClock())
{
Nit: Indentation is wrong. (Run git cl format?)

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_store.h (right):

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store.h:44: // Gains ownership of |clock|.
Nit: This comment is not necessary (the ownership transfer is implied by
scoped_ptr).

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_store_unittest.cc (right):

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:39: base::Time expiry_time,
int expired_count, int valid_count) {
The name "expiry_time" is misleading. The expiry times are different for each
suggestion (computed inside the for loop below). This value is the simulated
"now" time.

Can you rename it to current_time (optionally with a comment that this is the
simulated time, not the actual time).

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:40: int64 expiry_usec =
(expiry_time - base::Time::UnixEpoch())
Indentation (git cl format).

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:40: int64 expiry_usec =
(expiry_time - base::Time::UnixEpoch())
As above, this is not an expiry time, it is a simulated "now". Rename back to
now_usec.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:90: base::Time expiry_time;
As above, this is not an expiry time. Rename to current_time.

[Matt Giuca, 2014-12-19 02:54:49]

I'm about to leave on vacation, so I won't hold up this CL. lgtm, but please do
my above suggestions and run git cl format over the whole thing.

Have a good holiday season!

[gayane -on leave until 09-2017, 2014-12-22 16:29:38]

Done.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_store.cc (right):

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store.cc:20: clock_(new base::DefaultClock())
{
On 2014/12/18 22:51:13, Matt Giuca (OOO until Jan 5) wrote:
> Nit: Indentation is wrong. (Run git cl format?)

Done.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_store.h (right):

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store.h:44: // Gains ownership of |clock|.
On 2014/12/18 22:51:13, Matt Giuca (OOO until Jan 5) wrote:
> Nit: This comment is not necessary (the ownership transfer is implied by
> scoped_ptr).

Done.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_store_unittest.cc (right):

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:39: base::Time expiry_time,
int expired_count, int valid_count) {
On 2014/12/18 22:51:14, Matt Giuca (OOO until Jan 5) wrote:
> The name "expiry_time" is misleading. The expiry times are different for each
> suggestion (computed inside the for loop below). This value is the simulated
> "now" time.
> 
> Can you rename it to current_time (optionally with a comment that this is the
> simulated time, not the actual time).

Done.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:40: int64 expiry_usec =
(expiry_time - base::Time::UnixEpoch())
On 2014/12/18 22:51:13, Matt Giuca (OOO until Jan 5) wrote:
> Indentation (git cl format).

Done.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:40: int64 expiry_usec =
(expiry_time - base::Time::UnixEpoch())
On 2014/12/18 22:51:14, Matt Giuca (OOO until Jan 5) wrote:
> As above, this is not an expiry time, it is a simulated "now". Rename back to
> now_usec.

Renamed to current_time_usec, because having one now_usec and one current_time
variables might be misleading.

https://codereview.chromium.org/816693003/diff/20001/components/suggestions/s...
components/suggestions/suggestions_store_unittest.cc:90: base::Time expiry_time;
On 2014/12/18 22:51:14, Matt Giuca (OOO until Jan 5) wrote:
> As above, this is not an expiry time. Rename to current_time.

Done.

[gayane -on leave until 09-2017, 2014-12-22 16:29:44]

The CQ bit was checked by gayane@chromium.org

[commit-bot: I haz the power, 2014-12-22 16:30:40]

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

[commit-bot: I haz the power, 2014-12-22 17:22:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-22 17:23:43]

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