New snippets now replace old snippets and do not merge

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 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/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 32 matching lines @@
 using suggestions::SuggestionsService;
 
 namespace ntp_snippets {
 
 namespace {
 
 // Number of snippets requested to the server. Consider replacing sparse UMA
 // 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;
 const int kDefaultFetchingIntervalFallbackSeconds = 24 * 60 * 60;
 
 // Variation parameters than can override the default fetching intervals.
 const char kFetchingIntervalWifiChargingParamName[] =
     "fetching_interval_wifi_charging_seconds";
 const char kFetchingIntervalWifiParamName[] =
     "fetching_interval_wifi_seconds";
@@ skipping 107 matching lines @@
 
 void InsertAllIDs(const NTPSnippet::PtrVector& snippets,
                   std::set<std::string>* ids) {
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets) {
     ids->insert(snippet->id());
     for (const SnippetSource& source : snippet->sources())
       ids->insert(source.url.spec());
   }
 }
 
+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(
           snippets->begin(), snippets->end(),
           [](const std::unique_ptr<NTPSnippet>& snippet) { return !snippet; }),
       snippets->end());
 }
 
 }  // namespace
 
@@ skipping 173 matching lines @@
   if (!initialized())
     return;
 
   if (categories_.find(category) == categories_.end())
     return;
   CategoryContent* content = &categories_[category];
   if (content->snippets.empty())
     return;
 
   if (category == articles_category_)
     database_->DeleteSnippets(content->snippets);

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

Also delete the images? (Please go over all database_->Delete* calls and make
sure if images also need to get deleted)

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

I was not sure. But I can delete them :)
Actually, I'll be now more strict about deleting before clearing orphaned images
is implemented.

   content->snippets.clear();
 
   NotifyNewSuggestions();
 }
 
 void NTPSnippetsService::GetDismissedSuggestionsForDebugging(
     Category category,
     const DismissedSuggestionsCallback& callback) {
   DCHECK(categories_.find(category) != categories_.end());
 
@@ skipping 43 matching lines @@
 }
 
 // static
 int NTPSnippetsService::GetMaxSnippetCountForTesting() {
   return kMaxSnippetCount;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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) {
   if (image_data.empty())
     return;
 
   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);
 }
 
 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.
   CategoryContent* content = &categories_[articles_category_];
   for (std::unique_ptr<NTPSnippet>& snippet : snippets) {
     if (snippet->is_dismissed())
       content->dismissed.emplace_back(std::move(snippet));
     else
       content->snippets.emplace_back(std::move(snippet));
   }
 
   std::sort(content->snippets.begin(), content->snippets.end(),
             [](const std::unique_ptr<NTPSnippet>& lhs,
                const std::unique_ptr<NTPSnippet>& rhs) {
               return lhs->score() > rhs->score();
             });
 
   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();
 }
 
 void NTPSnippetsService::OnDatabaseError() {
   EnterState(State::ERROR_OCCURRED);
   UpdateAllCategoryStatus(CategoryStatus::LOADING_ERROR);
 }
 
 // TODO(dgn): name clash between content suggestions and suggestions hosts.
 // method name should be changed.
