Reading List iOS: Use external clock in ReadingListEntry.

Reading List iOS: Use external clock in ReadingListEntry.

Currently, tests need to spin the runloop to make sure clock advance.
Use an external clock so we can set the time as needed.

BUG=703187
Review-Url: https://codereview.chromium.org/2764533002
Cr-Commit-Position: refs/heads/master@{#458723}
Committed: https://chromium.googlesource.com/chromium/src/+/56ce061ee96941c746bb98be90331f6cf6c4002e

[Olivier, 2017-03-20 17:02:16]

The CQ bit was checked by olivierrobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 17:03:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 18:13:27]

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

[commit-bot: I haz the power, 2017-03-20 18:13:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Olivier, 2017-03-21 10:01:38]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org

[gambard, 2017-03-21 13:08:07]

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.cc:290: creation_time_us = (now -
base::Time::UnixEpoch()).InMicroseconds();
I am not sure I understand this. What is the use?

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.h (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.h:144: // Sets |title_| to
|title|. Sets |update_title_time_us_| to |now|.
Here and in the others comments of the Entry, you don't set
|update_title_time_us_| to |now|. You set |update_title_time_us_| to the
difference between |now| and the 01/01/1970.
Based on this comment and the one for |update_title_time_us_|, I would have
expected |now| to be this difference (i.e. using TimeToUS in the caller).
Can you update the comments?

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:61: e.SetRead(false,
base::Time::FromTimeT(20));
Test update time

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:64: e.SetRead(true,
base::Time::FromTimeT(30));
Test update time

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:73: // Getters are in
microseconds.
What about creating a kMicrosecondInOneSecond (or a better name) variable with a
1000000 value, then you can use it to have a better link between the time passed
at the creation and the one you test, as you do in the test right below.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:278:
pb_entry->set_update_title_time_us(3);
You are setting it but not checking it.
Same for the creation time.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:283:
base::Time::FromTimeT(10)));
You can test the deserialization value for the backoff?

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:295: int64_t
creation_time_us = entry.UpdateTime();
Maybe you should set a different update time and title update time? (and test
the title update time)

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:298:
entry.AsReadingListLocal(base::Time::Now()));
Why base::Time::Now()?

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:313:
entry.AsReadingListLocal(base::Time::Now()));
Same

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:328:
entry.AsReadingListLocal(base::Time::Now()));
Same

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:357:
pb_entry->set_update_time_us(2);
Can you add the title update time?

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:398:
EXPECT_EQ(local_entry.UpdateTime(), sync_update_time_us);
Maybe add a title update test? :)

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_unittest.cc:656: EXPECT_EQ(time *
1000000, entry.DistillationTime());
Same, use a variable instead of 1000000.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.cc (left):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.cc:91:
base::RunLoop().RunUntilIdle();
What is this used for?

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.cc:46: }
Add new line.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.cc:207:
EXPECT_EQ(specifics.title(), "read title");
We are not testing for all update times?

[Olivier, 2017-03-21 14:01:10]

Thanks

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.cc:290: creation_time_us = (now -
base::Time::UnixEpoch()).InMicroseconds();
On 2017/03/21 13:08:06, gambard wrote:
> I am not sure I understand this. What is the use?

We have to be conservative when deserializing data. So if there is no creation
date, just add one.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.h (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.h:144: // Sets |title_| to
|title|. Sets |update_title_time_us_| to |now|.
On 2017/03/21 13:08:06, gambard wrote:
> Here and in the others comments of the Entry, you don't set
> |update_title_time_us_| to |now|. You set |update_title_time_us_| to the
> difference between |now| and the 01/01/1970.
> Based on this comment and the one for |update_title_time_us_|, I would have
> expected |now| to be this difference (i.e. using TimeToUS in the caller).
> Can you update the comments?

Technically, update_title_time_us_ is a timestamp, not a difference. So now =
update_title_time_us_, just in a different unit (documented in the variable and
in the Time class). I will add a class comment.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:61: e.SetRead(false,
base::Time::FromTimeT(20));
On 2017/03/21 13:08:06, gambard wrote:
> Test update time

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:61: e.SetRead(false,
base::Time::FromTimeT(20));
On 2017/03/21 13:08:06, gambard wrote:
> Test update time

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:64: e.SetRead(true,
base::Time::FromTimeT(30));
On 2017/03/21 13:08:06, gambard wrote:
> Test update time

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:73: // Getters are in
microseconds.
On 2017/03/21 13:08:06, gambard wrote:
> What about creating a kMicrosecondInOneSecond (or a better name) variable with
a
> 1000000 value, then you can use it to have a better link between the time
passed
> at the creation and the one you test, as you do in the test right below.

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:278:
pb_entry->set_update_title_time_us(3);
On 2017/03/21 13:08:07, gambard wrote:
> You are setting it but not checking it.
> Same for the creation time.

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:283:
base::Time::FromTimeT(10)));
On 2017/03/21 13:08:06, gambard wrote:
> You can test the deserialization value for the backoff?

