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

chrome/browser/android/ntp/most_visited_sites.h

Index: chrome/browser/android/ntp/most_visited_sites.h
diff --git a/chrome/browser/android/ntp/most_visited_sites.h b/chrome/browser/android/ntp/most_visited_sites.h
index 548db8c3353925cec8f5d2b4a83e110ff2fd0608..1e486b749d0775f95a9f45234a0fc4840c636825 100644
--- a/chrome/browser/android/ntp/most_visited_sites.h
+++ b/chrome/browser/android/ntp/most_visited_sites.h
@@ -37,22 +37,41 @@ class PrefRegistrySyncable;
 class PopularSites;
 class Profile;
 
+// The observer to be notified when the list of most visited sites changes.
+class MostVisitedSitesObserver {
+ public:
+  virtual ~MostVisitedSitesObserver() {}
+
+  virtual void NotifyMostVisitedURLsObserver(
+      const std::vector<base::string16>& titles,
+      const std::vector<std::string>& urls,
+      const std::vector<std::string>& whitelist_icon_paths);
+  virtual void OnPopularSitesAvailable(
+      const std::vector<std::string>& urls,
+      const std::vector<std::string>& favicon_urls,
+      const std::vector<std::string>& large_icon_urls);

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

I think these should be pure virtual? (i.e. add " = 0" before the semicolon)
Or alternatively, add an empty implementation.

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

Pure virtual; I'd forgotten.

+};
+
 // Provides the list of most visited sites and their thumbnails to Java.
 class MostVisitedSites : public history::TopSitesObserver,
                          public SupervisedUserServiceObserver {
  public:
   explicit MostVisitedSites(Profile* profile);
   void Destroy(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+
+  // Java methods
+
   void SetMostVisitedURLsObserver(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       const base::android::JavaParamRef<jobject>& j_observer,
       jint num_sites);
-  void GetURLThumbnail(JNIEnv* env,
-                       const base::android::JavaParamRef<jobject>& obj,
-                       const base::android::JavaParamRef<jstring>& url,
-                       const base::android::JavaParamRef<jobject>& j_callback);
 
+  void GetURLThumbnail(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jstring>& url,
+      const base::android::JavaParamRef<jobject>& j_callback);
   void AddOrRemoveBlacklistedUrl(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
@@ -68,6 +87,18 @@ class MostVisitedSites : public history::TopSitesObserver,
       jint index,
       jint tile_type);
 
+  // C++ methods

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

All these can be private for now, right?

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

Could be. If I could choose, I'd rather make the Java methods private, since the
C++ functions describe the public interface and the Java methods are just glue.
But I have no idea how JNI works and if that's allowed.

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

AFAIK JNI doesn't care about public/private, so probably everything could be
private.
Anyway, I guess the next CL will split this class in two (the bridge part and
the logic part), so it doesn't really matter.

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

After digging into the generated code a little, I think it does care, so let's
just have overloads for a few days.

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

Acknowledged.

+
+  void SetMostVisitedURLsObserver(
+      std::unique_ptr<MostVisitedSitesObserver> observer, int num_sites);
+
+  using ThumbnailCallback = base::Callback<
+      void(bool /* is_local_thumbnail */, const SkBitmap* /* bitmap */)>;
+  void GetURLThumbnail(const GURL& url, const ThumbnailCallback& callback);

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

Hm.. generally, method overloading is kinda frowned upon in Chromium. But if
this is just a temporary state until the class is split in two, I guess that's
okay.

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

I double-checked the style guide and the verdict is basically "prefer not to;
only if you can tell at the call site what the difference is" and I think that's
the case here :)

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

Fair enough. And, again, it's a temporary state of affairs anyway.

+  void AddOrRemoveBlacklistedUrl(const GURL& url, bool add_url);
+  void RecordTileTypeMetrics(const std::vector<int>& tile_types);
+  void RecordOpenedMostVisitedItem(int index, int tile_type);
+
   // SupervisedUserServiceObserver implementation.
   void OnURLFilterChanged() override;
 
@@ -173,7 +204,7 @@ class MostVisitedSites : public history::TopSitesObserver,
       SuggestionsVector* src_suggestions,
       SuggestionsVector* dst_suggestions);
 
-  // Notifies the Java side observer about the availability of suggestions.
+  // Notifies the observer about the availability of suggestions.
   // Also records impressions UMA if not done already.
   void NotifyMostVisitedURLsObserver();
 
@@ -182,14 +213,14 @@ class MostVisitedSites : public history::TopSitesObserver,
   // Runs on the UI Thread.
   void OnLocalThumbnailFetched(
       const GURL& url,
-      std::unique_ptr<base::android::ScopedJavaGlobalRef<jobject>> j_callback,
+      const base::Callback<void(bool, const SkBitmap*)>& callback,

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

ThumbnailCallback?

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

Done.

       std::unique_ptr<SkBitmap> bitmap);
 
   // Callback for when the thumbnail lookup is complete.
   // Runs on the UI Thread.
   void OnObtainedThumbnail(
       bool is_local_thumbnail,
-      std::unique_ptr<base::android::ScopedJavaGlobalRef<jobject>> j_callback,
+      const base::Callback<void(bool, const SkBitmap*)>& callback,

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

Here too

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

Done. (and in the .cc file)

       const GURL& url,
       const SkBitmap* bitmap);
 
@@ -207,8 +238,7 @@ class MostVisitedSites : public history::TopSitesObserver,
   // The profile whose most visited sites will be queried.
   Profile* profile_;
 
-  // The observer to be notified when the list of most visited sites changes.
-  base::android::ScopedJavaGlobalRef<jobject> observer_;
+  std::unique_ptr<MostVisitedSitesObserver> observer_;

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

This is a very uncommon pattern - the observed object usually doesn't own the
observer.
But I guess this is also only temporary, while the class essentially observes
itself.

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

Again, I could do with some more knowledge about JNI. Do you know how the
observer object from Java is going to be owned in that case?

I could split this into two variables, an unowned one that will belong with C++
and one for the Java bridge.

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

Java objects always have shared ownership, AFAIK that remains true through JNI.
I guess when this class is split in two, something like that would happen
anyway? Then we can also leave it as-is for now.

I'd imagine the final state something like this:
A "MostVisitedBridge" object, which has the same lifetime and ownership as the
current MostVisitedSites, and which holds a Java ref to the Java observer
object. This class would implement your MostVisitedSitesObserver.

The logic would live in a new MostVisitedSites object, which is probably a
ProfileKeyedService, and which has an ObserverList. The MostVisitedBridge would
register/unregister itself as an (unowned) observer.

Does that match what you have in mind?

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

I think so, roughly, though I would put the Java ref in an inner class and have
the inner class implement MostVisitedSitesObserver.

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

Sure, SGTM.

 
   // The maximum number of most visited sites to return.
   int num_sites_;