Refactor Reading List Model to use URL as key.

components/reading_list/ios/reading_list_model.h

Index: components/reading_list/ios/reading_list_model.h
diff --git a/components/reading_list/ios/reading_list_model.h b/components/reading_list/ios/reading_list_model.h
index b3969fa6a6276c1dc59b0b40165266cb783765b3..cf10f97bae8a71fd67fae298bbff98f488b16639 100644
--- a/components/reading_list/ios/reading_list_model.h
+++ b/components/reading_list/ios/reading_list_model.h
@@ -16,7 +16,6 @@
 #include "components/reading_list/ios/reading_list_model_observer.h"
 
 class GURL;
-class ReadingListEntry;
 class ReadingListModel;
 class ScopedReadingListBatchUpdate;
 
@@ -53,9 +52,12 @@ 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.
-  virtual size_t unread_size() const = 0;
-  virtual size_t read_size() const = 0;
+  // Returns iterator to allow to iterate through reading list entries.
+  virtual const ReadingListEntries::iterator begin() const = 0;
+  virtual const ReadingListEntries::iterator end() const = 0;

[noyau (Ping after 24h), 2016/11/28 15:29:32]

Can this just iterate URLs instead of exposing the map<>?

[Olivier, 2016/11/28 15:38:10]

We can, I will check how to do that.

[Olivier, 2016/11/28 16:16:56]

It is actually not easy as ReadingListModel is an abstract class.
We would have to create a nested ReadingListModel::iterator that is another
abstract class, then a ReadingListModelImpl::iterator that inherits it.

Another option is to keep a vector of URLs, but IMO it is worse than the current
solution.

P.S. I am not an iterator expert, so a simpler solution may exist.

[Olivier, 2016/11/30 09:15:22]

After a discussion with sdefresne, it seems the standard way to do this is to
return a vector of GURL in a Keys method. I did that here.

+
+  // Returns the total number of entries in the model.
+  virtual size_t size() const = 0;

[noyau (Ping after 24h), 2016/11/28 15:29:32]

No need for the unread size? Isn't it what is used in the menu?

[Olivier, 2016/11/28 15:38:10]

Yes, we need it.
I cleaned it too fast
Added back in next PS.

 
   // Returns true if there are entries in the model that were not seen by the
   // user yet. Reset to true when new unread entries are added. Reset to false
@@ -63,23 +65,8 @@ 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;
-
   // 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;
-
-  // Synchronously calls the |callback| with entry associated with this |url|.
-  // Does nothing if there is no entry associated.
-  // Returns whether the callback has been called.
-  virtual bool CallbackEntryURL(
-      const GURL& url,
-      base::Callback<void(const ReadingListEntry&)> callback) const = 0;
+  virtual const ReadingListEntry* GetEntryByURL(const GURL& gurl) const = 0;
 
   // Adds |url| at the top of the unread entries, and removes entries with the
   // same |url| from everywhere else if they exist. The entry title will be a
@@ -93,14 +80,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

[jif-google, 2016/11/28 16:28:21]

missing "and"?
...reading list and entry(|url).read !=...

[Olivier, 2016/11/28 17:58:25]

Done.

+  // 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;

[noyau (Ping after 24h), 2016/11/28 15:29:31]

You removed all the comments about what's happening when something changed from
read to unread and vice versa.

I personally prefer the old two methods API rather than a method with a bool. It
is more readable when used.

[Olivier, 2016/11/28 15:38:10]

I changed because read/unread is now a field in the entry, and the model is just
forwarding this call to the entry (like the other SetTitle,
SetDistillationState...). The SetRead method of ReadingListEntry documents that
it will set the UpdateTime.
Also the documentation about "moving" to the top is not true anymore as the map
is unordered.

 
   // Methods to mutate an entry. Will locate the relevant entry by URL. Does
   // nothing if the entry is not found.
@@ -130,6 +113,18 @@ class ReadingListModel : public base::NonThreadSafe {
     DISALLOW_COPY_AND_ASSIGN(ScopedReadingListBatchUpdate);
   };
 
+  // TODO(crbug.com/664924): Remove temporary methods for transition.
+
+  // Allows to iterate through read entries in the model. Must be called on a

[jif-google, 2016/11/28 16:28:21]

s/Allows to iterate/Allows iterating/

[Olivier, 2016/11/28 17:58:25]

Done.

+  // singlerunloop to ensure no entry is returned twice and all entries are
+  // returned
+  virtual const ReadingListEntry& GetReadEntryAtIndex(size_t index) const = 0;
+  virtual const ReadingListEntry& GetUnreadEntryAtIndex(size_t index) const = 0;
+  virtual void MarkReadByURL(const GURL& url) = 0;
+  virtual void MarkUnreadByURL(const GURL& url) = 0;
+  virtual size_t unread_size() const = 0;
+  virtual size_t read_size() const = 0;
+
  protected:
   ReadingListModel();
   virtual ~ReadingListModel();