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..a0c7f52c2c4efa5db38abc87a7085ca17c5905bb 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>> ParseJsonFile(

[Marc Treib, 2016/05/10 08:32:43]

Just ParseJson now? Not a file anymore at this point.

[sfiera, 2016/05/11 09:00:24]

Done.

+    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,18 @@ 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?

[Marc Treib, 2016/05/10 08:32:43]

Not that I'm aware.

[sfiera, 2016/05/11 09:00:24]

Well.

+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);
+}
+
 }  // namespace
 
 PopularSites::Site::Site(const base::string16& title,
@@ -191,6 +193,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 +201,7 @@ PopularSites::PopularSites(PrefService* prefs,
     : PopularSites(prefs,
                    template_url_service,
                    download_context,
+                   directory,
                    GetCountryToUse(prefs,
                                    template_url_service,
                                    variations_service,
@@ -210,11 +214,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,6 +253,7 @@ 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,
@@ -255,7 +262,9 @@ PopularSites::PopularSites(PrefService* prefs,
     : callback_(callback),
       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),
@@ -275,9 +284,18 @@ PopularSites::PopularSites(PrefService* prefs,
       (time_since_last_download > redownload_interval) || country_changed ||
       version_changed;
 
+  if (local_path_.empty()) {
+    BrowserThread::GetBlockingPool()
+        ->GetTaskRunnerWithShutdownBehavior(
+            base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
+        ->PostTask(FROM_HERE, base::Bind(callback_, false));

[Marc Treib, 2016/05/10 08:32:43]

Why are you posting the callback to another thread? That's bound to mess up the
client.
In fact, why not just Run() it directly?

[sfiera, 2016/05/11 09:00:24]

I had thought that it was supposed to run on the blocking pool, but it's not.

I've kept the behavior of posting the callback, except now to the UI thread. I
want to keep the behavior the same in all cases (run callback after call
returns) because changing it only in an exceptional case seems like it could
harbor bugs.

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

Acknowledged.

+    return;
+  }
+
+  is_fallback_ = false;

[Marc Treib, 2016/05/10 08:32:42]

Initialize this in the initializer list above? I dislike uninitialized
variables, even if it's only for a very short time here.

[sfiera, 2016/05/11 09:00:24]

Done.

   FetchPopularSites(
       override_url.is_valid() ? override_url : GetPopularSitesURL(),
-      should_redownload_if_exists, false /* is_fallback */);
+      should_redownload_if_exists);
 }
 
 GURL PopularSites::GetPopularSitesURL() const {
@@ -286,53 +304,117 @@ 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::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::FetchPopularSites(const GURL& url, bool force_download) {

[Marc Treib, 2016/05/10 08:32:42]

Split this into two methods, since the force vs non-force cases are completely
different? Or maybe the non-force case should be inlined since it only has one
call site (as far as I can see)?

[sfiera, 2016/05/11 09:00:24]

I've inlined it above and kept only the actual fetch.

+  if (force_download) {
+    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();
+  } else {
+    base::PostTaskAndReplyWithResult(
+        BrowserThread::GetBlockingPool()
+            ->GetTaskRunnerWithShutdownBehavior(
+                base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
+            .get(),

[Marc Treib, 2016/05/10 08:32:43]

IMO these four lines are now repeated often enough to warrant a helper function
- WDYT?

[sfiera, 2016/05/11 09:00:24]

Did one further.

+        FROM_HERE, base::Bind(&base::PathExists, local_path_),
+        base::Bind(&PopularSites::OnFileExistsCheckDone,
+                   weak_ptr_factory_.GetWeakPtr(), url));
   }
 }
 
-void PopularSites::ParseSiteList(const base::FilePath& path) {
+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.
+    FetchPopularSites(GetPopularSitesURL(), true /* force_download */);
+  } else {
+    DLOG(WARNING) << "Download fallback site list failed";
+    callback_.Run(false);
+  }
+}
+
+void PopularSites::OnFileExistsCheckDone(const GURL& url, bool exists) {
+  if (exists) {
+    std::string json;
+    if (base::ReadFileToString(local_path_, &json)) {

[Marc Treib, 2016/05/10 08:32:43]

This runs on the UI thread, right? That thread isn't allowed to do file I/O.
Maybe make a helper that does both the FileExists check and (if it exists) the
read, and post that to another thread?
(Actually, you should hit a DCHECK when you try to do this. Have you run this?
Unit tests probably wouldn't catch it, even if we had any...)

[sfiera, 2016/05/10 08:44:51]

I should have said up front: I have no real idea what is correct w.r.t.
threading. I know not to do file IO on the UI thread, but I have no idea how to
tell when we're on the UI thread.

Does base::PostTaskAndReplyWithResult() run the second callback in a different
thread pool than the first? The docs for it and TaskRunner are not helpful on
this point.

There's a failure in the trybots, but I haven't diagnosed it yet. Certainly
could be this; it happens in dbg and not rel.

[Marc Treib, 2016/05/10 09:04:07]

Rule of thumb: We're on the UI thread. :D
DCHECK_CURRENTLY_ON might help.
Yes, the second callback is posted back to the original calling thread. I agree
that the documentation could be better. It's there somewhere, but we have so
many Runners and Loops and helpers and wrappers, and the comments aren't
replicated everywhere...
For this case:
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner.h...
All the trybots should have DCHECKs enabled, even the release bots. (In fact, I
recommend you do the same, if you usually use release builds - add
"dcheck_always_on = true" to your gn args.)

[sfiera, 2016/05/11 09:00:24]

Added it to the constructor. Should I add a comment to the header, or is it just
assumed?
I've redone this file to have proper threading, I think.

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

Adding a comment to the header is definitely appreciated, especially when this
moves to a component.

+      ParseSiteList(json);
+    } else {
+      // TODO(sfiera): remove and retry?
+      DLOG(WARNING) << "Failed reading file";
+      callback_.Run(false);
+    }
+  } else {
+    FetchPopularSites(url, true /* force_download */);
+  }
+}
+
+void PopularSites::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_EQ(fetcher_.get(), source);
+  fetcher_.reset();
+
+  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) {
+  base::PostTaskAndReplyWithResult(
+      BrowserThread::GetBlockingPool()
+          ->GetTaskRunnerWithShutdownBehavior(
+              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
+          .get(),
+      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) {
+    OnHaveNewFile(json);
+  } else {
+    DLOG(WARNING) << "could not write file to "

[Marc Treib, 2016/05/10 08:32:43]

nitty nit: Start with a capital letter

[sfiera, 2016/05/11 09:00:24]

Done.

+                  << local_path_.LossyDisplayName();
+    OnDownloadFailed();
+  }
+}
+
+void PopularSites::OnHaveNewFile(const std::string& json) {
+  prefs_->SetInt64(kPopularSitesLastDownloadPref,
+                   base::Time::Now().ToInternalValue());
+  prefs_->SetString(kPopularSitesCountryPref, pending_country_);
+  prefs_->SetString(kPopularSitesVersionPref, pending_version_);
+  ParseSiteList(json);
+}
+
+void PopularSites::ParseSiteList(const std::string& json) {
   base::PostTaskAndReplyWithResult(
       BrowserThread::GetBlockingPool()
           ->GetTaskRunnerWithShutdownBehavior(
               base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
           .get(),
-      FROM_HERE, base::Bind(&ReadAndParseJsonFile, path),
+      FROM_HERE, base::Bind(&ParseJsonFile, json),
       base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
 }