[Android NTP] Move suggestions in SuggestionSection into a separate TreeNode.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
index 9a4eaa2ab967de44516cbf5a09095fe4aad6c5a5..4158656bd39c8b7b877ffdb8c3befa6e7c9e24ca 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
@@ -9,6 +9,7 @@ import org.chromium.chrome.browser.ntp.snippets.CategoryInt;
 import org.chromium.chrome.browser.ntp.snippets.CategoryStatus.CategoryStatusEnum;
 import org.chromium.chrome.browser.ntp.snippets.SectionHeader;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
+import org.chromium.chrome.browser.ntp.snippets.SnippetArticleViewHolder;
 import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge;
 
 import java.util.ArrayList;
@@ -21,6 +22,7 @@ public class SuggestionsSection extends InnerNode {
     private final List<TreeNode> mChildren = new ArrayList<>();
     private final List<SnippetArticle> mSuggestions = new ArrayList<>();
     private final SectionHeader mHeader;
+    private final TreeNode mSuggestionsList = new SuggestionsList(this, mSuggestions);
     private final StatusItem mStatus;
     private final ProgressItem mProgressIndicator = new ProgressItem();
     private final ActionItem mMoreButton;
@@ -35,6 +37,42 @@ public class SuggestionsSection extends InnerNode {
         resetChildren();
     }
 
+    private static class SuggestionsList extends ChildNode {
+        private final List<SnippetArticle> mSuggestions;

[dgn, 2016/10/21 17:13:30]

I find it a bit weird that we have another reference to the suggestion list
stored here. It's fine in the current case because mSuggestions is final in both
objects, but if the class was accessed from outside, we would have no guarantee
that the list if used/modified properly, right?

Maybe make the inner class non static and use directly the mSuggestions list?

Actually, what is the target end state/goal of the refactor? Do we have a design
doc or quick description for that somewhere?

[Bernhard Bauer, 2016/10/22 16:43:47]

Yeah, I considered that as well. I'm not sure what would be preferable here, as
inner classes are somewhat discouraged, but then again in this case there is not
much risk of a memory leak.

So, uh, TL;DR: Done.
Hm, I suppose I should update that design Doc Nicole started at some point…

I don't have an exact end state in mind, but there are a couple of things I
would like to achieve:
1) Decoupling the various pieces of logic, by moving them from the adapter into
the tree structure. For example, dismissal is something that could be handled by
a TreeNode instead of doing the big switch statement we have right now in the
Adapter.
2) Stabilizing the structure of the tree. Right now we tend to throw away
everything and rebuild, which means we have to manually figure out what changes
exactly to notify about (cf. removeSuggestion). If we keep the tree structure
stable and just adjust the number of children below each node as necessary (like
SigninPromo already does), we get correct notifications for free, and it will
allow for some optimizations as well (we could cache the number of children for
each node and update it incrementally when a subtree changes. Right now we'd
have to contort ourselves a bit to make that work with things like
resetChildren()).

+
+        public SuggestionsList(NodeParent parent, List<SnippetArticle> suggestions) {
+            super(parent);
+            mSuggestions = suggestions;
+        }
+
+        @Override
+        public int getItemCount() {
+            return mSuggestions.size();
+        }
+
+        @Override
+        @ItemViewType
+        public int getItemViewType(int position) {
+            return ItemViewType.SNIPPET;
+        }
+
+        @Override
+        public void onBindViewHolder(NewTabPageViewHolder holder, int position) {
+            assert holder instanceof SnippetArticleViewHolder;
+            ((SnippetArticleViewHolder) holder).onBindViewHolder(getSuggestionAt(position));
+        }
+
+        @Override
+        public SnippetArticle getSuggestionAt(int position) {
+            return mSuggestions.get(position);
+        }
+
+        @Override
+        public int getDismissSiblingPosDelta(int position) {
+            return 0;
+        }
+    }
+
     @Override
     protected List<TreeNode> getChildren() {
         return mChildren;
@@ -43,7 +81,7 @@ public class SuggestionsSection extends InnerNode {
     private void resetChildren() {
         mChildren.clear();
         mChildren.add(mHeader);
-        mChildren.addAll(mSuggestions);
+        mChildren.add(mSuggestionsList);
 
         if (mSuggestions.isEmpty()) mChildren.add(mStatus);
         if (mCategoryInfo.hasMoreButton() || mSuggestions.isEmpty()) mChildren.add(mMoreButton);