Refactor the interface for the CrxDownloader to take a callback.

To be consistent with the rest of the artifacts in the component updater,
the non-blocking calls for the CrxDownloader take a callback to be called
when the non-blocking call completes.

This change allows adding further callbacks besides the completion
callback, such as a callback to be called when download progress is made.

This change is a pure refactoring and it should be a NOP in terms of
existing functionality.

BUG=366388

Review URL: https://codereview.chromium.org/250383008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266304 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/component_updater/background_downloader_win.cc

@@ -386,9 +386,8 @@ HRESULT CleanupStaleJobs(
 BackgroundDownloader::BackgroundDownloader(
     scoped_ptr<CrxDownloader> successor,
     net::URLRequestContextGetter* context_getter,
-    scoped_refptr<base::SequencedTaskRunner> task_runner,
-    const DownloadCallback& download_callback)
-    : CrxDownloader(successor.Pass(), download_callback),
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : CrxDownloader(successor.Pass()),
       context_getter_(context_getter),
       task_runner_(task_runner),
       is_completed_(false) {

chrome/browser/component_updater/background_downloader_win.h

@@ -28,8 +28,7 @@ class BackgroundDownloader : public CrxDownloader {
   friend class CrxDownloader;
   BackgroundDownloader(scoped_ptr<CrxDownloader> successor,
                        net::URLRequestContextGetter* context_getter,
-                       scoped_refptr<base::SequencedTaskRunner> task_runner,
-                       const DownloadCallback& download_callback);
+                       scoped_refptr<base::SequencedTaskRunner> task_runner);
   virtual ~BackgroundDownloader();
 
  private:

chrome/browser/component_updater/component_updater_service.cc

@@ -680,14 +680,13 @@ void CrxUpdateService::UpdateComponent(CrxUpdateItem* workitem) {
       !workitem->on_demand && allow_background_download &&
       config_->UseBackgroundDownloader();
 
-  crx_downloader_.reset(CrxDownloader::Create(
-      is_background_download,
-      config_->RequestContext(),
-      blocking_task_runner_,
-      base::Bind(&CrxUpdateService::DownloadComplete,
-                 base::Unretained(this),
-                 base::Passed(&crx_context))));
-  crx_downloader_->StartDownload(*urls);
+  crx_downloader_.reset(CrxDownloader::Create(is_background_download,
+                                              config_->RequestContext(),
+                                              blocking_task_runner_));
+  crx_downloader_->StartDownload(*urls,
+                                 base::Bind(&CrxUpdateService::DownloadComplete,
+                                            base::Unretained(this),
+                                            base::Passed(&crx_context)));
 }
 
 void CrxUpdateService::UpdateCheckComplete(

chrome/browser/component_updater/crx_downloader.cc

@@ -28,31 +28,25 @@ CrxDownloader::DownloadMetrics::DownloadMetrics()
 CrxDownloader* CrxDownloader::Create(
     bool is_background_download,
     net::URLRequestContextGetter* context_getter,
-    scoped_refptr<base::SequencedTaskRunner> task_runner,
-    const DownloadCallback& download_callback) {
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
   scoped_ptr<CrxDownloader> url_fetcher_downloader(
       new UrlFetcherDownloader(scoped_ptr<CrxDownloader>().Pass(),
                                context_getter,
-                               task_runner,
-                               download_callback));
+                               task_runner));
 #if defined (OS_WIN)
   if (is_background_download) {
     return new BackgroundDownloader(url_fetcher_downloader.Pass(),
                                     context_getter,
-                                    task_runner,
-                                    download_callback);
+                                    task_runner);
   }
 #endif
 
   return url_fetcher_downloader.release();
 }
 
