Thanks for spotting this!

We should try to keep the Fetch-More logic de-coupled from the regular fetches.
Ideally, we could handle this case at the UI level and trigger a regular fetch instead of fetch-more.
That said, this reminds me of https://bugs.chromium.org/p/chromium/issues/detail?id=723656 which is related. After all, we're dealing with the empty state (give me something) which is conceptually different from the fetch-more state (give me more -- I already see something).

The solution you propose sgtm.

Just want to clarify if this is a merge candidate into the M60 branch so that we can quickly find an owner :)

It is a bit tricky to get it quickly right, though.

Patrick, do you agree with conceptually distinguishing between "empty -- get me some" and "non-empty get me more" cases and the proposed treatment?

Nicolas, can you confirm that such a change is simple at the UI level?

Jan, if we change the UI to trigger a fetch instead of fetch-more, can we guarantee that this will result in a call to the backends with additional data? Does the fetch call support updating the UI when the results are there (i.e. telling the spinner to stop, etc. I'm not sure if we have a proper 'done' callback in place for this path).

I'm not feeling strongly about the merge. We need to see how clean the patch will be. To be honest, I don't think it's not too critical as it only happens in corner cases and there's an easy way to fix it. If all you the answers above are uncontroversially answered with "yes", we can consider a merge.

I understand the UI-only solution as 
 - 'More' button in empty sections behaving as 
 - 'Refresh' button in "That's all for now" case.

There is no 'done' callback for 'Refresh' but the UI somehow is able to cope with that.

Doing that change at the UI level should be quite easy, yes. After all, that was the old behaviour, and it is currently used on scheduled fetches and when we want to recover from the All Dismissed state.

That being said I find it strange to handle this edge case in the UI, deciding when we do a fetch that affects all tabs and when we do one that affects only the current one. This logic will have to be reimplemented in iOS.

From the point of view of the UI, we want to fetch an additional set of suggestions and that's all. Shouldn't the backend set up intercepting suggestions when it's empty? Or clearing history trigger a fetch for new content?

I see your concerns, Nicolas.
 - Clearing history triggering a fetch is no-go (from the privacy perspective).
 - The backend doing the work is certainly possible. It just makes it harder to keep the logic for "More" and for regular full fetch separate (as each of the calls has different interface).

I think it is not that much of a corner-case:
 - If any remote section is empty and displayed, its "More" button can do a regular fetch. The regular fetch won't affect other non-empty remote sections (that you've impressed). It will affect other empty remote sections but this is an improvement over the current situation, I think.
 - I even think that the "More" label for empty sections could be something clearer (e.g. "Refresh"). I believe this is a different UI situation that deserves a different treatment _in the UI_.

I don't think we want to change the UI, the fact that we do a full refresh is an implementation detail. For the user, that action is just "fetch 10 more suggestions". Which is the main reason why I'm not comfortable with coding the other behaviour in the UI.

I just discussed that with bauerb@. He agrees that it does not seem right to have that in the UI, but also would like to avoid having different fetch/update paths in the component. So what he suggested was something like:

- Extend the cache in the component: allow for more suggestions, maybe implement a faster decay when we have more than 10 suggestions stored, etc.
- Only one way to fetch: the returned suggestions are added in the local cache, which is reordered according to freshness, relevance, or anything we want (suggestions seen/tapped?)
- If the fetch originates from a MORE action, we can return the new suggestions, or since we have also the list of the displayed suggestions, we can send the next 10 according to the component cache
- Suggestions are still returned in batches of 10 regardless of how many are cached, so on new tabs and on additional fetches the behaviour stays same as existing.

WDYT?

Thanks for the input.

This is something we've discussed in the meeting around crbug.com/714031. To me it feels like a complex change: 
 - I am not sure the work is worth the improvement it brings (cleaner unified fetch/update). 
 - This solution adds new problems: When we have 30 suggestions in the cache and a new NTP displays only 10 suggestions, should we fetch newer ones or return the ones from the cache? How old is too old? ...
 - Lastly, this hardly can happen before M61 BP (given other tasks).

On the other hand, automatic pre-fetching of suggestions (paquete team) requires changes in the direction you sketched (offlined suggestions should stay around for longer so we need to extend the cache a bit).

Cannot we:
 - implement changes in the component for prefetching for M61 with the proposal in #7 in mind,
 - see after M61 BP how the code evolved and re-consider #7, and
 - implement a short-term fix in the UI in the mean-time (before M61 BP)?

I just uploaded the hotfix you request at https://chromium-review.googlesource.com/c/533015/. It has issues: fetches all the remote categories, would not refresh the categories if there is only one suggestion, so the other tabs would still only show one, etc.

I'm not very comfortable with this, as it uncovers inconsistencies and unexpected behaviours, but if you believe this is more important I suppose we can land it for M61. Please fix it properly later though.

