Skip to content

GitLab

T
tangled
Commit 27a90aee authored by olivierrobin's avatar olivierrobin Committed by Commit bot
Browse files
Options

Add merge rules for first read time 

Also fix the seen merge logic.

BUG=651041

Review-Url: https://codereview.chromium.org/2558183004
Cr-Commit-Position: refs/heads/master@{#437506}
parent 5277d315
Hide whitespace changes
Inline Side-by-side
Showing with 117 additions and 61 deletions
components/reading_list/ios/reading_list_entry.cc
View file @ 27a90aee
......@@ -324,7 +324,7 @@ std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListSpecifics(
int64_t first_read_time_us = 0;
if (pb_entry.has_first_read_time_us()) {
creation_time_us = pb_entry.first_read_time_us();
first_read_time_us = pb_entry.first_read_time_us();
}
int64_t update_time_us = 0;
......@@ -368,22 +368,27 @@ void ReadingListEntry::MergeWithEntry(const ReadingListEntry& other) {
}
if (creation_time_us_ < other.creation_time_us_) {
creation_time_us_ = std::move(other.creation_time_us_);
}
if (state_ == UNSEEN) {
state_ = std::move(other.state_);
} else if (other.state_ != UNSEEN) {
// Both are not UNSEEN, take the newer one.
if (update_time_us_ < other.update_time_us_) {
state_ = std::move(other.state_);
} else if (update_time_us_ == other.update_time_us_) {
// Both states are likely the same, but if they are not, READ should win.
if (other.state_ == READ) {
state_ = std::move(other.state_);
}
first_read_time_us_ = std::move(other.first_read_time_us_);
} else if (creation_time_us_ == other.creation_time_us_) {
// The first_time_read_us from |other| is used if
// - this.first_time_read_us == 0: the entry was never read in this device.
// - this.first_time_read_us > other.first_time_read_us: the entry was
// first read on another device.
if (first_read_time_us_ == 0 ||
(other.first_read_time_us_ != 0 &&
other.first_read_time_us_ < first_read_time_us_)) {
first_read_time_us_ = std::move(other.first_read_time_us_);
}
}
if (update_time_us_ < other.update_time_us_) {
update_time_us_ = std::move(other.update_time_us_);
state_ = std::move(other.state_);
} else if (update_time_us_ == other.update_time_us_) {
if (state_ == UNSEEN) {
state_ = std::move(other.state_);
} else if (other.state_ == READ) {
state_ = std::move(other.state_);
}
}
#if !defined(NDEBUG)
std::unique_ptr<sync_pb::ReadingListSpecifics> new_this_pb(
......
components/reading_list/ios/reading_list_entry_unittest.cc
View file @ 27a90aee
......@@ -317,6 +317,8 @@ TEST(ReadingListEntry, FromReadingListLocal) {
}
// Tests the merging of two ReadingListEntry.
// Additional merging tests are done in
// ReadingListStoreTest.CompareEntriesForSync
TEST(ReadingListEntry, MergeWithEntry) {
ReadingListEntry local_entry(GURL("http://example.com/"), "title");
local_entry.SetDistilledState(ReadingListEntry::ERROR);
......@@ -339,24 +341,3 @@ TEST(ReadingListEntry, MergeWithEntry) {
base::TimeDelta delta = merge_next_call - next_call;
EXPECT_NEAR(delta.InMillisecondsRoundedUp(), 0, 10);
}
// Tests the merging of two ReadingListEntry, the oldest one SEEN and the newer
// UNSEEN.
TEST(ReadingListEntry, MergeWithEntrySeen) {
ReadingListEntry local_entry(GURL("http://example.com/"), "title");
local_entry.SetRead(true);
int64_t local_update_time_us = local_entry.UpdateTime();
local_entry.SetDistilledPath(base::FilePath("distilled/page.html"));
ReadingListEntry sync_entry(GURL("http://example.com/"), "title2");
int64_t sync_update_time_us = sync_entry.UpdateTime();
EXPECT_NE(local_update_time_us, sync_update_time_us);
local_entry.MergeWithEntry(sync_entry);
EXPECT_EQ(local_entry.URL().spec(), "http://example.com/");
EXPECT_EQ(local_entry.Title(), "title2");
EXPECT_TRUE(local_entry.HasBeenSeen());
EXPECT_EQ(local_entry.UpdateTime(), sync_update_time_us);
EXPECT_EQ(local_entry.FailedDownloadCounter(), 0);
EXPECT_EQ(local_entry.DistilledState(), ReadingListEntry::PROCESSED);
EXPECT_EQ(local_entry.DistilledPath().value(), "distilled/page.html");
}
components/reading_list/ios/reading_list_store.cc
View file @ 27a90aee
......@@ -442,5 +442,14 @@ bool ReadingListStore::CompareEntriesForSync(
lhs.status() == sync_pb::ReadingListSpecifics::READ))
return false;
}
if (rhs.creation_time_us() == lhs.creation_time_us()) {
if (rhs.first_read_time_us() == 0 && lhs.first_read_time_us() != 0) {
return false;
}
if (rhs.first_read_time_us() > lhs.first_read_time_us() &&
lhs.first_read_time_us() != 0) {
return false;
}
}
return true;
}
components/reading_list/ios/reading_list_store.h
View file @ 27a90aee
......@@ -79,11 +79,23 @@ class ReadingListStore : public syncer::ModelTypeSyncBridge,
// Entries are in increasing order if all the fields respect increasing order.
// - URL must be the same.
// - title must verify rhs.title.compare(lhs.title) >= 0
// - rhs.creation_time_us >= lhs.creation_time_us
// - rhs.update_time_us >= lhs.update_time_us
// - if rhs.update_time_us > lhs.update_time_us, rhs.state can be anything.
// - if rhs.update_time_us == lhs.update_time_us, rhs.state >= lhs.state in
// the order UNSEEN, UNREAD, READ.
// - creation_time_us:
// rhs.creation_time_us >= lhs.creation_time_us
// - rhs.first_read_time_us:
// if rhs.creation_time_us > lhs.creation_time_us,
// rhs.first_read_time_us can be anything.
// if rhs.creation_time_us == lhs.creation_time_us
// and rhs.first_read_time_us == 0
// rhs.first_read_time_us can be anything.
// if rhs.creation_time_us == lhs.creation_time_us,
// rhs.first_read_time_us <= lhs.first_read_time_us
// - update_time_us:
// rhs.update_time_us >= lhs.update_time_us
// - state:
// if rhs.update_time_us > lhs.update_time_us
// rhs.state can be anything.
// if rhs.update_time_us == lhs.update_time_us
// rhs.state >= lhs.state in the order UNSEEN, UNREAD, READ.
static bool CompareEntriesForSync(const sync_pb::ReadingListSpecifics& lhs,
const sync_pb::ReadingListSpecifics& rhs);
......
components/reading_list/ios/reading_list_store_unittest.mm
View file @ 27a90aee
......@@ -17,6 +17,30 @@
#include "components/sync/model/model_type_store_test_util.h"
#include "testing/gtest/include/gtest/gtest.h"
// Tests that the transition from |entryA| to |entryB| is possible (|possible|
// is true) or not.
void ExpectAB(const sync_pb::ReadingListSpecifics& entryA,
const sync_pb::ReadingListSpecifics& entryB,
bool possible) {
EXPECT_EQ(ReadingListStore::CompareEntriesForSync(entryA, entryB), possible);
std::unique_ptr<ReadingListEntry> a =
ReadingListEntry::FromReadingListSpecifics(entryA);
std::unique_ptr<ReadingListEntry> b =
ReadingListEntry::FromReadingListSpecifics(entryB);
a->MergeWithEntry(*b);
std::unique_ptr<sync_pb::ReadingListSpecifics> mergedEntry =
a->AsReadingListSpecifics();
if (possible) {
// If transition is possible, the merge should be B.
EXPECT_EQ(entryB.SerializeAsString(), mergedEntry->SerializeAsString());
} else {
// If transition is not possible, the transition shold be possible to the
// merged state.
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, *mergedEntry));
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryB, *mergedEntry));
}
}
class FakeModelTypeChangeProcessorObserver {
public:
virtual void Put(const std::string& client_tag,
......@@ -293,48 +317,73 @@ TEST_F(ReadingListStoreTest, ApplySyncChangesOneRemove) {
TEST_F(ReadingListStoreTest, CompareEntriesForSync) {
sync_pb::ReadingListSpecifics entryA;
sync_pb::ReadingListSpecifics entryB;
entryA.set_url("http://foo.bar");
entryB.set_url("http://foo.bar");
entryA.set_entry_id("http://foo.bar/");
entryB.set_entry_id("http://foo.bar/");
entryA.set_url("http://foo.bar/");
entryB.set_url("http://foo.bar/");
entryA.set_title("Foo Bar");
entryB.set_title("Foo Bar");
entryA.set_status(sync_pb::ReadingListSpecifics::UNREAD);
entryB.set_status(sync_pb::ReadingListSpecifics::UNREAD);
entryA.set_creation_time_us(10);
entryB.set_creation_time_us(10);
entryA.set_first_read_time_us(50);
entryB.set_first_read_time_us(50);
entryA.set_update_time_us(100);
entryB.set_update_time_us(100);
// Equal entries can be submitted.
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, true);
// Try to update each field.
// You cannot change the URL of an entry.
entryA.set_url("http://foo.foo");
entryA.set_url("http://foo.foo/");
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
entryA.set_url("http://foo.bar");
entryA.set_url("http://foo.bar/");
// You can set a title to a title later in alphabetical order.
entryA.set_title("");
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_title("Foo Aar");
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_title("Foo Ba");
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_title("Foo Bar");
entryA.set_creation_time_us(9);
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_first_read_time_us(51);
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_first_read_time_us(49);
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_first_read_time_us(0);
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_first_read_time_us(50);
entryB.set_first_read_time_us(0);
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryB.set_first_read_time_us(50);
entryA.set_creation_time_us(10);
entryA.set_first_read_time_us(51);
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_first_read_time_us(0);
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
entryA.set_first_read_time_us(50);
entryA.set_update_time_us(99);
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
sync_pb::ReadingListSpecifics::ReadingListEntryStatus status_oder[3] = {
sync_pb::ReadingListSpecifics::UNSEEN,
sync_pb::ReadingListSpecifics::UNREAD,
......@@ -343,20 +392,20 @@ TEST_F(ReadingListStoreTest, CompareEntriesForSync) {
entryA.set_status(status_oder[index_a]);
for (int index_b = 0; index_b < 3; index_b++) {
entryB.set_status(status_oder[index_b]);
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
}
}
entryA.set_update_time_us(100);
for (int index_a = 0; index_a < 3; index_a++) {
entryA.set_status(status_oder[index_a]);
entryB.set_status(status_oder[index_a]);
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, true);
for (int index_b = index_a + 1; index_b < 3; index_b++) {
entryB.set_status(status_oder[index_b]);
EXPECT_TRUE(ReadingListStore::CompareEntriesForSync(entryA, entryB));
EXPECT_FALSE(ReadingListStore::CompareEntriesForSync(entryB, entryA));
ExpectAB(entryA, entryB, true);
ExpectAB(entryB, entryA, false);
}
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment