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

chrome/browser/android/ntp/most_visited_sites.h

 // 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.
 
 #ifndef CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_
 #define CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_
 
 #include <jni.h>
 #include <stddef.h>
 
@@ skipping 19 matching lines @@
 class SuggestionsService;
 }
 
 namespace user_prefs {
 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,
       const base::android::JavaParamRef<jstring>& j_url,
       jboolean add_url);
   void RecordTileTypeMetrics(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       const base::android::JavaParamRef<jintArray>& jtile_types);
   void RecordOpenedMostVisitedItem(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       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;
 
   // Registers JNI methods.
   static bool Register(JNIEnv* env);
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
  private:
   friend class MostVisitedSitesTest;
@@ skipping 85 matching lines @@
   // Inserts suggestions from |src_suggestions| at positions |insert_positions|
   // into |dst_suggestions| where ever empty starting from |start_position|.
   // Returns the last filled position so that future insertions can start from
   // there.
   static size_t InsertAllSuggestions(
       size_t start_position,
       const std::vector<size_t>& insert_positions,
       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();
 
   void OnPopularSitesAvailable(bool success);
 
   // 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);
 
   // Records thumbnail-related UMA histogram metrics.
   void RecordThumbnailUMAMetrics();
 
   // Records UMA histogram metrics related to the number of impressions.
   void RecordImpressionUMAMetrics();
 
   // history::TopSitesObserver implementation.
   void TopSitesLoaded(history::TopSites* top_sites) override;
   void TopSitesChanged(history::TopSites* top_sites,
                        ChangeReason change_reason) override;
 
   // 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_;
 
   // Whether we have received an initial set of most visited sites (from either
   // TopSites or the SuggestionsService).
   bool received_most_visited_sites_;
 
   // Whether we have received the set of popular sites. Immediately set to true
   // if popular sites are disabled.
@@ skipping 15 matching lines @@
 
   SuggestionsVector current_suggestions_;
 
   // For callbacks may be run after destruction.
   base::WeakPtrFactory<MostVisitedSites> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MostVisitedSites);
 };
 
 #endif  // CHROME_BROWSER_ANDROID_NTP_MOST_VISITED_SITES_H_