[Suggestions] Use a callback for the FetchMore action

Modified SuggestionsSection#fetchSuggestions to take a recieving
callback as input rather than relying on notifying back the right
observer for the result.

This makes SuggestionsSection.Observer simpler, with only
notification-style methods, opening the possibility to register
multiple observers later.

Bug: 721407
Change-Id: I0e735be3eaca7bdc428abd2d009d6bbfb230cbc5
Reviewed-on: https://chromium-review.googlesource.com/563410
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485601}

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java

@@ -80,9 +80,7 @@ public class ActionItem extends OptionalLeaf {
                     // TODO(jkrcal): Implement in the backend instead. See https://crbug.com/728570
                     uiDelegate.getSuggestionsSource().fetchRemoteSuggestions();
                 } else {
-                    uiDelegate.getSuggestionsSource().fetchSuggestions(mCategoryInfo.getCategory(),
-                            mParentSection.getDisplayedSuggestionIds());
-                    mParentSection.onFetchStarted();
+                    mParentSection.fetchSuggestions();
                 }
                 return;
             case ContentSuggestionsAdditionalAction.NONE:

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ChildNode.java

@@ -32,6 +32,11 @@ public abstract class ChildNode implements TreeNode {
         mParent = null;
     }
 
+    /** @return Whether the node is attached to a parent node. */
+    protected boolean isAttached() {
+        return mParent != null;
+    }
+
     @Override
     public final int getItemCount() {
         assert mNumItems == getItemCountForDebugging();

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java

@@ -128,18 +128,7 @@ public class SectionList
 
         SuggestionsSection section = mSections.get(category);
         section.setStatus(status);
-        section.updateSuggestions(mUiDelegate.getSuggestionsSource());
-    }
-
-    @Override
-    public void onMoreSuggestions(@CategoryInt int category, List<SnippetArticle> suggestions) {
-        @CategoryStatus
-        int status = mUiDelegate.getSuggestionsSource().getCategoryStatus(category);
-        if (!canProcessSuggestions(category, status)) return;
-
-        SuggestionsSection section = mSections.get(category);
-        section.setStatus(status);
-        section.appendSuggestions(suggestions, /* userRequested = */ true);
+        section.updateSuggestions();
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java

@@ -40,6 +40,7 @@ public class SuggestionsSection extends InnerNode {
     private final Delegate mDelegate;
     private final SuggestionsCategoryInfo mCategoryInfo;
     private final OfflineModelObserver mOfflineModelObserver;
+    private final SuggestionsSource mSuggestionsSource;
 
     // Children
     private final SectionHeader mHeader;
@@ -87,9 +88,10 @@ public class SuggestionsSection extends InnerNode {
             SuggestionsCategoryInfo info) {
         mDelegate = delegate;
         mCategoryInfo = info;
+        mSuggestionsSource = uiDelegate.getSuggestionsSource();
 
         mHeader = new SectionHeader(info.getTitle());
-        mSuggestionsList = new SuggestionsList(uiDelegate, ranker, info);
+        mSuggestionsList = new SuggestionsList(mSuggestionsSource, ranker, info);
         mStatus = StatusItem.createNoSuggestionsItem(info);
         mMoreButton = new ActionItem(this, ranker);
         mProgressIndicator = new ProgressItem();
@@ -104,14 +106,13 @@ public class SuggestionsSection extends InnerNode {
     private static class SuggestionsList extends ChildNode implements Iterable<SnippetArticle> {
         private final List<SnippetArticle> mSuggestions = new ArrayList<>();
 
-        // TODO(crbug.com/677672): Replace by SuggestionSource when it handles destruction.
-        private final SuggestionsUiDelegate mUiDelegate;
+        private final SuggestionsSource mSuggestionsSource;
         private final SuggestionsRanker mSuggestionsRanker;
         private final SuggestionsCategoryInfo mCategoryInfo;
 
-        public SuggestionsList(SuggestionsUiDelegate uiDelegate, SuggestionsRanker ranker,
+        public SuggestionsList(SuggestionsSource suggestionsSource, SuggestionsRanker ranker,
                 SuggestionsCategoryInfo categoryInfo) {
-            mUiDelegate = uiDelegate;
+            mSuggestionsSource = suggestionsSource;
             mSuggestionsRanker = ranker;
             mCategoryInfo = categoryInfo;
         }
@@ -194,8 +195,7 @@ public class SuggestionsSection extends InnerNode {
         @Override
         public void dismissItem(int position, Callback<String> itemRemovedCallback) {
             checkIndex(position);
-            SuggestionsSource suggestionsSource = mUiDelegate.getSuggestionsSource();
-            if (suggestionsSource == null) {
+            if (!isAttached()) {
                 // It is possible for this method to be called after the NewTabPage has had
                 // destroy() called. This can happen when
                 // NewTabPageRecyclerView.dismissWithAnimation() is called and the animation ends
@@ -205,7 +205,7 @@ public class SuggestionsSection extends InnerNode {
             }
 
             SnippetArticle suggestion = remove(position);
-            suggestionsSource.dismissSuggestion(suggestion);
+            mSuggestionsSource.dismissSuggestion(suggestion);
             itemRemovedCallback.onResult(suggestion.mTitle);
         }
 
@@ -355,10 +355,8 @@ public class SuggestionsSection extends InnerNode {
      * effect if changing the list of suggestions is not allowed (e.g. because the user has already
      * seen the suggestions). In that case, the section will be flagged as stale.
      * (see {@link #isDataStale()})
-     *
-     * @param suggestionsSource The source used to fetch the new suggestions.
      */
-    public void updateSuggestions(SuggestionsSource suggestionsSource) {
+    public void updateSuggestions() {
         if (mDelegate.isResetAllowed()) clearData();
         if (!canUpdateSuggestions()) {
             mIsDataStale = true;
@@ -368,7 +366,7 @@ public class SuggestionsSection extends InnerNode {
         }
 
         List<SnippetArticle> suggestions =
-                suggestionsSource.getSuggestionsForCategory(getCategory());
+                mSuggestionsSource.getSuggestionsForCategory(getCategory());
         Log.d(TAG, "Received %d new suggestions for category %d, had %d previously.",
                 suggestions.size(), getCategory(), mSuggestionsList.getItemCount());
 
@@ -455,8 +453,19 @@ public class SuggestionsSection extends InnerNode {
         return true;
     }
 
-    /** Lets the {@link SuggestionsSection} know when a suggestion fetch has been started. */
-    public void onFetchStarted() {
+    /** Fetches additional suggestions only for this section. */
+    public void fetchSuggestions() {
+        mSuggestionsSource.fetchSuggestions(mCategoryInfo.getCategory(),
+                getDisplayedSuggestionIds(), new Callback<List<SnippetArticle>>() {
+                    @Override
+                    public void onResult(List<SnippetArticle> additionalSuggestions) {
+                        if (!isAttached()) return; // The section has been dismissed.
+
+                        mProgressIndicator.setVisible(false);
+                        appendSuggestions(additionalSuggestions, /* userRequested = */ true);
+                    }
+                });
+
         mProgressIndicator.setVisible(true);
     }
 

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java

@@ -180,8 +180,9 @@ public class SnippetsBridge implements SuggestionsSource {
     }
 
     @Override
-    public void fetchSuggestions(@CategoryInt int category, String[] displayedSuggestionIds) {
-        nativeFetch(mNativeSnippetsBridge, category, displayedSuggestionIds);
+    public void fetchSuggestions(@CategoryInt int category, String[] displayedSuggestionIds,
+            Callback<List<SnippetArticle>> callback) {
+        nativeFetch(mNativeSnippetsBridge, category, displayedSuggestionIds, callback);
     }
 
     @CalledByNative
@@ -231,11 +232,6 @@ public class SnippetsBridge implements SuggestionsSource {
         if (mObserver != null) mObserver.onNewSuggestions(category);
     }
 
-    @CalledByNative
-    private void onMoreSuggestions(@CategoryInt int category, List<SnippetArticle> suggestions) {
-        if (mObserver != null) mObserver.onMoreSuggestions(category, suggestions);
-    }
-
     @CalledByNative
     private void onCategoryStatusChanged(@CategoryInt int category, @CategoryStatus int newStatus) {
         if (mObserver != null) mObserver.onCategoryStatusChanged(category, newStatus);
@@ -273,8 +269,8 @@ public class SnippetsBridge implements SuggestionsSource {
     private native void nativeFetchSuggestionFavicon(long nativeNTPSnippetsBridge, int category,
             String idWithinCategory, int minimumSizePx, int desiredSizePx,
             Callback<Bitmap> callback);
-    private native void nativeFetch(
-            long nativeNTPSnippetsBridge, int category, String[] knownSuggestions);
+    private native void nativeFetch(long nativeNTPSnippetsBridge, int category,
+            String[] knownSuggestions, Callback<List<SnippetArticle>> callback);
     private native void nativeFetchContextualSuggestions(
             long nativeNTPSnippetsBridge, String url, Callback<List<SnippetArticle>> callback);
     private native void nativeDismissSuggestion(long nativeNTPSnippetsBridge, String url,

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java

@@ -23,9 +23,6 @@ public interface SuggestionsSource extends DestructionObserver {
         /** Called when a category has a new list of content suggestions. */
         void onNewSuggestions(@CategoryInt int category);
 
-        /** Called when a request for additional suggestions completed. */
-        void onMoreSuggestions(@CategoryInt int category, List<SnippetArticle> suggestions);
-
         /** Called when a category changed its status. */
         void onCategoryStatusChanged(@CategoryInt int category, @CategoryStatus int newStatus);
 
@@ -93,8 +90,10 @@ public interface SuggestionsSource extends DestructionObserver {
      * Fetches new suggestions.
      * @param category the category to fetch new suggestions for.
      * @param displayedSuggestionIds ids of suggestions already known and that we want to keep.
+     * @param callback The callback to run with the received suggestions.
      */
-    void fetchSuggestions(@CategoryInt int category, String[] displayedSuggestionIds);
+    void fetchSuggestions(@CategoryInt int category, String[] displayedSuggestionIds,
+            Callback<List<SnippetArticle>> callback);
 
     /**
      * Fetches suggestions related to the provided URL.

chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java

@@ -173,8 +173,9 @@ public class SectionListTest {
         List<SnippetArticle> newSuggestions1 = createDummySuggestions(2, CATEGORY1, "new");
         List<SnippetArticle> newSuggestions2 = createDummySuggestions(2, CATEGORY2, "new");
 
-        sectionList.onMoreSuggestions(CATEGORY1, newSuggestions1.subList(0, 1));
-        sectionList.onMoreSuggestions(CATEGORY2, newSuggestions2);
+        sectionList.getSectionForTesting(CATEGORY1).appendSuggestions(
+                newSuggestions1.subList(0, 1), true);
+        sectionList.getSectionForTesting(CATEGORY2).appendSuggestions(newSuggestions2, true);
 
         bindViewHolders(sectionList, 3, sectionList.getItemCount());
 
@@ -207,7 +208,8 @@ public class SectionListTest {
         assertThat(newSuggestions2.get(1).getPerSectionRank(), equalTo(5));
 
         // Add one more suggestions1
-        sectionList.onMoreSuggestions(CATEGORY1, newSuggestions1.subList(1, 2));
+        sectionList.getSectionForTesting(CATEGORY1).appendSuggestions(
+                newSuggestions1.subList(1, 2), true);
         bindViewHolders(sectionList);
 
         // After the changes we should have:

chrome/browser/android/ntp/ntp_snippets_bridge.cc

@@ -323,7 +323,9 @@ void NTPSnippetsBridge::Fetch(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     jint j_category_id,
-    const JavaParamRef<jobjectArray>& j_displayed_suggestions) {
+    const JavaParamRef<jobjectArray>& j_displayed_suggestions,
+    const JavaParamRef<jobject>& j_callback) {
+  ScopedJavaGlobalRef<jobject> callback(j_callback);
   std::vector<std::string> known_suggestion_ids;
   AppendJavaStringArrayToStringVector(env, j_displayed_suggestions,
                                       &known_suggestion_ids);
@@ -334,7 +336,7 @@ void NTPSnippetsBridge::Fetch(
       std::set<std::string>(known_suggestion_ids.begin(),
                             known_suggestion_ids.end()),
       base::Bind(&NTPSnippetsBridge::OnSuggestionsFetched,
-                 weak_ptr_factory_.GetWeakPtr(), category));
+                 weak_ptr_factory_.GetWeakPtr(), callback, category));
 }
 
 void NTPSnippetsBridge::FetchContextualSuggestions(
@@ -456,13 +458,13 @@ void NTPSnippetsBridge::OnImageFetched(ScopedJavaGlobalRef<jobject> callback,
 }
 
 void NTPSnippetsBridge::OnSuggestionsFetched(
+    const ScopedJavaGlobalRef<jobject>& callback,
     Category category,
     ntp_snippets::Status status,
     std::vector<ContentSuggestion> suggestions) {
   // TODO(fhorschig, dgn): Allow refetch or show notification acc. to status.
   JNIEnv* env = AttachCurrentThread();
-  Java_SnippetsBridge_onMoreSuggestions(
-      env, bridge_, category.id(),
+  RunCallbackAndroid(callback,
                      ToJavaSuggestionList(env, category, suggestions));
 }
 

chrome/browser/android/ntp/ntp_snippets_bridge.h

@@ -73,7 +73,8 @@ class NTPSnippetsBridge
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       jint j_category_id,
-      const base::android::JavaParamRef<jobjectArray>& j_displayed_suggestions);
+      const base::android::JavaParamRef<jobjectArray>& j_displayed_suggestions,
+      const base::android::JavaParamRef<jobject>& j_callback);
 
   void FetchContextualSuggestions(
       JNIEnv* env,
@@ -119,6 +120,7 @@ class NTPSnippetsBridge
   void OnImageFetched(base::android::ScopedJavaGlobalRef<jobject> callback,
                       const gfx::Image& image);
   void OnSuggestionsFetched(
+      const base::android::ScopedJavaGlobalRef<jobject>& callback,
       ntp_snippets::Category category,
       ntp_snippets::Status status,
       std::vector<ntp_snippets::ContentSuggestion> suggestions);

chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.test.util.browser.suggestions;
 
+import static org.chromium.chrome.test.util.browser.suggestions.ContentSuggestionsTestUtils.createDummySuggestions;
+
 import android.graphics.Bitmap;
 
 import org.chromium.base.Callback;
@@ -66,6 +68,31 @@ public class FakeSuggestionsSource implements SuggestionsSource {
         if (mObserver != null) mObserver.onNewSuggestions(category);
     }
 
+    /**
+     * Creates and sets the suggestions to be returned for a given category.
+     * @return The suggestions created.
+     * @see ContentSuggestionsTestUtils#createDummySuggestions(int, int)
+     * @see #setSuggestionsForCategory(int, List)
+     */
+    public List<SnippetArticle> createAndSetSuggestions(int count, @CategoryInt int category) {
+        List<SnippetArticle> suggestions = createDummySuggestions(count, category);
+        setSuggestionsForCategory(category, suggestions);
+        return suggestions;
+    }
+
+    /**
+     * Creates and sets the suggestions to be returned for a given category.
+     * @return The suggestions created.
+     * @see ContentSuggestionsTestUtils#createDummySuggestions(int, int, String)
+     * @see #setSuggestionsForCategory(int, List)
+     */
+    public List<SnippetArticle> createAndSetSuggestions(
+            int count, @CategoryInt int category, String suffix) {
+        List<SnippetArticle> suggestions = createDummySuggestions(count, category, suffix);
+        setSuggestionsForCategory(category, suggestions);
+        return suggestions;
+    }
+
     /**
      * Sets the metadata to be returned for a given category.
      */
@@ -193,9 +220,8 @@ public class FakeSuggestionsSource implements SuggestionsSource {
     }
 
     @Override
-    public void fetchSuggestions(@CategoryInt int category, String[] displayedSuggestionIds) {
-        throw new UnsupportedOperationException();
-    }
+    public void fetchSuggestions(@CategoryInt int category, String[] displayedSuggestionIds,
+            Callback<List<SnippetArticle>> callback) {}
 
     @Override
     public void fetchContextualSuggestions(String url, Callback<List<SnippetArticle>> callback) {