Migrate CustomizationWallpaperDownloader to SimpleURLLoader

CL migrates CustomizationWallpaperDownloader away from URLFetcher et al
to use SimplerURLLoader (network services).

In the associated browsertests** it also simplifies the implementation
a lot by making use of EmbeddedTestServer rather than the URLFetcher
and FakeURLFetcherFactory machinery.

**customization_wallpaper_downloader_browsertest.cc

BUG=773295
TEST=CustomizationWallpaperDownloaderBrowserTest.*

Change-Id: I881ccb8749377a0043f15edfe20c223c57c5ea51
Reviewed-on: https://chromium-review.googlesource.com/1012877Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#551067}

chrome/browser/chromeos/customization/customization_document.cc

@@ -807,7 +807,7 @@ void ServicesCustomizationDocument::StartOEMWallpaperDownload(
   }
 
   wallpaper_downloader_.reset(new CustomizationWallpaperDownloader(
-      g_browser_process->system_request_context(), wallpaper_url, dir, file,
+      wallpaper_url, dir, file,
       base::Bind(&ServicesCustomizationDocument::OnOEMWallpaperDownloaded,
                  weak_ptr_factory_.GetWeakPtr(),
                  base::Passed(std::move(applying)))));

chrome/browser/chromeos/customization/customization_wallpaper_downloader.cc

@@ -10,12 +10,13 @@
 
 #include "base/files/file_util.h"
 #include "base/task_scheduler/post_task.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/net/system_network_context_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request_context_getter.h"
