New snippets now replace old snippets and do not merge

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index 1bb40f4ddc8c77e9919548ea6352b347552144cc..fb955cd0894cb2f080f8fe33b3a33f0c850593d5 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -50,6 +50,9 @@ namespace {
 // histograms with COUNTS() if this number increases beyond 50.
 const int kMaxSnippetCount = 10;
 
+// Number of archived snippets we keep around in memory.
+const int kMaxArchivedSnippetCount = 200;
+
 // Default values for snippets fetching intervals - once per day only.
 const int kDefaultFetchingIntervalWifiSeconds = 0;
 const int kDefaultFetchingIntervalFallbackSeconds = 24 * 60 * 60;
@@ -118,13 +121,48 @@ std::set<std::string> GetSuggestionsHostsImpl(
   return hosts;
 }
 
-void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
-                  std::set<std::string>* ids) {
+std::set<std::string> GetAllIDs(const NTPSnippet::PtrVector& snippets) {
+  std::set<std::string> ids;
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
-    ids->insert(snippet->id());
+    ids.insert(snippet->id());
     for (const SnippetSource& source : snippet->sources())
-      ids->insert(source.url.spec());
+      ids.insert(source.url.spec());
   }
+  return ids;
+}
+
+std::set<std::string> GetMainIDs(const NTPSnippet::PtrVector& snippets) {
+  std::set<std::string> ids;
+  for (const std::unique_ptr<NTPSnippet>& snippet : snippets)
+    ids.insert(snippet->id());
+  return ids;
+}
+
+bool IsSnippetInSet(const std::unique_ptr<NTPSnippet>& snippet,
+                    const std::set<std::string>& ids,
+                    bool match_all_ids) {
+  if (ids.count(snippet->id()))
+    return true;
+  if (!match_all_ids)
+    return false;
+  for (const SnippetSource& source : snippet->sources()) {
+    if (ids.count(source.url.spec()))
+      return true;
+  }
+  return false;
+}
+
+void EraseMatchingSnippets(NTPSnippet::PtrVector* snippets,
+                           const std::set<std::string>& matching_ids,
+                           bool match_all_ids) {
+  snippets->erase(
+      std::remove_if(snippets->begin(), snippets->end(),
+                     [&matching_ids, match_all_ids](
+                         const std::unique_ptr<NTPSnippet>& snippet) {
+                       return IsSnippetInSet(snippet, matching_ids,
+                                             match_all_ids);
+                     }),
+      snippets->end());
 }
 
 void Compact(NTPSnippet::PtrVector* snippets) {
@@ -323,8 +361,10 @@ void NTPSnippetsService::ClearCachedSuggestions(Category category) {
   if (content->snippets.empty())
     return;
 
-  if (category == articles_category_)
+  if (category == articles_category_) {
     database_->DeleteSnippets(content->snippets);
+    database_->DeleteImages(content->snippets);
+  }
   content->snippets.clear();
 
   NotifyNewSuggestions();
@@ -365,8 +405,10 @@ void NTPSnippetsService::ClearDismissedSuggestionsForDebugging(
   if (content->dismissed.empty())
     return;
 
-  if (category == articles_category_)
+  if (category == articles_category_) {
+    // The image got already deleted when the suggestion was dismissed.
     database_->DeleteSnippets(content->dismissed);
+  }
   content->dismissed.clear();
 }
 
@@ -388,6 +430,31 @@ int NTPSnippetsService::GetMaxSnippetCountForTesting() {
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
+GURL NTPSnippetsService::FindSnippetImageUrl(
+    Category category,
+    const std::string& snippet_id) const {
+  DCHECK(categories_.find(category) != categories_.end());
+
+  const CategoryContent& content = categories_.at(category);
+  // Search for the snippet in current and archived snippets.
+  auto it =
+      std::find_if(content.snippets.begin(), content.snippets.end(),
+                   [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
+                     return snippet->id() == snippet_id;
+                   });
+  if (it != content.snippets.end())
+    return (*it)->salient_image_url();
+
+  it = std::find_if(content.archived.begin(), content.archived.end(),
+                    [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
+                      return snippet->id() == snippet_id;
+                    });
+  if (it != content.archived.end())
+    return (*it)->salient_image_url();
+
+  return GURL();
+}
+
 // image_fetcher::ImageFetcherDelegate implementation.
 void NTPSnippetsService::OnImageDataFetched(const std::string& suggestion_id,
                                             const std::string& image_data) {
@@ -397,21 +464,15 @@ void NTPSnippetsService::OnImageDataFetched(const std::string& suggestion_id,
   Category category = GetCategoryFromUniqueID(suggestion_id);
   std::string snippet_id = GetWithinCategoryIDFromUniqueID(suggestion_id);
 
-  auto category_it = categories_.find(category);
-  if (category_it == categories_.end())
+  if (categories_.find(category) == categories_.end())
     return;
 
-  const CategoryContent& content = category_it->second;
-
   // Only save the image if the corresponding snippet still exists.
-  auto it =
-      std::find_if(content.snippets.begin(), content.snippets.end(),
-                   [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
-                     return snippet->id() == snippet_id;
-                   });
-  if (it == content.snippets.end())
+  if (FindSnippetImageUrl(category, snippet_id).is_empty())
     return;
 
+  // Only cache the data in the DB, the actual serving is done in the callback
+  // provided to |image_fetcher_| (OnSnippetImageDecodedFromNetwork()).
   database_->SaveImage(snippet_id, image_data);
 }
 
@@ -419,7 +480,7 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
   if (state_ == State::ERROR_OCCURRED)
     return;
   DCHECK(state_ == State::NOT_INITED);
-  DCHECK(categories_.size() == 1);  // Only articles category, so far.
+  DCHECK_EQ(1u, categories_.size());  // Only articles category, so far.
   DCHECK(categories_.find(articles_category_) != categories_.end());
 
   // TODO(sfiera): support non-article categories in database.
@@ -437,7 +498,8 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
               return lhs->score() > rhs->score();
             });
 
-  ClearExpiredSnippets();
+  ClearExpiredDismissedSnippets();
+  ClearOrphanedImages();
   FinishInitialization();
 }
 
@@ -472,8 +534,7 @@ void NTPSnippetsService::OnSuggestionsChanged(
       to_delete.emplace_back(std::move(snippet));
   }
   Compact(&content->snippets);
-  // Then delete the removed snippets from the database.
-  database_->DeleteSnippets(to_delete);
+  ArchiveSnippets(articles_category_, &to_delete);
 
   StoreSnippetHostsToPrefs(hosts);
 
@@ -495,9 +556,14 @@ void NTPSnippetsService::OnFetchFinished(
     content->provided_by_server = false;
   }
 
+  // Clear up expired dismissed snippets before we use them to filter new ones.
+  ClearExpiredDismissedSnippets();
+
   // If snippets were fetched successfully, update our |categories_| from each
   // category provided by the server.
   if (snippets) {
+    // TODO(jkrcal): A bit hard to understand with so many variables called
+    // "*categor*". Isn't here some room for simplification?
     for (NTPSnippetsFetcher::FetchedCategory& fetched_category : *snippets) {
       Category category = fetched_category.category;
 
@@ -508,6 +574,7 @@ void NTPSnippetsService::OnFetchFinished(
         categories_[category].localized_title =
             fetched_category.localized_title;
       }
+      categories_[category].provided_by_server = true;
 
       DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
       // TODO(sfiera): histograms for server categories.
@@ -518,28 +585,10 @@ void NTPSnippetsService::OnFetchFinished(
                                     fetched_category.snippets.size());
       }
 
-      MergeSnippets(category, std::move(fetched_category.snippets));
-
-      // If there are more snippets than we want to show, delete the extra ones.
-      CategoryContent* content = &categories_[category];
-      content->provided_by_server = true;
-      if (content->snippets.size() > kMaxSnippetCount) {
-        NTPSnippet::PtrVector to_delete(
-            std::make_move_iterator(content->snippets.begin() +
-                                    kMaxSnippetCount),
-            std::make_move_iterator(content->snippets.end()));
-        content->snippets.resize(kMaxSnippetCount);
-        if (category == articles_category_)
-          database_->DeleteSnippets(to_delete);
-      }
+      ReplaceSnippets(category, std::move(fetched_category.snippets));
     }
   }
 
