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

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 <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 204 matching lines @@
   FetchSnippetsFromHosts(GetSuggestionsHosts());
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDontRestrict)) {
     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() {
   // The scheduler only exists on Android so far, it's null on other platforms.
   if (!scheduler_)
     return;
 
   if (enabled_) {
     base::Time now = base::Time::Now();
     scheduler_->Schedule(
@@ skipping 70 matching lines @@
   StoreSnippetsToPrefs();
   StoreSnippetHostsToPrefs(hosts);
 
   FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
                     NTPSnippetsServiceLoaded());
 
   FetchSnippetsFromHosts(hosts);
 }
 
 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),
       base::Bind(&NTPSnippetsService::OnJsonError,
                  weak_ptr_factory_.GetWeakPtr(), snippets_json));
 }
 
 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) {
   const base::DictionaryValue* top_dict = nullptr;
   if (!value.GetAsDictionary(&top_dict))
     return false;
 
   const base::ListValue* list = nullptr;
   if (!top_dict->GetList("recos", &list))
     return false;
@@ skipping 25 matching lines @@
     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
       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() {
   pref_service_->Set(prefs::kSnippets, *SnippetsToListValue(snippets_));
 }
 
 void NTPSnippetsService::LoadDiscardedSnippetsFromPrefs() {
   discarded_snippets_.clear();
   bool success = AddSnippetsFromListValue(
       *pref_service_->GetList(prefs::kDiscardedSnippets),
@@ skipping 27 matching lines @@
 }
 
 bool NTPSnippetsService::HasDiscardedSnippet(const GURL& url) const {
   auto it = std::find_if(discarded_snippets_.begin(), discarded_snippets_.end(),
                          [&url](const scoped_ptr<NTPSnippet>& snippet) {
                            return snippet->url() == url;
                          });
   return it != discarded_snippets_.end();
 }
 
-void NTPSnippetsService::RemoveExpiredSnippets() {
+void NTPSnippetsService::LoadingSnippetsFinished() {
+  // Remove expired snippets.
   base::Time expiry = base::Time::Now();
 
   snippets_.erase(
       std::remove_if(snippets_.begin(), snippets_.end(),
                      [&expiry](const scoped_ptr<NTPSnippet>& snippet) {
                        return snippet->expiry_date() <= expiry;
                      }),
       snippets_.end());
   StoreSnippetsToPrefs();
 
@@ skipping 16 matching lines @@
   for (const auto& snippet : snippets_) {
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   for (const auto& snippet : discarded_snippets_) {
     if (snippet->expiry_date() < next_expiry)
       next_expiry = snippet->expiry_date();
   }
   DCHECK_GT(next_expiry, expiry);
   expiry_timer_.Start(FROM_HERE, next_expiry - expiry,
-                      base::Bind(&NTPSnippetsService::RemoveExpiredSnippets,
+                      base::Bind(&NTPSnippetsService::LoadingSnippetsFinished,
                                  base::Unretained(this)));
 }
 
 }  // namespace ntp_snippets