@@ skipping 37 matching lines @@
 void NTPSnippetsService::OnFetchFinished(
     NTPSnippetsFetcher::OptionalSnippets snippets) {
   if (!ready())
     return;
 
   for (auto& item : categories_) {
     CategoryContent* content = &item.second;
     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) {
     for (NTPSnippetsFetcher::FetchedCategory& fetched_category : *snippets) {
       Category category = fetched_category.category;
 
       // TODO(sfiera): Avoid hard-coding articles category checks in so many
       // places.
       if (category != articles_category_) {
         // Only update titles from server-side provided categories.
         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.
       // Sparse histogram used because the number of snippets is small (bound by
       // kMaxSnippetCount).
       if (category == articles_category_) {
         UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticlesFetched",
                                     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);
   }
 
   // TODO(sfiera): equivalent metrics for non-articles.
   const CategoryContent& content = categories_[articles_category_];
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumArticles",
                               content.snippets.size());
   if (content.snippets.empty() && !content.dismissed.empty()) {
     UMA_HISTOGRAM_COUNTS("NewTabPage.Snippets.NumArticlesZeroDueToDiscarded",
                          content.dismissed.size());
   }
 
   // TODO(sfiera): notify only when a category changed above.
   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) {
     if (snippet->publish_date().is_null())
       snippet->set_publish_date(base::Time::Now());
     if (snippet->expiry_date().is_null()) {
       snippet->set_expiry_date(
           snippet->publish_date() +
           base::TimeDelta::FromMinutes(kDefaultExpiryTimeMins));
     }
@@ skipping 14 matching lines @@
         new_snippets.end());
     int num_snippets_dismissed = num_new_snippets - new_snippets.size();
     UMA_HISTOGRAM_BOOLEAN("NewTabPage.Snippets.IncompleteSnippetsAfterFetch",
                           num_snippets_dismissed > 0);
     if (num_snippets_dismissed > 0) {
       UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.Snippets.NumIncompleteSnippets",
                                   num_snippets_dismissed);
     }
   }
 
-  // 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);
+  }
 
-  // Insert the new snippets at the front.
+  // 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 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 {
   std::set<std::string> hosts;
   const base::ListValue* list = pref_service_->GetList(prefs::kSnippetHosts);
   for (const auto& value : *list) {
     std::string str;
     bool success = value->GetAsString(&str);
     DCHECK(success) << "Failed to parse snippet host from prefs";
     hosts.insert(std::move(str));
   }
   return hosts;
 }
 
 void NTPSnippetsService::StoreSnippetHostsToPrefs(
     const std::set<std::string>& hosts) {
   base::ListValue list;
   for (const std::string& host : hosts)
     list.AppendString(host);
   pref_service_->Set(prefs::kSnippetHosts, list);
 }
 
 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);
 
     // Finally, actually delete the removed snippets from the DB.

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

nitty nit: "Finally" doesn't apply anymore.

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

Done.

     if (category == articles_category_)
       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;

[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() {
   std::vector<Category> categories_to_erase;
 
   // Empty the ARTICLES category and remove all others, since they may or may
   // not be personalized.
   for (const auto& item : categories_) {
     Category category = item.first;
 
@@ skipping 54 matching lines @@
 
   FetchSnippetImageFromNetwork(suggestion_id, callback);
 }
 
 void NTPSnippetsService::FetchSnippetImageFromNetwork(
     const std::string& suggestion_id,
     const ImageFetchedCallback& callback) {
   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
     // original FetchSuggestionImage() call - an asynchronous database query has
     // happened in the meantime.
     OnSnippetImageDecodedFromNetwork(callback, suggestion_id, gfx::Image());
     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));
 }
 
 void NTPSnippetsService::OnSnippetImageDecodedFromNetwork(
     const ImageFetchedCallback& callback,
     const std::string& suggestion_id,
     const gfx::Image& image) {
   callback.Run(image);
 }
@@ skipping 32 matching lines @@
     if (category != articles_category_) {
       UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
       categories_to_erase.push_back(category);
     }
   }
 
   for (Category category : categories_to_erase) {
     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();
 }
 
 void NTPSnippetsService::FinishInitialization() {
   if (nuke_after_load_) {
     NukeAllSnippets();
     nuke_after_load_ = false;
   }
@@ skipping 134 matching lines @@
 }
 
 NTPSnippetsService::CategoryContent::CategoryContent() = default;
 NTPSnippetsService::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 NTPSnippetsService::CategoryContent::~CategoryContent() = default;
 NTPSnippetsService::CategoryContent& NTPSnippetsService::CategoryContent::
 operator=(CategoryContent&&) = default;
 
 }  // namespace ntp_snippets