Remove PopularSites' dependencies on //chrome/....

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 e33ac30575519c74a3e9d6d095d7f0b6bf8b96df..f6a9acebe6360e20f543fc609d4fca2883a94ecf 100644
--- a/chrome/browser/android/ntp/popular_sites.cc
+++ b/chrome/browser/android/ntp/popular_sites.cc
@@ -11,25 +11,27 @@
 #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/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
-#include "chrome/common/chrome_paths.h"
 #include "components/google/core/browser/google_util.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/safe_json/json_sanitizer.h"
 #include "components/search_engines/search_engine_type.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/variations/service/variations_service.h"
 #include "content/public/browser/browser_thread.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_status_code.h"
 
 using content::BrowserThread;
+using net::URLFetcher;
 using variations::VariationsService;
 
 namespace {
@@ -123,20 +125,8 @@ std::string GetVersionToUse(const PrefService* prefs,
   return version;
 }
 
-base::FilePath GetPopularSitesPath() {
-  base::FilePath dir;
-  PathService::Get(chrome::DIR_USER_DATA, &dir);
-  return dir.AppendASCII(kPopularSitesLocalFilename);
-}
-
-std::unique_ptr<std::vector<PopularSites::Site>> ReadAndParseJsonFile(
-    const base::FilePath& path) {
-  std::string json;
-  if (!base::ReadFileToString(path, &json)) {
-    DLOG(WARNING) << "Failed reading file";
-    return nullptr;
-  }
-
+std::unique_ptr<std::vector<PopularSites::Site>> ParseJson(
+    const std::string& json) {
   std::unique_ptr<base::Value> value =
       base::JSONReader::Read(json, base::JSON_ALLOW_TRAILING_COMMAS);
   base::ListValue* list;
@@ -170,6 +160,32 @@ std::unique_ptr<std::vector<PopularSites::Site>> ReadAndParseJsonFile(
   return sites;
 }
 
+// Write |data| to a temporary file, then move it to |path|, thereby ensuring
+// that a partial file cannot be written.
+//
+// TODO(sfiera): is there some helper for this somewhere?
+bool WriteStringToFileAtomic(const std::string& data,
+                             const base::FilePath& path) {
+  base::FilePath temporary_path;
+  return base::CreateTemporaryFile(&temporary_path) &&
+         base::WriteFile(temporary_path, data.data(), data.size()) &&
+         base::Move(temporary_path, path);
+}
+
+// Run |call| on the thread pool for blocking file operations, then call |done|
+// with its result on the original thread.
+template <typename T>
+void RunBlockingTaskAndReply(const tracked_objects::Location& from_here,
+                             const base::Callback<T()>& call,
+                             const base::Callback<void(T)>& done) {
+  base::PostTaskAndReplyWithResult(
+      BrowserThread::GetBlockingPool()
+          ->GetTaskRunnerWithShutdownBehavior(
+              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
+          .get(),
+      from_here, call, done);
+}
+
 }  // namespace
 
 PopularSites::Site::Site(const base::string16& title,
@@ -191,6 +207,7 @@ PopularSites::PopularSites(PrefService* prefs,
                            const TemplateURLService* template_url_service,
                            VariationsService* variations_service,
                            net::URLRequestContextGetter* download_context,
+                           const base::FilePath& directory,
                            const std::string& variation_param_country,
                            const std::string& variation_param_version,
                            bool force_download,
@@ -198,6 +215,7 @@ PopularSites::PopularSites(PrefService* prefs,
     : PopularSites(prefs,
                    template_url_service,
                    download_context,
+                   directory,
                    GetCountryToUse(prefs,
                                    template_url_service,
                                    variations_service,
@@ -210,11 +228,13 @@ PopularSites::PopularSites(PrefService* prefs,
 PopularSites::PopularSites(PrefService* prefs,
                            const TemplateURLService* template_url_service,
                            net::URLRequestContextGetter* download_context,
+                           const base::FilePath& directory,
                            const GURL& url,
                            const FinishedCallback& callback)
     : PopularSites(prefs,
                    template_url_service,
                    download_context,
+                   directory,
                    std::string(),
                    std::string(),
                    url,
@@ -247,19 +267,24 @@ void PopularSites::RegisterProfilePrefs(
 PopularSites::PopularSites(PrefService* prefs,
                            const TemplateURLService* template_url_service,
                            net::URLRequestContextGetter* download_context,
+                           const base::FilePath& directory,
                            const std::string& country,
                            const std::string& version,
                            const GURL& override_url,
                            bool force_download,
                            const FinishedCallback& callback)
     : callback_(callback),
+      is_fallback_(false),
       pending_country_(country),
       pending_version_(version),
-      local_path_(GetPopularSitesPath()),
+      local_path_(directory.empty()
+                      ? base::FilePath()
+                      : directory.AppendASCII(kPopularSitesLocalFilename)),
       prefs_(prefs),
       template_url_service_(template_url_service),
       download_context_(download_context),
       weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   const base::Time last_download_time = base::Time::FromInternalValue(
       prefs_->GetInt64(kPopularSitesLastDownloadPref));
   const base::TimeDelta time_since_last_download =
@@ -270,14 +295,31 @@ PopularSites::PopularSites(PrefService* prefs,
   const bool country_changed = GetCountry() != pending_country_;
   const bool version_changed = GetVersion() != pending_version_;
 
-  const bool should_redownload_if_exists =
-      force_download || download_time_is_future ||
+  const GURL url =
+      override_url.is_valid() ? override_url : GetPopularSitesURL();
+
+  // No valid path to save to. Immediately post failure.
+  if (local_path_.empty()) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(callback_, false));
+    return;
+  }
+
+  // Download forced, or we need to download a new file.
+  if (force_download || download_time_is_future ||
       (time_since_last_download > redownload_interval) || country_changed ||
-      version_changed;
+      version_changed) {
+    FetchPopularSites(url);
+    return;
+  }
 
-  FetchPopularSites(
-      override_url.is_valid() ? override_url : GetPopularSitesURL(),
-      should_redownload_if_exists, false /* is_fallback */);
+  std::unique_ptr<std::string> file_data(new std::string);
+  std::string* file_data_ptr = file_data.get();
+  RunBlockingTaskAndReply(
+      FROM_HERE,
+      base::Bind(&base::ReadFileToString, local_path_, file_data_ptr),
+      base::Bind(&PopularSites::OnReadFileDone, weak_ptr_factory_.GetWeakPtr(),
+                 url, base::Passed(std::move(file_data))));

[Marc Treib, 2016/05/11 10:20:29]

There's also base::Owned which takes ownership of a raw pointer, and is arguably
a bit more convenient. Not sure which way I'd prefer, it's kinda icky either
way.

[sfiera, 2016/05/11 14:23:53]

I like the current way because a reader of OnReadFileDone() doesn't have to
wonder about the lifetime of the pointer.

[Marc Treib, 2016/05/11 14:47:25]

Acknowledged.

 }
 
 GURL PopularSites::GetPopularSitesURL() const {
@@ -286,53 +328,89 @@ GURL PopularSites::GetPopularSitesURL() const {
                                  pending_version_.c_str()));
 }
 
-void PopularSites::FetchPopularSites(const GURL& url,
-                                     bool force_download,
-                                     bool is_fallback) {
-  downloader_.reset(new FileDownloader(
-      url, local_path_, force_download, download_context_,
-      base::Bind(&PopularSites::OnDownloadDone, base::Unretained(this),
-                 is_fallback)));
+void PopularSites::FetchPopularSites(const GURL& url) {
+  fetcher_ = URLFetcher::Create(url, URLFetcher::GET, this);
+  fetcher_->SetRequestContext(download_context_);
+  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                         net::LOAD_DO_NOT_SAVE_COOKIES);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
+  fetcher_->Start();
 }
 
-void PopularSites::OnDownloadDone(bool is_fallback,
-                                  FileDownloader::Result result) {
-  downloader_.reset();
-  switch (result) {
-    case FileDownloader::DOWNLOADED:
-      prefs_->SetInt64(kPopularSitesLastDownloadPref,
-                      base::Time::Now().ToInternalValue());
-      prefs_->SetString(kPopularSitesCountryPref, pending_country_);
-      prefs_->SetString(kPopularSitesVersionPref, pending_version_);
-      ParseSiteList(local_path_);
-      break;
-    case FileDownloader::EXISTS:
-      ParseSiteList(local_path_);
-      break;
-    case FileDownloader::FAILED:
-      if (!is_fallback) {
-        DLOG(WARNING) << "Download country site list failed";
-        pending_country_ = kPopularSitesDefaultCountryCode;
-        pending_version_ = kPopularSitesDefaultVersion;
-        // It is fine to force the download as Fallback is only triggered after
-        // a failed download.
-        FetchPopularSites(GetPopularSitesURL(), true /* force_download */,
-                          true /* is_fallback */);
-      } else {
-        DLOG(WARNING) << "Download fallback site list failed";
-        callback_.Run(false);
-      }
-      break;
+void PopularSites::OnDownloadFailed() {
+  if (!is_fallback_) {
+    DLOG(WARNING) << "Download country site list failed";
+    is_fallback_ = true;
+    pending_country_ = kPopularSitesDefaultCountryCode;
+    pending_version_ = kPopularSitesDefaultVersion;
+    // It is fine to force the download as Fallback is only triggered after
+    // a failed download.

[Marc Treib, 2016/05/11 10:20:29]

This comment is kinda obsolete now.

[sfiera, 2016/05/11 14:23:53]

Done.

+    FetchPopularSites(GetPopularSitesURL());
+  } else {
+    DLOG(WARNING) << "Download fallback site list failed";
+    callback_.Run(false);
   }
 }
 
-void PopularSites::ParseSiteList(const base::FilePath& path) {
-  base::PostTaskAndReplyWithResult(
-      BrowserThread::GetBlockingPool()
-          ->GetTaskRunnerWithShutdownBehavior(
-              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
-          .get(),
-      FROM_HERE, base::Bind(&ReadAndParseJsonFile, path),
+void PopularSites::OnReadFileDone(const GURL& url,
+                                  std::unique_ptr<std::string> data,
+                                  bool success) {
+  if (success) {
+    ParseSiteList(*data);
+  } else {
+    FetchPopularSites(url);

[Marc Treib, 2016/05/11 10:20:29]

Add a comment? I guess this usually means the file didn't exist?

[sfiera, 2016/05/11 14:23:53]

Done.

+  }
+}
+
+void PopularSites::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_EQ(fetcher_.get(), source);
+  fetcher_.reset();

[Marc Treib, 2016/05/11 10:20:29]

I think this may be why stuff fails: This deletes source, which you use later.
One pattern to solve this is to have a local unique_ptr<URLFetcher> and move
fetcher_ in there.

[sfiera, 2016/05/11 11:06:02]

Oh yeah, obviously. Now I wonder why it doesn't fail locally...

I know there are ways to build like is_asan to check things like this. Are there
others and do they generally work on Android?

[Marc Treib, 2016/05/11 11:35:28]

I think there's a few more, maybe "is_msan" or something. It's been a while
since I used any of those locally. Not sure if they work on Android.
I'd recommend just running the corresponding trybots when you're worried about
something. Doing this locally was rather painful.

[sfiera, 2016/05/11 14:23:53]

Acknowledged.

+
+  std::string sketchy_json;
+  if (!(source->GetStatus().is_success() &&
+        source->GetResponseCode() == net::HTTP_OK &&
+        source->GetResponseAsString(&sketchy_json))) {
+    OnDownloadFailed();
+    return;
+  }
+
+  safe_json::JsonSanitizer::Sanitize(
+      sketchy_json, base::Bind(&PopularSites::OnJsonSanitized,
+                               weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&PopularSites::OnJsonSanitizationFailed,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PopularSites::OnJsonSanitized(const std::string& valid_minified_json) {
+  RunBlockingTaskAndReply(
+      FROM_HERE,
+      base::Bind(&WriteStringToFileAtomic, valid_minified_json, local_path_),
+      base::Bind(&PopularSites::OnFileWriteDone, weak_ptr_factory_.GetWeakPtr(),
+                 valid_minified_json));
+}
+
+void PopularSites::OnJsonSanitizationFailed(const std::string& error_message) {
+  DLOG(WARNING) << "JSON sanitization failed: " << error_message;
+  OnDownloadFailed();
+}
+
+void PopularSites::OnFileWriteDone(const std::string& json, bool success) {
+  if (success) {
+    prefs_->SetInt64(kPopularSitesLastDownloadPref,
+                     base::Time::Now().ToInternalValue());
+    prefs_->SetString(kPopularSitesCountryPref, pending_country_);
+    prefs_->SetString(kPopularSitesVersionPref, pending_version_);
+    ParseSiteList(json);
+  } else {
+    DLOG(WARNING) << "Could not write file to "
+                  << local_path_.LossyDisplayName();
+    OnDownloadFailed();
+  }
+}
+
+void PopularSites::ParseSiteList(const std::string& json) {
+  RunBlockingTaskAndReply(
+      FROM_HERE, base::Bind(&ParseJson, json),
       base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
 }