Update MostVisitedSites observer interface.

chrome/browser/android/ntp/most_visited_sites.h

 // Copyright 2013 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 CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_
 #define CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_
 
 #include <stddef.h>
 
 #include <memory>
@@ skipping 15 matching lines @@
 
 namespace suggestions {
 class SuggestionsService;
 }
 
 namespace user_prefs {
 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
 // while we are extracting the functionality of the Java class to make available
 // in C++.
 //
 // TODO(sfiera): finalize interface.
 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 */)>;
   void GetURLThumbnail(const GURL& url, const ThumbnailCallback& callback);
   void AddOrRemoveBlacklistedUrl(const GURL& url, bool add_url);
   void RecordTileTypeMetrics(const std::vector<int>& tile_types);
   void RecordOpenedMostVisitedItem(int index, int tile_type);
 
   // SupervisedUserServiceObserver implementation.
   void OnURLFilterChanged() override;
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
  private:
   friend class MostVisitedSitesTest;
 
   // 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
   // enabled, or if it returns no data.
   void InitiateTopSitesQuery();
 
   // If there's a whitelist entry point for the URL, return the large icon path.
   base::FilePath GetWhitelistLargeIconPath(const GURL& url);
 
   // Callback for when data is available from TopSites.
@@ skipping 81 matching lines @@
   void RecordImpressionUMAMetrics();
 
   // history::TopSitesObserver implementation.
   void TopSitesLoaded(history::TopSites* top_sites) override;
   void TopSitesChanged(history::TopSites* top_sites,
                        ChangeReason change_reason) override;
 
   // 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_;
 
   // Whether we have received an initial set of most visited sites (from either
   // TopSites or the SuggestionsService).
   bool received_most_visited_sites_;
 
   // Whether we have received the set of popular sites. Immediately set to true
   // if popular sites are disabled.
@@ skipping 14 matching lines @@
   std::unique_ptr<PopularSites> popular_sites_;
 
   SuggestionsVector current_suggestions_;
 
   // For callbacks may be run after destruction.
   base::WeakPtrFactory<MostVisitedSites> weak_ptr_factory_;
 
   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_