Reading List create protobuf store

ios/chrome/browser/reading_list/reading_list_entry.h

 // 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.
 
 #ifndef IOS_CHROME_BROWSER_READING_LIST_READING_LIST_ENTRY_H_
 #define IOS_CHROME_BROWSER_READING_LIST_READING_LIST_ENTRY_H_
 
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
+#include "base/time/default_tick_clock.h"
+#include "base/time/time.h"
 #include "net/base/backoff_entry.h"
 #include "url/gurl.h"
 
+namespace sync_pb {
+class ReadingListLocal;
+}
+
 // An entry in the reading list. The URL is a unique identifier for an entry, as
 // such it should not be empty and is the only thing considered when comparing
 // entries.
 class ReadingListEntry {
  public:
-  ReadingListEntry(const GURL& url, const std::string& title);
-  ReadingListEntry(const GURL& url,
-                   const std::string& title,
-                   std::unique_ptr<net::BackoffEntry> backoff);
-  ReadingListEntry(ReadingListEntry&& entry);
-  ~ReadingListEntry();
-
   // Entries are created in WAITING state. At some point they will be PROCESSING
   // into one of the three state: PROCESSED, the only state a distilled URL
   // would be set, WILL_RETRY, similar to wait, but with exponential delays or
   // ERROR where the system will not retry at all.
   enum DistillationState { WAITING, PROCESSING, PROCESSED, WILL_RETRY, ERROR };
 
+  ReadingListEntry(const GURL& url, const std::string& title);
+  ReadingListEntry(const GURL& url,
+                   const std::string& title,
+                   std::unique_ptr<net::BackoffEntry> backoff);
+  ReadingListEntry(const GURL& url,
+                   const std::string& title,
+                   int64_t creation_time,
+                   int64_t update_time,
+                   DistillationState distilled_state,
+                   const GURL& distilled_url,
+                   std::unique_ptr<net::BackoffEntry> backoff);
+  ReadingListEntry(ReadingListEntry&& entry);
+  ~ReadingListEntry();
+
+
   static const net::BackoffEntry::Policy kBackoffPolicy;
 
   // The URL of the page the user would like to read later.
   const GURL& URL() const;
   // The title of the entry. Might be empty.
   const std::string& Title() const;
   // What state this entry is in.
   DistillationState DistilledState() const;
   // The local file URL for the distilled version of the page. This should only
   // be called if the state is "PROCESSED".
   const GURL& DistilledURL() const;
   // The time before the next try. This is automatically increased when the
   // state is set to WILL_RETRY or ERROR from a non-error state.
   base::TimeDelta TimeUntilNextTry() const;
   // The number of time chrome failed to download this entry. This is
   // automatically increased when the state is set to WILL_RETRY or ERROR from a
   // non-error state.
   int FailedDownloadCounter() const;
 
+  // The last update time of the entry. Thes value may be used to sort the

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

s/Thes/These/

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

I think it is s/Thes/This/

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

Done.

+  // entries.
+  int64_t UpdateTime() const;

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

use base::time instead of int64_t ?
If not, then put in the comment which unit the time is in and the reference
point.

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

I did use base::time, but as the value is used only for sorting and must be
converted to int64 each time it is read/written to database, I opted for int64.
Adding comment.

+
+  // Set the update time to now.
+  void MarkEntryUpdated();
+
+  // Converts a ReadingListEntry to the protobuf format.

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

How about:
// Returns a protobuf encoding the content of this ReadingListEntry.

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

Done.

+  std::unique_ptr<sync_pb::ReadingListLocal> AsReadingListLocal(
+      bool read) const;
+
+  // Created a ReadingListEntry from the protobuf format.
+  static std::unique_ptr<ReadingListEntry> FromReadingListLocal(
+      const sync_pb::ReadingListLocal& pb_entry);
+
   ReadingListEntry& operator=(ReadingListEntry&& other);
+
   bool operator==(const ReadingListEntry& other) const;
 
+  // Compares two entries using update time, most recent first.

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

Maybe be a bit more explicit? Like // returns whether |lhs| is more recent than
|rhs|

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

Done.

+  static bool CompareEntryUpdateTime(const ReadingListEntry& lhs,
+                                     const ReadingListEntry& rhs);
+
   // Sets the title.
   void SetTitle(const std::string& title);
   // Sets the distilled URL and switch the state to PROCESSED and reset the time
   // until the next try.
   void SetDistilledURL(const GURL& url);
   // Sets the state to one of PROCESSING, WILL_RETRY or ERROR.
   void SetDistilledState(DistillationState distilled_state);
 
  private:
   GURL url_;
   std::string title_;
   GURL distilled_url_;
   DistillationState distilled_state_;
+
   std::unique_ptr<net::BackoffEntry> backoff_;
   int failed_download_counter_;
+  int64_t creation_time_us_;

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

base::time ?

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

what do the "us" part of the variable name mean? Can you add a comment?

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

same comment as above.
This value is fundamentally used for storage and sorting the database.
Converting it each time to base::time seems useless to me (but I can do it if
you really want).

+  int64_t update_time_us_;
 
   DISALLOW_COPY_AND_ASSIGN(ReadingListEntry);
 };
 
+typedef std::vector<ReadingListEntry> ReadingListEntries;

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

Use "using" instead of "typedef". It's better, and it's in the style guide:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao

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

move ReadingListEntries definition to where it's used, i.e.
reading_list_model_impl.h ?

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

Done.

+
 #endif  // IOS_CHROME_BROWSER_READING_LIST_READING_LIST_ENTRY_H_