NTP tiles: Split C++ and Java MostVisitedSites classes.

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 f16b5ead7c6b89e5eb6e7ec773da10020d7a2004..3869299eea63857b89ad56ebec70759be52182c9 100644
--- a/chrome/browser/android/ntp/most_visited_sites.h
+++ b/chrome/browser/android/ntp/most_visited_sites.h
@@ -52,45 +52,16 @@ class MostVisitedSitesObserver {
       const std::vector<std::string>& large_icon_urls) = 0;
 };
 
-// Provides the list of most visited sites and their thumbnails to Java.
+// Tracks the list of most visited sites and their thumbnails.
 class MostVisitedSites : public history::TopSitesObserver,
                          public SupervisedUserServiceObserver {
  public:
   explicit MostVisitedSites(Profile* profile);
-  void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
 
-  // Java methods
+  ~MostVisitedSites() override;
 
   void SetMostVisitedURLsObserver(
-      JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& obj,
-      const base::android::JavaParamRef<jobject>& j_observer,
-      jint num_sites);
-
-  void GetURLThumbnail(
-      JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& obj,
-      const base::android::JavaParamRef<jstring>& url,
-      const base::android::JavaParamRef<jobject>& j_callback);
-  void AddOrRemoveBlacklistedUrl(
-      JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& obj,
-      const base::android::JavaParamRef<jstring>& j_url,
-      jboolean add_url);
-  void RecordTileTypeMetrics(
-      JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& obj,
-      const base::android::JavaParamRef<jintArray>& jtile_types);
-  void RecordOpenedMostVisitedItem(
-      JNIEnv* env,
-      const base::android::JavaParamRef<jobject>& obj,
-      jint index,
-      jint tile_type);
-
-  // C++ methods
-
-  void SetMostVisitedURLsObserver(
-      std::unique_ptr<MostVisitedSitesObserver> observer, int num_sites);
+      MostVisitedSitesObserver* observer, int num_sites);
 
   using ThumbnailCallback = base::Callback<
       void(bool /* is_local_thumbnail */, const SkBitmap* /* bitmap */)>;
@@ -102,9 +73,6 @@ class MostVisitedSites : public history::TopSitesObserver,
   // SupervisedUserServiceObserver implementation.
   void OnURLFilterChanged() override;
 
-  // Registers JNI methods.
-  static bool Register(JNIEnv* env);
-
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
  private:
@@ -136,7 +104,6 @@ class MostVisitedSites : public history::TopSitesObserver,
 
   using SuggestionsVector = std::vector<std::unique_ptr<Suggestion>>;
 
-  ~MostVisitedSites() override;
   void QueryMostVisitedURLs();
 
   // Initialize the query to Top Sites. Called if the SuggestionsService is not
@@ -238,7 +205,7 @@ class MostVisitedSites : public history::TopSitesObserver,
   // The profile whose most visited sites will be queried.
   Profile* profile_;
 
-  std::unique_ptr<MostVisitedSitesObserver> observer_;
+  MostVisitedSitesObserver* observer_;
 
   // The maximum number of most visited sites to return.
   int num_sites_;
@@ -273,4 +240,69 @@ class MostVisitedSites : public history::TopSitesObserver,
   DISALLOW_COPY_AND_ASSIGN(MostVisitedSites);
 };
 
+// Provides the list of most visited sites and their thumbnails to Java.
+class MostVisitedSitesJavaBridge {

[Marc Treib, 2016/04/19 08:14:47]

Also here: just "MostVisitedSitesBridge" will do. The "Java" is implicit in the
"Bridge".
I'd also move this into a separate file.

[sfiera, 2016/04/19 12:15:13]

Renamed.

What's the end goal? Do we want "android/.../most_visited_sites_bridge.h" and
"components/.../most_visited_sites.h"?

I'd been assuming "android/.../most_visited_sites.h" and
"components/.../most_visited_sites.h" (same name), but maybe having different
names is more Chromium style?

[Marc Treib, 2016/04/19 13:20:54]

Yes, that would be my preference.
Not sure if there's anything specific in the style guide about this, but I'd
avoid classes with the same name - IMO that's bound to lead to confusion. And at
least the "...Bridge" classes are common in Chromium.

[sfiera, 2016/04/19 16:53:43]

OK. If the bridge class is going to end up in a different file, now is the time
to move it.

+ public:
+  explicit MostVisitedSitesJavaBridge(Profile* profile);
+
+  void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+
+  void SetMostVisitedURLsObserver(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jobject>& j_observer,
+      jint num_sites);
+
+  void GetURLThumbnail(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jstring>& url,
+      const base::android::JavaParamRef<jobject>& j_callback);
+  void AddOrRemoveBlacklistedUrl(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jstring>& j_url,
+      jboolean add_url);
+  void RecordTileTypeMetrics(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jintArray>& jtile_types);
+  void RecordOpenedMostVisitedItem(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      jint index,
+      jint tile_type);
+
+  // Registers JNI methods.
+  static bool Register(JNIEnv* env);
+
+ private:
+  ~MostVisitedSitesJavaBridge();
+
+  class Observer : public MostVisitedSitesObserver {
+   public:
+    Observer(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+
+    void OnMostVisitedURLsAvailable(
+        const std::vector<base::string16>& titles,
+        const std::vector<std::string>& urls,
+        const std::vector<std::string>& whitelist_icon_paths) override;
+
+    void OnPopularURLsAvailable(
+        const std::vector<std::string>& urls,
+        const std::vector<std::string>& favicon_urls,
+        const std::vector<std::string>& large_icon_urls) override;
+
+   private:
+    base::android::ScopedJavaGlobalRef<jobject> observer_;
+
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
+  std::unique_ptr<Observer> observer_;
+
+  MostVisitedSites mv_;

[Marc Treib, 2016/04/19 08:14:47]

nit: most_visited_?

[sfiera, 2016/04/19 12:15:13]

OK.

+
+  DISALLOW_COPY_AND_ASSIGN(MostVisitedSitesJavaBridge);
+};
+
 #endif  // CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_