provide static popular sites for first run

components/ntp_tiles/popular_sites_impl.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_tiles/popular_sites_impl.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
 #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"
 #include "base/time/time.h"
 #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"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/search_engine_type.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/variations/service/variations_service.h"
 #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"
 #endif
 
 using net::URLFetcher;
 using variations::VariationsService;
 
 namespace ntp_tiles {
 
@@ skipping 76 matching lines @@
     item->GetString("thumbnail_url", &thumbnail_url);
     std::string large_icon_url;
     item->GetString("large_icon_url", &large_icon_url);
 
     sites.emplace_back(title, GURL(url), GURL(favicon_url),
                        GURL(large_icon_url), GURL(thumbnail_url));
   }
   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,
                          const GURL& url,
                          const GURL& favicon_url,
                          const GURL& large_icon_url,
                          const GURL& thumbnail_url)
     : title(title),
       url(url),
       favicon_url(favicon_url),
@@ skipping 31 matching lines @@
   }
 }
 
 PopularSitesImpl::~PopularSitesImpl() {}
 
 bool PopularSitesImpl::MaybeStartFetch(bool force_download,
                                        const FinishedCallback& callback) {
   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 =
       base::Time::Now() - last_download_time;
   const base::TimeDelta redownload_interval =
       base::TimeDelta::FromHours(kPopularSitesRedownloadIntervalHours);
   const bool download_time_is_future = base::Time::Now() < last_download_time;
 
   pending_url_ = GetURLToFetch();
   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 {
   return sites_;
 }
 
 GURL PopularSitesImpl::GetLastURLFetched() const {
   return GURL(prefs_->GetString(kPopularSitesURLPref));
 }
 
@@ skipping 64 matching lines @@
     user_prefs::PrefRegistrySyncable* user_prefs) {
   user_prefs->RegisterStringPref(ntp_tiles::prefs::kPopularSitesOverrideURL,
                                  std::string());
   user_prefs->RegisterStringPref(ntp_tiles::prefs::kPopularSitesOverrideCountry,
                                  std::string());
   user_prefs->RegisterStringPref(ntp_tiles::prefs::kPopularSitesOverrideVersion,
                                  std::string());
 
   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() {
   fetcher_ = URLFetcher::Create(pending_url_, URLFetcher::GET, this);
   data_use_measurement::DataUseUserData::AttachToFetcher(
       fetcher_.get(), data_use_measurement::DataUseUserData::NTP_TILES);
   fetcher_->SetRequestContext(download_context_);
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                          net::LOAD_DO_NOT_SAVE_COOKIES);
   fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
@@ skipping 48 matching lines @@
     pending_url_ = GetPopularSitesURL(kPopularSitesDefaultCountryCode,
                                       kPopularSitesDefaultVersion);
     FetchPopularSites();
   } else {
     DLOG(WARNING) << "Download fallback site list failed";
     callback_.Run(false);
   }
 }
 
 }  // namespace ntp_tiles