I believe this is a clear improvement over the current situation. Thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e81b29d1f8b849d6bc8c7b11c86b4aee3a5d0dee

commit e81b29d1f8b849d6bc8c7b11c86b4aee3a5d0dee
Author: Nicolas Dossou-gbete <dgn@chromium.org>
Date: Tue Jun 13 12:39:37 2017

[Suggestions] Do remote refresh when more pressed on empty section

The standard fetch action fetches suggestions for only the surface
that initiates the request, but when there are no suggestions, it
would fetch some that will not be there on other surfaces or after
a refresh.

This patch triggers a full refresh of the remote suggestions as a
hotfix for M61. A more complete rewrite of the component logic
should replace it later

BUG= 728570 

Change-Id: I0b318b2357e22cca5a669f5590a546c3df2c3d59
Reviewed-on: https://chromium-review.googlesource.com/533015
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478985}
[modify] https://crrev.com/e81b29d1f8b849d6bc8c7b11c86b4aee3a5d0dee/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
[modify] https://crrev.com/e81b29d1f8b849d6bc8c7b11c86b4aee3a5d0dee/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsCategoryInfo.java

Pri-0 bugs are critical regressions or serious emergencies, and this bug has not been updated in three days. Could you please provide an update, or adjust the priority to a more appropriate level if applicable?

If a fix is in active development, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Decreasing the prio for the follow-up cleanings as the emergency has been solved by Nicolas.

Sorry that I only get back to this discussion now.
Thanks for implementing the immediate fix.

I have a couple of concerns with the contents of comment #7.
1) We should not do any re-ordering or merging of results on the client side. It's only going to get us into troubles. It did in the past, and I'm happy we're getting this logic out of the client. The only exception might be experiments like what we discussed for keeping prefetched articles around for longer. But even there, if it turns out successfully, we need to revisit that.

2) The buttons on the UI are in fact different. It's a difference if you request more results in addition to what you see, or if you are asking for a refresh. These are fundamentally different operations and we should not handle them the same way at the component level. And they should be handled differently at the UI level, too.

So, I strongly vote against a shared cache serving "fetch more" and "refresh" operations. Happy to chat in person.

So, how would you suggest we fully fix this bug then?

I propose to discuss the bug when in London
(I'll schedule a slot when the plan of the conclave gets clearer).

Summary of the discussion in London:
We have inconsistency on "More" button pressed 
  - we do a "Reload suggestions" if the UI has zero article suggestions, while
  - we do "Fetch more" if the UI has >0 but <10 items (e.g. after dismissing something).
In the latter case, user gets more suggestions on the current NTP but the user again has <10 suggestions on another NTP opened.

Agreement: 
We should not do cache-and-reorder logic in the component. We should keep UI as logic-free as possible. Tim argues that having 0 suggestions is a specific situation in the UI where "More" button can behave differently.
gambard@: What happens on iOS if you have 0 suggestions and you press "More"?


AIs:
1) Find out if this is a real issue. This is blocked on crbug.com/737602

2) If it is a real issue, extend the scheduler <-> provider communication so that the scheduler issues a refetch if we have less than optimal number of suggestions (but maybe not too often, there is already a functionality to block new fetches for 30 minutes after history gets cleared, maybe we can reuse it for this corner-case).

3) If it is still an issue after the fix in 2), discuss with UX to change the UI treatment: append new suggestions to the list even if you have e.g. 3 suggestions and you have seen all of them (update it because the new set contains 10 suggestions which is >3, so we have stuff to append).

Assigning to vitaliii@ because he owns the blocking bug. Feel free to reassign if you think this should be handled by someone else.

On iOS the more button behave the same with 0 or more items (i.e. "fetch more").
I have seen a case during the development where I had 0 items, pressing more, and having 0 suggestions again in the next NTP. It feels weird.
I agree that the cache logic should live in the component.

Can we track the follow-up items (from  #18) in a separate bug and mark this one as fixed once it's also fixed for iOS?
This issue is about the empty articles section (which can be triggered through many scenarios which are not always directly clear to the user). The other topic is around the case where we show only few suggestions (e.g. because the user dismissed some). The latter can be fixed by smarter soft fetches. The zero-articles case cannot in some scenarios (e.g. the user wiped history).

About doing this in the UI layer:
We already do have this special cased in the UI (We show the message "That's all for now. Your suggested articles appear here"). The suggested resolution of this bug is that the UI triggers a refresh (Fetch instead of FetchMore) in case there is no suggestion.
That said, I'd even argue that the "more" button doesn't make sense below the "That's all for now" message" and should be a "refresh" button like we have in the all-categories-dismissed state. However, that's a much lower priority for me.


gambard@: Should we file a separate iOS bug, or should we use this one for the iOS change, too?

Good idea. Filed a new bug crbug.com/737929 to track that.

Gauthier, could you possible pick up this bug for iOS?

The merit of the change:
 - Call content_suggestions_service->ReloadSuggestions() instead of Fetch(...) on pressing the "more" button in an empty section.
 - c.f. the CL above: https://chromium-review.googlesource.com/533015

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7894e094226dcf568c93644f461e3bc5a759e695

commit 7894e094226dcf568c93644f461e3bc5a759e695
Author: gambard <gambard@chromium.org>
Date: Thu Jun 29 13:54:56 2017

Save new suggestions when section is empty

When the user press "more" on an empty section, the suggestions
displayed should be stored.
To have this behavior, reload the suggestions instead of fetching new
ones.

BUG= 728570 

Change-Id: Ibb135a4736d90d56f2925ac97c663a8cf6d89370
Reviewed-on: https://chromium-review.googlesource.com/555151
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483351}
[modify] https://crrev.com/7894e094226dcf568c93644f461e3bc5a759e695/ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm

