NTP tiles: split methods into C++ and Java parts.

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 25fb37c92abc7a1f3bf322cc7b36c477ddb626c9..593b3afb3c2be501c3596f90d3143582dfdb4c2f 100644
--- a/chrome/browser/android/ntp/most_visited_sites.cc
+++ b/chrome/browser/android/ntp/most_visited_sites.cc
@@ -210,12 +210,59 @@ void MostVisitedSites::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   delete this;
 }
 
+namespace {

[Marc Treib, 2016/04/14 16:57:36]

This should be merged into the anonymous namespace above.

[sfiera, 2016/04/15 13:35:59]

Done.

+
+class JavaObserverBridge : public MostVisitedSitesObserver {
+ public:
+  JavaObserverBridge(
+      JNIEnv* env, const base::android::JavaParamRef<jobject>& obj)

[Marc Treib, 2016/04/14 16:57:35]

nit: "base::android::" isn't required here.

[sfiera, 2016/04/15 13:35:59]

Done.

+      : observer_(env, obj) {}
+
+  void NotifyMostVisitedURLsObserver(

[Marc Treib, 2016/04/14 16:57:36]

I think this should be called "OnMostVisitedSitesAvailable" or something like
that?

[sfiera, 2016/04/15 13:35:59]

Done. (also below, OnPopularSitesAvailable => OnPopularURLsAvailable)

+      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 OnPopularSitesAvailable(
+      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:
+  base::android::ScopedJavaGlobalRef<jobject> observer_;

[Marc Treib, 2016/04/14 16:57:36]

Also here: no "base::android::" required.

[sfiera, 2016/04/15 13:35:59]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(JavaObserverBridge);
+};
+
+}  // namespace
+
 void MostVisitedSites::SetMostVisitedURLsObserver(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jobject>& j_observer,
     jint num_sites) {
-  observer_.Reset(env, j_observer);
+  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);
   num_sites_ = num_sites;
 
   if (ShouldShowPopularSites() &&
@@ -256,30 +303,40 @@ void MostVisitedSites::SetMostVisitedURLsObserver(
   suggestions_service->FetchSuggestionsData();
 }
 
+static void CallJavaWithBitmap(

[Marc Treib, 2016/04/14 16:57:36]

This should go into the anonymous namespace above.

[sfiera, 2016/04/15 13:35:59]

Any reason? It's static already.

[Marc Treib, 2016/04/15 15:21:41]

Ah, the code style indeed allows either. But putting it in a namespace is a lot
more common in Chromium; I don't think I've ever seen a static function.
Putting it in the namespace above will also remove the need to forward-declare
it ;)

[sfiera, 2016/04/15 16:09:06]

There's actually no need to forward-declare it either; I could have defined it
up where I declared it. I was keeping the code in the original order for a clear
diff.

[Marc Treib, 2016/04/15 16:30:03]

Acknowledged. Please do eventually move it into the anonymous namespace though.
(Next CL is fine, if you want to keep this diff small.)

+    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) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback(
       new ScopedJavaGlobalRef<jobject>());
   j_callback->Reset(env, j_callback_obj);

[Marc Treib, 2016/04/14 16:57:36]

Not your doing, but do you see any reason why we construct a ScopedJavaGlobalRef
and then immediately reset it?!

[sfiera, 2016/04/15 13:35:59]

That's not exactly what's happening. We're calling
j_callback->[ScopedJavaGlobalRef::]Reset(), not
j_callback.[unique_ptr::]reset().

I still don't know why. Maybe you can't base::Passed() a ScopedJavaGlobalRef<>?

[Marc Treib, 2016/04/15 15:21:41]

Ah, good point, I misread that.
ScopedJavaGlobalRef has a ctor that takes two args (and in fact just calls
Reset) though, so I'd just move the args into the ctor above and remove the
Reset line.
base::Passed explicitly transfers ownership, which doesn't make sense for a
ScopedJavaGlobalRef. It should be cheap to just copy it though? Anyway, I'd
leave that unchanged.

[sfiera, 2016/04/15 16:09:06]

Acknowledged.

-
+  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 base::Callback<void(bool, const SkBitmap*)>& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   scoped_refptr<TopSites> top_sites(TopSitesFactory::GetForProfile(profile_));
 
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::DB, FROM_HERE,
       base::Bind(&MaybeFetchLocalThumbnail, url, top_sites),
       base::Bind(&MostVisitedSites::OnLocalThumbnailFetched,
-                 weak_ptr_factory_.GetWeakPtr(), url,
-                 base::Passed(&j_callback)));
+                 weak_ptr_factory_.GetWeakPtr(), url, callback));
 }
 
 void MostVisitedSites::OnLocalThumbnailFetched(
     const GURL& url,
-    std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback,
+    const base::Callback<void(bool, const SkBitmap*)>& callback,
     std::unique_ptr<SkBitmap> bitmap) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!bitmap.get()) {
@@ -301,26 +358,31 @@ void MostVisitedSites::OnLocalThumbnailFetched(
         return suggestions_service->GetPageThumbnailWithURL(
             url, it->thumbnail_url,
             base::Bind(&MostVisitedSites::OnObtainedThumbnail,
-                       weak_ptr_factory_.GetWeakPtr(), false,
-                       base::Passed(&j_callback)));
+                       weak_ptr_factory_.GetWeakPtr(), false, callback));
       }
     }
     if (mv_source_ == SUGGESTIONS_SERVICE) {
       return suggestions_service->GetPageThumbnail(
           url, base::Bind(&MostVisitedSites::OnObtainedThumbnail,
-                          weak_ptr_factory_.GetWeakPtr(), false,
-                          base::Passed(&j_callback)));
+                          weak_ptr_factory_.GetWeakPtr(), false, callback));
     }
   }
