NTPSnippetsService: Garbage collect orphaned images at startup.

components/ntp_snippets/remote/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/remote/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
@@ skipping 462 matching lines @@
   database_->SaveImage(id_within_category, image_data);
 }
 
 void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
   if (state_ == State::ERROR_OCCURRED)
     return;
   DCHECK(state_ == State::NOT_INITED);
   DCHECK_EQ(1u, categories_.size());  // Only articles category, so far.
   DCHECK(categories_.find(articles_category_) != categories_.end());
 
+  ClearExpiredDismissedSnippets();

[Marc Treib, 2016/10/06 10:21:30]

This won't do anything, since |categories_| is still empty at this point.

[tschumann, 2016/10/06 10:56:11]

are you sure? we have a DCHECK() above verifying it includes the articles
category...

[Marc Treib, 2016/10/06 11:37:20]

Ah okay, correction: The category itself will be there, but it won't have any
suggestions - those are added just below. So my point "this won't do anything"
still stands :)

If you make a set of IDs anyway, you might as well collect them from
categories_[articles_category_]->snippets and ->dismissed directly in
ClearOrphanedImages instead of passing in |snippets|, and move these two lines
down again. Or am I missing something?

[tschumann, 2016/10/06 12:21:44]

nope -- you're not :-)
added a TODO() to increase test coverage as no test was failing.
thanks again for the catch!

+  ClearOrphanedImages(snippets);
+
   // 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();
             });
 
-  ClearExpiredDismissedSnippets();
-  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 64 matching lines @@
       categories_[category].provided_by_server = true;
 
       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());
       }
-
       ReplaceSnippets(category, std::move(fetched_category.snippets));
     }
   }
 
   for (const auto& item : categories_) {
     Category category = item.first;
     UpdateCategoryStatus(category, CategoryStatus::AVAILABLE);
   }
 
   // TODO(sfiera): equivalent metrics for non-articles.
@@ skipping 153 matching lines @@
       categories_to_erase.push_back(category);
     }
   }
 
   for (Category category : categories_to_erase) {
     UpdateCategoryStatus(category, CategoryStatus::NOT_PROVIDED);
     categories_.erase(category);
   }
 }
 
-void NTPSnippetsService::ClearOrphanedImages() {
-  // TODO(jkrcal): Implement. crbug.com/649009
+void NTPSnippetsService::ClearOrphanedImages(
+    const NTPSnippet::PtrVector& snippets) {
+  auto alive_snippets = base::MakeUnique<std::set<std::string>>();
+  for (const auto& snippet_ptr : snippets) {
+    alive_snippets->insert(snippet_ptr->id());
+  }
+  database_->GarbageCollectImages(std::move(alive_snippets));
 }
 
 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 302 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