ReadingListProvider handles dismissal

components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc

Index: components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
diff --git a/components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc b/components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
index 223257baf68b02284c7f859a6ecfbcec3e017ad0..efbffc59919e24fe67f5ae3eea3193e00abe5cd6 100644
--- a/components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
+++ b/components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc
@@ -70,7 +70,9 @@ CategoryInfo ReadingListSuggestionsProvider::GetCategoryInfo(
 
 void ReadingListSuggestionsProvider::DismissSuggestion(
     const ContentSuggestion::ID& suggestion_id) {
-  // TODO(crbug.com/702241): Implement this method.
+  DCHECK(reading_list_model_->loaded());
+  GURL url(suggestion_id.id_within_category());
+  SetDismissedState(url, true);
 }
 
 void ReadingListSuggestionsProvider::FetchSuggestionImage(
@@ -98,7 +100,7 @@ void ReadingListSuggestionsProvider::ClearHistory(
     base::Time begin,
     base::Time end,
     const base::Callback<bool(const GURL& url)>& filter) {
-  // TODO(crbug.com/702241): Implement this method.
+  // Ignored.

[Marc Treib, 2017/04/11 12:35:15]

nit: Mention why it's okay to ignore? (I'd assume the ReadingListModel already
handles this correctly.)

Background: This is for the case when the provider itself stores any
history-related state, which should get cleared during the "clear browsing data"
process.

[gambard, 2017/04/11 13:29:53]

Reading List has nothing to do with history :)
I have added a comment

 }
 
 void ReadingListSuggestionsProvider::ClearCachedSuggestions(Category category) {
@@ -109,11 +111,15 @@ void ReadingListSuggestionsProvider::ClearCachedSuggestions(Category category) {
 void ReadingListSuggestionsProvider::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
-  // TODO(crbug.com/702241): Implement this method.
+  // The provider does not store the dismissed suggestions, returning an empty
+  // vector.

[Marc Treib, 2017/04/11 12:35:15]

So essentially "not implemented"... hm.
Couldn't you just go over reading_list_model_ and find all entries that have the
dismissed flag set?

This is used for about:snippets-internals (which I'm not sure exists on iOS, but
if it doesn't, we should implement it).

[gambard, 2017/04/11 13:29:53]

Ok, but I guess the comment in the content_suggestions_provider.h should be
updated :)
We are not using about:snippets-internals on iOS. Should we?

[Marc Treib, 2017/04/11 13:41:08]

You mean the "may return an empty vector" part? I think that still applies; it's
just that here it's not very difficult to implement it properly, so why not do
it :)
Yes, we should definitely get that working before launch. It's proven very
useful in debugging problems in the wild.

[gambard, 2017/04/11 15:01:18]

Isn't it the page implemented in chrome/browser/ui/webui? We don't pull that
code in iOS.

[Marc Treib, 2017/04/11 15:28:14]

Currently it is, yes. See e.g. components/ntp_tiles/webui/ for an example of
what needs to be done to make things work on iOS.

[gambard, 2017/04/12 07:50:43]

I have created crbug.com/710769

+  callback.Run(std::vector<ContentSuggestion>());
 }
 void ReadingListSuggestionsProvider::ClearDismissedSuggestionsForDebugging(

[Marc Treib, 2017/04/11 12:35:15]

nit: empty line before

[gambard, 2017/04/11 13:29:53]

Done.

     Category category) {
-  // TODO(crbug.com/702241): Implement this method.
+  for (const auto& url : reading_list_model_->Keys()) {
+    SetDismissedState(url, false);
+  }
 }
 
 void ReadingListSuggestionsProvider::ReadingListModelLoaded(
@@ -152,7 +158,7 @@ void ReadingListSuggestionsProvider::FetchReadingListInternal() {
   std::vector<const ReadingListEntry*> entries;
   for (const GURL& url : reading_list_model_->Keys()) {
     const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
-    if (!entry->IsRead()) {
+    if (!entry->IsRead() && !entry->ContentSuggestionsExtra()->dismissed) {
       entries.emplace_back(entry);
     }
   }
@@ -208,4 +214,11 @@ void ReadingListSuggestionsProvider::NotifyStatusChanged(
   observer()->OnCategoryStatusChanged(this, provided_category_, new_status);
 }
 
+void ReadingListSuggestionsProvider::SetDismissedState(const GURL& url,
+                                                       bool dismissed) {
+  reading_list::ContentSuggestionsExtra extra;
+  extra.dismissed = dismissed;
+  reading_list_model_->SetContentSuggestionsExtra(url, extra);

[Marc Treib, 2017/04/11 12:35:15]

Is the ContentSuggestionsExtra stuff actually implemented anywhere? Code search
doesn't find it..?

[gambard, 2017/04/11 13:29:53]

It is the depend CL :)

[Marc Treib, 2017/04/11 13:41:08]

Aah, missed that. Thanks!

+}
+
 }  // namespace ntp_snippets