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

chrome/browser/android/ntp/most_visited_sites.cc

 // 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.
 
 #include "chrome/browser/android/ntp/most_visited_sites.h"
 
 #include <utility>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
@@ skipping 192 matching lines @@
 MostVisitedSites::~MostVisitedSites() {
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   supervised_user_service->RemoveObserver(this);
 }
 
 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() &&
       NeedPopularSites(profile_->GetPrefs(), num_sites_)) {
     popular_sites_.reset(new PopularSites(
         profile_,
         GetPopularSitesCountry(),
         GetPopularSitesVersion(),
         false,
         base::Bind(&MostVisitedSites::OnPopularSitesAvailable,
@@ skipping 20 matching lines @@
                  base::Unretained(this)));
 
   // Immediately get the current suggestions from the cache. If the cache is
   // empty, this will fall back to TopSites.
   OnSuggestionsProfileAvailable(
       suggestions_service->GetSuggestionsDataFromCache());
   // Also start a request for fresh suggestions.
   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);
+}
 
-  GURL url(ConvertJavaStringToUTF8(env, j_url));
+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()) {
     // A thumbnail is not locally available for |url|. Make sure it is put in
     // the list to be fetched at the next visit to this site.
     scoped_refptr<TopSites> top_sites(TopSitesFactory::GetForProfile(profile_));
     if (top_sites)
       top_sites->AddForcedURL(url, base::Time::Now());
     // Also fetch a remote thumbnail if possible. PopularSites or the
     // SuggestionsService can supply a thumbnail download URL.
     SuggestionsService* suggestions_service =
         SuggestionsServiceFactory::GetForProfile(profile_);
     if (popular_sites_) {
       const std::vector<PopularSites::Site>& sites = popular_sites_->sites();
       auto it = std::find_if(
           sites.begin(), sites.end(),
           [&url](const PopularSites::Site& site) { return site.url == url; });
       if (it != sites.end() && it->thumbnail_url.is_valid()) {
         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)
     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.
   scoped_refptr<TopSites> top_sites = TopSitesFactory::GetForProfile(profile_);
   if (top_sites) {
     if (add_url)
       top_sites->AddBlacklistedURL(url);
     else
       top_sites->RemoveBlacklistedURL(url);
   }
 
   // Only blacklist in the server-side suggestions service if it's active.
   if (mv_source_ == SUGGESTIONS_SERVICE) {
     SuggestionsService* suggestions_service =
         SuggestionsServiceFactory::GetForProfile(profile_);
     if (add_url)
       suggestions_service->BlacklistURL(url);
     else
       suggestions_service->UndoBlacklistURL(url);
   }
 }
 
 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) {
   int counts_per_type[NUM_TILE_TYPES] = {0};
   for (size_t i = 0; i < tile_types.size(); ++i) {
     int tile_type = tile_types[i];
     ++counts_per_type[tile_type];
     std::string histogram = base::StringPrintf(
         "NewTabPage.TileType.%s",
         current_suggestions_[i]->GetSourceHistogramName().c_str());
     LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
   }
 
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.IconsReal",
                               counts_per_type[ICON_REAL]);
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.IconsColor",
                               counts_per_type[ICON_COLOR]);
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.IconsGray",
                               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()));
   std::string histogram = base::StringPrintf(
       "NewTabPage.MostVisited.%s",
       current_suggestions_[index]->GetSourceHistogramName().c_str());
   LogHistogramEvent(histogram, index, num_sites_);
 
   histogram = base::StringPrintf(
       "NewTabPage.TileTypeClicked.%s",
       current_suggestions_[index]->GetSourceHistogramName().c_str());
@@ skipping 397 matching lines @@
 
 void MostVisitedSites::NotifyMostVisitedURLsObserver() {
   size_t num_suggestions = current_suggestions_.size();
   if (received_most_visited_sites_ && received_popular_sites_ &&
       !recorded_uma_) {
     RecordImpressionUMAMetrics();
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.NumberOfTiles", num_suggestions);
     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;
   std::vector<std::string> urls;
   std::vector<std::string> whitelist_icon_paths;
   titles.reserve(num_suggestions);
   urls.reserve(num_suggestions);
   for (const auto& suggestion : current_suggestions_) {
     titles.push_back(suggestion->title);
     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) {
   received_popular_sites_ = true;
 
   if (!success) {
     LOG(WARNING) << "Download of popular sites failed";
     return;
   }
 
-  if (observer_.is_null())
+  if (!observer_.get())
     return;
 
   std::vector<std::string> urls;
   std::vector<std::string> favicon_urls;
   std::vector<std::string> large_icon_urls;
   for (const PopularSites::Site& popular_site : popular_sites_->sites()) {
     urls.push_back(popular_site.url.spec());
     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();
 }
 
 void MostVisitedSites::RecordImpressionUMAMetrics() {
   for (size_t i = 0; i < current_suggestions_.size(); i++) {
     std::string histogram = base::StringPrintf(
         "NewTabPage.SuggestionsImpression.%s",
         current_suggestions_[i]->GetSourceHistogramName().c_str());
     LogHistogramEvent(histogram, static_cast<int>(i), num_sites_);
   }
 }
 
 void MostVisitedSites::TopSitesLoaded(TopSites* top_sites) {}
 
 void MostVisitedSites::TopSitesChanged(TopSites* top_sites,
                                        ChangeReason change_reason) {
   if (mv_source_ == TOP_SITES) {
     // The displayed suggestions are invalidated.
     InitiateTopSitesQuery();
   }
 }
 
 static jlong Init(JNIEnv* env,
                   const JavaParamRef<jobject>& obj,
                   const JavaParamRef<jobject>& jprofile) {
   MostVisitedSites* most_visited_sites =
       new MostVisitedSites(ProfileAndroid::FromProfileAndroid(jprofile));
   return reinterpret_cast<intptr_t>(most_visited_sites);
 }