ntp_tiles: Migrate to multi-observer model

chrome/browser/android/ntp/most_visited_sites_bridge.cc

Index: chrome/browser/android/ntp/most_visited_sites_bridge.cc
diff --git a/chrome/browser/android/ntp/most_visited_sites_bridge.cc b/chrome/browser/android/ntp/most_visited_sites_bridge.cc
index 4f3538763c856e0fd6bf336958b1dab6d40b8dcd..386219084e346548f0747a99213e2ac2ef14160b 100644
--- a/chrome/browser/android/ntp/most_visited_sites_bridge.cc
+++ b/chrome/browser/android/ntp/most_visited_sites_bridge.cc
@@ -37,7 +37,7 @@ 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;
 
@@ -45,14 +45,16 @@ class MostVisitedSitesBridge::JavaObserver : public MostVisitedSites::Observer {
 
  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) {
@@ -67,6 +69,8 @@ void MostVisitedSitesBridge::JavaObserver::OnMostVisitedURLsAvailable(
   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());
@@ -81,6 +85,8 @@ void MostVisitedSitesBridge::JavaObserver::OnMostVisitedURLsAvailable(
 
 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()));
@@ -94,7 +100,10 @@ MostVisitedSitesBridge::MostVisitedSitesBridge(Profile* 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) {
@@ -106,8 +115,12 @@ void MostVisitedSitesBridge::SetMostVisitedURLsObserver(
     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(