Reading List create protobuf store

ios/chrome/browser/reading_list/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 "ios/chrome/browser/reading_list/reading_list_entry.h"
 
+#include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
+#include "components/sync/protocol/reading_list_specifics.pb.h"
+#include "net/base/backoff_entry_serializer.h"
+
+namespace {
+// Default Tick clock for |net::BackoffEntry|.
+base::TickClock* ReadingListTickClock() {

[gambard, 2016/09/28 09:19:08]

Do you need with? You are always using the default clock in the constructors,
use nullptr instead.

[Olivier, 2016/09/28 11:20:06]

This is needed for the serialization (not sure why)...

+  static base::TickClock* clock = new base::DefaultTickClock();

[jif-google, 2016/09/27 16:39:03]

Wouldn't that work:

static base::DefaultTickClock clock;
return &clock;

?

[Olivier, 2016/09/28 11:20:06]

Done.

+  return clock;
+}
+}
 
 const net::BackoffEntry::Policy ReadingListEntry::kBackoffPolicy = {
     // Number of initial errors (in sequence) to ignore before applying
     // exponential back-off rules.
     0,
 
     // Initial delay for exponential back-off in ms.
     1000,  // 1 second.
 
     // Factor by which the waiting time will be multiplied.
@@ skipping 12 matching lines @@
 
     false,  // Don't use initial delay unless the last request was an error.
 };
 
 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, 0, 0, WAITING, GURL(), std::move(backoff)) {}
+
+ReadingListEntry::ReadingListEntry(
+    const GURL& url,
+    const std::string& title,
+    int64_t creation_time,
+    int64_t update_time,
+    ReadingListEntry::DistillationState distilled_state,
+    const GURL& distilled_url,
+    std::unique_ptr<net::BackoffEntry> backoff)
     : url_(url),
       title_(title),
-      distilled_state_(WAITING),
-      failed_download_counter_(0) {
+      distilled_url_(distilled_url),
+      distilled_state_(distilled_state),
+      failed_download_counter_(0),
+      creation_time_us_(creation_time),
+      update_time_us_(update_time) {
   if (backoff) {
     backoff_ = std::move(backoff);
   } else {
-    backoff_ = base::MakeUnique<net::BackoffEntry>(&kBackoffPolicy);
+    backoff_ = base::MakeUnique<net::BackoffEntry>(&kBackoffPolicy,
+                                                   ReadingListTickClock());
+  }
+  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_)),
       distilled_url_(std::move(entry.distilled_url_)),
       distilled_state_(std::move(entry.distilled_state_)),
@@ skipping 59 matching lines @@
   // non-error state to an error state.
   if ((distilled_state == WILL_RETRY || distilled_state == ERROR) &&
       distilled_state_ != WILL_RETRY && distilled_state_ != ERROR) {
     backoff_->InformOfRequest(false);
     failed_download_counter_++;
   }
 
   distilled_state_ = distilled_state;
   distilled_url_ = GURL();
 }
