Refactor and extend SnippetsBridge for multi-section support

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
index 61285b3c01e8f6e5a16645bd3e795654449869e4..6a3e0c28def4ed0e6da82a3a7d532b54b4f02e00 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
@@ -19,15 +19,13 @@ import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
 import org.chromium.chrome.browser.ntp.UiConfig;
 import org.chromium.chrome.browser.ntp.snippets.CategoryStatus;
 import org.chromium.chrome.browser.ntp.snippets.CategoryStatus.CategoryStatusEnum;
-import org.chromium.chrome.browser.ntp.snippets.KnownCategories;
-import org.chromium.chrome.browser.ntp.snippets.KnownCategories.KnownCategoriesEnum;
+import org.chromium.chrome.browser.ntp.snippets.ContentSuggestionsSource;
+import org.chromium.chrome.browser.ntp.snippets.ContentSuggestionsSource.ContentSuggestionsSourceObserver;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticleListItem;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder;
 import org.chromium.chrome.browser.ntp.snippets.SnippetHeaderListItem;
 import org.chromium.chrome.browser.ntp.snippets.SnippetHeaderViewHolder;
 import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge;
-import org.chromium.chrome.browser.ntp.snippets.SnippetsSource;
-import org.chromium.chrome.browser.ntp.snippets.SnippetsSource.SnippetsObserver;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -41,12 +39,13 @@ import java.util.TreeMap;
  * the above-the-fold view (containing the logo, search box, and most visited tiles) and subsequent
  * elements will be the cards shown to the user
  */
