Add Unseen state to Reading List Entry

components/reading_list/ios/reading_list_entry.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/reading_list/ios/reading_list_entry.h"
 
 #include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
 #include "components/reading_list/ios/offline_url_utils.h"
 #include "components/reading_list/ios/proto/reading_list.pb.h"
@@ skipping 28 matching lines @@
 };
 
 ReadingListEntry::ReadingListEntry(const GURL& url, const std::string& title)
     : ReadingListEntry(url, title, nullptr){};
 
 ReadingListEntry::ReadingListEntry(const GURL& url,
                                    const std::string& title,
                                    std::unique_ptr<net::BackoffEntry> backoff)
     : ReadingListEntry(url,
                        title,
-                       false,
+                       UNSEEN,
                        0,
                        0,
                        WAITING,
                        base::FilePath(),
                        0,
                        std::move(backoff)) {}
 
 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,
     const base::FilePath& distilled_path,
     int failed_download_counter,
     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),
       creation_time_us_(creation_time),
       update_time_us_(update_time) {
   if (backoff) {
     backoff_ = std::move(backoff);
   } else {
     backoff_ = base::MakeUnique<net::BackoffEntry>(&kBackoffPolicy);
   }
   if (creation_time_us_ == 0) {
     DCHECK(update_time_us_ == 0);
     creation_time_us_ =
         (base::Time::Now() - base::Time::UnixEpoch()).InMicroseconds();
     update_time_us_ = creation_time_us_;
   }
   DCHECK(!url.is_empty());
   DCHECK(url.is_valid());
 }
 
 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_)),
       failed_download_counter_(std::move(entry.failed_download_counter_)),
       creation_time_us_(std::move(entry.creation_time_us_)),
       update_time_us_(std::move(entry.update_time_us_)) {}
 
 ReadingListEntry::~ReadingListEntry() {}
 
 const GURL& ReadingListEntry::URL() const {
@@ skipping 19 matching lines @@
 int ReadingListEntry::FailedDownloadCounter() const {
   return failed_download_counter_;
 }
 
 ReadingListEntry& ReadingListEntry::operator=(ReadingListEntry&& other) {
   url_ = std::move(other.url_);
   title_ = std::move(other.title_);
   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_);
   return *this;
 }
 
 bool ReadingListEntry::operator==(const ReadingListEntry& other) const {
   return url_ == other.url_;
 }
 
 void ReadingListEntry::SetTitle(const std::string& title) {
   title_ = 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) {
   DCHECK(!path.empty());
   distilled_path_ = path;
   distilled_state_ = PROCESSED;
   backoff_->Reset();
   failed_download_counter_ = 0;
 }
 
@@ skipping 43 matching lines @@
   int64_t creation_time_us = 0;
   if (pb_entry.has_creation_time_us()) {
     creation_time_us = pb_entry.creation_time_us();
   }
 
   int64_t update_time_us = 0;
   if (pb_entry.has_update_time_us()) {
     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 =
       ReadingListEntry::WAITING;
   if (pb_entry.has_distillation_state()) {
     switch (pb_entry.distillation_state()) {
       case reading_list::ReadingListLocal::WAITING:
         distillation_state = ReadingListEntry::WAITING;
         break;
       case reading_list::ReadingListLocal::PROCESSING:
@@ skipping 26 matching lines @@
     JSONStringValueDeserializer deserializer(pb_entry.backoff());
     std::unique_ptr<base::Value> value(
         deserializer.Deserialize(nullptr, nullptr));
     if (value) {
       backoff = net::BackoffEntrySerializer::DeserializeFromValue(
           *value, &kBackoffPolicy, nullptr, base::Time::Now());
     }
   }
 
   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)));
 }
 
 // static
 std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListSpecifics(
     const sync_pb::ReadingListSpecifics& pb_entry) {
   if (!pb_entry.has_url()) {
     return nullptr;
   }
   GURL url(pb_entry.url());
   if (url.is_empty() || !url.is_valid()) {
     return nullptr;
   }
   std::string title;
   if (pb_entry.has_title()) {
     title = pb_entry.title();
   }
 
   int64_t creation_time_us = 0;
   if (pb_entry.has_creation_time_us()) {
     creation_time_us = pb_entry.creation_time_us();
   }
 
   int64_t update_time_us = 0;
   if (pb_entry.has_update_time_us()) {
     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));
 }
 
 void ReadingListEntry::MergeLocalStateFrom(ReadingListEntry& other) {
   distilled_path_ = std::move(other.distilled_path_);
   distilled_state_ = std::move(other.distilled_state_);
   backoff_ = std::move(other.backoff_);
   failed_download_counter_ = std::move(other.failed_download_counter_);
 }
 
 std::unique_ptr<reading_list::ReadingListLocal>
 ReadingListEntry::AsReadingListLocal() const {
   std::unique_ptr<reading_list::ReadingListLocal> pb_entry =
       base::MakeUnique<reading_list::ReadingListLocal>();
 
   // URL is used as the key for the database and sync as there is only one entry
   // per URL.
   pb_entry->set_entry_id(URL().spec());
   pb_entry->set_title(Title());
   pb_entry->set_url(URL().spec());
   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 if (state_== UNREAD) {
+    pb_entry->set_status(reading_list::ReadingListLocal::UNREAD);
   } else {
-    pb_entry->set_status(reading_list::ReadingListLocal::UNREAD);
+    pb_entry->set_status(reading_list::ReadingListLocal::UNSEEN);
   }
 
   reading_list::ReadingListLocal::DistillationState distilation_state;
   switch (DistilledState()) {
     case ReadingListEntry::WAITING:
       distilation_state = reading_list::ReadingListLocal::WAITING;
       break;
     case ReadingListEntry::PROCESSING:
       distilation_state = reading_list::ReadingListLocal::PROCESSING;
       break;
@@ skipping 32 matching lines @@
       base::MakeUnique<sync_pb::ReadingListSpecifics>();
 
   // URL is used as the key for the database and sync as there is only one entry
   // per URL.
   pb_entry->set_entry_id(URL().spec());
   pb_entry->set_title(Title());
   pb_entry->set_url(URL().spec());
   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 if (state_== UNREAD) {
+    pb_entry->set_status(sync_pb::ReadingListSpecifics::UNREAD);
   } else {
-    pb_entry->set_status(sync_pb::ReadingListSpecifics::UNREAD);
+    pb_entry->set_status(sync_pb::ReadingListSpecifics::UNSEEN);
   }
 
   return pb_entry;
 }
 
 bool ReadingListEntry::CompareEntryUpdateTime(const ReadingListEntry& lhs,
                                               const ReadingListEntry& rhs) {
   return lhs.UpdateTime() > rhs.UpdateTime();
 }