Remove NTP dependency on //content/...

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 a9fdd7685ba3195ad483f304153e4341e8174011..4002e19bdad72e83aaf0b930e19c6d4a6b46ca18 100644
--- a/chrome/browser/android/ntp/popular_sites.cc
+++ b/chrome/browser/android/ntp/popular_sites.cc
@@ -29,11 +29,9 @@
 #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;
 
@@ -186,16 +184,21 @@ 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,
+PopularSites::PopularSites(
+    const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread,
+    const scoped_refptr<base::TaskRunner>& blocking_runner,

[blundell, 2016/05/24 15:12:48]

this needs to be a SequencedWorkerPool*, no?

[sfiera, 2016/05/24 16:59:20]

Done.

+    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(ui_thread,
+                   blocking_runner,
+                   prefs,
                    download_context,
                    directory,
                    GetCountryToUse(prefs,
@@ -207,12 +210,17 @@ PopularSites::PopularSites(PrefService* prefs,
                    force_download,
                    callback) {}
 
-PopularSites::PopularSites(PrefService* prefs,
-                           net::URLRequestContextGetter* download_context,
-                           const base::FilePath& directory,
-                           const GURL& url,
-                           const FinishedCallback& callback)
-    : PopularSites(prefs,
+PopularSites::PopularSites(
+    const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread,
+    const scoped_refptr<base::TaskRunner>& blocking_runner,
+    PrefService* prefs,
+    net::URLRequestContextGetter* download_context,
+    const base::FilePath& directory,
+    const GURL& url,
+    const FinishedCallback& callback)
+    : PopularSites(ui_thread,
+                   blocking_runner,
+                   prefs,
                    download_context,
                    directory,
                    std::string(),
@@ -244,14 +252,17 @@ void PopularSites::RegisterProfilePrefs(
   user_prefs->RegisterStringPref(kPopularSitesVersionPref, std::string());
 }
 
-PopularSites::PopularSites(PrefService* prefs,
-                           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)
+PopularSites::PopularSites(
+    const scoped_refptr<base::SingleThreadTaskRunner>& ui_thread,
+    const scoped_refptr<base::TaskRunner>& blocking_runner,
+    PrefService* prefs,
+    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),
@@ -261,11 +272,10 @@ PopularSites::PopularSites(PrefService* prefs,
                       : directory.AppendASCII(kPopularSitesLocalFilename)),
       prefs_(prefs),
       download_context_(download_context),
-      runner_(
-          BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(

[blundell, 2016/05/24 15:12:48]

and this code stays in this class, just called on worker_pool.

[Bernhard Bauer, 2016/05/24 15:14:47]

Could you keep this shutdown behavior?

[sfiera, 2016/05/24 16:59:20]

Done.

-              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)),
+      ui_thread_(ui_thread),
+      blocking_runner_(blocking_runner),
       weak_ptr_factory_(this) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(ui_thread_->BelongsToCurrentThread());
   const base::Time last_download_time = base::Time::FromInternalValue(
       prefs_->GetInt64(kPopularSitesLastDownloadPref));
   const base::TimeDelta time_since_last_download =
@@ -281,8 +291,7 @@ PopularSites::PopularSites(PrefService* prefs,
 
   // No valid path to save to. Immediately post failure.
   if (local_path_.empty()) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(callback_, false));
+    ui_thread_->PostTask(FROM_HERE, base::Bind(callback_, false));

[Marc Treib, 2016/05/24 15:16:11]

Hm. Do we actually need/want the UI thread here? Can we just post a task to the
current thread? (That's what the other places in this file do.)

I realize it's a pre-existing problem, but if we change that now we don't have
to pass around the UI thread.

[sfiera, 2016/05/24 16:59:20]

We want a single thread and not a thread pool (so that this task can't run
immediately), but it doesn't have to be the UI thread, even though it always is
in practice. I'm not familiar enough with Chrome threading to know the best way
to guarantee that, but I'll take any pointers.

[Marc Treib, 2016/05/24 17:12:03]

I think ThreadTaskRunnerHandle::Get()->PostTask(...) is what you're looking for.

[sfiera, 2016/05/24 18:18:59]

Done.

I removed the UI thread references from this file, but I still have them in
most_visited_sites.cc to preserve DCHECKs. It looks like maybe I could save
base::ThreadTaskRunnerHandle::Get() as original_thread_ and
DCHECK(original_thread_->BelongsToCurrentThread()). Would that be correct?

     return;
   }
 
@@ -297,7 +306,7 @@ PopularSites::PopularSites(PrefService* prefs,
   std::unique_ptr<std::string> file_data(new std::string);
   std::string* file_data_ptr = file_data.get();
   base::PostTaskAndReplyWithResult(
-      runner_.get(), FROM_HERE,
+      blocking_runner_.get(), 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))));
@@ -350,7 +359,7 @@ void PopularSites::OnURLFetchComplete(const net::URLFetcher* source) {
 
 void PopularSites::OnJsonSanitized(const std::string& valid_minified_json) {
   base::PostTaskAndReplyWithResult(
-      runner_.get(), FROM_HERE,
+      blocking_runner_.get(), FROM_HERE,
       base::Bind(&base::ImportantFileWriter::WriteFileAtomically, local_path_,
                  valid_minified_json),
       base::Bind(&PopularSites::OnFileWriteDone, weak_ptr_factory_.GetWeakPtr(),
@@ -378,7 +387,7 @@ void PopularSites::OnFileWriteDone(const std::string& json, bool success) {
 
 void PopularSites::ParseSiteList(const std::string& json) {
   base::PostTaskAndReplyWithResult(
-      runner_.get(), FROM_HERE, base::Bind(&ParseJson, json),
+      blocking_runner_.get(), FROM_HERE, base::Bind(&ParseJson, json),
       base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
 }