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

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 <jni.h>
 #include <stddef.h>
 
@@ skipping 34 matching lines @@
   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;
 };
 
-// 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 */)>;
   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;
 
-  // Registers JNI methods.
-  static bool Register(JNIEnv* env);
-
   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;
@@ skipping 11 matching lines @@
 
     // 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>>;
 
-  ~MostVisitedSites() override;
   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_;
 
-  std::unique_ptr<MostVisitedSitesObserver> observer_;
+  MostVisitedSitesObserver* 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);
 };
 
+// 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_