Add title update merge logic to reading list entry

Add title update merge logic to reading list entry

This CL adds a update_title_time field to the reading list entries, allowing
the title to be correctly merged during sync.

BUG=675644
Committed: https://crrev.com/dfabb5669652d28e9e20c83de16ac7c1aaa03cb4
Cr-Commit-Position: refs/heads/master@{#441354}

[gambard, 2016-12-21 14:45:37]

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

[gambard, 2016-12-21 14:45:38]

PTAL.

[Olivier, 2016-12-21 15:14:01]

You must update CompareEntriesForSync and ReadingListStoreUnittests.

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

https://codereview.chromium.org/2592043002/diff/20001/components/reading_list...
components/reading_list/ios/reading_list_entry.cc:389: title_ =
std::move(other.title_);
what if there is a tie?

[gambard, 2016-12-21 16:38:00]

Thanks, PTAL.

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

https://codereview.chromium.org/2592043002/diff/20001/components/reading_list...
components/reading_list/ios/reading_list_entry.cc:389: title_ =
std::move(other.title_);
On 2016/12/21 15:14:01, Olivier Robin wrote:
> what if there is a tie?

Done.

[Olivier, 2016-12-21 17:03:46]

LGTM after you update the comment.

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

https://codereview.chromium.org/2592043002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:415: bool
ReadingListStore::CompareEntriesForSync(
Update comment of function.

[gambard, 2016-12-22 08:54:16]

The CQ bit was checked by gambard@chromium.org

[gambard, 2016-12-22 08:54:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2592043002/#ps60001
(title: "Update function comment")

[commit-bot: I haz the power, 2016-12-22 08:54:34]

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

[gambard, 2016-12-22 08:54:36]

Thanks!

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

https://codereview.chromium.org/2592043002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:415: bool
ReadingListStore::CompareEntriesForSync(
On 2016/12/21 17:03:46, Olivier Robin wrote:
> Update comment of function.

Done.

[gambard, 2016-12-22 08:57:15]

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

[gambard, 2016-12-22 08:57:16]

pavely: PTAL for sync.
Additional question: can we change existing field name without changing their
type?
Here it would be changing:
"optional int64 update_time_us = 5;"
in
"optional int64 update_state_time_us = 5;"
for example.

[commit-bot: I haz the power, 2016-12-22 09:00:41]

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

[commit-bot: I haz the power, 2016-12-22 09:00:42]

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

[gambard, 2016-12-23 08:38:48]

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

[gambard, 2016-12-23 08:38:49]

+stanisc for sync owner.

[pavely, 2017-01-04 00:12:32]

lgtm

https://codereview.chromium.org/2592043002/diff/60001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.mm (right):

https://codereview.chromium.org/2592043002/diff/60001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.mm:358: 
I think set_update_title_time_us is logically related to corresponding call to
set_title. I would put empty line before set_update_title_time_us, not after it.
The same applies to two more places below.

[gambard, 2017-01-04 09:57:02]

Thanks.

https://codereview.chromium.org/2592043002/diff/60001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.mm (right):

https://codereview.chromium.org/2592043002/diff/60001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.mm:358: 
On 2017/01/04 00:12:32, pavely wrote:
> I think set_update_title_time_us is logically related to corresponding call to
> set_title. I would put empty line before set_update_title_time_us, not after
it.
> The same applies to two more places below.

set_update_title_time_us(110); reset the value of the update_title_time_us to
its default value. This is why it is the last line of the paragraph.

[gambard, 2017-01-04 09:57:07]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-01-04 09:57:15]

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-01-04 11:02:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483523827596570,
"parent_rev": "5906d1185d2a7ecb4762d891d32d7bb2f63a04d1", "commit_rev":
"7a4d8ecb105c73d634fab67b3d0e64404eeded44"}

[commit-bot: I haz the power, 2017-01-04 11:02:40]

Description was changed from

==========
Add title update merge logic to reading list entry

This CL adds a update_title_time field to the reading list entries, allowing
the title to be correctly merged during sync.

BUG=675644
==========

to

==========
Add title update merge logic to reading list entry

This CL adds a update_title_time field to the reading list entries, allowing
the title to be correctly merged during sync.

BUG=675644

Review-Url: https://codereview.chromium.org/2592043002
==========

[commit-bot: I haz the power, 2017-01-04 11:02:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2017-01-04 11:05:31]

Description was changed from

==========
Add title update merge logic to reading list entry

This CL adds a update_title_time field to the reading list entries, allowing
the title to be correctly merged during sync.

BUG=675644

Review-Url: https://codereview.chromium.org/2592043002
==========

to

==========
Add title update merge logic to reading list entry

This CL adds a update_title_time field to the reading list entries, allowing
the title to be correctly merged during sync.

BUG=675644

Committed: https://crrev.com/dfabb5669652d28e9e20c83de16ac7c1aaa03cb4
Cr-Commit-Position: refs/heads/master@{#441354}
==========

[commit-bot: I haz the power, 2017-01-04 11:05:33]

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