[NTP Snippets] Implement snippets merging

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 b68911ba00c03953b23eaf1678019165eedca1fa..aa1ee36d8a7c98ba0cd13848d76694e37c7c4607 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -6,7 +6,7 @@
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
-#include "base/json/json_string_value_serializer.h"
+#include "base/json/json_reader.h"
 #include "base/location.h"
 #include "base/path_service.h"
 #include "base/task_runner_util.h"
@@ -89,38 +89,42 @@ void NTPSnippetsService::OnFileReadDone(const std::string& json) {
 }
 
 bool NTPSnippetsService::LoadFromJSONString(const std::string& str) {
-  snippets_.clear();
-
-  JSONStringValueDeserializer deserializer(str);

[Marc Treib, 2016/02/23 12:57:54]

Not sure what the point of this class is, it just wraps over JSONReader which is
just as easy to use directly.

[Bernhard Bauer, 2016/02/23 13:42:21]

¯\_(ツ)_/¯ It does return an error message and code, which are useful in some
cases.

[Marc Treib, 2016/02/23 13:58:33]

Which JSONReader also does, if you call ReadAndReturnError, which is what
JSONStringValueDeserializer does. It provides literally no benefit.

-  int error_code;
-  std::string error_message;
-
-  scoped_ptr<base::Value> deserialized =
-      deserializer.Deserialize(&error_code, &error_message);
+  scoped_ptr<base::Value> deserialized = base::JSONReader::Read(str);
   if (!deserialized)
     return false;
 
-  const base::DictionaryValue* top_dict = NULL;
+  const base::DictionaryValue* top_dict = nullptr;
   if (!deserialized->GetAsDictionary(&top_dict))
     return false;
 
-  const base::ListValue* list = NULL;
+  const base::ListValue* list = nullptr;
   if (!top_dict->GetList("recos", &list))
     return false;
 
-  for (base::Value* const value : *list) {
-    const base::DictionaryValue* dict = NULL;
+  for (const base::Value* const value : *list) {
+    const base::DictionaryValue* dict = nullptr;
     if (!value->GetAsDictionary(&dict))
       return false;
 
-    const base::DictionaryValue* content = NULL;
+    const base::DictionaryValue* content = nullptr;
     if (!dict->GetDictionary("contentInfo", &content))
       return false;
     scoped_ptr<NTPSnippet> snippet =
         NTPSnippet::NTPSnippetFromDictionary(*content);
     if (!snippet)
       return false;
-    snippets_.push_back(std::move(snippet));
+
+    // Check if we already have a snippet with the same URL. If so, replace it
+    // rather than adding a duplicate.
+    const GURL& url = snippet->url();
+    auto it = std::find_if(snippets_.begin(), snippets_.end(),
+                           [&url](const scoped_ptr<NTPSnippet>& old_snippet) {
+      return old_snippet->url() == url;
+    });
+    if (it != snippets_.end())
+      *it = std::move(snippet);
+    else

[Bernhard Bauer, 2016/02/23 13:42:21]

This kind of feels like it should go into a map...

[Marc Treib, 2016/02/23 13:58:33]

Maybe? That would mess up the order though, and we'd need a custom comparator.
And we'd probably end up storing all the URLs twice...
So, I'd stay with the vector for now, especially given that the exact storage
method will probably change soon anyway.

[Bernhard Bauer, 2016/02/23 14:48:28]

Weeeellll... we could of course make this a std::set with a comparator that only
looks at URLs 😃

Also, note that now you'll end up with an order that depends on the order of old
versions of the snippet list. That may or may not be what you want?

But if you're fine with that, LGTM.

[Marc Treib, 2016/02/23 15:05:09]

Good point :)
Probably not - but AFAIK we haven't defined a "correct order" yet, so "order of
appearance" is as good as any I guess :)
Yeah, given that it'll change anyway, I'll just keep it as is for now.

+      snippets_.push_back(std::move(snippet));
   }
   loaded_ = true;