-  // Trigger expiration.  This probably won't expire any current snippets (old
-  // ones should have already been expired by the timer, and new ones shouldn't
-  // have expired yet), but it will update the timer for the next run.
-  ClearExpiredSnippets();
-
   for (const auto& item : categories_) {
     Category category = item.first;
     UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
@@ -565,28 +614,43 @@ void NTPSnippetsService::OnFetchFinished(
     RescheduleFetching();
 }
 
-void NTPSnippetsService::MergeSnippets(Category category,
-                                       NTPSnippet::PtrVector new_snippets) {
+void NTPSnippetsService::ArchiveSnippets(Category category,
+                                         NTPSnippet::PtrVector* to_archive) {
+  CategoryContent* content = &categories_[category];
+
+  // TODO(sfiera): handle DB for non-articles too.
+  if (category == articles_category_) {
+    database_->DeleteSnippets(*to_archive);
+    // Do not delete the thumbnail images as they are still handy on open NTPs.
+  }
+
+  // Archive previous snippets - move them at the beginning of the list.
+  content->archived.insert(content->archived.begin(),
+                           std::make_move_iterator(to_archive->begin()),
+                           std::make_move_iterator(to_archive->end()));
+  Compact(to_archive);
+
+  // If there are more archived snippets than we want to keep, delete the
+  // oldest ones by their fetch time (which are always in the back).
+  if (content->archived.size() > kMaxArchivedSnippetCount) {
+    NTPSnippet::PtrVector to_delete(
+        std::make_move_iterator(content->archived.begin() +
+                                kMaxArchivedSnippetCount),
+        std::make_move_iterator(content->archived.end()));
+    content->archived.resize(kMaxArchivedSnippetCount);
+    if (category == articles_category_)
+      database_->DeleteImages(to_delete);
+  }
+}
+
+void NTPSnippetsService::ReplaceSnippets(Category category,
+                                         NTPSnippet::PtrVector new_snippets) {
   DCHECK(ready());
   CategoryContent* content = &categories_[category];
 
-  // Remove new snippets that we already have, or that have been dismissed.
-  std::set<std::string> old_snippet_ids;
-  InsertAllIDs(content->dismissed, &old_snippet_ids);
-  InsertAllIDs(content->snippets, &old_snippet_ids);
-  new_snippets.erase(
-      std::remove_if(
-          new_snippets.begin(), new_snippets.end(),
-          [&old_snippet_ids](const std::unique_ptr<NTPSnippet>& snippet) {
-            if (old_snippet_ids.count(snippet->id()))
-              return true;
-            for (const SnippetSource& source : snippet->sources()) {
-              if (old_snippet_ids.count(source.url.spec()))
-                return true;
-            }
-            return false;
-          }),
-      new_snippets.end());
+  // Remove new snippets that have been dismissed.
+  EraseMatchingSnippets(&new_snippets, GetAllIDs(content->dismissed),
+                        /*match_all_ids=*/true);
 
   // Fill in default publish/expiry dates where required.
   for (std::unique_ptr<NTPSnippet>& snippet : new_snippets) {
@@ -621,15 +685,24 @@ void NTPSnippetsService::MergeSnippets(Category category,
     }
   }
 
-  // Save new articles to the DB.
-  // TODO(sfiera): save non-articles to DB too.
-  if (category == articles_category_)
+  // Do not touch the current set of snippets if the newly fetched one is empty.
+  if (new_snippets.empty())
+    return;
+
+  // Remove current snippets that have been fetched again. We do not need to
+  // archive those as they will be in the new current set.
+  EraseMatchingSnippets(&content->snippets, GetMainIDs(new_snippets),
+                        /*match_all_ids=*/false);
+
+  ArchiveSnippets(category, &content->snippets);
+
+  // TODO(sfiera): handle DB for non-articles too.
+  if (category == articles_category_) {
+    // Save new articles to the DB.
     database_->SaveSnippets(new_snippets);
+  }
 
-  // Insert the new snippets at the front.
-  content->snippets.insert(content->snippets.begin(),
-                           std::make_move_iterator(new_snippets.begin()),
-                           std::make_move_iterator(new_snippets.end()));
+  content->snippets = std::move(new_snippets);
 }
 
 std::set<std::string> NTPSnippetsService::GetSnippetHostsFromPrefs() const {
@@ -652,48 +725,32 @@ void NTPSnippetsService::StoreSnippetHostsToPrefs(
   pref_service_->Set(prefs::kSnippetHosts, list);
 }
 
-void NTPSnippetsService::ClearExpiredSnippets() {
+void NTPSnippetsService::ClearExpiredDismissedSnippets() {
   std::vector<Category> categories_to_erase;
 
-  const base::Time expiry = base::Time::Now();
-  base::Time next_expiry = base::Time::Max();
+  const base::Time now = base::Time::Now();
 
   for (auto& item : categories_) {
     Category category = item.first;
     CategoryContent* content = &item.second;
 
-    // Move expired snippets over into |to_delete|.
     NTPSnippet::PtrVector to_delete;
-    for (std::unique_ptr<NTPSnippet>& snippet : content->snippets) {
-      if (snippet->expiry_date() <= expiry)
-        to_delete.emplace_back(std::move(snippet));
-    }
-    Compact(&content->snippets);
-
-    // Move expired dismissed snippets over into |to_delete| as well.
+    // Move expired dismissed snippets over into |to_delete|.
     for (std::unique_ptr<NTPSnippet>& snippet : content->dismissed) {
-      if (snippet->expiry_date() <= expiry)
+      if (snippet->expiry_date() <= now)
         to_delete.emplace_back(std::move(snippet));
     }
     Compact(&content->dismissed);
 
-    // Finally, actually delete the removed snippets from the DB.
-    if (category == articles_category_)
+    // Delete the removed article suggestions from the DB.
+    if (category == articles_category_) {
+      // The image got already deleted when the suggestion was dismissed.
       database_->DeleteSnippets(to_delete);
-
-    if (content->snippets.empty() && content->dismissed.empty()) {
-      if ((category != articles_category_) && !content->provided_by_server)
-        categories_to_erase.push_back(category);
-      continue;
     }
 
-    for (const auto& snippet : content->snippets) {
-      if (snippet->expiry_date() < next_expiry)
-        next_expiry = snippet->expiry_date();
-    }
-    for (const auto& snippet : content->dismissed) {
-      if (snippet->expiry_date() < next_expiry)
-        next_expiry = snippet->expiry_date();
+    if (content->snippets.empty() && content->dismissed.empty() &&
+        category != articles_category_ && !content->provided_by_server) {
+      categories_to_erase.push_back(category);
     }
   }
 
@@ -701,14 +758,10 @@ void NTPSnippetsService::ClearExpiredSnippets() {
     UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
     categories_.erase(category);
   }
+}
 
-  // Unless there are no snippets left, schedule a timer for the next expiry.
-  DCHECK_GT(next_expiry, expiry);
-  if (next_expiry < base::Time::Max()) {
-    expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
-                        base::Bind(&NTPSnippetsService::ClearExpiredSnippets,
-                                   base::Unretained(this)));
-  }
+void NTPSnippetsService::ClearOrphanedImages() {
+  // TODO(jkrcal): Implement. crbug.com/649009
 }
 
 void NTPSnippetsService::NukeAllSnippets() {
@@ -783,20 +836,14 @@ void NTPSnippetsService::FetchSnippetImageFromNetwork(
   Category category = GetCategoryFromUniqueID(suggestion_id);
   std::string snippet_id = GetWithinCategoryIDFromUniqueID(suggestion_id);
 
-  auto category_it = categories_.find(category);
-  if (category_it == categories_.end()) {
+  if (categories_.find(category) == categories_.end()) {
     OnSnippetImageDecodedFromNetwork(callback, suggestion_id, gfx::Image());
     return;
   }
 
-  const CategoryContent& content = category_it->second;
-  auto it =
-      std::find_if(content.snippets.begin(), content.snippets.end(),
-                   [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
-                     return snippet->id() == snippet_id;
-                   });
+  GURL image_url = FindSnippetImageUrl(category, snippet_id);
 
-  if (it == content.snippets.end() ||
+  if (image_url.is_empty() ||
       !thumbnail_requests_throttler_.DemandQuotaForRequest(
           /*interactive_request=*/true)) {
     // Return an empty image. Directly, this is never synchronous with the
@@ -806,16 +853,8 @@ void NTPSnippetsService::FetchSnippetImageFromNetwork(
     return;
   }
 
-  const NTPSnippet& snippet = *it->get();
-
-  // TODO(jkrcal): We probably should rename OnImageDataFetched() to
-  // CacheImageData(). This would document that this is actually independent
-  // from the individual fetch-flow.
-  // The image fetcher calls OnImageDataFetched() with the raw data (this object
-  // is an ImageFetcherDelegate) and then also
-  // OnSnippetImageDecodedFromNetwork() after the raw data gets decoded.
   image_fetcher_->StartOrQueueNetworkRequest(
-      suggestion_id, snippet.salient_image_url(),
+      suggestion_id, image_url,
       base::Bind(&NTPSnippetsService::OnSnippetImageDecodedFromNetwork,
                  base::Unretained(this), callback));
 }
@@ -862,12 +901,10 @@ void NTPSnippetsService::EnterStateReady() {
 
 void NTPSnippetsService::EnterStateDisabled() {
   NukeAllSnippets();
-  expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
 }
 
 void NTPSnippetsService::EnterStateError() {
-  expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
   snippets_status_service_.reset();
 }