Add Unseen state to Reading List Entry

components/reading_list/ios/reading_list_entry.cc

Index: components/reading_list/ios/reading_list_entry.cc
diff --git a/components/reading_list/ios/reading_list_entry.cc b/components/reading_list/ios/reading_list_entry.cc
index 5e0c65e6888c9f8758b2ac239f8c43974dd8d549..2f9b9e2bb75d7ba8f541d7339903080852f065d0 100644
--- a/components/reading_list/ios/reading_list_entry.cc
+++ b/components/reading_list/ios/reading_list_entry.cc
@@ -46,7 +46,7 @@ ReadingListEntry::ReadingListEntry(const GURL& url,
                                    std::unique_ptr<net::BackoffEntry> backoff)
     : ReadingListEntry(url,
                        title,
-                       false,
+                       UNSEEN,
                        0,
                        0,
                        WAITING,
@@ -57,7 +57,7 @@ ReadingListEntry::ReadingListEntry(const GURL& url,
 ReadingListEntry::ReadingListEntry(
     const GURL& url,
     const std::string& title,
-    bool read,
+    State state,
     int64_t creation_time,
     int64_t update_time,
     ReadingListEntry::DistillationState distilled_state,
@@ -66,7 +66,7 @@ ReadingListEntry::ReadingListEntry(
     std::unique_ptr<net::BackoffEntry> backoff)
     : url_(url),
       title_(title),
-      read_(read),
+      state_(state),
       distilled_path_(distilled_path),
       distilled_state_(distilled_state),
       failed_download_counter_(failed_download_counter),
@@ -90,7 +90,7 @@ ReadingListEntry::ReadingListEntry(
 ReadingListEntry::ReadingListEntry(ReadingListEntry&& entry)
     : url_(std::move(entry.url_)),
       title_(std::move(entry.title_)),
-      read_(std::move(entry.read_)),
+      state_(std::move(entry.state_)),
       distilled_path_(std::move(entry.distilled_path_)),
       distilled_state_(std::move(entry.distilled_state_)),
       backoff_(std::move(entry.backoff_)),
@@ -130,7 +130,7 @@ ReadingListEntry& ReadingListEntry::operator=(ReadingListEntry&& other) {
   distilled_path_ = std::move(other.distilled_path_);
   distilled_state_ = std::move(other.distilled_state_);
   backoff_ = std::move(other.backoff_);
-  read_ = std::move(other.read_);
+  state_ = std::move(other.state_);
   failed_download_counter_ = std::move(other.failed_download_counter_);
   creation_time_us_ = std::move(other.creation_time_us_);
   update_time_us_ = std::move(other.update_time_us_);
@@ -146,12 +146,16 @@ void ReadingListEntry::SetTitle(const std::string& title) {
 }
 
 void ReadingListEntry::SetRead(bool read) {
-  read_ = read;
+  state_ = read ? READ : UNREAD;
   MarkEntryUpdated();
 }
 
 bool ReadingListEntry::IsRead() const {
-  return read_;
+  return state_ == READ;
+}
+
+bool ReadingListEntry::HasBeenSeen() const {
+  return state_ != UNSEEN;
 }
 
 void ReadingListEntry::SetDistilledPath(const base::FilePath& path) {
@@ -215,9 +219,13 @@ std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
     update_time_us = pb_entry.update_time_us();
   }
 
-  bool read = false;
+  State state = UNSEEN;

[pavely, 2016/12/05 19:10:31]

Absent status is the same as UNSEEN. Is it worth adding comment in proto file
that they are treated the same?

[Olivier, 2016/12/06 09:59:29]

Done.

   if (pb_entry.has_status()) {
-    read = pb_entry.status() == reading_list::ReadingListLocal::READ;
+    if (pb_entry.status() == reading_list::ReadingListLocal::READ) {
+      state = READ;
+    } else if (pb_entry.status() == reading_list::ReadingListLocal::UNREAD) {
+      state = UNREAD;
+    }
   }
 
   ReadingListEntry::DistillationState distillation_state =
@@ -264,7 +272,7 @@ std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(
   }
 
   return base::WrapUnique<ReadingListEntry>(new ReadingListEntry(
-      url, title, read, creation_time_us, update_time_us, distillation_state,
+      url, title, state, creation_time_us, update_time_us, distillation_state,
       distilled_path, failed_download_counter, std::move(backoff)));
 }
 
@@ -293,13 +301,17 @@ std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListSpecifics(
     update_time_us = pb_entry.update_time_us();
   }
 
-  bool read = false;
+  State state = UNSEEN;
   if (pb_entry.has_status()) {
-    read = pb_entry.status() == sync_pb::ReadingListSpecifics::READ;
+    if (pb_entry.status() == sync_pb::ReadingListSpecifics::READ) {
+      state = READ;
+    } else if (pb_entry.status() == sync_pb::ReadingListSpecifics::UNREAD) {
+      state = UNREAD;
+    }
   }
 
   return base::WrapUnique<ReadingListEntry>(
-      new ReadingListEntry(url, title, read, creation_time_us, update_time_us,
+      new ReadingListEntry(url, title, state, creation_time_us, update_time_us,
                            WAITING, base::FilePath(), 0, nullptr));
 }
 
@@ -323,10 +335,12 @@ ReadingListEntry::AsReadingListLocal() const {
   pb_entry->set_creation_time_us(CreationTime());
   pb_entry->set_update_time_us(UpdateTime());
 
-  if (read_) {
+  if (state_== READ) {

[pavely, 2016/12/05 19:10:31]

When handling enum values, it is better to to use switch statement,makes it
easier to read (no chance of seeing unexpected condition in one of the "if"s in
the middle). It also helps you to not forget to update code when set of values
changes.

     pb_entry->set_status(reading_list::ReadingListLocal::READ);
-  } else {
+  } else if (state_== UNREAD) {
     pb_entry->set_status(reading_list::ReadingListLocal::UNREAD);
+  } else {
+    pb_entry->set_status(reading_list::ReadingListLocal::UNSEEN);
   }
 
   reading_list::ReadingListLocal::DistillationState distilation_state;
@@ -379,10 +393,12 @@ ReadingListEntry::AsReadingListSpecifics() const {
   pb_entry->set_creation_time_us(CreationTime());
   pb_entry->set_update_time_us(UpdateTime());
 
-  if (read_) {
+  if (state_== READ) {
     pb_entry->set_status(sync_pb::ReadingListSpecifics::READ);
-  } else {
+  } else if (state_== UNREAD) {
     pb_entry->set_status(sync_pb::ReadingListSpecifics::UNREAD);
+  } else {
+    pb_entry->set_status(sync_pb::ReadingListSpecifics::UNSEEN);
   }
 
   return pb_entry;