📰 Stop refreshing the whole NTP for every adapter change

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 886dd20d85d56b8cacb93fa8c6c2113e53588d80..4017344db6c4180aa6be7e2fc956add167a710d1 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
@@ -42,7 +42,7 @@
  * elements will be the cards shown to the user
  */
 public class NewTabPageAdapter extends Adapter<NewTabPageViewHolder>
-        implements SuggestionsSource.Observer {
+        implements SuggestionsSource.Observer, ItemGroup.Observer {
     private static final String TAG = "Ntp";
 
     private final NewTabPageManager mNewTabPageManager;
@@ -132,13 +132,20 @@ public void onChildDraw(Canvas c, RecyclerView recyclerView, ViewHolder viewHold
      * @param manager the NewTabPageManager to use to interact with the rest of the system.
      * @param aboveTheFoldView the layout encapsulating all the above-the-fold elements
      *                         (logo, search box, most visited tiles)
-     * @param suggestionsSource the bridge to interact with the content suggestions service.
      * @param uiConfig the NTP UI configuration, to be passed to created views.
      */
     public NewTabPageAdapter(NewTabPageManager manager, View aboveTheFoldView, UiConfig uiConfig) {
         mNewTabPageManager = manager;
         mAboveTheFoldView = aboveTheFoldView;
         mUiConfig = uiConfig;
+    }
+
+    /**
+     * Initialises the sections to be handled by this adapter. Events about categories for which
+     * a section has not been registered at this point will be ignored.
+     */
+    public void initializeSections() {

[Michael van Ouwerkerk, 2016/09/28 09:36:23]

Why was this split from the constructor? Afaict it's kind of for testing
purposes, but this type of two-stage initialization (construct and init) can be
fragile and reduce readability of the code. Also, the way this is used in the
test, initializeSections() is called multiple times on the same adapter
instance, which seems a bit counter to the concept of initialization which
should happen only once, asap after construction. This multiple initialization
is also something that does not happen in the production code path, leading to
production code that is only there for test purposes, such as clearing
mSections.

[dgn, 2016/09/28 21:31:19]

Because of what I suppose is another antipattern: we send the reference to the
object being constructed to somewhere else. Because of that wrapping the adapter
in Mockito.spy is not possible, so I was not able to verify the calls made on it
from inside the SuggestionsSections. 

I made the 2-stage initialisation an implementation detail and exposed a factory
function instead. Is that okay? Also I reverted the the initializeSection calls
in the tests when not necessary.

[Michael van Ouwerkerk, 2016/09/29 09:20:17]

Oh that makes sense. Thanks for clarifying! The create function and the docs
also help.

+        mSections.clear();
 
         SuggestionsSource suggestionsSource = mNewTabPageManager.getSuggestionsSource();
 
@@ -203,7 +210,6 @@ public void onNewSuggestions(@CategoryInt int category) {
         if (suggestions.isEmpty()) return;
 
         setSuggestions(category, suggestions, status);
-        updateGroups();
 
         NewTabPageUma.recordSnippetAction(NewTabPageUma.SNIPPETS_ACTION_SHOWN);
     }
@@ -222,18 +228,16 @@ public void onCategoryStatusChanged(@CategoryInt int category, @CategoryStatusEn
         if (status == CategoryStatus.CATEGORY_EXPLICITLY_DISABLED
                 || status == CategoryStatus.LOADING_ERROR) {
             // Need to remove the entire section from the UI immediately.
-            mSections.remove(category);
+            removeSection(mSections.get(category));
         } else {
             mSections.get(category).setStatus(status);
         }
-        updateGroups();
     }
 
     @Override
     public void onSuggestionInvalidated(@CategoryInt int category, String suggestionId) {
         if (!mSections.containsKey(category)) return;
         mSections.get(category).removeSuggestionById(suggestionId);
-        updateGroups();
     }
 
     @Override
@@ -361,10 +365,41 @@ private void updateGroups() {
             mGroups.add(mBottomSpacer);
         }
 
-        // TODO(bauerb): Notify about a smaller range: https://crbug.com/627512
         notifyDataSetChanged();
     }
 
+    private void removeSection(SuggestionsSection section) {
+        mSections.remove(section.getCategory());
+        int startPos = getGroupPositionOffset(section);
+        mGroups.remove(section);
+        int removedItems = section.getItems().size();
+
+        if (mSections.isEmpty()) {
+            if (mGroups.remove(mFooter)) ++removedItems;
+            if (mGroups.remove(mBottomSpacer)) ++removedItems;
+        }
+
+        notifyItemRangeRemoved(startPos, removedItems);
+    }
+
+    @Override
+    public void notifyItemRemoved(ItemGroup group, int itemPosition) {
+        notifyItemRemoved(getGroupPositionOffset(group) + itemPosition);
+    }
+
+    @Override
+    public void notifyGroupChanged(ItemGroup group, int itemCountBefore, int itemCountAfter) {
+        int startPos = getGroupPositionOffset(group);
+        // TODO(dgn): Currently the header never changes. Exclude it from the change notification?
+        if (itemCountBefore < itemCountAfter) {
+            notifyItemRangeChanged(startPos, itemCountBefore);
+            notifyItemRangeInserted(startPos + itemCountBefore, itemCountAfter - itemCountBefore);
+        } else {
+            notifyItemRangeChanged(startPos, itemCountAfter);
+            notifyItemRangeRemoved(startPos + itemCountAfter, itemCountBefore - itemCountAfter);
+        }
+    }
+
     @Override
     public void onAttachedToRecyclerView(RecyclerView recyclerView) {
         super.onAttachedToRecyclerView(recyclerView);
@@ -395,9 +430,7 @@ public void dismissItem(int position) {
 
     private void dismissSection(SuggestionsSection section) {
         mNewTabPageManager.getSuggestionsSource().dismissCategory(section.getCategory());
-
-        mSections.remove(section.getCategory());
-        updateGroups();
+        removeSection(section);
     }
 
     private void dismissSuggestion(int position) {
@@ -421,22 +454,11 @@ public void onResult(Boolean result) {
             }
         });
 
-        mRecyclerView.announceForAccessibility(mRecyclerView.getResources().getString(
-                R.string.ntp_accessibility_item_removed, suggestion.mTitle));
+        announceItemRemoved(suggestion.mTitle);
 
         suggestionsSource.dismissSuggestion(suggestion);
         SuggestionsSection section = (SuggestionsSection) getGroup(position);
         section.removeSuggestion(suggestion);
-
-        if (section.hasSuggestions()) {
-            // If one of many suggestions was dismissed, it's a simple item removal, which can be
-            // animated smoothly by the RecyclerView.
-            notifyItemRemoved(position);
-        } else {
-            // If the last suggestion was dismissed, multiple items will have changed, so mark
-            // everything as changed.
-            notifyDataSetChanged();
-        }
     }
 
     /**
@@ -474,4 +496,10 @@ private int getGroupPositionOffset(ItemGroup group) {
         }
         return RecyclerView.NO_POSITION;
     }
+
+    @VisibleForTesting
+    void announceItemRemoved(String suggestionTitle) {
+        mRecyclerView.announceForAccessibility(mRecyclerView.getResources().getString(
+                R.string.ntp_accessibility_item_removed, suggestionTitle));
+    }
 }