-CrxDownloader::CrxDownloader(
-    scoped_ptr<CrxDownloader> successor,
-    const DownloadCallback& download_callback)
-    : successor_(successor.Pass()),
-      download_callback_(download_callback) {
-      DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+CrxDownloader::CrxDownloader(scoped_ptr<CrxDownloader> successor)
+    : successor_(successor.Pass()) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 CrxDownloader::~CrxDownloader() {
@@ -74,20 +68,23 @@ CrxDownloader::download_metrics() const {
   return retval;
 }
 
-void CrxDownloader::StartDownloadFromUrl(const GURL& url) {
+void CrxDownloader::StartDownloadFromUrl(
+    const GURL& url,
+    const DownloadCallback& download_callback) {
   std::vector<GURL> urls;
   urls.push_back(url);
-  StartDownload(urls);
+  StartDownload(urls, download_callback);
 }
 
-void CrxDownloader::StartDownload(const std::vector<GURL>& urls) {
+void CrxDownloader::StartDownload(const std::vector<GURL>& urls,
+                                  const DownloadCallback& download_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (urls.empty()) {
     // Make a result and complete the download with a generic error for now.
     Result result;
     result.error = -1;
-    download_callback_.Run(result);
+    download_callback.Run(result);
     return;
   }
 
@@ -97,6 +94,7 @@ void CrxDownloader::StartDownload(const std::vector<GURL>& urls) {
   // reset at this point, and the iterator will be valid in all conditions.
   urls_ = urls;
   current_url_ = urls_.begin();
+  download_callback_ = download_callback;
 
   DoStartDownload(*current_url_);
 }
@@ -133,7 +131,7 @@ void CrxDownloader::OnDownloadComplete(
     // of urls. Otherwise, the request ends here since the current downloader
     // has tried all urls and it can't fall back on any other downloader.
     if (successor_ && !urls_.empty()) {
-      successor_->StartDownload(urls_);
+      successor_->StartDownload(urls_, download_callback_);
       return;
     }
   }

chrome/browser/component_updater/crx_downloader.h

@@ -78,22 +78,22 @@ class CrxDownloader {
   static CrxDownloader* Create(
       bool is_background_download,
       net::URLRequestContextGetter* context_getter,
-      scoped_refptr<base::SequencedTaskRunner> task_runner,
-      const DownloadCallback& download_callback);
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
   virtual ~CrxDownloader();
 
   // Starts the download. One instance of the class handles one download only.
   // One instance of CrxDownloader can only be started once, otherwise the
   // behavior is undefined. The callback gets invoked if the download can't
   // be started.
-  void StartDownloadFromUrl(const GURL& url);
-  void StartDownload(const std::vector<GURL>& urls);
+  void StartDownloadFromUrl(const GURL& url,
+                            const DownloadCallback& download_callback);
+  void StartDownload(const std::vector<GURL>& urls,
+                     const DownloadCallback& download_callback);
 
   const std::vector<DownloadMetrics> download_metrics() const;
 
  protected:
-  CrxDownloader(scoped_ptr<CrxDownloader> successor,
-                const DownloadCallback& download_callback);
+  explicit CrxDownloader(scoped_ptr<CrxDownloader> successor);
 
   // Handles the fallback in the case of multiple urls and routing of the
   // download to the following successor in the chain. Derived classes must call

chrome/browser/component_updater/test/crx_downloader_unittest.cc

@@ -54,6 +54,8 @@ class CrxDownloaderTest : public testing::Test {
  protected:
   scoped_ptr<CrxDownloader> crx_downloader_;
 
+  CrxDownloader::DownloadCallback callback_;
+
   int crx_context_;
   int error_;
   base::FilePath response_;
@@ -73,7 +75,10 @@ class CrxDownloaderTest : public testing::Test {
 const int CrxDownloaderTest::kExpectedContext;
 
 CrxDownloaderTest::CrxDownloaderTest()
-    : crx_context_(0),
+    : callback_(base::Bind(&CrxDownloaderTest::DownloadComplete,
+                           base::Unretained(this),
+                           kExpectedContext)),
+      crx_context_(0),
       error_(0),
       num_calls_(0),
       blocking_task_runner_(BrowserThread::GetBlockingPool()->
@@ -94,10 +99,7 @@ void CrxDownloaderTest::SetUp() {
   crx_downloader_.reset(CrxDownloader::Create(
       false,    // Do not use the background downloader in these tests.
       context_.get(),
-      blocking_task_runner_,
-      base::Bind(&CrxDownloaderTest::DownloadComplete,
-                 base::Unretained(this),
-                 kExpectedContext)));
+      blocking_task_runner_));
 }
 
 void CrxDownloaderTest::TearDown() {
@@ -137,7 +139,7 @@ void CrxDownloaderTest::RunThreadsUntilIdle() {
 // Tests that starting a download without a url results in an error.
 TEST_F(CrxDownloaderTest, NoUrl) {
   std::vector<GURL> urls;
-  crx_downloader_->StartDownload(urls);
+  crx_downloader_->StartDownload(urls, callback_);
 
   RunThreadsUntilIdle();
   EXPECT_EQ(-1, error_);
@@ -156,7 +158,7 @@ TEST_F(CrxDownloaderTest, OneUrl) {
        expected_crx_url,
        test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
 
-  crx_downloader_->StartDownloadFromUrl(expected_crx_url);
+  crx_downloader_->StartDownloadFromUrl(expected_crx_url, callback_);
 
   RunThreads();
   EXPECT_EQ(1, interceptor.GetHitCount());
@@ -187,7 +189,7 @@ TEST_F(CrxDownloaderTest, MAYBE_TwoUrls) {
   urls.push_back(expected_crx_url);
   urls.push_back(expected_crx_url);
 
-  crx_downloader_->StartDownload(urls);
+  crx_downloader_->StartDownload(urls, callback_);
 
   RunThreads();
   EXPECT_EQ(1, interceptor.GetHitCount());
@@ -209,7 +211,8 @@ TEST_F(CrxDownloaderTest, OneUrl_InvalidHost) {
 
   crx_downloader_->StartDownloadFromUrl(
       GURL("http://no.such.host"
-           "/download/jebgalgnebhfojomionfpkfelancnnkf.crx"));
+           "/download/jebgalgnebhfojomionfpkfelancnnkf.crx"),
+      callback_);
 
   RunThreads();
   EXPECT_EQ(0, interceptor.GetHitCount());
@@ -229,7 +232,9 @@ TEST_F(CrxDownloaderTest, OneUrl_InvalidPath) {
        expected_crx_url,
        test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
 
-  crx_downloader_->StartDownloadFromUrl(GURL("http://localhost/no/such/file"));
+  crx_downloader_->StartDownloadFromUrl(
+      GURL("http://localhost/no/such/file"),
+      callback_);
 
   RunThreads();
   EXPECT_EQ(0, interceptor.GetHitCount());
@@ -259,7 +264,7 @@ TEST_F(CrxDownloaderTest, MAYBE_TwoUrls_FirstInvalid) {
   urls.push_back(GURL("http://localhost/no/such/file"));
   urls.push_back(expected_crx_url);
 
-  crx_downloader_->StartDownload(urls);
+  crx_downloader_->StartDownload(urls, callback_);
 
   RunThreads();
   EXPECT_EQ(1, interceptor.GetHitCount());
@@ -284,7 +289,7 @@ TEST_F(CrxDownloaderTest, TwoUrls_SecondInvalid) {
   urls.push_back(expected_crx_url);
   urls.push_back(GURL("http://localhost/no/such/file"));
 
-  crx_downloader_->StartDownload(urls);
+  crx_downloader_->StartDownload(urls, callback_);
 
   RunThreads();
   EXPECT_EQ(1, interceptor.GetHitCount());
@@ -309,7 +314,7 @@ TEST_F(CrxDownloaderTest, TwoUrls_BothInvalid) {
   urls.push_back(GURL("http://no.such.host/"
                       "/download/jebgalgnebhfojomionfpkfelancnnkf.crx"));
 
-  crx_downloader_->StartDownload(urls);
+  crx_downloader_->StartDownload(urls, callback_);
 
   RunThreads();
   EXPECT_EQ(0, interceptor.GetHitCount());
@@ -320,4 +325,3 @@ TEST_F(CrxDownloaderTest, TwoUrls_BothInvalid) {
 }
 
 }  // namespace component_updater
-

chrome/browser/component_updater/url_fetcher_downloader.cc

@@ -17,9 +17,8 @@ namespace component_updater {
 UrlFetcherDownloader::UrlFetcherDownloader(
     scoped_ptr<CrxDownloader> successor,
     net::URLRequestContextGetter* context_getter,
-    scoped_refptr<base::SequencedTaskRunner> task_runner,
-    const DownloadCallback& download_callback)
-    : CrxDownloader(successor.Pass(), download_callback),
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : CrxDownloader(successor.Pass()),
       context_getter_(context_getter),
       task_runner_(task_runner),
       downloaded_bytes_(-1),

chrome/browser/component_updater/url_fetcher_downloader.h

@@ -26,8 +26,7 @@ class UrlFetcherDownloader : public CrxDownloader,
   friend class CrxDownloader;
   UrlFetcherDownloader(scoped_ptr<CrxDownloader> successor,
                        net::URLRequestContextGetter* context_getter,
-                       scoped_refptr<base::SequencedTaskRunner> task_runner,
-                       const DownloadCallback& download_callback);
+                       scoped_refptr<base::SequencedTaskRunner> task_runner);
   virtual ~UrlFetcherDownloader();
 
  private: