[NTP Snippets] Implement snippets merging

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 "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"
 #include "base/values.h"
 
 namespace {
 
 // TODO(crbug.com/587857): This is an extremely small value, for development.
 // Replace it by something sensible and add a command line param to control it.
 const int kDefaultFetchingIntervalSeconds = 60;
@@ skipping 62 matching lines @@
 
 void NTPSnippetsService::RemoveObserver(NTPSnippetsServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 void NTPSnippetsService::OnFileReadDone(const std::string& json) {
   LoadFromJSONString(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;
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded(this));
   return true;
 }
 
 void NTPSnippetsService::OnSnippetsDownloaded(
     const base::FilePath& download_path) {
   base::PostTaskAndReplyWithResult(
       file_task_runner_.get(), FROM_HERE,
       base::Bind(&ReadFileToString, download_path),
       base::Bind(&NTPSnippetsService::OnFileReadDone,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 }  // namespace ntp_snippets