[Suggestions] Consider hidden categories when checking staleness

Some content suggestions categories are hidden when no content is
available. The UI used to completely ignore them, when when checking
that the list of categories didn't change, that caused the UI's data
to be always considered stale.

We now keep track of these hidden categories, blacklisting them when
they should not be shown, and removing them from the blacklist when
we get notified of a change about them.

BUG=689962

Review-Url: https://codereview.chromium.org/2846123002
Cr-Commit-Position: refs/heads/master@{#467950}

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

@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.ntp.cards;
 
 import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.ntp.snippets.CategoryInt;
 import org.chromium.chrome.browser.ntp.snippets.CategoryStatus;
@@ -17,9 +18,13 @@ import org.chromium.chrome.browser.suggestions.DestructionObserver;
 import org.chromium.chrome.browser.suggestions.SuggestionsRanker;
 import org.chromium.chrome.browser.suggestions.SuggestionsUiDelegate;
 
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * A node in the tree containing a list of all suggestions sections. It listens to changes in the
@@ -31,6 +36,8 @@ public class SectionList
 
     /** Maps suggestion categories to sections, with stable iteration ordering. */
     private final Map<Integer, SuggestionsSection> mSections = new LinkedHashMap<>();
+    /** List of categories that are hidden because they have no content to show. */
+    private final Set<Integer> mBlacklistedCategories = new HashSet<>();
     private final SuggestionsUiDelegate mUiDelegate;
     private final OfflinePageBridge mOfflinePageBridge;
     private final SuggestionsRanker mSuggestionsRanker;
@@ -99,8 +106,11 @@ public class SectionList
 
         // Do not show an empty section if not allowed.
         if (suggestions.isEmpty() && !info.showIfEmpty() && !alwaysAllowEmptySections) {
+            mBlacklistedCategories.add(category);
             if (section != null) removeSection(section);
             return 0;
+        } else {
+            mBlacklistedCategories.remove(category);
         }
 
         // Create the section if needed.
@@ -147,6 +157,9 @@ public class SectionList
         // Observers should not be registered for this state.
         assert status != CategoryStatus.ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
 
+        // If the category was blacklisted, we note that there might be new content to show.
+        mBlacklistedCategories.remove(category);
+
         // If there is no section for this category there is nothing to do.
         if (!mSections.containsKey(category)) return;
 
@@ -227,12 +240,9 @@ public class SectionList
     public void synchroniseWithSource() {
         int[] categories = mUiDelegate.getSuggestionsSource().getCategories();
 
-        // TODO(dgn): this naive implementation does not quite work as some sections are removed
-        // from the UI when they are empty, even though they stay around in the backend. Also, we
-        // might not always get notifications of new content for local sections.
-        // See https://crbug.com/711414, https://crbug.com/689962
-
         if (categoriesChanged(categories)) {
+            Log.d(TAG, "The categories have changed: old=%s, new=%s - Resetting all the sections.",
+                    mSections.keySet(), Arrays.toString(categories));
             // The number or the order of the sections changed. We reset everything.
             resetSections(/* alwaysAllowEmptySections = */ false);
             return;
@@ -243,6 +253,7 @@ public class SectionList
 
             @CategoryInt
             int category = sectionsEntry.getKey();
+            Log.d(TAG, "The section for category %d is stale - Resetting.", category);
             resetSection(category, mUiDelegate.getSuggestionsSource().getCategoryStatus(category),
                     /* alwaysAllowEmptySections = */ false);
         }
@@ -273,15 +284,19 @@ public class SectionList
      * Checks that the list of categories currently displayed by this list is the same as
      * {@code newCategories}: same categories in the same order.
      */
-    private boolean categoriesChanged(@CategoryInt int[] newCategories) {
-        if (mSections.size() != newCategories.length) return true;
-
-        int index = 0;
-        for (@CategoryInt int category : mSections.keySet()) {
-            if (category != newCategories[index++]) return true;
+    @VisibleForTesting
+    boolean categoriesChanged(@CategoryInt int[] newCategories) {
+        Iterator<Integer> shownCategories = mSections.keySet().iterator();
+        for (int category : newCategories) {
+            if (mBlacklistedCategories.contains(category)) {
+                Log.d(TAG, "categoriesChanged: ignoring blacklisted category %d", category);
+                continue;
+            }
+            if (!shownCategories.hasNext()) return true;
+            if (shownCategories.next() != category) return true;
         }
 
-        return false;
+        return shownCategories.hasNext();
     }
 
     /**
@@ -290,6 +305,9 @@ public class SectionList
      * compatible with displaying content.
      */
     private boolean canProcessSuggestions(@CategoryInt int category, @CategoryStatus int status) {
+        // If the category was blacklisted, we note that there might be new content to show.
+        mBlacklistedCategories.remove(category);
+
         // We never want to add suggestions from unknown categories.
         if (!mSections.containsKey(category)) return false;
 

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

@@ -39,6 +39,7 @@ import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.DisableHistogramsRule;
 import org.chromium.chrome.browser.ntp.snippets.CategoryInt;
+import org.chromium.chrome.browser.ntp.snippets.CategoryStatus;
 import org.chromium.chrome.browser.ntp.snippets.KnownCategories;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
@@ -402,4 +403,57 @@ public class SectionListTest {
         inOrder.verify(mSuggestionSource).getSuggestionsForCategory(CATEGORY1);
         inOrder.verify(mSuggestionSource).getSuggestionsForCategory(CATEGORY2);
     }
+
+    @Test
+    public void testCategoryChangeWithSameCategories() {
+        registerCategory(mSuggestionSource, CATEGORY1, 1);
+        registerCategory(mSuggestionSource, CATEGORY2, 1);
+
+        SectionList sectionList = spy(new SectionList(mUiDelegate, mOfflinePageBridge));
+        sectionList.refreshSuggestions();
+
+        assertFalse(sectionList.categoriesChanged(mSuggestionSource.getCategories()));
+    }
+
+    @Test
+    public void testCategoryChangeWithDifferentOrderOrNumberInCategories() {
+        registerCategory(mSuggestionSource, CATEGORY1, 1);
+        registerCategory(mSuggestionSource, CATEGORY2, 1);
+
+        SectionList sectionList = spy(new SectionList(mUiDelegate, mOfflinePageBridge));
+        sectionList.refreshSuggestions();
+
+        // Not using the same categories as present in the source here, change should be detected.
+        assertTrue(sectionList.categoriesChanged(new int[] {CATEGORY2, CATEGORY1}));
+        assertTrue(sectionList.categoriesChanged(new int[] {CATEGORY1}));
+        assertTrue(sectionList.categoriesChanged(new int[] {CATEGORY1, CATEGORY2, CATEGORY2 + 1}));
+    }
+
+    @Test
+    public void testCategoryChangeWithEmptyHiddenCategory() {
+        registerCategory(mSuggestionSource, CATEGORY1, 1);
+        registerCategory(mSuggestionSource, new CategoryInfoBuilder(CATEGORY2).build(), 0);
+
+        SectionList sectionList = spy(new SectionList(mUiDelegate, mOfflinePageBridge));
+        sectionList.refreshSuggestions();
+
+        // The check here ignores |CATEGORY2| which is present during the construction but not shown
+        // because empty. It does not detect changes whether the reference array includes it or not.
+        assertThat(
+                mSuggestionSource.getCategories(), is(equalTo(new int[] {CATEGORY1, CATEGORY2})));
+        assertFalse(sectionList.categoriesChanged(mSuggestionSource.getCategories()));
+        assertFalse(sectionList.categoriesChanged(new int[] {CATEGORY1}));
+
+        mSuggestionSource.setStatusForCategory(CATEGORY2, CategoryStatus.AVAILABLE_LOADING);
+
+        // After notifying of a change for the category, it stops being ignored.
+        assertTrue(sectionList.categoriesChanged(mSuggestionSource.getCategories()));
+        assertFalse(sectionList.categoriesChanged(new int[] {CATEGORY1}));
+
+        sectionList.refreshSuggestions();
+
+        // And after a refresh we start ignoring it again.
+        assertFalse(sectionList.categoriesChanged(mSuggestionSource.getCategories()));
+        assertFalse(sectionList.categoriesChanged(new int[] {CATEGORY1}));
+    }
 }