[NTP Popular Sites] Store country/version in prefs

chrome/browser/android/ntp/popular_sites.cc

Index: chrome/browser/android/ntp/popular_sites.cc
diff --git a/chrome/browser/android/ntp/popular_sites.cc b/chrome/browser/android/ntp/popular_sites.cc
index d4582423273f83758f66ac519a1a0d47f8ee5778..2d6bfd1752515535d4a438496723ed5abc1377cc 100644
--- a/chrome/browser/android/ntp/popular_sites.cc
+++ b/chrome/browser/android/ntp/popular_sites.cc
@@ -1,4 +1,4 @@
-// Copyright 2013 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -18,7 +18,6 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/net/file_downloader.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/chrome_paths.h"
@@ -36,14 +35,16 @@ using content::BrowserThread;
 
 namespace {
 
-const char kPopularSitesURLFormat[] = "https://www.gstatic.com/chrome/ntp/%s";
-const char kPopularSitesServerFilenameFormat[] = "suggested_sites_%s_%s.json";
+const char kPopularSitesURLFormat[] =
+    "https://www.gstatic.com/chrome/ntp/suggested_sites_%s_%s.json";
 const char kPopularSitesDefaultCountryCode[] = "DEFAULT";
 const char kPopularSitesDefaultVersion[] = "5";
 const char kPopularSitesLocalFilename[] = "suggested_sites.json";
 const int kPopularSitesRedownloadIntervalHours = 24;
 
 const char kPopularSitesLastDownloadPref[] = "popular_sites_last_download";
+const char kPopularSitesCountryPref[] = "popular_sites_country";
+const char kPopularSitesVersionPref[] = "popular_sites_version";
 
 // Extract the country from the default search engine if the default search
 // engine is Google.
@@ -92,7 +93,11 @@ std::string GetVariationsServiceCountry() {
 // Google is the default search engine set. If Google is not the default search
 // engine use the country provided by VariationsService. Fallback to a default
 // if we can't make an educated guess.
-std::string GetCountryCode(Profile* profile) {
+std::string GetCountryCode(Profile* profile,
+                           const std::string& override_country) {
+  if (!override_country.empty())
+    return override_country;
+
   std::string country_code = GetDefaultSearchEngineCountryCode(profile);
 
   if (country_code.empty())
@@ -104,30 +109,16 @@ std::string GetCountryCode(Profile* profile) {
   return base::ToUpperASCII(country_code);
 }
 
-std::string GetPopularSitesServerFilename(
-    Profile* profile,
-    const std::string& override_country,
-    const std::string& override_version) {
-  std::string country =
-      !override_country.empty() ? override_country : GetCountryCode(profile);
-  std::string version = !override_version.empty() ? override_version
-                                                  : kPopularSitesDefaultVersion;
-  return base::StringPrintf(kPopularSitesServerFilenameFormat,
-                            country.c_str(), version.c_str());
-}
-
-GURL GetPopularSitesURL(Profile* profile,
-                        const std::string& override_country,
-                        const std::string& override_version) {
-  return GURL(base::StringPrintf(
-      kPopularSitesURLFormat,
-      GetPopularSitesServerFilename(profile, override_country, override_version)
-          .c_str()));
+std::string GetVersion(const std::string& override_version) {
+  if (!override_version.empty())
+    return override_version;
+  return kPopularSitesDefaultVersion;
 }
 
-GURL GetPopularSitesFallbackURL(Profile* profile) {
-  return GetPopularSitesURL(profile, kPopularSitesDefaultCountryCode,
-                            kPopularSitesDefaultVersion);
+GURL GetPopularSitesURL(const std::string& country,
+                        const std::string& version) {
+  return GURL(base::StringPrintf(kPopularSitesURLFormat, country.c_str(),
+                                 version.c_str()));
 }
 
 base::FilePath GetPopularSitesPath() {
@@ -198,15 +189,17 @@ PopularSites::PopularSites(Profile* profile,
                            bool force_download,
                            const FinishedCallback& callback)
     : PopularSites(profile,
-                   GetPopularSitesURL(profile, override_country,
-                                      override_version),
+                   GetCountryCode(profile, override_country),
+                   GetVersion(override_version),
+                   GURL(),
                    force_download,
                    callback) {}
 
 PopularSites::PopularSites(Profile* profile,
                            const GURL& url,
                            const FinishedCallback& callback)
-    : PopularSites(profile, url, true, callback) {}
+    : PopularSites(profile, std::string(), std::string(), url, true, callback) {
+}
 
 PopularSites::~PopularSites() {}
 
@@ -214,14 +207,20 @@ PopularSites::~PopularSites() {}
 void PopularSites::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* user_prefs) {
   user_prefs->RegisterInt64Pref(kPopularSitesLastDownloadPref, 0);
+  user_prefs->RegisterStringPref(kPopularSitesCountryPref, std::string());
+  user_prefs->RegisterStringPref(kPopularSitesVersionPref, std::string());
 }
 
 PopularSites::PopularSites(Profile* profile,
-                           const GURL& url,
+                           const std::string& country,
+                           const std::string& version,
+                           const GURL& override_url,
                            bool force_download,
                            const FinishedCallback& callback)
     : callback_(callback),
-      popular_sites_local_path_(GetPopularSitesPath()),
+      country_(country),
+      version_(version),
+      local_path_(GetPopularSitesPath()),
       profile_(profile),
       weak_ptr_factory_(this) {
   const base::Time last_download_time = base::Time::FromInternalValue(
@@ -235,31 +234,46 @@ PopularSites::PopularSites(Profile* profile,
   const bool should_redownload_if_exists =
       force_download || download_time_is_future ||
       (time_since_last_download > redownload_interval);
+  // TODO(treib): If country/version don't match what we have, we should
+  // probably also redownload?
 
-  FetchPopularSites(url, should_redownload_if_exists, false /* is_fallback */);
+  FetchPopularSites(override_url.is_valid()
+                        ? override_url
+                        : GetPopularSitesURL(country_, version_),
+                    should_redownload_if_exists, false /* is_fallback */);
 }
 
 void PopularSites::FetchPopularSites(const GURL& url,
                                      bool force_download,
                                      bool is_fallback) {
-  downloader_.reset(
-      new FileDownloader(url, popular_sites_local_path_, force_download,
-                         profile_->GetRequestContext(),
-                         base::Bind(&PopularSites::OnDownloadDone,
-                                    base::Unretained(this), is_fallback)));
+  downloader_.reset(new FileDownloader(
+      url, local_path_, force_download, profile_->GetRequestContext(),
+      base::Bind(&PopularSites::OnDownloadDone, base::Unretained(this),
+                 is_fallback)));
 }
 
-void PopularSites::OnDownloadDone(bool is_fallback, bool success) {
+void PopularSites::OnDownloadDone(bool is_fallback,
+                                  FileDownloader::Result result) {
   downloader_.reset();
-  if (success) {
-    profile_->GetPrefs()->SetInt64(kPopularSitesLastDownloadPref,
-                                   base::Time::Now().ToInternalValue());
-    ParseSiteList(popular_sites_local_path_);
+  if (FileDownloader::IsSuccess(result)) {

[Bernhard Bauer, 2016/04/01 15:45:11]

I might be more straight-forward to switch on the result value instead of having
this check and another one in line 261.

[Marc Treib, 2016/04/01 16:33:28]

Eh... sure. Done.

+    PrefService* prefs = profile_->GetPrefs();
+    if (result == FileDownloader::DOWNLOADED) {
+      prefs->SetInt64(kPopularSitesLastDownloadPref,
+                      base::Time::Now().ToInternalValue());
+      prefs->SetString(kPopularSitesCountryPref, country_);
+      prefs->SetString(kPopularSitesVersionPref, version_);
+    } else {
+      country_ = prefs->GetString(kPopularSitesCountryPref);

[Bernhard Bauer, 2016/04/01 15:49:47]

Why do we have to do this?

[Marc Treib, 2016/04/01 16:33:28]

Added a comment: If we loaded an existing file, then its country/version might
not match what |country_| and |version_| say, so load the correct values.
(In that case, we should probably have re-downloaded, see the TODO I added
elsewhere. But I'm leaving that for another CL.)

[Bernhard Bauer, 2016/04/01 17:40:48]

Oof. So, |country_| and |version_| contain the values for the last downloaded
file, unless we're currently trying to download a new one, in which case they
contain the new ones? I'm starting to see what you mean with the other comment,
but this just seems confusing.

Thinking about it, don't we have the correct values in prefs anyway? Could we
have the accessors return those values, rename the members to
|pending_country_|/|pending_version_| and clear them when we're not trying to
download a file? Or am I missing something?

[Marc Treib, 2016/04/06 11:39:38]

Done.

+      version_ = prefs->GetString(kPopularSitesVersionPref);
+    }
+    ParseSiteList(local_path_);
   } else if (!is_fallback) {
     DLOG(WARNING) << "Download country site list failed";
     // It is fine to force the download as Fallback is only triggered after a
     // failed download.
-    FetchPopularSites(GetPopularSitesFallbackURL(profile_),
+    country_ = kPopularSitesDefaultCountryCode;
+    version_ = kPopularSitesDefaultVersion;
+    FetchPopularSites(GetPopularSitesURL(country_, version_),
                       true /* force_download */, true /* is_fallback */);
   } else {
     DLOG(WARNING) << "Download fallback site list failed";