-#include "url/gurl.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 
 namespace chromeos {
 namespace {
@@ -59,14 +60,12 @@ void RenameTemporaryFile(const base::FilePath& from,
 }  // namespace
 
 CustomizationWallpaperDownloader::CustomizationWallpaperDownloader(
-    net::URLRequestContextGetter* url_context_getter,
     const GURL& wallpaper_url,
     const base::FilePath& wallpaper_dir,
     const base::FilePath& wallpaper_downloaded_file,
     base::Callback<void(bool success, const GURL&)>
         on_wallpaper_fetch_completed)
-    : url_context_getter_(url_context_getter),
-      wallpaper_url_(wallpaper_url),
+    : wallpaper_url_(wallpaper_url),
       wallpaper_dir_(wallpaper_dir),
       wallpaper_downloaded_file_(wallpaper_downloaded_file),
       wallpaper_temporary_file_(wallpaper_downloaded_file.value() +
@@ -86,19 +85,30 @@ void CustomizationWallpaperDownloader::StartRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(wallpaper_url_.is_valid());
 
-  url_fetcher_ =
-      net::URLFetcher::Create(wallpaper_url_, net::URLFetcher::GET, this);
-  url_fetcher_->SetRequestContext(url_context_getter_.get());
-  url_fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE |
-                             net::LOAD_DISABLE_CACHE |
-                             net::LOAD_DO_NOT_SAVE_COOKIES |
-                             net::LOAD_DO_NOT_SEND_COOKIES |
-                             net::LOAD_DO_NOT_SEND_AUTH_DATA);
-  url_fetcher_->SaveResponseToFileAtPath(
-      wallpaper_temporary_file_,
-      base::CreateSequencedTaskRunnerWithTraits(
-          {base::MayBlock(), base::TaskPriority::BACKGROUND}));
-  url_fetcher_->Start();
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = wallpaper_url_;
+  resource_request->load_flags =
+      net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+      net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES |
+      net::LOAD_DO_NOT_SEND_AUTH_DATA;
+  // TODO(crbug.com/833390): Add a real traffic annotation here.
+  simple_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
+                                                    MISSING_TRAFFIC_ANNOTATION);
+
+  SystemNetworkContextManager* system_network_context_manager =
+      g_browser_process->system_network_context_manager();
+  // In unit tests, the browser process can return a null context manager
+  if (!system_network_context_manager)
+    return;
+
+  network::mojom::URLLoaderFactory* loader_factory =
+      system_network_context_manager->GetURLLoaderFactory();
+
+  simple_loader_->DownloadToFile(
+      loader_factory,
+      base::BindOnce(&CustomizationWallpaperDownloader::OnSimpleLoaderComplete,
+                     base::Unretained(this)),
+      wallpaper_temporary_file_);
 }
 
 void CustomizationWallpaperDownloader::Retry() {
@@ -139,36 +149,29 @@ void CustomizationWallpaperDownloader::OnWallpaperDirectoryCreated(
     StartRequest();
 }
 
-void CustomizationWallpaperDownloader::OnURLFetchComplete(
-    const net::URLFetcher* source) {
+void CustomizationWallpaperDownloader::OnSimpleLoaderComplete(
+    const base::FilePath& response_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK_EQ(url_fetcher_.get(), source);
 
-  const net::URLRequestStatus status = source->GetStatus();
-  const int response_code = source->GetResponseCode();
-
-  const bool server_error =
-      !status.is_success() ||
-      (response_code >= net::HTTP_INTERNAL_SERVER_ERROR &&
-       response_code < (net::HTTP_INTERNAL_SERVER_ERROR + 100));
+  const bool error = response_path.empty();
 
   VLOG(1) << "CustomizationWallpaperDownloader::OnURLFetchComplete(): status="
-          << status.status();
+          << simple_loader_->NetError();
+
+  // Save the response_path before resetting SimplerURLLoader. It gets nulled
+  // out afterwards.
+  base::FilePath copy_response_path(response_path);
+  simple_loader_.reset();
 
-  if (server_error) {
-    url_fetcher_.reset();
+  if (error) {
     Retry();
     return;
   }
 
-  base::FilePath response_path;
-  url_fetcher_->GetResponseAsFilePath(true, &response_path);
-  url_fetcher_.reset();
-
   std::unique_ptr<bool> success(new bool(false));
 
   base::OnceClosure rename_closure = base::BindOnce(
-      &RenameTemporaryFile, response_path, wallpaper_downloaded_file_,
+      &RenameTemporaryFile, copy_response_path, wallpaper_downloaded_file_,
       base::Unretained(success.get()));
   base::OnceClosure on_rename_closure = base::BindOnce(
       &CustomizationWallpaperDownloader::OnTemporaryFileRenamed,

chrome/browser/chromeos/customization/customization_wallpaper_downloader.h

@@ -15,19 +15,15 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
-#include "net/url_request/url_fetcher_delegate.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 
-namespace net {
-class URLRequestContextGetter;
-}  // namespace net
-
 namespace chromeos {
 
 // Download customized wallpaper.
 // Owner of this class must provide callback, which will be called on
 // finished (either successful or failed) wallpaper download.
-class CustomizationWallpaperDownloader : public net::URLFetcherDelegate {
+class CustomizationWallpaperDownloader {
  public:
   // - |url_context_getter| - Context to initialize net::URLFetcher.
   // - |wallpaper_url| - wallpaper URL to download.
@@ -39,21 +35,17 @@ class CustomizationWallpaperDownloader : public net::URLFetcherDelegate {
   // After download is completed, temporary file will be renamed to
   // |wallpaper_downloaded_file|.
   CustomizationWallpaperDownloader(
-      net::URLRequestContextGetter* url_context_getter,
       const GURL& wallpaper_url,
       const base::FilePath& wallpaper_dir,
       const base::FilePath& wallpaper_downloaded_file,
       base::Callback<void(bool success, const GURL&)>
           on_wallpaper_fetch_completed);
 
-  ~CustomizationWallpaperDownloader() override;
+  ~CustomizationWallpaperDownloader();
 
   // Start download.
   void Start();
 
-  // net::URLFetcherDelegate
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
-
   // This is called in tests to modify (lower) retry delay.
   void set_retry_delay_for_testing(base::TimeDelta value) {
     retry_delay_ = value;
@@ -70,17 +62,17 @@ class CustomizationWallpaperDownloader : public net::URLFetcherDelegate {
   // Schedules retry.
   void Retry();
 
+  // This is called when the download has finished.
+  void OnSimpleLoaderComplete(const base::FilePath& file_path);
+
   // Called on UI thread.
   void OnWallpaperDirectoryCreated(std::unique_ptr<bool> success);
 
   // Called on UI thread.
   void OnTemporaryFileRenamed(std::unique_ptr<bool> success);
 
-  // This is used to initialize net::URLFetcher object.
-  scoped_refptr<net::URLRequestContextGetter> url_context_getter_;
-
-  // This fetcher is used to download wallpaper file.
-  std::unique_ptr<net::URLFetcher> url_fetcher_;
+  // This loader is used to download wallpaper file.
+  std::unique_ptr<network::SimpleURLLoader> simple_loader_;
 
   // The wallpaper URL to fetch.
   const GURL wallpaper_url_;

chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc

@@ -22,6 +22,7 @@
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "net/url_request/url_fetcher_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -30,7 +31,7 @@ namespace chromeos {
 
 namespace {
 
-constexpr char kOEMWallpaperURL[] = "http://somedomain.com/image.png";
+constexpr char kOEMWallpaperRelativeURL[] = "/image.png";
 
 constexpr char kServicesManifest[] =
     "{"
@@ -142,24 +143,42 @@ class TestWallpaperObserver : public ash::mojom::WallpaperObserver {
 
 }  // namespace
 
-// This is helper class for net::FakeURLFetcherFactory.
-class TestWallpaperImageURLFetcherCallback {
+class CustomizationWallpaperDownloaderBrowserTest
+    : public InProcessBrowserTest {
  public:
-  TestWallpaperImageURLFetcherCallback(
-      const GURL& url,
-      const size_t require_retries,
-      const std::vector<unsigned char>& jpeg_data_raw)
-      : url_(url), require_retries_(require_retries), factory_(nullptr) {
-    jpeg_data_.resize(jpeg_data_raw.size());
-    std::copy(jpeg_data_raw.begin(), jpeg_data_raw.end(), jpeg_data_.begin());
+  CustomizationWallpaperDownloaderBrowserTest() {}
+  ~CustomizationWallpaperDownloaderBrowserTest() override {}
+
+  // InProcessBrowserTest overrides:
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+
+    std::vector<unsigned char> oem_wallpaper;
+    ASSERT_TRUE(ash::WallpaperController::CreateJPEGImageForTesting(
+        kWallpaperSize, kWallpaperSize, kCustomizedDefaultWallpaperColor,
+        &oem_wallpaper));
+    jpeg_data_.resize(oem_wallpaper.size());
+    std::copy(oem_wallpaper.begin(), oem_wallpaper.end(), jpeg_data_.begin());
+
+    // Set up the test server.
+    embedded_test_server()->RegisterRequestHandler(base::BindRepeating(
+        &CustomizationWallpaperDownloaderBrowserTest::HandleRequest,
+        base::Unretained(this)));
+    ASSERT_TRUE(embedded_test_server()->Start());
   }
 
-  std::unique_ptr<net::FakeURLFetcher> CreateURLFetcher(
-      const GURL& url,
-      net::URLFetcherDelegate* delegate,
-      const std::string& response_data,
-      net::HttpStatusCode response_code,
-      net::URLRequestStatus::Status status) {
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(chromeos::switches::kLoginManager);
+    command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "user");
+  }
+
+  void SetRequiredRetries(size_t retries) { required_retries_ = retries; }
+
+  size_t num_attempts() const { return attempts_.size(); }
+
+ private:
+  std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
+      const net::test_server::HttpRequest& request) {
     chromeos::ServicesCustomizationDocument* customization =
         chromeos::ServicesCustomizationDocument::GetInstance();
     customization->wallpaper_downloader_for_testing()
@@ -181,106 +200,26 @@ class TestWallpaperImageURLFetcherCallback {
           << " * (retry=" << retry
           << " * retry)= " << base_interval * retry * retry << " seconds.";
     }
-    if (attempts_.size() > require_retries_) {
-      response_code = net::HTTP_OK;
-      status = net::URLRequestStatus::SUCCESS;
-      factory_->SetFakeResponse(url, response_data, response_code, status);
+    if (attempts_.size() > required_retries_) {
+      std::unique_ptr<net::test_server::BasicHttpResponse> response =
+          std::make_unique<net::test_server::BasicHttpResponse>();
+      response->set_content_type("image/jpeg");
+      response->set_code(net::HTTP_OK);
+      response->set_content(jpeg_data_);
+      return std::move(response);
     }
-    std::unique_ptr<net::FakeURLFetcher> fetcher(new net::FakeURLFetcher(
-        url, delegate, response_data, response_code, status));
-    scoped_refptr<net::HttpResponseHeaders> download_headers =
-        new net::HttpResponseHeaders(std::string());
-    download_headers->AddHeader("Content-Type: image/jpeg");
-    fetcher->set_response_headers(download_headers);
-    return fetcher;
+    return nullptr;
   }
 
-  void Initialize(net::FakeURLFetcherFactory* factory) {
-    factory_ = factory;
-    factory_->SetFakeResponse(url_,
-                              jpeg_data_,
-                              net::HTTP_INTERNAL_SERVER_ERROR,
-                              net::URLRequestStatus::FAILED);
-  }
-
-  size_t num_attempts() const { return attempts_.size(); }
-
- private:
-  const GURL url_;
-  // Respond with OK on required retry attempt.
-  const size_t require_retries_;
-  net::FakeURLFetcherFactory* factory_;
-  std::vector<base::TimeTicks> attempts_;
+  // Sample Wallpaper content.
   std::string jpeg_data_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestWallpaperImageURLFetcherCallback);
-};
-
-// This implements fake remote source for wallpaper image.
-// JPEG image is created here and served to CustomizationWallpaperDownloader
-// via net::FakeURLFetcher.
-class WallpaperImageFetcherFactory {
- public:
-  WallpaperImageFetcherFactory(const GURL& url,
-                               int width,
-                               int height,
-                               SkColor color,
-                               const size_t require_retries) {
-    // ASSERT_TRUE() cannot be directly used in constructor.
-    Initialize(url, width, height, color, require_retries);
-  }
-
-  ~WallpaperImageFetcherFactory() {
-    fetcher_factory_.reset();
-    net::URLFetcherImpl::set_factory(fallback_fetcher_factory_.get());
-    fallback_fetcher_factory_.reset();
-  }
-
-  size_t num_attempts() const { return url_callback_->num_attempts(); }
-
- private:
-  void Initialize(const GURL& url,
-                  int width,
-                  int height,
-                  SkColor color,
-                  const size_t require_retries) {
-    std::vector<unsigned char> oem_wallpaper_;
-    ASSERT_TRUE(ash::WallpaperController::CreateJPEGImageForTesting(
-        width, height, color, &oem_wallpaper_));
-
-    url_callback_.reset(new TestWallpaperImageURLFetcherCallback(
-        url, require_retries, oem_wallpaper_));
-    fallback_fetcher_factory_.reset(new net::TestURLFetcherFactory);
-    net::URLFetcherImpl::set_factory(nullptr);
-    fetcher_factory_.reset(new net::FakeURLFetcherFactory(
-        fallback_fetcher_factory_.get(),
-        base::Bind(&TestWallpaperImageURLFetcherCallback::CreateURLFetcher,
-                   base::Unretained(url_callback_.get()))));
-    url_callback_->Initialize(fetcher_factory_.get());
-  }
-
-  std::unique_ptr<TestWallpaperImageURLFetcherCallback> url_callback_;
-
-  // Use a test factory as a fallback so we don't have to deal with other
-  // requests.
-  std::unique_ptr<net::TestURLFetcherFactory> fallback_fetcher_factory_;
-  std::unique_ptr<net::FakeURLFetcherFactory> fetcher_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(WallpaperImageFetcherFactory);
-};
-
-class CustomizationWallpaperDownloaderBrowserTest
-    : public InProcessBrowserTest {
- public:
-  CustomizationWallpaperDownloaderBrowserTest() {}
-  ~CustomizationWallpaperDownloaderBrowserTest() override {}
+  // Number of loads performed.
+  std::vector<base::TimeTicks> attempts_;
 
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitch(chromeos::switches::kLoginManager);
-    command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "user");
-  }
+  // Number of retries required.
+  size_t required_retries_ = 0;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(CustomizationWallpaperDownloaderBrowserTest);
 };
 
@@ -292,14 +231,15 @@ IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
   observer.WaitForWallpaperChanged();
   observer.Reset();
 
+  // Set the number of required retries.
+  SetRequiredRetries(0);
+
   // Start fetching the customized default wallpaper.
-  WallpaperImageFetcherFactory url_factory(
-      GURL(kOEMWallpaperURL), kWallpaperSize, kWallpaperSize,
-      kCustomizedDefaultWallpaperColor, 0 /* require_retries */);
+  GURL url = embedded_test_server()->GetURL(kOEMWallpaperRelativeURL);
   chromeos::ServicesCustomizationDocument* customization =
       chromeos::ServicesCustomizationDocument::GetInstance();
   EXPECT_TRUE(
-      customization->LoadManifestFromString(ManifestForURL(kOEMWallpaperURL)));
+      customization->LoadManifestFromString(ManifestForURL(url.spec())));
   observer.WaitForWallpaperChanged();
   observer.Reset();
 
@@ -312,7 +252,7 @@ IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
         EXPECT_TRUE(ImageIsNearColor(image, kCustomizedDefaultWallpaperColor));
       }));
   run_loop.Run();
-  EXPECT_EQ(1U, url_factory.num_attempts());
+  EXPECT_EQ(1U, num_attempts());
 }
 
 IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
@@ -323,14 +263,15 @@ IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
   observer.WaitForWallpaperChanged();
   observer.Reset();
 
+  // Set the number of required retries.
+  SetRequiredRetries(1);
+
   // Start fetching the customized default wallpaper.
-  WallpaperImageFetcherFactory url_factory(
-      GURL(kOEMWallpaperURL), kWallpaperSize, kWallpaperSize,
-      kCustomizedDefaultWallpaperColor, 1 /* require_retries */);
+  GURL url = embedded_test_server()->GetURL(kOEMWallpaperRelativeURL);
   chromeos::ServicesCustomizationDocument* customization =
       chromeos::ServicesCustomizationDocument::GetInstance();
   EXPECT_TRUE(
-      customization->LoadManifestFromString(ManifestForURL(kOEMWallpaperURL)));
+      customization->LoadManifestFromString(ManifestForURL(url.spec())));
   observer.WaitForWallpaperChanged();
   observer.Reset();
 
@@ -343,7 +284,7 @@ IN_PROC_BROWSER_TEST_F(CustomizationWallpaperDownloaderBrowserTest,
         EXPECT_TRUE(ImageIsNearColor(image, kCustomizedDefaultWallpaperColor));
       }));
   run_loop.Run();
-  EXPECT_EQ(2U, url_factory.num_attempts());
+  EXPECT_EQ(2U, num_attempts());
 }
 
 }  // namespace chromeos