-public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements SnippetsObserver {
+public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
+        implements ContentSuggestionsSourceObserver {
     private static final String TAG = "Ntp";
 
     private final NewTabPageManager mNewTabPageManager;
     private final NewTabPageLayout mNewTabPageLayout;
-    private SnippetsSource mSnippetsSource;
+    private ContentSuggestionsSource mSnippetsSource;

[Michael van Ouwerkerk, 2016/08/09 13:52:20]

Please also update the variable name to e.g. mSuggestionsSource

[Philipp Keck, 2016/08/09 16:15:07]

Done.

     private final UiConfig mUiConfig;
     private final ItemTouchCallbacks mItemTouchCallbacks = new ItemTouchCallbacks();
     private NewTabPageRecyclerView mRecyclerView;
@@ -120,17 +119,16 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
      * @param uiConfig the NTP UI configuration, to be passed to created views.
      */
     public NewTabPageAdapter(NewTabPageManager manager, NewTabPageLayout newTabPageLayout,
-            SnippetsSource snippetsSource, UiConfig uiConfig) {
+            ContentSuggestionsSource snippetsSource, UiConfig uiConfig) {

[Michael van Ouwerkerk, 2016/08/09 13:52:20]

Also update the param name here and in the docs above to suggestionsSource

[Philipp Keck, 2016/08/09 16:15:06]

Done.

         mNewTabPageManager = manager;
         mNewTabPageLayout = newTabPageLayout;
         mSnippetsSource = snippetsSource;
         mUiConfig = uiConfig;
 
-        // TODO(mvanouwerkerk): Do not hard code ARTICLES. Maybe do not initialize an empty
-        // section in the constructor.
-        setSuggestions(KnownCategories.ARTICLES,
-                Collections.<SnippetArticleListItem>emptyList(),
-                snippetsSource.getCategoryStatus(KnownCategories.ARTICLES));
+        for (int category : mSnippetsSource.getCategories()) {

[Michael van Ouwerkerk, 2016/08/09 13:52:20]

If there are no categories, this means the above the fold section is also not
shown, as setSuggestions is never called. You'll need to separate building up
the suggestion section items from the rest (mAboveTheFold and mBottomSpacer).

[Philipp Keck, 2016/08/09 16:15:06]

Moved code from setSuggestions to a new method "updateGroups()". This loop stays
as is, but the updateGroups() is always called afterwards to initialize the UI.
I tested what happens if there are no sections (see also
https://bugs.chromium.org/p/chromium/issues/detail?id=629817). And it doesn't
add the bottom spacer anymore if there are no sections. This will result in an
empty Zine. To test this, you can make SnippetsBridge#getCategories() return an
empty array. The NTP has no below-the-fold and is not scrollable on my tablet.

+            setSuggestions(category, snippetsSource.getSuggestionsForCategory(category),
+                    snippetsSource.getCategoryStatus(category));
+        }
         snippetsSource.setObserver(this);
     }
 
@@ -140,8 +138,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
     }
 
     @Override
-    public void onSuggestionsReceived(
-            @KnownCategoriesEnum int category, List<SnippetArticleListItem> suggestions) {
+    public void onNewSuggestions(int category) {
         // We never want to refresh the suggestions if we already have some content.
         if (mSections.containsKey(category) && mSections.get(category).hasSuggestions()) return;
 
@@ -152,6 +149,9 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
             return;
         }
 
+        List<SnippetArticleListItem> suggestions =
+                mSnippetsSource.getSuggestionsForCategory(category);
+
         Log.d(TAG, "Received %d new suggestions for category %d.", suggestions.size(), category);
 
         // At first, there might be no suggestions available, we wait until they have been fetched.
@@ -163,8 +163,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
     }
 
     @Override
-    public void onCategoryStatusChanged(
-            @KnownCategoriesEnum int category, @CategoryStatusEnum int status) {
+    public void onCategoryStatusChanged(int category, @CategoryStatusEnum int status) {
         // Observers should not be registered for this state.
         assert status != CategoryStatus.ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
 
@@ -257,14 +256,15 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
         SnippetsBridge.fetchSnippets(/*forceRequest=*/true);
     }
 
-    private void setSuggestions(@KnownCategoriesEnum int category,
-            List<SnippetArticleListItem> suggestions, @CategoryStatusEnum int status) {
+    private void setSuggestions(int category, List<SnippetArticleListItem> suggestions,
+            @CategoryStatusEnum int status) {
         mGroups.clear();
         mGroups.add(mAboveTheFold);
 
         if (!mSections.containsKey(category)) {
-            mSections.put(category,
-                    new SuggestionsSection(suggestions, status, this));
+            SuggestionsCategoryInfo info = mSnippetsSource.getCategoryInfo(category);
+            if (info == null) return;

[Bernhard Bauer, 2016/08/09 13:10:41]

Similar to https://codereview.chromium.org/2207493002/, do we actually expect
this to happen?

[Michael van Ouwerkerk, 2016/08/09 13:52:20]

Returning here is not appropriate, considering what the rest of the method does.

[Philipp Keck, 2016/08/09 14:02:53]

True. I removed the check. If it should ever be null, SuggestionsSection.java:30
will fail with an NPE that clearly points at the problem.

[Bernhard Bauer, 2016/08/09 14:30:11]

Exactly. Thanks!

[Philipp Keck, 2016/08/09 16:15:07]

Acknowledged.

+            mSections.put(category, new SuggestionsSection(suggestions, status, info, this));
         } else {
             mSections.get(category).setSuggestions(suggestions, status, this);
         }
@@ -297,7 +297,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
         int position = itemViewHolder.getAdapterPosition();
         SnippetArticleListItem suggestion = (SnippetArticleListItem) getItems().get(position);
 
-        mSnippetsSource.getSnippedVisited(suggestion, new Callback<Boolean>() {
+        mSnippetsSource.getSuggestionVisited(suggestion, new Callback<Boolean>() {
             @Override
             public void onResult(Boolean result) {
                 NewTabPageUma.recordSnippetAction(result
@@ -306,7 +306,7 @@ public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder> implements
             }
         });
 
-        mSnippetsSource.discardSnippet(suggestion);
+        mSnippetsSource.dismissSuggestion(suggestion);
         SuggestionsSection section = (SuggestionsSection) getGroup(position);
         section.dismissSuggestion(suggestion);