Display status message for "Add snippets" on chrome://snippets-internals

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 781604d83829cd94d44a3f2bbc93004931897e8c..654a20ff1e61dc57bd2036314ff6db3904b1ef19 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -222,8 +222,12 @@ void NTPSnippetsService::FetchSnippetsFromHosts(
     snippets_fetcher_->FetchSnippets(std::set<std::string>());
     return;
   }
-  if (!hosts.empty())
+  if (!hosts.empty()) {
     snippets_fetcher_->FetchSnippets(hosts);
+  } else {
+    snippets_fetcher_last_status_ = "Cannot fetch for empty hosts list.";

[Bernhard Bauer, 2016/04/21 16:28:41]

Can you extract these into constants and move to the top of the file?

[jkrcal, 2016/04/22 09:30:28]

Done.

What is the reason (apart from having English messages in one place)? Is it okay
to leave LOG and DLOG messages as literals? If so, why?

[Bernhard Bauer, 2016/04/22 09:42:17]

It's mostly that the status messages are part of the observable interface of the
class, whereas log messages are only for developers. Even now it feels a bit
strange to me to expose English messages as opposed to e.g. status codes, but if
it's just meant to be shown in the debugging page, I guess that's okay.

[jkrcal, 2016/04/22 11:11:59]

Thanks for explaining!

+    LoadingSnippetsFinished();
+  }
 }
 
 void NTPSnippetsService::RescheduleFetching() {
@@ -314,7 +318,9 @@ void NTPSnippetsService::OnSuggestionsChanged(
 }
 
 void NTPSnippetsService::OnSnippetsDownloaded(
-    const std::string& snippets_json) {
+    const std::string& snippets_json, const std::string& status) {
+  snippets_fetcher_last_status_ = status;

[Marc Treib, 2016/04/21 13:47:20]

Now we also get here if the download failed (and so snippets_json is empty),
right? In that case, there's no point in calling the parser, just call
LoadingSnippetsFinished instead.

[jkrcal, 2016/04/22 09:30:28]

Done.

Good point, thanks! This is something I've totally overlooked.

+
   parse_json_callback_.Run(
       snippets_json, base::Bind(&NTPSnippetsService::OnJsonParsed,
                                 weak_ptr_factory_.GetWeakPtr(), snippets_json),
@@ -324,13 +330,22 @@ void NTPSnippetsService::OnSnippetsDownloaded(
 
 void NTPSnippetsService::OnJsonParsed(const std::string& snippets_json,
                                       scoped_ptr<base::Value> parsed) {
-  LOG_IF(WARNING, !LoadFromValue(*parsed)) << "Received invalid snippets: "
-                                           << snippets_json;
+  bool success = LoadFromValue(*parsed);

[Marc Treib, 2016/04/21 13:47:20]

No need for the "success" variable, just say if(LoadFromValue(..))

[jkrcal, 2016/04/22 09:30:28]

Done.

+  if (!success) {
+    LOG(WARNING) << "Received invalid snippets: " << snippets_json;
+    snippets_fetcher_last_status_ = "Invalid / empty list.";
+  } else {
+    snippets_fetcher_last_status_ = "OK";
+  }
+
+  LoadingSnippetsFinished();
 }
 
 void NTPSnippetsService::OnJsonError(const std::string& snippets_json,
                                      const std::string& error) {
   LOG(WARNING) << "Received invalid JSON (" << error << "): " << snippets_json;

[Marc Treib, 2016/04/21 13:47:20]

Set snippets_fetcher_last_status_ to something meaningful here.

[jkrcal, 2016/04/22 09:30:28]

Done.

+
+  LoadingSnippetsFinished();
 }
 
 bool NTPSnippetsService::LoadFromValue(const base::Value& value) {
@@ -376,16 +391,14 @@ bool NTPSnippetsService::LoadFromListValue(const base::ListValue& list) {
       snippets_.push_back(std::move(snippet));
   }
 
-  // Immediately remove any already-expired snippets. This will also notify our
-  // observers and schedule the expiry timer.
-  RemoveExpiredSnippets();
-
   return true;
 }
 
 void NTPSnippetsService::LoadSnippetsFromPrefs() {
   bool success = LoadFromListValue(*pref_service_->GetList(prefs::kSnippets));
   DCHECK(success) << "Failed to parse snippets from prefs";
+
+  LoadingSnippetsFinished();
 }
 
 void NTPSnippetsService::StoreSnippetsToPrefs() {
@@ -433,7 +446,8 @@ bool NTPSnippetsService::HasDiscardedSnippet(const GURL& url) const {
   return it != discarded_snippets_.end();
 }
 
-void NTPSnippetsService::RemoveExpiredSnippets() {
+void NTPSnippetsService::LoadingSnippetsFinished() {
+  // Remove expired snippets.
   base::Time expiry = base::Time::Now();
 
   snippets_.erase(
@@ -470,7 +484,7 @@ void NTPSnippetsService::RemoveExpiredSnippets() {
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
-                      base::Bind(&NTPSnippetsService::RemoveExpiredSnippets,
+                      base::Bind(&NTPSnippetsService::LoadingSnippetsFinished,
                                  base::Unretained(this)));
 }