There is no serialized backoff in ReadingListSpecifics

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:295: int64_t
creation_time_us = entry.UpdateTime();
On 2017/03/21 13:08:06, gambard wrote:
> Maybe you should set a different update time and title update time? (and test
> the title update time)

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:298:
entry.AsReadingListLocal(base::Time::Now()));
On 2017/03/21 13:08:06, gambard wrote:
> Why base::Time::Now()?

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:313:
entry.AsReadingListLocal(base::Time::Now()));
On 2017/03/21 13:08:06, gambard wrote:
> Same

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:328:
entry.AsReadingListLocal(base::Time::Now()));
On 2017/03/21 13:08:06, gambard wrote:
> Same

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:357:
pb_entry->set_update_time_us(2);
On 2017/03/21 13:08:07, gambard wrote:
> Can you add the title update time?

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:398:
EXPECT_EQ(local_entry.UpdateTime(), sync_update_time_us);
On 2017/03/21 13:08:06, gambard wrote:
> Maybe add a title update test? :)

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_unittest.cc:656: EXPECT_EQ(time *
1000000, entry.DistillationTime());
On 2017/03/21 13:08:07, gambard wrote:
> Same, use a variable instead of 1000000.

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.cc:46: }
On 2017/03/21 13:08:07, gambard wrote:
> Add new line.

Done.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.cc:207:
EXPECT_EQ(specifics.title(), "read title");
On 2017/03/21 13:08:07, gambard wrote:
> We are not testing for all update times?

This is tested in ReadingListEntry unittests.

[gambard, 2017-03-21 15:36:53]

lgtm with one comment.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.cc:290: creation_time_us = (now -
base::Time::UnixEpoch()).InMicroseconds();
On 2017/03/21 14:01:09, Olivier Robin wrote:
> On 2017/03/21 13:08:06, gambard wrote:
> > I am not sure I understand this. What is the use?
> 
> We have to be conservative when deserializing data. So if there is no creation
> date, just add one.

Acknowledged.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.h (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.h:144: // Sets |title_| to
|title|. Sets |update_title_time_us_| to |now|.
On 2017/03/21 14:01:09, Olivier Robin wrote:
> On 2017/03/21 13:08:06, gambard wrote:
> > Here and in the others comments of the Entry, you don't set
> > |update_title_time_us_| to |now|. You set |update_title_time_us_| to the
> > difference between |now| and the 01/01/1970.
> > Based on this comment and the one for |update_title_time_us_|, I would have
> > expected |now| to be this difference (i.e. using TimeToUS in the caller).
> > Can you update the comments?
> 
> Technically, update_title_time_us_ is a timestamp, not a difference. So now =
> update_title_time_us_, just in a different unit (documented in the variable
and
> in the Time class). I will add a class comment.

Acknowledged.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:283:
base::Time::FromTimeT(10)));
On 2017/03/21 14:01:09, Olivier Robin wrote:
> On 2017/03/21 13:08:06, gambard wrote:
> > You can test the deserialization value for the backoff?
> 
> There is no serialized backoff in ReadingListSpecifics 

Acknowledged.

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.cc:207:
EXPECT_EQ(specifics.title(), "read title");
On 2017/03/21 14:01:10, Olivier Robin wrote:
> On 2017/03/21 13:08:07, gambard wrote:
> > We are not testing for all update times?
> 
> This is tested in ReadingListEntry unittests.

Acknowledged.

https://codereview.chromium.org/2764533002/diff/60001/components/reading_list...
File components/reading_list/ios/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/60001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:282:
pb_entry->set_creation_time_us(1);
You are still not checking the creation time?

[Olivier, 2017-03-21 17:22:47]

Done, thanks

https://codereview.chromium.org/2764533002/diff/60001/components/reading_list...
File components/reading_list/ios/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2764533002/diff/60001/components/reading_list...
components/reading_list/ios/reading_list_entry_unittest.cc:282:
pb_entry->set_creation_time_us(1);
On 2017/03/21 15:36:53, gambard wrote:
> You are still not checking the creation time?
Done, thanks

[Olivier, 2017-03-21 17:22:51]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-21 17:22:52]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2764533002/#ps80001
(title: "fix")

[commit-bot: I haz the power, 2017-03-21 17:23:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 18:30:43]

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

[commit-bot: I haz the power, 2017-03-21 18:30:45]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Olivier, 2017-03-22 09:02:15]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-03-22 09:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2764533002/#ps100001
(title: "jitter")

[commit-bot: I haz the power, 2017-03-22 09:02:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 10:06:02]

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

[commit-bot: I haz the power, 2017-03-22 10:06:03]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Olivier, 2017-03-22 10:07:05]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2017-03-22 10:07:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 11:09:56]

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

[commit-bot: I haz the power, 2017-03-22 11:09:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Olivier, 2017-03-22 11:43:38]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2017-03-22 11:43:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 12:08:45]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490183018565780,
"parent_rev": "66d333e3d935e43745ad8ae27b5b76a3a34f4917", "commit_rev":
"56ce061ee96941c746bb98be90331f6cf6c4002e"}

[commit-bot: I haz the power, 2017-03-22 12:09:30]

Description was changed from

==========
Reading List iOS: Use external clock in ReadingListEntry.

Currently, tests need to spin the runloop to make sure clock advance.
Use an external clock so we can set the time as needed.

BUG=703187
==========

to

==========
Reading List iOS: Use external clock in ReadingListEntry.

Currently, tests need to spin the runloop to make sure clock advance.
Use an external clock so we can set the time as needed.

BUG=703187

Review-Url: https://codereview.chromium.org/2764533002
Cr-Commit-Position: refs/heads/master@{#458723}
Committed:
https://chromium.googlesource.com/chromium/src/+/56ce061ee96941c746bb98be9033...
==========

[commit-bot: I haz the power, 2017-03-22 12:09:31]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/56ce061ee96941c746bb98be9033...