📰 Stop clearing suggestions to show the RELOAD card.

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 69ccca4556404bf62277b00a7ce2326223eb1f7f..78e85e47a85fee6de3296c0055850b7117fe92fb 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
@@ -50,7 +50,7 @@
     private final List<NewTabPageListItem> mNewTabPageListItems;
     private final ItemTouchCallbacks mItemTouchCallbacks;
     private NewTabPageRecyclerView mRecyclerView;
-    private boolean mWantsSnippets;
+    private int mServiceStatus;
 
     private SnippetsBridge mSnippetsBridge;
 
@@ -119,7 +119,7 @@ public NewTabPageAdapter(NewTabPageManager manager, NewTabPageLayout newTabPageL
         mHeaderListItem = new SnippetHeaderListItem();
         mItemTouchCallbacks = new ItemTouchCallbacks();
         mNewTabPageListItems = new ArrayList<NewTabPageListItem>();
-        mWantsSnippets = true;
+        mServiceStatus = DisabledReason.NONE;
         mSnippetsBridge = snippetsBridge;
         mStatusListItem = StatusListItem.create(snippetsBridge.getDisabledReason(), this, manager);
 
@@ -134,7 +134,13 @@ public NewTabPageAdapter(NewTabPageManager manager, NewTabPageLayout newTabPageL
 
     @Override
     public void onSnippetsReceived(List<SnippetArticleListItem> listSnippets) {
-        if (!mWantsSnippets) return;
+        // We never want to refresh the suggestions if we already have some content.
+        if (hasSuggestions()) return;
+
+        if (!(mServiceStatus == DisabledReason.NONE
+                    || mServiceStatus == DisabledReason.HISTORY_SYNC_STATE_UNKNOWN)) {
+            return;
+        }
 
         int newSnippetCount = listSnippets.size();
         Log.d(TAG, "Received %d new snippets.", newSnippetCount);
@@ -144,8 +150,6 @@ public void onSnippetsReceived(List<SnippetArticleListItem> listSnippets) {
 
         loadSnippets(listSnippets);
 
-        // We don't want to get notified of other changes.
-        mWantsSnippets = false;
         NewTabPageUma.recordSnippetAction(NewTabPageUma.SNIPPETS_ACTION_SHOWN);
     }
 
@@ -154,18 +158,23 @@ public void onDisabledReasonChanged(int disabledReason) {
         // Observers should not be registered for that state
         assert disabledReason != DisabledReason.EXPLICITLY_DISABLED;
 
-        mStatusListItem = StatusListItem.create(disabledReason, this, mNewTabPageManager);
-        if (getItemCount() > 4 /* above-the-fold + header + card + spacing */) {
+        mServiceStatus = disabledReason;
+        mStatusListItem = StatusListItem.create(mServiceStatus, this, mNewTabPageManager);
+
+        // We had suggestions but we just got notified about the service being enabled. Nothing to
+        // do then.
+        if (disabledReason == DisabledReason.NONE && hasSuggestions()) return;
+
+        if (hasSuggestions()) {
             // We had many items, implies that the service was previously enabled and just

[tschumann, 2016/07/22 08:58:57]

I wonder if we should verify the bit "was previously enabled.". This assumption
was incorrect before and let to issues. Should we check for disabledReason !=
DisabledReason.None?

Also, this would potentially lead to the UI dropping suggestion cards the user
is looking at right now. What would be a valid, concrete scenario for that? Just
wondering if we ever want this to happen...

[Philipp Keck, 2016/07/22 09:07:28]

FYI: My upcoming CL replaces DisabledReason with
ContentSuggestionsCategoryStatus:
https://codereview.chromium.org/2158883002/diff/100001/chrome/android/java/sr...
It's pretty much equivalent, but there is a fine difference with "INITIALIZING":
The status that is set at the very beginning is INITIALIZING, just as
DisabledReason.NONE was set initially. But for
DisabledReason.HISTORY_SYNC_STATE_UNKNOWN you also get INITIALIZING. That's
because it should be equivalent to the user of that state (to the UI) because
they both mean: Not fully started yet, cannot tell yet whether content will be
available, but stay tuned.
However, in the code here, they're sometimes treated differently.

So when we reason about different scenarios, we should take the new enum into
account.

[dgn, 2016/07/22 09:47:14]

We already check above (l166) that we are in the enabled state and that we have
suggestions. In that case we don't do anything.

We want to clear suggestions when the user signs out, or the snippets get
disabled for some reason. If we don't clear them we don't really have a way to
let the user know if something is wrong, since we only show status when there
are no suggestions.

[tschumann, 2016/07/22 10:00:51]

ok, given that Philipp is changing this anyways, let's table this. (given that
we know only few cases in which we actually want to clear the snippets, we might
enumerate those explicitly).
You're right with that condition being enforced by line 166 -- i was just a
little afraid of hidden assumptions.

[dgn, 2016/07/22 10:10:30]

Ok, I'll look at his CL after the merge to see how this can be clarified. Thanks
for the feedback!

-            // transitioned. to a disabled state. We now clear it.
+            // transitioned to a disabled state. We now clear it.
             loadSnippets(new ArrayList<SnippetArticleListItem>());
         } else {
             mNewTabPageListItems.set(FIRST_CARD_POSITION, mStatusListItem);
-            notifyItemRangeChanged(FIRST_CARD_POSITION, 2); // Update both the first card and the
-            // spacing item coming after it.
-        }
 
-        if (disabledReason == DisabledReason.NONE) mWantsSnippets = true;
+            // Update both the first card and the spacing item coming after it.
+            notifyItemRangeChanged(FIRST_CARD_POSITION, 2);
+        }
     }
 
     @Override
@@ -214,7 +223,6 @@ public int getItemCount() {
 
     /** Start a request for new snippets. */
     public void reloadSnippets() {
-        mWantsSnippets = true;
         SnippetsBridge.fetchSnippets();
     }
 
@@ -294,6 +302,11 @@ private void addStatusCardIfNecessary() {
         }
     }
 
+    /** Returns whether we have some suggested content to display. */
+    private boolean hasSuggestions() {
+        return getItemViewType(FIRST_CARD_POSITION) == NewTabPageListItem.VIEW_TYPE_SNIPPET;
+    }
+
     List<NewTabPageListItem> getItemsForTesting() {
         return mNewTabPageListItems;
     }