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

chrome/browser/android/ntp/most_visited_sites.cc

Index: chrome/browser/android/ntp/most_visited_sites.cc
diff --git a/chrome/browser/android/ntp/most_visited_sites.cc b/chrome/browser/android/ntp/most_visited_sites.cc
index 6a42bd4f66b33761d75502b1591dd0dc419505b2..e2b1a64c3151de337602039412344b9ef31c3367 100644
--- a/chrome/browser/android/ntp/most_visited_sites.cc
+++ b/chrome/browser/android/ntp/most_visited_sites.cc
@@ -162,39 +162,17 @@ bool AreURLsEquivalent(const GURL& url1, const GURL& url2) {
   return url1.host() == url2.host() && url1.path() == url2.path();
 }
 
-class JavaObserverBridge : public MostVisitedSitesObserver {
- public:
-  JavaObserverBridge(JNIEnv* env, const JavaParamRef<jobject>& obj)
-      : observer_(env, obj) {}
-
-  void OnMostVisitedURLsAvailable(
-      const std::vector<base::string16>& titles,
-      const std::vector<std::string>& urls,
-      const std::vector<std::string>& whitelist_icon_paths) override {
-    JNIEnv* env = AttachCurrentThread();
-    DCHECK_EQ(titles.size(), urls.size());
-    Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
-        env, observer_.obj(), ToJavaArrayOfStrings(env, titles).obj(),
-        ToJavaArrayOfStrings(env, urls).obj(),
-        ToJavaArrayOfStrings(env, whitelist_icon_paths).obj());
-  }
-
-  void OnPopularURLsAvailable(
-      const std::vector<std::string>& urls,
-      const std::vector<std::string>& favicon_urls,
-      const std::vector<std::string>& large_icon_urls) override {
-    JNIEnv* env = AttachCurrentThread();
-    Java_MostVisitedURLsObserver_onPopularURLsAvailable(
-        env, observer_.obj(), ToJavaArrayOfStrings(env, urls).obj(),
-        ToJavaArrayOfStrings(env, favicon_urls).obj(),
-        ToJavaArrayOfStrings(env, large_icon_urls).obj());
-  }
-
- private:
-  ScopedJavaGlobalRef<jobject> observer_;
-
-  DISALLOW_COPY_AND_ASSIGN(JavaObserverBridge);
-};
+void CallJavaWithBitmap(
+    std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback,
+    bool is_local_thumbnail,
+    const SkBitmap* bitmap) {
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> j_bitmap;
+  if (bitmap)
+    j_bitmap = gfx::ConvertToJavaBitmap(bitmap);
+  Java_ThumbnailCallback_onMostVisitedURLsThumbnailAvailable(
+      env, j_callback->obj(), j_bitmap.obj(), is_local_thumbnail);
+}
 
 }  // namespace
 
@@ -220,9 +198,9 @@ std::string MostVisitedSites::Suggestion::GetSourceHistogramName() const {
 }
 
 MostVisitedSites::MostVisitedSites(Profile* profile)