The behavior described on #21 has been implemented. From my understanding those are the only changes related to this bug.

The fix for android triggers an interesting bug for the use case when the user dismisses all cards and then hits the "More" button. In that case, we don't add any cards to the UI: SuggestionsSection.updateSuggestions() knows that mNumberOfSuggestionsSeen is 10 and hence trimIncomingSuggestions() will kick out all of the received snippets.

This is getting super hacky. The culprit is that in ActionItem, we cannot simply use condition

if (mParentSection.getSuggestionsCount() == 0
                        && mParentSection.getCategoryInfo().isRemote()) 

but also need to take into account if the user interacted with that section.
Or, we simply revert that CL.

This is a regression we need to get fixed.

after having a little closer look at the code, I think the flaw is that we don't take card-dismissals into account when maintaining mNumberOfSuggestionsSeen. I'll give it a try.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0d5ddb814706c395097aa7402cbac08693dcf85

commit e0d5ddb814706c395097aa7402cbac08693dcf85
Author: Tim Schumann <tschumann@chromium.org>
Date: Fri Jul 28 05:58:53 2017

Fix keeping track of number of seen cards.

The recent change to issue a fetch instead of a fetch-more surfaced
some side-effects with how already seen surfaces get updated.
When dismissing a card, we did not adjust the number of seen cards
which lead to a situation where the append logic thinks more cards are
present than actually are. The result is an empty or not fully
populated list.
This CL
-- properly adjusts the counter when a suggestion gets removed or dismissed.
-- moves a bit more logic into appendSuggestions() to give it clearer semantics.

Bug:  728570 
Change-Id: I010c92b470f59a54fb66fd87cb60e7bbb5f74521
Reviewed-on: https://chromium-review.googlesource.com/584447
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Reviewed-by: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490291}
[modify] https://crrev.com/e0d5ddb814706c395097aa7402cbac08693dcf85/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
[modify] https://crrev.com/e0d5ddb814706c395097aa7402cbac08693dcf85/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
[modify] https://crrev.com/e0d5ddb814706c395097aa7402cbac08693dcf85/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
[modify] https://crrev.com/e0d5ddb814706c395097aa7402cbac08693dcf85/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
[modify] https://crrev.com/e0d5ddb814706c395097aa7402cbac08693dcf85/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java

We'd like to merge https://chromium-review.googlesource.com/584447 (from comment 31) into M61 as it fixes a user visible bug (see comment #25): If a user dismisses all cards, fetch-more does not show more cards. The regression was introduced in M61.

Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f

commit 54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f
Author: Tim Schumann <tschumann@chromium.org>
Date: Mon Aug 07 09:17:20 2017

Fix keeping track of number of seen cards.

The recent change to issue a fetch instead of a fetch-more surfaced
some side-effects with how already seen surfaces get updated.
When dismissing a card, we did not adjust the number of seen cards
which lead to a situation where the append logic thinks more cards are
present than actually are. The result is an empty or not fully
populated list.
This CL
-- properly adjusts the counter when a suggestion gets removed or dismissed.
-- moves a bit more logic into appendSuggestions() to give it clearer semantics.

TBR=tschumann@chromium.org

(cherry picked from commit e0d5ddb814706c395097aa7402cbac08693dcf85)

Bug:  728570 
Change-Id: I010c92b470f59a54fb66fd87cb60e7bbb5f74521
Reviewed-on: https://chromium-review.googlesource.com/584447
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Reviewed-by: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490291}
Reviewed-on: https://chromium-review.googlesource.com/603288
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#343}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
[modify] https://crrev.com/54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SectionList.java
[modify] https://crrev.com/54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
[modify] https://crrev.com/54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java
[modify] https://crrev.com/54c03c3bc117c4f56b2f459934fb3a1f8b5d6a3f/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java

Verified in 61.0.3163.42 build

Can this issue be closed now? It seems to be fixed and verified but it's a long discussion.

Is this issue fixed?

Should be fixed for iOS, implementation done in #23.

tschumann@ is this considered fixed for Android now as well?

yes. it's also fixed on Android.