ntp_tiles: Migrate to multi-observer model

chrome/browser/android/ntp/most_visited_sites_bridge.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_bridge.h"
 
 #include <utility>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
@@ skipping 19 matching lines @@
 using base::android::ToJavaArrayOfStrings;
 using base::android::ToJavaIntArray;
 using ntp_tiles::MostVisitedSites;
 using ntp_tiles::NTPTileSource;
 using ntp_tiles::NTPTilesVector;
 using ntp_tiles::metrics::MostVisitedTileType;
 using ntp_tiles::metrics::TileImpression;
 
 class MostVisitedSitesBridge::JavaObserver : public MostVisitedSites::Observer {
  public:
-  JavaObserver(JNIEnv* env, const JavaParamRef<jobject>& obj);
+  JavaObserver(JNIEnv* env, const JavaParamRef<jobject>& obj, int num_sites);
 
   void OnMostVisitedURLsAvailable(const NTPTilesVector& tiles) override;
 
   void OnIconMadeAvailable(const GURL& site_url) override;
 
  private:
   ScopedJavaGlobalRef<jobject> observer_;
+  const int num_sites_;
 
   DISALLOW_COPY_AND_ASSIGN(JavaObserver);
 };
 
 MostVisitedSitesBridge::JavaObserver::JavaObserver(
     JNIEnv* env,
-    const JavaParamRef<jobject>& obj)
-    : observer_(env, obj) {}
+    const JavaParamRef<jobject>& obj,
+    int num_sites)
+    : observer_(env, obj), num_sites_(num_sites) {}
 
 void MostVisitedSitesBridge::JavaObserver::OnMostVisitedURLsAvailable(
     const NTPTilesVector& tiles) {
   JNIEnv* env = AttachCurrentThread();
   std::vector<base::string16> titles;
   std::vector<std::string> urls;
   std::vector<std::string> whitelist_icon_paths;
   std::vector<int> sources;
 
   titles.reserve(tiles.size());
   urls.reserve(tiles.size());
   whitelist_icon_paths.reserve(tiles.size());
   sources.reserve(tiles.size());
   for (const auto& tile : tiles) {
+    if (static_cast<int>(titles.size()) >= num_sites_)
+      break;
     titles.emplace_back(tile.title);
     urls.emplace_back(tile.url.spec());
     whitelist_icon_paths.emplace_back(tile.whitelist_icon_path.value());
     sources.emplace_back(static_cast<int>(tile.source));
   }
   Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
       env, observer_, ToJavaArrayOfStrings(env, titles),
       ToJavaArrayOfStrings(env, urls),
       ToJavaArrayOfStrings(env, whitelist_icon_paths),
       ToJavaIntArray(env, sources));
 }
 
 void MostVisitedSitesBridge::JavaObserver::OnIconMadeAvailable(
     const GURL& site_url) {
+  // Note that this implementation doesn't honor num_sites_, so client code is
+  // expected to handle well the case where the URL doesn't match any known URL.
   JNIEnv* env = AttachCurrentThread();
   Java_MostVisitedURLsObserver_onIconMadeAvailable(
       env, observer_, ConvertUTF8ToJavaString(env, site_url.spec()));
 }
 
 MostVisitedSitesBridge::MostVisitedSitesBridge(Profile* profile)
     : most_visited_(ChromeMostVisitedSitesFactory::NewForProfile(profile)) {
   // Register the thumbnails debugging page.
   // TODO(sfiera): find thumbnails a home. They don't belong here.
   content::URLDataSource::Add(profile, new ThumbnailListSource(profile));
   DCHECK(!profile->IsOffTheRecord());
 }
 
-MostVisitedSitesBridge::~MostVisitedSitesBridge() {}
+MostVisitedSitesBridge::~MostVisitedSitesBridge() {
+  if (java_observer_)
+    most_visited_->RemoveObserver(java_observer_.get());
+}
 
 void MostVisitedSitesBridge::Destroy(
     JNIEnv* env, const JavaParamRef<jobject>& obj) {
   delete this;
 }
 
 void MostVisitedSitesBridge::SetMostVisitedURLsObserver(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jobject>& j_observer,
     jint num_sites) {
-  java_observer_.reset(new JavaObserver(env, j_observer));
-  most_visited_->SetMostVisitedURLsObserver(java_observer_.get(), num_sites);
+  if (java_observer_)
+    most_visited_->RemoveObserver(java_observer_.get());
+
+  java_observer_.reset(new JavaObserver(env, j_observer, num_sites));

[sfiera, 2017/01/18 09:58:33]

Can we change JavaObserver to call {Add,Remove}Observer() in its
constructor/destructor? This would match the behavior of the ios implementation
(and if we choose one behavior, that one seems better than this one).

I think I would keep the Refresh() external, though (as it is in both places).

+  most_visited_->AddObserver(java_observer_.get());
+  most_visited_->Refresh();
 }
 
 void MostVisitedSitesBridge::AddOrRemoveBlacklistedUrl(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& j_url,
     jboolean add_url) {
   GURL url(ConvertJavaStringToUTF8(env, j_url));
   most_visited_->AddOrRemoveBlacklistedUrl(url, add_url);
 }
@@ skipping 44 matching lines @@
 }
 
 static jlong Init(JNIEnv* env,
                   const JavaParamRef<jobject>& obj,
                   const JavaParamRef<jobject>& jprofile) {
   MostVisitedSitesBridge* most_visited_sites =
       new MostVisitedSitesBridge(
           ProfileAndroid::FromProfileAndroid(jprofile));
   return reinterpret_cast<intptr_t>(most_visited_sites);
 }