NTPSnippetsService: Garbage collect orphaned images at startup.

components/ntp_snippets/remote/ntp_snippets_database.h

Index: components/ntp_snippets/remote/ntp_snippets_database.h
diff --git a/components/ntp_snippets/remote/ntp_snippets_database.h b/components/ntp_snippets/remote/ntp_snippets_database.h
index 9c903f0a86c9343b46ae41a6c8a137f1927b9aa3..37c8b78a220bd2e5496686626ac0a6edd13adb03 100644
--- a/components/ntp_snippets/remote/ntp_snippets_database.h
+++ b/components/ntp_snippets/remote/ntp_snippets_database.h
@@ -6,6 +6,7 @@
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_DATABASE_H_
 
 #include <memory>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -74,6 +75,10 @@ class NTPSnippetsDatabase {
   void DeleteImage(const std::string& snippet_id);
   // Deletes the image data for the given snippets (identified by their IDs).
   void DeleteImages(const NTPSnippet::PtrVector& snippets);
+  // Deletes all images which are not associated with any of the provided
+  // snippets.
+  void GarbageCollectImages(
+      std::unique_ptr<std::set<std::string>> alive_snippets);

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

Any particular reason for using std::set here? I'd pass in a const
NTPSnippet::PtrVector&, to be consistent with the other methods (and build the
set inside).

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

hmmm... it feels a bit weird that the function call wants a whole snippet if the
snippet-id is sufficient. I'd  actually vote for changing DeleteImages() to take
a vector of ids as well.
if we want to go that route (i'm happy to write the patch), then passing the
data in the data structure we actually need seems more plausible.

If you feel strongly, I can change that of course ;-)

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

I think the main reason for doing it that way was laziness :)  The caller had a
vector of snippets anyway, so this saved some copying.
Not feeling too strongly either way, I just like consistency. If you promise to
do a followup that makes things consistent (either way), then I'm fine with this
for now.

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

These methods were not called in many places. Just updated within this CL. PTAL.

 
  private:
   friend class NTPSnippetsDatabaseTest;
@@ -111,6 +116,10 @@ class NTPSnippetsDatabase {
                      const SnippetImageCallback& callback);
   void DeleteImagesImpl(
       std::unique_ptr<std::vector<std::string>> keys_to_remove);
+  void DeleteUnreferencedImages(
+      std::unique_ptr<std::set<std::string>> references,
+      bool success,

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

nit: load_keys_success? Took me a while to understand what that bool was doing
in the middle there.

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

Done.

+      std::unique_ptr<std::vector<std::string>> keys);
 
   std::unique_ptr<leveldb_proto::ProtoDatabase<SnippetProto>> database_;
   bool database_initialized_;