ReadingListProvider handles dismissal

components/ntp_snippets/reading_list/reading_list_suggestions_provider.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/reading_list/reading_list_suggestions_provider.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 52 matching lines @@
                           IDS_NTP_READING_LIST_SUGGESTIONS_SECTION_HEADER),
                       ContentSuggestionsCardLayout::FULL_CARD,
                       ContentSuggestionsAdditionalAction::VIEW_ALL,
                       /*show_if_empty=*/false,
                       l10n_util::GetStringUTF16(
                           IDS_NTP_READING_LIST_SUGGESTIONS_SECTION_EMPTY));
 }
 
 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(
     const ContentSuggestion::ID& suggestion_id,
     const ImageFetchedCallback& callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback, gfx::Image()));
 }
 
 void ReadingListSuggestionsProvider::Fetch(
     const Category& category,
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
   LOG(DFATAL) << "ReadingListSuggestionsProvider has no |Fetch| functionality!";
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(callback,
                  Status(StatusCode::PERMANENT_ERROR,
                         "ReadingListSuggestionsProvider has no |Fetch| "
                         "functionality!"),
                  base::Passed(std::vector<ContentSuggestion>())));
 }
 
 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) {
   DCHECK_EQ(category, provided_category_);
   // Ignored.
 }
 
 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(
     const ReadingListModel* model) {
   DCHECK(model == reading_list_model_);
   FetchReadingListInternal();
 }
 
 void ReadingListSuggestionsProvider::ReadingListModelBeingDeleted(
     const ReadingListModel* model) {
@@ skipping 18 matching lines @@
 
 void ReadingListSuggestionsProvider::FetchReadingListInternal() {
   if (!reading_list_model_ || reading_list_model_->IsPerformingBatchUpdates()) {
     return;
   }
 
   DCHECK(reading_list_model_->loaded());
   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);
     }
   }
 
   if (entries.size() > kMaxEntries) {
     // Get the |kMaxEntries| most recent entries.
     std::partial_sort(entries.begin(), entries.begin() + kMaxEntries,
                       entries.end(), CompareEntries);
     entries.resize(kMaxEntries);
   } else {
@@ skipping 35 matching lines @@
 
 void ReadingListSuggestionsProvider::NotifyStatusChanged(
     CategoryStatus new_status) {
   if (category_status_ == new_status) {
     return;
   }
   category_status_ = new_status;
   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