provide static popular sites for first run

components/ntp_tiles/popular_sites_impl.cc

Index: components/ntp_tiles/popular_sites_impl.cc
diff --git a/components/ntp_tiles/popular_sites_impl.cc b/components/ntp_tiles/popular_sites_impl.cc
index ce69227f271e9376aba9c43dd4b9217778926d26..6e9d2c93cc8c5bec885027a5bce373662f4bc41e 100644
--- a/components/ntp_tiles/popular_sites_impl.cc
+++ b/components/ntp_tiles/popular_sites_impl.cc
@@ -11,6 +11,7 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/json/json_reader.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -18,6 +19,7 @@
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/google/core/browser/google_util.h"
+#include "components/grit/components_resources.h"
 #include "components/ntp_tiles/constants.h"
 #include "components/ntp_tiles/pref_names.h"
 #include "components/ntp_tiles/switches.h"
@@ -29,6 +31,7 @@
 #include "components/variations/variations_associated_data.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
+#include "ui/base/resource/resource_bundle.h"
 
 #if defined(OS_IOS)
 #include "components/ntp_tiles/country_code_ios.h"
@@ -125,6 +128,18 @@ PopularSites::SitesVector ParseSiteList(const base::ListValue& list) {
   return sites;
 }
 
+base::ListValue* DefaultPopularSites() {
+  base::JSONReader json_reader;
+  std::unique_ptr<base::Value> value = json_reader.ReadToValue(
+      ResourceBundle::GetSharedInstance().GetRawDataResource(
+          IDR_DEFAULT_POPULAR_SITES_JSON));
+  if (!value) {

[sfiera, 2017/02/09 12:39:06]

ListValue::From() accepts nullptr, returning nullptr in that case, so this could
just be a DCHECK(). I would like to see us DCHECK() that we got a list value
too. You could combine that all into:

std::unique_ptr<base::ListValue> list_value = ListValue::From(value);
DCHECK(list_value);
return list_value;

[fhorschig, 2017/02/09 15:30:18]

This is what I was looking for, thank you!

+    NOTREACHED();
+    return nullptr;
+  }
+  return base::ListValue::From(std::move(value)).release();

[sfiera, 2017/02/09 12:39:06]

Return a std::unique_ptr<> and have the caller release().

[fhorschig, 2017/02/09 15:30:18]

Done.

+}
+
 }  // namespace
 
 PopularSites::Site::Site(const base::string16& title,
@@ -176,6 +191,13 @@ bool PopularSitesImpl::MaybeStartFetch(bool force_download,
   DCHECK(!callback_);
   callback_ = callback;
 
+  const base::ListValue* json = prefs_->GetList(kPopularSitesJsonPref);
+  // Assuming the prefs were cleared, there would be no popular sites.

[sfiera, 2017/02/09 12:39:06]

I don't understand this comment.
 - What do you mean by "Assuming…"? Do you mean "If…"?
 - If the prefs are cleared, then we'd get the default value, right? (which is
not empty)

I think the case of json being null is not one we have to handle, is it?

[fhorschig, 2017/02/09 15:30:18]

Thank you for the clarification, I was assuming the defaults could be 
cleared as well.
In this case, json is never null.

+  if (json) {
+    // Note that we don't run the callback.
+    sites_ = ParseSiteList(*json);

[sfiera, 2017/02/09 12:39:06]

Any reason we wouldn't want to do this in the constructor, or even just in the
site list's accessor?

[fhorschig, 2017/02/09 15:30:18]

Doing this in the constructor sounds good.
Parsing in the accessor would have no advantages. 
Refetching in the accessor would be a pretty huge side effect.

+  }
+
   const base::Time last_download_time = base::Time::FromInternalValue(
       prefs_->GetInt64(kPopularSitesLastDownloadPref));
   const base::TimeDelta time_since_last_download =
@@ -188,23 +210,13 @@ bool PopularSitesImpl::MaybeStartFetch(bool force_download,
   const bool url_changed =
       pending_url_.spec() != prefs_->GetString(kPopularSitesURLPref);
 
-  // Download forced, or we need to download a new file.
-  if (force_download || download_time_is_future ||
+  // Cache empty, download forced, or we need to download a newer file.
+  if (!json || force_download || download_time_is_future ||
       (time_since_last_download > redownload_interval) || url_changed) {
     FetchPopularSites();
     return true;
   }
-
-  const base::ListValue* json = prefs_->GetList(kPopularSitesJsonPref);
-  if (!json) {
-    // Cache didn't exist.
-    FetchPopularSites();
-    return true;
-  } else {
-    // Note that we don't run the callback.
-    sites_ = ParseSiteList(*json);
-    return false;
-  }
+  return false;
 }
 
 const PopularSites::SitesVector& PopularSitesImpl::sites() const {
@@ -289,7 +301,8 @@ void PopularSitesImpl::RegisterProfilePrefs(
 
   user_prefs->RegisterInt64Pref(kPopularSitesLastDownloadPref, 0);
   user_prefs->RegisterStringPref(kPopularSitesURLPref, std::string());
-  user_prefs->RegisterListPref(kPopularSitesJsonPref);
+  // RegisterListPref takes ownership of the default list.
+  user_prefs->RegisterListPref(kPopularSitesJsonPref, DefaultPopularSites());
 }
 
 void PopularSitesImpl::FetchPopularSites() {