Remove deleted offline page suggestions from opened NTPs

components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc

 // Copyright 2016 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/offline_pages/offline_page_suggestions_provider.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/guid.h"
@@ skipping 198 matching lines @@
 
 void OfflinePageSuggestionsProvider::OfflinePageModelChanged(
     OfflinePageModel* model) {
   DCHECK_EQ(offline_page_model_, model);
   FetchOfflinePages();
 }
 
 void OfflinePageSuggestionsProvider::OfflinePageDeleted(
     int64_t offline_id,
     const offline_pages::ClientId& client_id) {
-  // TODO(pke): Implement, suggestion has to be removed from UI immediately.
+  std::string offline_page_id = base::IntToString(offline_id);
+  if (recent_tabs_status_ != CategoryStatus::NOT_PROVIDED &&
+      client_id.name_space == offline_pages::kLastNNamespace) {
+    auto it = std::find(dismissed_recent_tab_ids_.begin(),
+                        dismissed_recent_tab_ids_.end(), offline_page_id);
+    if (it == dismissed_recent_tab_ids_.end()) {
+      observer()->OnSuggestionInvalidated(

[Marc Treib, 2016/08/16 09:03:54]

I think we should send the invalidation in any case, even if it was dismissed -
there might still be *another* open NTP that has it. (super edge case, but
still)

Probably also if the category status is "bad" - since we don't clear the UI in
that case, the suggestion might also still be there.

[Philipp Keck, 2016/08/16 10:54:40]

I will fix this in an upcoming CL (all this code is totally refactored there).
There is another detail worth noting: The OfflinePageSuggestionsProvider doesn't
know what it's forwarding here. As we only return the first 10 results from the
OfflinePageModel when delivering suggestions, we have a good chance of reporting
a suggestion as invalidated which has never existed in the first place. I will
also document this in the next CL.
If we switched to the NOT_PROVIDED status dynamically, you would be right - it's
the only case where the category becomes unavailable but the section is still
visible on the UI. But the OfflinePageSuggestionsProvider doesn't do that.
We could still forward it, though I don't see why we would need to.

Calling OnSuggestionInvalidated for categories that never existed is a bit
weird, but not a problem.

[Philipp Keck, 2016/08/16 11:21:18]

The former is now implemented here:
https://codereview.chromium.org/2251743002/diff/20001/components/ntp_snippets...

[Marc Treib, 2016/08/16 11:40:09]

Fair enough, but probably worth a comment?

[Philipp Keck, 2016/08/16 11:51:03]

Done.

+          this, recent_tabs_category_,
+          MakeUniqueID(recent_tabs_category_, offline_page_id));
+    } else {
+      dismissed_recent_tab_ids_.erase(it);
+      StoreDismissedIDsToPrefs(prefs::kDismissedRecentOfflineTabSuggestions,
+                               dismissed_recent_tab_ids_);
+    }
+  } else if (downloads_status_ != CategoryStatus::NOT_PROVIDED &&
+             client_id.name_space == offline_pages::kAsyncNamespace &&
+             base::IsValidGUID(client_id.id)) {
+    auto it = std::find(dismissed_download_ids_.begin(),
+                        dismissed_download_ids_.end(), offline_page_id);
+    if (it == dismissed_download_ids_.end()) {
+      observer()->OnSuggestionInvalidated(
+          this, downloads_category_,
+          MakeUniqueID(downloads_category_, offline_page_id));
+    } else {
+      dismissed_download_ids_.erase(it);
+      StoreDismissedIDsToPrefs(prefs::kDismissedDownloadSuggestions,
+                               dismissed_download_ids_);
+    }
+  }
 }
 
 void OfflinePageSuggestionsProvider::FetchOfflinePages() {
   offline_page_model_->GetAllPages(
       base::Bind(&OfflinePageSuggestionsProvider::OnOfflinePagesLoaded,
                  base::Unretained(this)));
 }
 
 void OfflinePageSuggestionsProvider::OnOfflinePagesLoaded(
     const MultipleOfflinePageItemResult& result) {
   bool need_recent_tabs = recent_tabs_status_ != CategoryStatus::NOT_PROVIDED;
   bool need_downloads = downloads_status_ != CategoryStatus::NOT_PROVIDED;
   if (need_recent_tabs)
     NotifyStatusChanged(recent_tabs_category_, CategoryStatus::AVAILABLE);
   if (need_downloads)
     NotifyStatusChanged(downloads_category_, CategoryStatus::AVAILABLE);
 
   std::vector<const OfflinePageItem*> recent_tab_items;
   std::vector<const OfflinePageItem*> download_items;
   for (const OfflinePageItem& item : result) {
     if (need_recent_tabs &&
         item.client_id.name_space == offline_pages::kLastNNamespace &&
         !dismissed_recent_tab_ids_.count(base::IntToString(item.offline_id))) {
       recent_tab_items.push_back(&item);
     }
 
     // TODO(pke): Use kDownloadNamespace once the OfflinePageModel uses that.
     // The current logic is taken from DownloadUIAdapter::IsVisibleInUI.
+    // Note: This is also copied in OfflinePageDeleted above.
     if (need_downloads &&
         item.client_id.name_space == offline_pages::kAsyncNamespace &&
         base::IsValidGUID(item.client_id.id) &&
         !dismissed_download_ids_.count(base::IntToString(item.offline_id))) {
       download_items.push_back(&item);
     }
   }
 
   // TODO(pke): Once we have our OfflinePageModel getter and that doesn't do it
   // already, filter out duplicate URLs for recent tabs here. Duplicates for
@@ skipping 85 matching lines @@
 void OfflinePageSuggestionsProvider::StoreDismissedIDsToPrefs(
     const std::string& pref_name,
     const std::set<std::string>& dismissed_ids) {
   base::ListValue list;
   for (const std::string& dismissed_id : dismissed_ids)
     list.AppendString(dismissed_id);
   pref_service_->Set(pref_name, list);
 }
 
 }  // namespace ntp_snippets