-  OnObtainedThumbnail(true, std::move(j_callback), url, bitmap.get());
+  OnObtainedThumbnail(true, callback, url, bitmap.get());
 }
 
 void MostVisitedSites::OnObtainedThumbnail(
     bool is_local_thumbnail,
-    std::unique_ptr<ScopedJavaGlobalRef<jobject>> j_callback,
+    const base::Callback<void(bool, const SkBitmap*)>& callback,
     const GURL& url,
     const SkBitmap* bitmap) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  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)
@@ -335,7 +397,11 @@ void MostVisitedSites::AddOrRemoveBlacklistedUrl(
     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.
   scoped_refptr<TopSites> top_sites = TopSitesFactory::GetForProfile(profile_);
   if (top_sites) {
@@ -363,7 +429,11 @@ void MostVisitedSites::RecordTileTypeMetrics(
   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) {
   int counts_per_type[NUM_TILE_TYPES] = {0};
   for (size_t i = 0; i < tile_types.size(); ++i) {
     int tile_type = tile_types[i];
@@ -387,6 +457,10 @@ void MostVisitedSites::RecordOpenedMostVisitedItem(
     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()));
   std::string histogram = base::StringPrintf(
@@ -804,7 +878,7 @@ void MostVisitedSites::NotifyMostVisitedURLsObserver() {
     recorded_uma_ = true;
   }
 
-  if (observer_.is_null())
+  if (!observer_.get())

[Marc Treib, 2016/04/14 16:57:35]

nit: I think ".get()" isn't required.

[sfiera, 2016/04/15 13:36:00]

Correct you are.

     return;
 
   std::vector<base::string16> titles;
@@ -817,12 +891,8 @@ void MostVisitedSites::NotifyMostVisitedURLsObserver() {
     urls.push_back(suggestion->url.spec());
     whitelist_icon_paths.push_back(suggestion->whitelist_icon_path.value());
   }
-  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());
+
+  observer_->NotifyMostVisitedURLsObserver(titles, urls, whitelist_icon_paths);
 }
 
 void MostVisitedSites::OnPopularSitesAvailable(bool success) {
@@ -833,7 +903,7 @@ void MostVisitedSites::OnPopularSitesAvailable(bool success) {
     return;
   }
 
-  if (observer_.is_null())
+  if (!observer_.get())
     return;
 
   std::vector<std::string> urls;
@@ -844,11 +914,7 @@ void MostVisitedSites::OnPopularSitesAvailable(bool success) {
     favicon_urls.push_back(popular_site.favicon_url.spec());
     large_icon_urls.push_back(popular_site.large_icon_url.spec());
   }
-  JNIEnv* env = AttachCurrentThread();
-  Java_MostVisitedURLsObserver_onPopularURLsAvailable(
-      env, observer_.obj(), ToJavaArrayOfStrings(env, urls).obj(),
-      ToJavaArrayOfStrings(env, favicon_urls).obj(),
-      ToJavaArrayOfStrings(env, large_icon_urls).obj());
+  observer_->OnPopularSitesAvailable(urls, favicon_urls, large_icon_urls);
   QueryMostVisitedURLs();
 }