chrome/browser/ui/ash/wallpaper_policy_handler.cc

@@ -155,8 +155,8 @@ void WallpaperPolicyHandler::OnDeviceWallpaperFileExists(bool exists) {
     GURL wallpaper_url(url);
     device_wallpaper_downloader_.reset(
         new chromeos::CustomizationWallpaperDownloader(
-            g_browser_process->system_request_context(), wallpaper_url,
-            device_wallpaper_file_path_.DirName(), device_wallpaper_file_path_,
+            wallpaper_url, device_wallpaper_file_path_.DirName(),
+            device_wallpaper_file_path_,
             base::BindRepeating(
                 &WallpaperPolicyHandler::OnDeviceWallpaperDownloaded,
                 weak_factory_.GetWeakPtr(), hash)));
@@ -177,8 +177,8 @@ void WallpaperPolicyHandler::OnCheckExistingDeviceWallpaperMatchHash(
   GURL wallpaper_url(url);
   device_wallpaper_downloader_.reset(
       new chromeos::CustomizationWallpaperDownloader(
-          g_browser_process->system_request_context(), wallpaper_url,
-          device_wallpaper_file_path_.DirName(), device_wallpaper_file_path_,
+          wallpaper_url, device_wallpaper_file_path_.DirName(),
+          device_wallpaper_file_path_,
           base::BindRepeating(
               &WallpaperPolicyHandler::OnDeviceWallpaperDownloaded,
               weak_factory_.GetWeakPtr(), hash)));