Support server categories in NTPSnippetsService.

components/ntp_snippets/ntp_snippets_database.h

Index: components/ntp_snippets/ntp_snippets_database.h
diff --git a/components/ntp_snippets/ntp_snippets_database.h b/components/ntp_snippets/ntp_snippets_database.h
index 983166235f0ca70265f66149e3ae6c9b6aa842d5..2c76a45e44ed6b8bd838e66b2f2bb71e7359f3ef 100644
--- a/components/ntp_snippets/ntp_snippets_database.h
+++ b/components/ntp_snippets/ntp_snippets_database.h
@@ -29,7 +29,7 @@ class SnippetProto;
 class NTPSnippetsDatabase {
  public:
   using SnippetsCallback = base::Callback<void(NTPSnippet::PtrVector)>;
-  using SnippetImageCallback = base::Callback<void(std::string)>;
+  using SnippetImageCallback = base::Callback<void(std::string suggestion_id)>;
 
   NTPSnippetsDatabase(
       const base::FilePath& database_dir,
@@ -63,14 +63,15 @@ class NTPSnippetsDatabase {
 
   // Loads the image data for the snippet with the given ID and passes it to
   // |callback|. Passes an empty string if not found.
-  void LoadImage(const std::string& snippet_id,
+  void LoadImage(const std::string& suggestion_id,

[Marc Treib, 2016/08/22 15:06:46]

So now, suggestions themselves are indexed by within-category-ID, but images by
global-ID? That seems extremely confusing and error-prone, especially since both
IDs have the same type. (We really need to do something about that!)
How about just staying with within-category-ID? If the server returns two
suggestions from different categories but with the same ID, then they'd better
have to same image :)

[tschumann, 2016/08/22 16:00:23]

Yes, we should make sure snippet ids are unique. Actually, I think that
assumption is made in other places of the code, too. Can you think of a concrete
case where this might become an issue? Shouldn't de-duplicate elements based on
snippet ids?

[sfiera, 2016/08/24 14:35:56]

We plan to cache all server suggestions in the database, right? I see this as a
stopgap before converting |snippet_id| to |suggestion_id| everywhere in the
database. Converting the images was easy but I wasn't sure about other
suggestion types yet.

[Marc Treib, 2016/08/24 15:22:08]

I guess that's the plan, but that will require more changes (e.g. also caching
category info somewhere). Also, eventually we'd like to just store the proto we
receive on the wire, which doesn't work if we mangle IDs.
Anyway, I'm very unhappy about having two different kinds of IDs in the same
class, both of which happen to be strings. This has caused problems before in
other places, and I'm planning to get rid of the combined suggestion_id concept
(and instead make a class composed of category+within_category_id).

Anyway, proposal for now: Can we just keep using snippet_id, and ensure on the
server side that we never return two suggestions with the same ID, even if
they're in different categories?

[tschumann, 2016/08/24 15:33:08]

I had a closer look, and I'm fine with the current implementation. The class
name NTPSnippetsService just confused me -- it's a provider.
How the provider deals with the data is it's own business -- as long as the
service the other components talk to only knows the globally unique ID that's
fine.
However I don't like the name 'snippets_id'. The name should make sure that
these are internal ids from server-side suggestions. Not sure if 'server' is
clear in the chrome context. We could call it 'cloud_suggestion_id' --
'internal_cloud_suggestion_id' might be a bit chatty, but clearly differentiates
these and makes clear that it's an implementation details of this provider.
WDYT?

[Marc Treib, 2016/08/24 15:38:54]

The main thing that bothers me here is that the DB class now has some methods
which take the global unique IDs, and others that take the internal ones. Since
both are strings, it's extremely easy to mix them up, and it's just confusing.

[tschumann, 2016/08/25 16:15:21]

Correct! Yes, the NTPSnippetsDataBase should only handle internal ids (i.e. ids
of one type). 
I guess we don't have to care about existing data and can just change that.
-->Separate CL?

                  const SnippetImageCallback& callback);
 
   // Adds or updates the image data for the given snippet ID.
-  void SaveImage(const std::string& snippet_id, const std::string& image_data);
+  void SaveImage(const std::string& suggestion_id,
+                 const std::string& image_data);
 
   // Deletes the image data for the given snippet ID.
-  void DeleteImage(const std::string& snippet_id);
+  void DeleteImage(const std::string& suggestion_id);
 
  private:
   friend class NTPSnippetsDatabaseTest;
@@ -104,7 +105,7 @@ class NTPSnippetsDatabase {
   void DeleteSnippetsImpl(
       std::unique_ptr<std::vector<std::string>> keys_to_remove);
 
-  void LoadImageImpl(const std::string& snippet_id,
+  void LoadImageImpl(const std::string& suggestion_id,
                      const SnippetImageCallback& callback);
   void DeleteImagesImpl(
       std::unique_ptr<std::vector<std::string>> keys_to_remove);