Update MostVisitedSites observer interface.

chrome/browser/android/ntp/most_visited_sites.h

Index: chrome/browser/android/ntp/most_visited_sites.h
diff --git a/chrome/browser/android/ntp/most_visited_sites.h b/chrome/browser/android/ntp/most_visited_sites.h
index 9e1d49cdba6d660c87ea78523678165b13c0bb8a..f76f99d720bb8f0c98c8225dcbac5ce192a6d44a 100644
--- a/chrome/browser/android/ntp/most_visited_sites.h
+++ b/chrome/browser/android/ntp/most_visited_sites.h
@@ -33,23 +33,9 @@ class PrefRegistrySyncable;
 }
 
 class PopularSites;
+struct PopularSites_Site;
 class Profile;
 
-// The observer to be notified when the list of most visited sites changes.
-class MostVisitedSitesObserver {
- public:
-  virtual ~MostVisitedSitesObserver() {}
-
-  virtual void OnMostVisitedURLsAvailable(
-      const std::vector<base::string16>& titles,
-      const std::vector<std::string>& urls,
-      const std::vector<std::string>& whitelist_icon_paths) = 0;
-  virtual void OnPopularURLsAvailable(
-      const std::vector<std::string>& urls,
-      const std::vector<std::string>& favicon_urls,
-      const std::vector<std::string>& large_icon_urls) = 0;
-};
-
 // Tracks the list of most visited sites and their thumbnails.
 //
 // Do not use, except from MostVisitedSitesBridge. The interface is in flux
@@ -60,13 +46,19 @@ class MostVisitedSitesObserver {
 class MostVisitedSites : public history::TopSitesObserver,
                          public SupervisedUserServiceObserver {
  public:
+  class Observer;
+  class Suggestion;

[Marc Treib, 2016/04/25 13:58:38]

Any reason for not defining these inline here? Since they're part of the public
interface, I'd find that easier to read.

[sfiera, 2016/04/25 16:26:08]

I find out-of-line easier, though some csearching suggests in-line is far more
common in Chrome.

The one technical thing is that Suggestion has to be defined after
MostVisitedSource. Maybe it's weird that a private enum is in a public class,
though?

[Marc Treib, 2016/04/25 16:42:36]

That's my impression as well, but I don't have numbers to back that up ;)
Huh. I find it weird that that even works. I'd make the enum public then I
guess.

[sfiera, 2016/04/26 09:18:17]

My numbers say it's about 100:1 in favor of in-line.
https://cs.corp.google.com/search/?q=package:clankium+file:h$+-file:third_par...
https://cs.corp.google.com/search/?q=package:clankium+file:h$+-file:third_par...
Done and inlined.

[Marc Treib, 2016/04/26 09:57:55]

Thanks for checking!

+
+  using SuggestionsVector = std::vector<std::unique_ptr<Suggestion>>;
+  using PopularSiteVector = std::vector<PopularSites_Site>;

[Marc Treib, 2016/04/25 13:58:38]

Why is one of these a vector of pointers, the other of instances? That seems
inconsistent and surprising - copying should be similarly expensive for both.
I'd prefer vector of instances, unless there's a good reason for pointers?

Also, nit: Please use either singular or plural for both.

[sfiera, 2016/04/25 16:26:08]

These were the types that MostVisitedSites and PopularSites already were using.

Internally, MostVisitedSites wants pointers because when merging sources, it
uses nulls to indicate where suggestions are still needed. It wouldn't be hard
to make the public interface use instances, though. We'd really only need to
change SaveNewNTPSuggestions() to use a more complicated swap.

PopularSites's public interface is with the vector of instances; I didn't look
into what it would take to change that.

[Marc Treib, 2016/04/25 16:42:36]

Ah, I see.
Much of that merging/order-persisting code is going to go away: crbug.com/601734
After that's removed, it should be less work to switch to instances. So it's
okay to leave it as-is, but please add a TODO(treib) so I'll remember. Or,
alternatively, feel free to take that part over too :)

[sfiera, 2016/04/26 09:18:17]

Well, in that case, how about I go ahead and update the public interface to use
instances, and add the TODO to stop using pointers internally?

(I've made that change but that part is again easy to revert)

[Marc Treib, 2016/04/26 09:37:28]

Sure, SGTM.
I just wanted to avoid unnecessary work in updating code that'll be deleted
soon.

+
   explicit MostVisitedSites(Profile* profile);
 
   ~MostVisitedSites() override;
 
   // Does not take ownership of |observer|, which must outlive this object.
   void SetMostVisitedURLsObserver(
-      MostVisitedSitesObserver* observer, int num_sites);
+      Observer* observer, int num_sites);
 
   using ThumbnailCallback = base::Callback<
       void(bool /* is_local_thumbnail */, const SkBitmap* /* bitmap */)>;
@@ -86,29 +78,6 @@ class MostVisitedSites : public history::TopSitesObserver,
   // The source of the Most Visited sites.
   enum MostVisitedSource { TOP_SITES, SUGGESTIONS_SERVICE, POPULAR, WHITELIST };
 
-  struct Suggestion {
-    base::string16 title;
-    GURL url;
-    MostVisitedSource source;
-
-    // Only valid for source == WHITELIST (empty otherwise).
-    base::FilePath whitelist_icon_path;
-
-    // Only valid for source == SUGGESTIONS_SERVICE (-1 otherwise).
-    int provider_index;
-
-    Suggestion();
-    ~Suggestion();
-
-    // Get the Histogram name associated with the source.
-    std::string GetSourceHistogramName() const;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(Suggestion);
-  };
-
-  using SuggestionsVector = std::vector<std::unique_ptr<Suggestion>>;
-
   void QueryMostVisitedURLs();
 
   // Initialize the query to Top Sites. Called if the SuggestionsService is not
@@ -210,7 +179,7 @@ class MostVisitedSites : public history::TopSitesObserver,
   // The profile whose most visited sites will be queried.
   Profile* profile_;
 
-  MostVisitedSitesObserver* observer_;
+  Observer* observer_;
 
   // The maximum number of most visited sites to return.
   int num_sites_;
@@ -245,4 +214,36 @@ class MostVisitedSites : public history::TopSitesObserver,
   DISALLOW_COPY_AND_ASSIGN(MostVisitedSites);
 };
 
+// The observer to be notified when the list of most visited sites changes.
+class MostVisitedSites::Observer {
+ public:
+  virtual ~Observer() {}
+
+  virtual void OnMostVisitedURLsAvailable(
+      const SuggestionsVector& suggestions) = 0;
+  virtual void OnPopularURLsAvailable(const PopularSiteVector& sites) = 0;
+};
+
+class MostVisitedSites::Suggestion {
+ public:

[Marc Treib, 2016/04/25 13:58:38]

If you want to keep the public member variables, then this should remain a
struct.

[sfiera, 2016/04/25 16:26:08]

Done.

+  base::string16 title;
+  GURL url;
+  MostVisitedSource source;
+
+  // Only valid for source == WHITELIST (empty otherwise).
+  base::FilePath whitelist_icon_path;
+
+  // Only valid for source == SUGGESTIONS_SERVICE (-1 otherwise).
+  int provider_index;
+
+  Suggestion();
+  ~Suggestion();
+
+  // Get the Histogram name associated with the source.
+  std::string GetSourceHistogramName() const;

[Marc Treib, 2016/04/25 13:58:38]

Since Suggestion is now part of the public interface, I'd move this into
MostVisitedSites.

[sfiera, 2016/04/26 09:18:17]

I removed it from the header, since it no longer needs private access to the
class.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(Suggestion);
+};
+
 #endif  // CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_