Fetch snippets from ChromeReader and show them on the NTP

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 f3c0670f7af808486dbfd6cd80572c73bfc971f5..66a923fe1e367cdc84df1e3f5a7338e637c392e3 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -4,14 +4,37 @@
 
 #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/location.h"
+#include "base/path_service.h"
+#include "base/task_runner_util.h"
 #include "base/values.h"
 
 namespace ntp_snippets {
 
+std::string ReadFileToString(const base::FilePath& path) {
+  std::string data;
+  bool success = base::ReadFileToString(path, &data);
+  if (!success)
+    DLOG(ERROR) << "Error reading file " << path.LossyDisplayName();

[noyau (Ping after 24h), 2016/02/23 14:23:45]

I'm not sure this is correct. In a release build the DLOG is completely removed,
so the if without curly brackets is potentially very very dangerous.

Why don't you like DCHECK?

DCHECK(success) << "Error reading..."...

You really want to stop in debug rather than putting out  a log that everyone
will ignore.

[Marc Treib, 2016/02/23 14:29:50]

I'm fairly sure this works, but we do have DLOG_IF for this kind of thing.
According to the coding style (http://dev.chromium.org/developers/coding-style),
(D)CHECKs should be treated like asserts, i.e. they should NEVER fail, even on
disk corruption etc.

[Bernhard Bauer, 2016/02/23 14:32:27]

(Note that the DLOG isn't completely compiled out in a Release build, but it
gets turned into a dummy stream that eats everything without printing).

[blundell, 2016/02/23 14:37:48]

<drive-by> As is, this code seems dangerous: in the !success case, we're
returning a string that is presumably garbage without any indication that it's
so. As an external observer, it seems like we should either (1) assert that this
doesn't happen if we think it should never happen (i.e., DCHECK) or (2) handle
it if we think it can happen in legitimate operation (e.g., propagate the
success value to our own callers for them to check or whatever).

[May, 2016/02/23 21:34:18]

Good point. It's definitely a legit operation to fail, and the |data| var could
potentially be garbage. I'll change it to propagate the success flag also.

[May, 2016/02/23 21:34:18]

I initially had DLOG_IF, but I was only using the success variable for it so it
was compiled out in Release (and then clang complained about unused variables,
which is how I ended up with this. 

+  return data;
+}
+
 NTPSnippetsService::NTPSnippetsService(
-    const std::string& application_language_code)
-    : loaded_(false), application_language_code_(application_language_code) {}
+    scoped_refptr<base::SequencedTaskRunner> file_task_runner,
+    const std::string& application_language_code,
+    scoped_ptr<NTPSnippetsFetcher> snippets_fetcher)
+    : loaded_(false),
+      file_task_runner_(file_task_runner),
+      application_language_code_(application_language_code),
+      snippets_fetcher_(std::move(snippets_fetcher)),
+      weak_ptr_factory_(this) {
+  snippets_fetcher_callback_ = snippets_fetcher_->AddCallback(
+      base::Bind(&NTPSnippetsService::OnSnippetsDownloaded,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
 
 NTPSnippetsService::~NTPSnippetsService() {}
 
@@ -21,6 +44,10 @@ void NTPSnippetsService::Shutdown() {
   loaded_ = false;
 }
 
+void NTPSnippetsService::FetchSnippets(bool overwrite) {
+  snippets_fetcher_->FetchSnippets(overwrite);
+}
+
 void NTPSnippetsService::AddObserver(NTPSnippetsServiceObserver* observer) {
   observers_.AddObserver(observer);
   if (loaded_)
@@ -31,6 +58,10 @@ void NTPSnippetsService::RemoveObserver(NTPSnippetsServiceObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
+void NTPSnippetsService::OnFileReadDone(const std::string& json) {
+  LoadFromJSONString(json);
+}
+
 bool NTPSnippetsService::LoadFromJSONString(const std::string& str) {
   JSONStringValueDeserializer deserializer(str);
   int error_code;
@@ -57,16 +88,26 @@ bool NTPSnippetsService::LoadFromJSONString(const std::string& str) {
     const base::DictionaryValue* content = NULL;
     if (!dict->GetDictionary("contentInfo", &content))
       return false;
-    std::unique_ptr<NTPSnippet> snippet =
+    scoped_ptr<NTPSnippet> snippet =
         NTPSnippet::NTPSnippetFromDictionary(*content);
     if (!snippet)
       return false;
     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