Adding support for forced URLs to TopSites.

chrome/browser/history/top_sites.h

Index: chrome/browser/history/top_sites.h
diff --git a/chrome/browser/history/top_sites.h b/chrome/browser/history/top_sites.h
index 92e8eee3430fb9e7ec9225173d23a9cd0091dfe2..94f364f2c9f16a0e8cba51d5742356db5beaedf6 100644
--- a/chrome/browser/history/top_sites.h
+++ b/chrome/browser/history/top_sites.h
@@ -61,16 +61,21 @@ class TopSites
 
   typedef base::Callback<void(const MostVisitedURLList&)>
       GetMostVisitedURLsCallback;
-  typedef std::vector<GetMostVisitedURLsCallback> PendingCallbacks;
 
-  // Returns a list of most visited URLs via a callback. This may be invoked on
-  // any thread.
+  // Returns a list of most visited URLs via a callback, includes only
+  // non-forced URLs. This may be invoked on any thread.
   // NOTE: the callback is called immediately if we have the data cached. If
   // data is not available yet, callback will later be posted to the thread
   // called this function.
   virtual void GetMostVisitedURLs(
       const GetMostVisitedURLsCallback& callback) = 0;
 
+  // Implementation of GetMostVisitedURLs that can retrieve both forced and
+  // non-forced URLs. his may be invoked on any thread.

[brettw, 2013/11/08 22:56:07]

Spelling "his"

[beaudoin, 2013/11/11 21:58:09]

Done.

+  virtual void GetAllMostVisitedURLs(

[brettw, 2013/11/08 22:56:07]

I think that the new parameter should just be added to the existing function
rather than adding this new one. I think there are only a couple (one?)
caller(s) to update.

[beaudoin, 2013/11/11 21:58:09]

That's what I did first but there are surprisingly many callers to
GetMostVisitedURLs (~8 non-test calls). The parameter was a bit opaque too...

Another option would be to have a GetAllMostVisitedURLs that doesn't take a bool
as param, both of them piping into a private GetMostVisitedImpl(callback, bool).

What do you thing?

[brettw, 2013/11/11 22:03:30]

8 doesn't sound like many to me. For simple renames you can TBR owners of
calling code. So I'd like to change this in a followup.

+      const GetMostVisitedURLsCallback& callback,
+      bool include_forced_urls) = 0;
+
   // Gets a thumbnail for a given page. Returns true iff we have the thumbnail.
   // This may be invoked on any thread.
   // If an exact thumbnail URL match fails, |prefix_match| specifies whether or
@@ -134,10 +139,15 @@ class TopSites
   // return it without modification.
   virtual const std::string& GetCanonicalURLString(const GURL& url) const = 0;
 
-  // Returns true if the top sites list is full (i.e. we already have the
-  // maximum number of top sites).  This function also returns false if
-  // TopSites isn't loaded yet.
-  virtual bool IsFull() = 0;
+  // Returns true if the top sites list of non-forced URLs is full (i.e. we
+  // already have the maximum number of non-forced top sites).  This function
+  // also returns false if TopSites isn't loaded yet.
+  virtual bool IsNonForcedFull() = 0;
+
+  // Returns true if the top sites list of forced URLs is full (i.e. we already
+  // have the maximum number of forced top sites).  This function also returns
+  // false if TopSites isn't loaded yet.
+  virtual bool IsForcedFull() = 0;
 
   virtual bool loaded() const = 0;