+
+int64_t ReadingListEntry::UpdateTime() const {
+  return update_time_us_;
+}
+
+void ReadingListEntry::MarkEntryUpdated() {
+  update_time_us_ =
+      (base::Time::Now() - base::Time::UnixEpoch()).InMicroseconds();
+}
+
+std::unique_ptr<ReadingListEntry> ReadingListEntry::FromReadingListLocal(

[gambard, 2016/09/28 09:19:08]

Please add // static.

[Olivier, 2016/09/28 11:20:06]

Done.

+    const sync_pb::ReadingListLocal& pb_entry) {
+  if (!pb_entry.has_entry() || !pb_entry.entry().has_url()) {
+    return nullptr;
+  }
+  GURL url(pb_entry.entry().url());
+  if (url.is_empty() || !url.is_valid()) {
+    return nullptr;
+  }
+  std::string title;
+  if (pb_entry.entry().has_title()) {
+    title = pb_entry.entry().title();
+  }
+
+  int64_t creation_time_us = 0;
+  if (pb_entry.entry().has_creation_time_us()) {
+    creation_time_us = pb_entry.entry().creation_time_us();
+  }
+
+  int64_t update_time_us = 0;
+  if (pb_entry.entry().has_update_time_us()) {
+    update_time_us = pb_entry.entry().update_time_us();
+  }
+
+  ReadingListEntry::DistillationState distillation_state =
+      ReadingListEntry::WAITING;
+  if (pb_entry.has_distillation_state()) {
+    switch (pb_entry.distillation_state()) {
+      case sync_pb::ReadingListLocal::WAITING:
+        distillation_state = ReadingListEntry::WAITING;
+      case sync_pb::ReadingListLocal::PROCESSING:
+        distillation_state = ReadingListEntry::PROCESSING;
+      case sync_pb::ReadingListLocal::PROCESSED:
+        distillation_state = ReadingListEntry::PROCESSED;
+      case sync_pb::ReadingListLocal::WILL_RETRY:
+        distillation_state = ReadingListEntry::WILL_RETRY;
+      case sync_pb::ReadingListLocal::ERROR:
+        distillation_state = ReadingListEntry::ERROR;
+    }
+  }
+
+  GURL distilled_url;
+  if (pb_entry.has_distilled_url()) {
+    distilled_url = GURL(pb_entry.distilled_url());
+  }
+
+  std::unique_ptr<net::BackoffEntry> backoff;
+  if (pb_entry.has_backoff()) {
+    JSONStringValueDeserializer deserializer(pb_entry.backoff());
+    std::unique_ptr<base::Value> value(
+        deserializer.Deserialize(nullptr, nullptr));
+    if (value) {
+      backoff = net::BackoffEntrySerializer::DeserializeFromValue(
+          *value, &kBackoffPolicy, ReadingListTickClock(), base::Time::Now());
+    }
+  }
+
+  std::unique_ptr<ReadingListEntry> entry(base::MakeUnique<ReadingListEntry>(

[jif-google, 2016/09/27 16:39:03]

this would be slightly more readable:

std::unique_ptr<ReadingListEntry> entry = base::MakeUnique<ReadingListEntry>(...

[Olivier, 2016/09/28 11:20:06]

Done.

+      url, title, creation_time_us, update_time_us, distillation_state,
+      distilled_url, std::move(backoff)));
+  return entry;
+}
+
+std::unique_ptr<sync_pb::ReadingListLocal> ReadingListEntry::AsReadingListLocal(
+    bool read) const {
+  std::unique_ptr<sync_pb::ReadingListLocal> pb_entry =
+      base::MakeUnique<sync_pb::ReadingListLocal>();
+
+  pb_entry->mutable_entry()->set_title(Title());
+  pb_entry->mutable_entry()->set_url(URL().spec());
+  pb_entry->mutable_entry()->set_entry_id(URL().spec());

[jif-google, 2016/09/27 16:39:03]

How about adding a comment such as "there can only be one entry per URL" ?

[Olivier, 2016/09/28 11:20:06]

Done.

+  pb_entry->mutable_entry()->set_creation_time_us(creation_time_us_);
+  pb_entry->mutable_entry()->set_update_time_us(UpdateTime());
+
+  if (read) {
+    pb_entry->mutable_entry()->set_status(sync_pb::ReadingListSpecifics::READ);
+  } else {
+    pb_entry->mutable_entry()->set_status(
+        sync_pb::ReadingListSpecifics::UNREAD);
+  }
+
+  sync_pb::ReadingListLocal::DistillationState distilation_state;
+  switch (DistilledState()) {
+    case ReadingListEntry::WAITING:
+      distilation_state = sync_pb::ReadingListLocal::WAITING;
+    case ReadingListEntry::PROCESSING:
+      distilation_state = sync_pb::ReadingListLocal::PROCESSING;
+    case ReadingListEntry::PROCESSED:
+      distilation_state = sync_pb::ReadingListLocal::PROCESSED;
+    case ReadingListEntry::WILL_RETRY:
+      distilation_state = sync_pb::ReadingListLocal::WILL_RETRY;
+    case ReadingListEntry::ERROR:
+      distilation_state = sync_pb::ReadingListLocal::ERROR;
+  }
+  pb_entry->set_distillation_state(distilation_state);
+  if (DistilledURL().is_valid()) {
+    pb_entry->set_distilled_url(DistilledURL().spec());
+  }
+
+  if (backoff_) {
+    std::unique_ptr<base::Value> backoff =
+        net::BackoffEntrySerializer::SerializeToValue(*backoff_,
+                                                      base::Time::Now());
+
+    std::string output;
+    JSONStringValueSerializer serializer(&output);
+    serializer.Serialize(*backoff);
+    pb_entry->set_backoff(output);
+  }
+  return pb_entry;
+}
+
+bool ReadingListEntry::CompareEntryUpdateTime(const ReadingListEntry& lhs,
+                                              const ReadingListEntry& rhs) {
+  return lhs.UpdateTime() > rhs.UpdateTime();
+}