Refactor Reading List Model to use URL as key.

ios/chrome/browser/reading_list/reading_list_model.h

Index: ios/chrome/browser/reading_list/reading_list_model.h
diff --git a/ios/chrome/browser/reading_list/reading_list_model.h b/ios/chrome/browser/reading_list/reading_list_model.h
index 3b9bc28a9383248624ac7c5ca3e85dada6cadc83..7cb5daed87f9b0d22768ae68cc7d8aeb7a8046ec 100644
--- a/ios/chrome/browser/reading_list/reading_list_model.h
+++ b/ios/chrome/browser/reading_list/reading_list_model.h
@@ -58,8 +58,13 @@ class ReadingListModel : public base::NonThreadSafe {
   // Creates a batch token that will freeze the model while in scope.
   virtual std::unique_ptr<ScopedReadingListBatchUpdate> CreateBatchToken();
 
-  // Returns the size of read and unread entries.
+  // Returns the total number of entries in the model.
+  virtual size_t size() const = 0;
+
+  // Returns the total number of unread entries in the model.
   virtual size_t unread_size() const = 0;
+
+  // Returns the total number of read entries in the model.
   virtual size_t read_size() const = 0;

[gambard, 2016/11/23 12:11:06]

This can probably be removed.

[Olivier, 2016/11/28 14:54:14]

Done.

 
   // Returns true if there are entries in the model that were not seen by the
@@ -68,16 +73,20 @@ class ReadingListModel : public base::NonThreadSafe {
   virtual bool HasUnseenEntries() const = 0;
   virtual void ResetUnseenEntries() = 0;
 
-  // TODO(659099): Remove methods.
-  // Returns a specific entry.
-  virtual const ReadingListEntry& GetUnreadEntryAtIndex(size_t index) const = 0;
-  virtual const ReadingListEntry& GetReadEntryAtIndex(size_t index) const = 0;

[gambard, 2016/11/23 12:11:06]

You are breaking downstream by renaming them no?

[Olivier, 2016/11/28 14:54:14]

Done.

-
   // Returns a specific entry. Returns null if the entry does not exist.
-  // If |read| is not null and the entry is found, |*read| is the read status
-  // of the entry.
-  virtual const ReadingListEntry* GetEntryFromURL(const GURL& gurl,
-                                                  bool* read) const = 0;
+  virtual const ReadingListEntry* GetEntryFromURL(const GURL& gurl) const = 0;
+
+  // Allows to iterate through entries in the model. Must be called on a single
+  // runloop to ensure no entry is returned twice and all entries are returned.
+  virtual const ReadingListEntry* GetEntryAt(size_t index) const = 0;

[jif-google, 2016/11/22 18:21:45]

I think it would be useful to offer range based for-loops on the reading list
model, like:

for (const ReadingListEntry& entry : reading_list_model) {


}

[Olivier, 2016/11/28 14:54:14]

Done.

+  // Allows to iterate through read entries in the model. Must be called on a
+  // singlerunloop to ensure no entry is returned twice and all entries are
+  // returned
+  virtual const ReadingListEntry* GetReadEntryAt(size_t index) const = 0;

[jif-google, 2016/11/22 18:21:45]

this will be removed, right?

[Olivier, 2016/11/28 14:54:14]

Done.

+  // Allows to iterate through unread entries in the model. Must be called on a
+  // singlerunloop to ensure no entry is returned twice and all entries are
+  // returned
+  virtual const ReadingListEntry* GetUnreadEntryAt(size_t index) const = 0;

[jif-google, 2016/11/22 18:21:45]

this will be removed, right?

[Olivier, 2016/11/28 14:54:14]

Done.

 
   // Synchronously calls the |callback| with entry associated with this |url|.
   // Does nothing if there is no entry associated.
@@ -98,14 +107,10 @@ class ReadingListModel : public base::NonThreadSafe {
   // immediately.
   virtual void RemoveEntryByURL(const GURL& url) = 0;
 
-  // If the |url| is in the reading list and unread, mark it read. If it is in
-  // the reading list and read, move it to the top of unread if it is not here
-  // already. This may trigger deletion of old read entries.
-  virtual void MarkReadByURL(const GURL& url) = 0;
-  // If the |url| is in the reading list and read, mark it unread. If it is in
-  // the reading list and unread, move it to the top of read if it is not here
-  // already.
-  virtual void MarkUnreadByURL(const GURL& url) = 0;
+  // If the |url| is in the reading list entry(|url|).read != |read|, sets the
+  // read state of the URL to read. This will also update the update time of
+  // the entry.
+  virtual void SetReadStatus(const GURL& url, bool read) = 0;
 
   // Methods to mutate an entry. Will locate the relevant entry by URL. Does
   // nothing if the entry is not found.