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..742e7f9c045a603e3593fb67681e4fc6014809c5 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;

[Marc Treib, 2016/09/21 19:10:16]

Out of curiosity: Any particular reason for this number?

[jkrcal, 2016/09/22 09:12:42]

Not really. I was not sure if 100 is large enough so I picked 200 :)

+
 // Default values for snippets fetching intervals - once per day only.
 const int kDefaultFetchingIntervalWifiChargingSeconds = 0;
 const int kDefaultFetchingIntervalWifiSeconds = 0;
@@ -177,6 +180,30 @@ void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
   }
 }
 
+bool IsSnippetInSet(const std::unique_ptr<NTPSnippet>& snippet,
+                    std::set<std::string> ids) {

[Marc Treib, 2016/09/21 19:10:16]

const &

[jkrcal, 2016/09/22 09:12:42]

Oh, shit :) Thanks!

+  if (ids.count(snippet->id()))
+    return true;
+  for (const SnippetSource& source : snippet->sources()) {
+    if (ids.count(source.url.spec()))
+      return true;
+  }
+  return false;
+}
+
+void FilterSnippets(NTPSnippet::PtrVector* all_snippets,

[Marc Treib, 2016/09/21 19:10:16]

Hm, I find this name somewhat misleading, but I can't think of something much
better... EraseMatchingSnippets or something?

[jkrcal, 2016/09/22 09:12:42]

Better, done.

+                    const NTPSnippet::PtrVector& snippets_to_filter_out) {
+  std::set<std::string> filtered_snippet_ids;
+  InsertAllIDs(snippets_to_filter_out, &filtered_snippet_ids);
+  all_snippets->erase(
+      std::remove_if(
+          all_snippets->begin(), all_snippets->end(),
+          [&filtered_snippet_ids](const std::unique_ptr<NTPSnippet>& snippet) {
+            return IsSnippetInSet(snippet, filtered_snippet_ids);
+          }),
+      all_snippets->end());
+}
+
 void Compact(NTPSnippet::PtrVector* snippets) {
   snippets->erase(
       std::remove_if(
@@ -434,6 +461,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->get()->salient_image_url();

[Marc Treib, 2016/09/21 19:10:16]

(*it)->salient_image_url() ?

[jkrcal, 2016/09/22 09:12:42]

Done.

+
+  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->get()->salient_image_url();

[Marc Treib, 2016/09/21 19:10:17]

Same here

[jkrcal, 2016/09/22 09:12:42]

Done.

+
+  return GURL();
+}
+
 // image_fetcher::ImageFetcherDelegate implementation.
 void NTPSnippetsService::OnImageDataFetched(const std::string& suggestion_id,
                                             const std::string& image_data) {
@@ -443,21 +495,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()).

[Marc Treib, 2016/09/21 19:10:16]

nit: pipes around image_fetcher_

[jkrcal, 2016/09/22 09:12:42]

Done.

   database_->SaveImage(snippet_id, image_data);
 }
 
@@ -465,7 +511,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.
@@ -484,6 +530,7 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
             });
 
   ClearExpiredSnippets();

[Marc Treib, 2016/09/21 19:10:16]

Didn't you rename this to ClearExpiredDismissedSnippets? Forgot to hit "save"?
:)

[jkrcal, 2016/09/22 09:12:42]

More like forgot to hit "compile" :)

+  ClearOrphanedImages();
   FinishInitialization();
 }
 
@@ -541,6 +588,9 @@ void NTPSnippetsService::OnFetchFinished(
     content->provided_by_server = false;
   }
 
+  // Clear up expired dismissed snippets before we use them to filter new ones.
+  ClearExpiredSnippets();
+
   // If snippets were fetched successfully, update our |categories_| from each
   // category provided by the server.
   if (snippets) {
@@ -554,6 +604,7 @@ void NTPSnippetsService::OnFetchFinished(
         categories_[category].localized_title =
             fetched_category.localized_title;
       }
+      categories_[category].provided_by_server = true;

[Marc Treib, 2016/09/21 19:10:17]

Not your doing, but this is super hard to read, with so many variables called
"category"...

[jkrcal, 2016/09/22 09:12:42]

Agreed. I also had hard time understanding what is going on :)
Added a TODO.

 
       DCHECK_LE(snippets->size(), static_cast<size_t>(kMaxSnippetCount));
       // TODO(sfiera): histograms for server categories.
@@ -564,28 +615,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 +637,13 @@ void NTPSnippetsService::OnFetchFinished(
   NotifyNewSuggestions();
 }
 
-void NTPSnippetsService::MergeSnippets(Category category,
-                                       NTPSnippet::PtrVector new_snippets) {
+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.
+  FilterSnippets(&new_snippets, content->dismissed);
 
   // Fill in default publish/expiry dates where required.
   for (std::unique_ptr<NTPSnippet>& snippet : new_snippets) {
@@ -660,15 +678,43 @@ 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;

[Marc Treib, 2016/09/21 19:10:17]

I don't think returning here is correct, e.g. the database_->DeleteSnippets call
should still happen.

[jkrcal, 2016/09/22 09:12:42]

I do not agree. If we get a new fetch that is practically empty, I do not think
we should change anything about the current set of suggestions.

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

Okay, makes sense - mind adding a brief comment to explain?

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

Done.

+
+  // Remove current snippets that have been fetched again. We do not need to
+  // archive those as they will be in the new current set.
+  FilterSnippets(&content->snippets, new_snippets);
+
+  // TODO(sfiera): handle DB for non-articles too.
+  if (category == articles_category_) {
+    // Remove previous snippets from the DB.
+    database_->DeleteSnippets(content->snippets);
+    // Save new articles to the DB.
     database_->SaveSnippets(new_snippets);
+  }
+
+  // Archive previous snippets - move them at the beginning of the list.
+  content->archived.insert(content->archived.begin(),
+                           std::make_move_iterator(content->snippets.begin()),
+                           std::make_move_iterator(content->snippets.end()));
+  Compact(&content->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()));
+
+  // 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);
+  }
 }
 
 std::set<std::string> NTPSnippetsService::GetSnippetHostsFromPrefs() const {
@@ -694,24 +740,16 @@ void NTPSnippetsService::StoreSnippetHostsToPrefs(
 void NTPSnippetsService::ClearExpiredSnippets() {
   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);
@@ -725,29 +763,16 @@ void NTPSnippetsService::ClearExpiredSnippets() {
         categories_to_erase.push_back(category);
       continue;

[Marc Treib, 2016/09/21 19:10:16]

The "continue" doesn't do anything anymore.

[jkrcal, 2016/09/22 09:12:42]

Huh, good point :)

     }
-
-    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();
-    }
   }
 
   for (Category category : categories_to_erase) {
     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.

[Marc Treib, 2016/09/21 19:10:16]

Have a bug to reference? :)

[jkrcal, 2016/09/22 09:12:42]

Done.

 }
 
 void NTPSnippetsService::NukeAllSnippets() {
@@ -822,20 +847,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 +864,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 +918,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();