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 503a13b25692b52715ca5e8061fc5f65c736b4dc..264307c13a1aa1ac5d146aa64c977e7665a92b45 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 kDefaultFetchingIntervalWifiChargingSeconds = 0;
 const int kDefaultFetchingIntervalWifiSeconds = 0;
@@ -177,6 +180,31 @@ void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
   }
 }
 
+bool IsSnippetInSet(const std::unique_ptr<NTPSnippet>& snippet,
+                    const std::set<std::string>& ids) {
+  if (ids.count(snippet->id()))
+    return true;
+  for (const SnippetSource& source : snippet->sources()) {
+    if (ids.count(source.url.spec()))
+      return true;
+  }
+  return false;
+}
+
+void EraseMatchingSnippets(
+    NTPSnippet::PtrVector* snippets,

[Marc Treib, 2016/09/22 11:27:06]

nit: This can go on the previous line

[jkrcal, 2016/09/22 12:50:05]

Done.

+    const NTPSnippet::PtrVector& matching) {
+  std::set<std::string> matching_ids;
+  InsertAllIDs(matching, &matching_ids);
+  snippets->erase(
+      std::remove_if(
+          snippets->begin(), snippets->end(),
+          [&matching_ids](const std::unique_ptr<NTPSnippet>& snippet) {
+            return IsSnippetInSet(snippet, matching_ids);
+          }),
+      snippets->end());
+}
+
 void Compact(NTPSnippet::PtrVector* snippets) {
   snippets->erase(
       std::remove_if(
@@ -369,8 +397,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();
@@ -411,8 +441,10 @@ void NTPSnippetsService::ClearDismissedSuggestionsForDebugging(
   if (content->dismissed.empty())
     return;
 
-  if (category == articles_category_)
+  if (category == articles_category_) {
+    // Image gets deleted when each snippet gets dismissed.

[Marc Treib, 2016/09/22 11:27:06]

nit: This sounds like something that will happen in the future, but in fact it's
something that should already have happened. "The image already got deleted then
the suggestion was dismissed"?

[jkrcal, 2016/09/22 12:50:05]

Done.

     database_->DeleteSnippets(content->dismissed);
+  }
   content->dismissed.clear();
 }
 
@@ -434,6 +466,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) {
@@ -443,21 +500,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);
 }
 
@@ -465,7 +516,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.
@@ -483,7 +534,8 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
               return lhs->score() > rhs->score();
             });
 
-  ClearExpiredSnippets();
+  ClearExpiredDismissedSnippets();
+  ClearOrphanedImages();
   FinishInitialization();
 }
 
@@ -518,8 +570,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);
 
@@ -541,9 +592,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;
 
@@ -554,6 +610,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.
@@ -564,28 +621,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);
@@ -604,28 +643,44 @@ void NTPSnippetsService::OnFetchFinished(
   NotifyNewSuggestions();
 }
 
-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);
+    // TODO(jkrcal): remove when orphaned images are deleted at start.
+    // crbug.com/649009
+    database_->DeleteImages(*to_archive);

[Marc Treib, 2016/09/22 11:27:06]

Hm. So when there is an open NTP that has a snippet, but not its image, then we
might download and store it again (and it'll get orphaned). So this solves only
half the problem, and introduces a new one (unnecessary re-downloads).
So I'm not sure this improves things. Let's just make sure to get the
orphan-cleanup into 55 :)

[jkrcal, 2016/09/22 12:50:05]

Okay :) Removed the line.

+  }
+
+  // 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, content->dismissed);
 
   // Fill in default publish/expiry dates where required.
   for (std::unique_ptr<NTPSnippet>& snippet : new_snippets) {
@@ -660,12 +715,22 @@ void NTPSnippetsService::MergeSnippets(Category category,
     }
   }
 
-  // Save new articles to the DB.
-  // TODO(sfiera): save non-articles to DB too.
-  if (category == articles_category_)
+  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, new_snippets);

[Marc Treib, 2016/09/22 11:27:06]

Hm. For this purpose, should we only check the primary ID? If only one of the
"sub-IDs" matches, then we might not be able to associate e.g. an image request
to the new snippet. It'd also make image management a bit easier.

[jkrcal, 2016/09/22 12:50:05]

Done.

+
+  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.
+  // Insert new snippets.
   content->snippets.insert(content->snippets.begin(),
                            std::make_move_iterator(new_snippets.begin()),
                            std::make_move_iterator(new_snippets.end()));

[Marc Treib, 2016/09/22 11:27:06]

Would
content->snippets = std::move(new_snippets);
work?

[jkrcal, 2016/09/22 12:50:05]

Done.

@@ -691,48 +756,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_) {
+      // Image gets deleted when each snippet gets dismissed.

[Marc Treib, 2016/09/22 11:27:06]

Same as the identical comment above :)

[jkrcal, 2016/09/22 12:50:05]

Done.

       database_->DeleteSnippets(to_delete);
+    }
 
     if (content->snippets.empty() && content->dismissed.empty()) {
       if ((category != articles_category_) && !content->provided_by_server)

[Marc Treib, 2016/09/22 11:27:06]

nit: Now you could merge the two "if"s

[jkrcal, 2016/09/22 12:50:05]

Done.

         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();
     }
   }
 
@@ -740,14 +789,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() {
@@ -822,20 +867,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
@@ -845,16 +884,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));
 }
@@ -907,13 +938,11 @@ void NTPSnippetsService::EnterStateDisabled() {
     categories_.erase(category);
   }
 
-  expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
   RescheduleFetching();
 }
 
 void NTPSnippetsService::EnterStateError() {
-  expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
   RescheduleFetching();
   snippets_status_service_.reset();