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

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 144 matching lines @@
   }
   // The whole grid is already filled with personal suggestions, no point in
   // bothering with popular ones.
   return false;
 }
 
 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
 
 MostVisitedSites::Suggestion::Suggestion() : provider_index(-1) {}
 
 MostVisitedSites::Suggestion::~Suggestion() {}
 
 std::string MostVisitedSites::Suggestion::GetSourceHistogramName() const {
   switch (source) {
     case MostVisitedSites::TOP_SITES:
       return kHistogramClientName;
     case MostVisitedSites::POPULAR:
       return kHistogramPopularName;
     case MostVisitedSites::WHITELIST:
       return kHistogramWhitelistName;
     case MostVisitedSites::SUGGESTIONS_SERVICE:
       return provider_index >= 0
                  ? base::StringPrintf(kHistogramServerFormat, provider_index)
                  : kHistogramServerName;
   }
   NOTREACHED();
   return std::string();
 }
 
 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_,
                               new suggestions::SuggestionsSource(profile_));
   content::URLDataSource::Add(profile_, new ThumbnailListSource(profile_));
 
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   supervised_user_service->AddObserver(this);
 }
 
 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;
-}
-
 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() &&
       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(
-    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) {
   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,
@@ skipping 38 matching lines @@
 
 void MostVisitedSites::OnObtainedThumbnail(
     bool is_local_thumbnail,
     const ThumbnailCallback& 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) {
+  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];
     ++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());
   LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
 }
 
 void MostVisitedSites::OnURLFilterChanged() {
   QueryMostVisitedURLs();
 }
 
 // static
-bool MostVisitedSites::Register(JNIEnv* env) {
-  return RegisterNativesImpl(env);
-}
-
-// static
 void MostVisitedSites::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(prefs::kNTPSuggestionsURL);
   registry->RegisterListPref(prefs::kNTPSuggestionsIsPersonal);
 }
 
 void MostVisitedSites::QueryMostVisitedURLs() {
   SuggestionsService* suggestions_service =
       SuggestionsServiceFactory::GetForProfile(profile_);
   if (suggestions_service->FetchSuggestionsData()) {
@@ skipping 433 matching lines @@
 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
+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);
 }