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

chrome/browser/android/ntp/popular_sites.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 "chrome/browser/android/ntp/popular_sites.h"
 
 #include <stddef.h>
 
 #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/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 {
 
 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;
@@ skipping 73 matching lines @@
 
   if (version.empty())
     version = variation_param_version;
 
   if (version.empty())
     version = kPopularSitesDefaultVersion;
 
   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;
   if (!value || !value->GetAsList(&list)) {
     DLOG(WARNING) << "Failed parsing json";
     return nullptr;
   }
 
   std::unique_ptr<std::vector<PopularSites::Site>> sites(
       new std::vector<PopularSites::Site>);
@@ skipping 13 matching lines @@
     item->GetString("large_icon_url", &large_icon_url);
 
     sites->push_back(PopularSites::Site(title, GURL(url), GURL(favicon_url),
                                         GURL(large_icon_url),
                                         GURL(thumbnail_url)));
   }
 
   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,
                          const GURL& url,
                          const GURL& favicon_url,
                          const GURL& large_icon_url,
                          const GURL& thumbnail_url)
     : title(title),
       url(url),
       favicon_url(favicon_url),
       large_icon_url(large_icon_url),
       thumbnail_url(thumbnail_url) {}
 
 PopularSites::Site::Site(const Site& other) = default;
 
 PopularSites::Site::~Site() {}
 
 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,
                            const FinishedCallback& callback)
     : PopularSites(prefs,
                    template_url_service,
                    download_context,
+                   directory,
                    GetCountryToUse(prefs,
                                    template_url_service,
                                    variations_service,
                                    variation_param_country),
                    GetVersionToUse(prefs, variation_param_version),
                    GURL(),
                    force_download,
                    callback) {}
 
 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,
                    true,
                    callback) {}
 
 PopularSites::~PopularSites() {}
 
 std::string PopularSites::GetCountry() const {
   return prefs_->GetString(kPopularSitesCountryPref);
@@ skipping 12 matching lines @@
                                  std::string());
 
   user_prefs->RegisterInt64Pref(kPopularSitesLastDownloadPref, 0);
   user_prefs->RegisterStringPref(kPopularSitesCountryPref, std::string());
   user_prefs->RegisterStringPref(kPopularSitesVersionPref, std::string());
 }
 
 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),
       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) {
   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;
   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 ||
       (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 {
   return GURL(base::StringPrintf(kPopularSitesURLFormat,
                                  pending_country_.c_str(),
                                  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(&ReadAndParseJsonFile, path),
+      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(&ParseJsonFile, json),
       base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
 }
 
 void PopularSites::OnJsonParsed(std::unique_ptr<std::vector<Site>> sites) {
   if (sites)
     sites_.swap(*sites);
   else
     sites_.clear();
   callback_.Run(!!sites);
 }