-    : profile_(profile), num_sites_(0), received_most_visited_sites_(false),
-      received_popular_sites_(false), recorded_uma_(false),
-      scoped_observer_(this), weak_ptr_factory_(this) {
+    : profile_(profile), observer_(nullptr), num_sites_(0),
+      received_most_visited_sites_(false), received_popular_sites_(false),
+      recorded_uma_(false), scoped_observer_(this), weak_ptr_factory_(this) {
   // Register the debugging page for the Suggestions Service and the thumbnails
   // debugging page.
   content::URLDataSource::Add(profile_,
@@ -240,24 +218,9 @@ MostVisitedSites::~MostVisitedSites() {
   supervised_user_service->RemoveObserver(this);
 }
 
-void MostVisitedSites::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
-  delete this;
-}
-
 void MostVisitedSites::SetMostVisitedURLsObserver(
-    JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jobject>& j_observer,
-    jint num_sites) {
-  SetMostVisitedURLsObserver(
-      std::unique_ptr<MostVisitedSitesObserver>(
-          new JavaObserverBridge(env, j_observer)),
-      num_sites);
-}
-
-void MostVisitedSites::SetMostVisitedURLsObserver(
-      std::unique_ptr<MostVisitedSitesObserver> observer, int num_sites) {
-  observer_ = std::move(observer);
+      MostVisitedSitesObserver* observer, int num_sites) {
+  observer_ = observer;

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

DCHECK that observer_ wasn't set before?
(Eventually, we'll probably want to support multiple observers, but that will
require some more changes.)

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

Done.

I looked at the standard observer stuff in base/ and it seems like the "int
num_sites" is going to be a bit of an annoyance. Maybe in the future it will be
enough to make it a constructor parameter?

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

Yup, the "num_pages" is weird, and we should get rid of it at some point. Maybe
just always setting it to some default maximum value will be good enough.

   num_sites_ = num_sites;
 
   if (ShouldShowPopularSites() &&
@@ -298,23 +261,6 @@ void MostVisitedSites::SetMostVisitedURLsObserver(
   suggestions_service->FetchSuggestionsData();
 }
 
-static void CallJavaWithBitmap(
-    std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback,
-    bool is_local_thumbnail,
-    const SkBitmap* bitmap);
-
-void MostVisitedSites::GetURLThumbnail(
-    JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jstring>& j_url,
-    const JavaParamRef<jobject>& j_callback_obj) {
-  std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback(
-      new ScopedJavaGlobalRef<jobject>(env, j_callback_obj));
-  auto callback = base::Bind(&CallJavaWithBitmap, base::Passed(&j_callback));
-  GURL url(ConvertJavaStringToUTF8(env, j_url));
-  GetURLThumbnail(url, callback);
-}
-
 void MostVisitedSites::GetURLThumbnail(
     const GURL& url,
     const ThumbnailCallback& callback) {
@@ -373,27 +319,6 @@ void MostVisitedSites::OnObtainedThumbnail(
   callback.Run(is_local_thumbnail, bitmap);
 }
 
-static void CallJavaWithBitmap(
-    std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback,
-    bool is_local_thumbnail,
-    const SkBitmap* bitmap) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> j_bitmap;
-  if (bitmap)
-    j_bitmap = gfx::ConvertToJavaBitmap(bitmap);
-  Java_ThumbnailCallback_onMostVisitedURLsThumbnailAvailable(
-      env, j_callback->obj(), j_bitmap.obj(), is_local_thumbnail);
-}
-
-void MostVisitedSites::AddOrRemoveBlacklistedUrl(
-    JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jstring>& j_url,
-    jboolean add_url) {
-  GURL url(ConvertJavaStringToUTF8(env, j_url));
-  AddOrRemoveBlacklistedUrl(url, add_url);
-}
-
 void MostVisitedSites::AddOrRemoveBlacklistedUrl(
     const GURL& url, bool add_url) {
   // Always blacklist in the local TopSites.
@@ -417,17 +342,8 @@ void MostVisitedSites::AddOrRemoveBlacklistedUrl(
 }
 
 void MostVisitedSites::RecordTileTypeMetrics(
-    JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jintArray>& jtile_types) {
-  std::vector<int> tile_types;
-  base::android::JavaIntArrayToIntVector(env, jtile_types, &tile_types);
-  DCHECK_EQ(current_suggestions_.size(), tile_types.size());
-  RecordTileTypeMetrics(tile_types);
-}
-
-void MostVisitedSites::RecordTileTypeMetrics(
     const std::vector<int>& tile_types) {
+  DCHECK_EQ(current_suggestions_.size(), tile_types.size());
   int counts_per_type[NUM_TILE_TYPES] = {0};
   for (size_t i = 0; i < tile_types.size(); ++i) {
     int tile_type = tile_types[i];
@@ -446,14 +362,6 @@ void MostVisitedSites::RecordTileTypeMetrics(
                               counts_per_type[ICON_DEFAULT]);
 }
 
-void MostVisitedSites::RecordOpenedMostVisitedItem(
-    JNIEnv* env,
-    const JavaParamRef<jobject>& obj,
-    jint index,
-    jint tile_type) {
-  RecordOpenedMostVisitedItem(index, tile_type);
-}
-
 void MostVisitedSites::RecordOpenedMostVisitedItem(int index, int tile_type) {
   DCHECK_GE(index, 0);
   DCHECK_LT(index, static_cast<int>(current_suggestions_.size()));
@@ -473,11 +381,6 @@ void MostVisitedSites::OnURLFilterChanged() {
 }
 
 // static
-bool MostVisitedSites::Register(JNIEnv* env) {
-  return RegisterNativesImpl(env);
-}
-
-// static
 void MostVisitedSites::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(prefs::kNTPSuggestionsURL);
@@ -931,10 +834,100 @@ void MostVisitedSites::TopSitesChanged(TopSites* top_sites,
   }
 }
 
+// static
+bool MostVisitedSitesJavaBridge::Register(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+MostVisitedSitesJavaBridge::MostVisitedSitesJavaBridge(Profile* profile)
+    : mv_(profile) { }
+
+MostVisitedSitesJavaBridge::~MostVisitedSitesJavaBridge() { }
+
+void MostVisitedSitesJavaBridge::Destroy(
+    JNIEnv* env, const JavaParamRef<jobject>& obj) {
+  delete this;
+}
+
+void MostVisitedSitesJavaBridge::SetMostVisitedURLsObserver(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& j_observer,
+    jint num_sites) {
+  observer_.reset(new Observer(env, j_observer));
+  mv_.SetMostVisitedURLsObserver(observer_.get(), num_sites);
+}
+
+void MostVisitedSitesJavaBridge::GetURLThumbnail(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jstring>& j_url,
+    const JavaParamRef<jobject>& j_callback_obj) {
+  std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback(
+      new ScopedJavaGlobalRef<jobject>(env, j_callback_obj));
+  auto callback = base::Bind(&CallJavaWithBitmap, base::Passed(&j_callback));
+  GURL url(ConvertJavaStringToUTF8(env, j_url));

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

nit: I'd inline both of these below.

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

That pushes the line over 80 chars. If it's going to be three lines either way,
I find this way clearer. Opinions?

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

I tend to prefer "one" (broken) line - I guess I've gotten used to broken lines.
But it's a matter of preference, so it's also fine to leave as is.

+  mv_.GetURLThumbnail(url, callback);
+}
+
+void MostVisitedSitesJavaBridge::AddOrRemoveBlacklistedUrl(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jstring>& j_url,
+    jboolean add_url) {
+  GURL url(ConvertJavaStringToUTF8(env, j_url));

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

Also here

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

Also here

+  mv_.AddOrRemoveBlacklistedUrl(url, add_url);
+}
+
+void MostVisitedSitesJavaBridge::RecordTileTypeMetrics(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jintArray>& jtile_types) {
+  std::vector<int> tile_types;
+  base::android::JavaIntArrayToIntVector(env, jtile_types, &tile_types);
+  mv_.RecordTileTypeMetrics(tile_types);
+}
+
+void MostVisitedSitesJavaBridge::RecordOpenedMostVisitedItem(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    jint index,
+    jint tile_type) {
+  mv_.RecordOpenedMostVisitedItem(index, tile_type);
+}
+
+MostVisitedSitesJavaBridge::Observer::Observer(
+    JNIEnv* env, const JavaParamRef<jobject>& obj)
+    : observer_(env, obj) {}
+
+void MostVisitedSitesJavaBridge::Observer::OnMostVisitedURLsAvailable(
+    const std::vector<base::string16>& titles,
+    const std::vector<std::string>& urls,
+    const std::vector<std::string>& whitelist_icon_paths) {
+  JNIEnv* env = AttachCurrentThread();
+  DCHECK_EQ(titles.size(), urls.size());

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

Not your doing, but it would be good to add another DCHECK for
whitelist_icon_paths.size()

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

Sure. (I'm also going to count this as a vote for changing the observer method
to take a single vector<Suggestion> in a later CL)

+  Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
+      env, observer_.obj(), ToJavaArrayOfStrings(env, titles).obj(),
+      ToJavaArrayOfStrings(env, urls).obj(),
+      ToJavaArrayOfStrings(env, whitelist_icon_paths).obj());
+}
+
+void MostVisitedSitesJavaBridge::Observer::OnPopularURLsAvailable(
+    const std::vector<std::string>& urls,
+    const std::vector<std::string>& favicon_urls,
+    const std::vector<std::string>& large_icon_urls) {
+  JNIEnv* env = AttachCurrentThread();
+  Java_MostVisitedURLsObserver_onPopularURLsAvailable(
+      env, observer_.obj(), ToJavaArrayOfStrings(env, urls).obj(),
+      ToJavaArrayOfStrings(env, favicon_urls).obj(),
+      ToJavaArrayOfStrings(env, large_icon_urls).obj());
+}
+
 static jlong Init(JNIEnv* env,
                   const JavaParamRef<jobject>& obj,
                   const JavaParamRef<jobject>& jprofile) {
-  MostVisitedSites* most_visited_sites =
-      new MostVisitedSites(ProfileAndroid::FromProfileAndroid(jprofile));
+  MostVisitedSitesJavaBridge* most_visited_sites =
+      new MostVisitedSitesJavaBridge(
+          ProfileAndroid::FromProfileAndroid(jprofile));
   return reinterpret_cast<intptr_t>(most_visited_sites);
 }