Move timeout timer into WebApkIconHasher

This CL adds a timer to WebApkIconHasher. Consequently, classes that
invoke WebApkIconHasher::DownloadAndComputeMurmur2Hash no longer need
to maintain a timer dedicated to WebApkIconHasher.

This CL also simplifies the error checking and handling logic.
WebApkIconHasher now checks for URL validity itself, and would return
an empty hash string for any errors encountered.

BUG=649771

Review-Url: https://codereview.chromium.org/2771793003
Cr-Commit-Position: refs/heads/master@{#460508}

chrome/browser/android/webapk/webapk_icon_hasher.cc

@@ -12,13 +12,15 @@
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "third_party/smhasher/src/MurmurHash2.h"
-#include "url/gurl.h"
 
 namespace {
 
 // The seed to use when taking the murmur2 hash of the icon.
 const uint64_t kMurmur2HashSeed = 0;
 
+// The default number of milliseconds to wait for the icon download to complete.
+const int kDownloadTimeoutInMilliseconds = 60000;
+
 // Computes Murmur2 hash of |raw_image_data|.
 std::string ComputeMurmur2Hash(const std::string& raw_image_data) {
   // WARNING: We are running in the browser process. |raw_image_data| is the
@@ -32,33 +34,49 @@ std::string ComputeMurmur2Hash(const std::string& raw_image_data) {
 
 }  // anonymous namespace
 
-WebApkIconHasher::WebApkIconHasher() {}
+WebApkIconHasher::WebApkIconHasher(
+    net::URLRequestContextGetter* url_request_context_getter,
+    const GURL& icon_url,
+    const Murmur2HashCallback& callback)
+    : url_request_context_getter_(url_request_context_getter),
+      icon_url_(icon_url),
+      callback_(callback) {}
 
 WebApkIconHasher::~WebApkIconHasher() {}
 
-void WebApkIconHasher::DownloadAndComputeMurmur2Hash(
-    net::URLRequestContextGetter* request_context_getter,
-    const GURL& icon_url,
-    const Murmur2HashCallback& callback) {
-  if (icon_url.SchemeIs(url::kDataScheme)) {
+void WebApkIconHasher::DownloadAndComputeMurmur2Hash() {
+  if (!icon_url_.is_valid()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback_, ""));
+    return;
+  }
+
+  if (icon_url_.SchemeIs(url::kDataScheme)) {
     std::string mime_type, char_set, data;
     std::string hash;
-    if (net::DataURL::Parse(icon_url, &mime_type, &char_set, &data) &&
+    if (net::DataURL::Parse(icon_url_, &mime_type, &char_set, &data) &&
         !data.empty()) {
       hash = ComputeMurmur2Hash(data);
     }
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  base::Bind(callback, hash));
+                                                  base::Bind(callback_, hash));
     return;
   }
 
-  callback_ = callback;
-  url_fetcher_ = net::URLFetcher::Create(icon_url, net::URLFetcher::GET, this);
-  url_fetcher_->SetRequestContext(request_context_getter);
+  download_timeout_timer_.Start(
+      FROM_HERE,
+      base::TimeDelta::FromMilliseconds(kDownloadTimeoutInMilliseconds),
+      base::Bind(&WebApkIconHasher::OnDownloadTimedOut,
+                 base::Unretained(this)));
+
+  url_fetcher_ = net::URLFetcher::Create(icon_url_, net::URLFetcher::GET, this);
+  url_fetcher_->SetRequestContext(url_request_context_getter_);
   url_fetcher_->Start();
 }
 
 void WebApkIconHasher::OnURLFetchComplete(const net::URLFetcher* source) {
+  download_timeout_timer_.Stop();
+
   if (!source->GetStatus().is_success() ||
       source->GetResponseCode() != net::HTTP_OK) {
     callback_.Run("");
@@ -73,3 +91,10 @@ void WebApkIconHasher::OnURLFetchComplete(const net::URLFetcher* source) {
   source->GetResponseAsString(&raw_image_data);
   callback_.Run(ComputeMurmur2Hash(raw_image_data));
 }
+
+void WebApkIconHasher::OnDownloadTimedOut() {
+  url_fetcher_.reset();
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                base::Bind(callback_, ""));
+}

chrome/browser/android/webapk/webapk_icon_hasher.h

@@ -10,44 +10,54 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "net/url_request/url_fetcher_delegate.h"
+#include "url/gurl.h"
 
 namespace net {
 class URLFetcher;
 class URLRequestContextGetter;
 }
 
-class GURL;
-
 // Downloads an icon and takes a Murmur2 hash of the downloaded image.
 class WebApkIconHasher : public net::URLFetcherDelegate {
  public:
   using Murmur2HashCallback =
       base::Callback<void(const std::string& /* icon_murmur2_hash */)>;
 
-  WebApkIconHasher();
-  ~WebApkIconHasher() override;
-
-  // Downloads |icon_url|. Calls |callback| with the Murmur2 hash of the
-  // downloaded image. The hash is taken over the raw image bytes (no image
-  // encoding/decoding beforehand). |callback| is called with an empty string if
-  // the image cannot not be downloaded (e.g. 404 HTTP error code).
-  void DownloadAndComputeMurmur2Hash(
+  WebApkIconHasher(
       net::URLRequestContextGetter* request_context_getter,
       const GURL& icon_url,
       const Murmur2HashCallback& callback);
+  ~WebApkIconHasher() override;
+
+  // Downloads |icon_url_|. Calls |callback_| with the Murmur2 hash of the
+  // downloaded image. The hash is taken over the raw image bytes (no image
+  // encoding/decoding beforehand). |callback_| is called with an empty string
+  // if the image cannot not be downloaded in time (e.g. 404 HTTP error code).
+  void DownloadAndComputeMurmur2Hash();
 
  private:
   // net::URLFetcherDelegate:
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
-  // Fetches the image.
-  std::unique_ptr<net::URLFetcher> url_fetcher_;
+  // Called if downloading the icon takes too long.
+  void OnDownloadTimedOut();
+
+  // For retrieving URLRequestContext. Owned by the caller of this class.
+  net::URLRequestContextGetter* url_request_context_getter_;
+
+  // The icon URL.
+  GURL icon_url_;
 
   // Called with the image hash.
   Murmur2HashCallback callback_;
 
+  std::unique_ptr<net::URLFetcher> url_fetcher_;
+
+  // Fails WebApkIconHasher if the download takes too long.
+  base::OneShotTimer download_timeout_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(WebApkIconHasher);
 };
 

chrome/browser/android/webapk/webapk_icon_hasher_unittest.cc

@@ -37,11 +37,10 @@ class WebApkIconHasherRunner {
   ~WebApkIconHasherRunner() {}
 
   void Run(const GURL& icon_url) {
-    WebApkIconHasher hasher;
-    hasher.DownloadAndComputeMurmur2Hash(
-        url_request_context_getter_.get(), icon_url,
+    WebApkIconHasher hasher(url_request_context_getter_.get(), icon_url,
         base::Bind(&WebApkIconHasherRunner::OnCompleted,
                    base::Unretained(this)));
+    hasher.DownloadAndComputeMurmur2Hash();
 
     base::RunLoop run_loop;
     on_completed_callback_ = run_loop.QuitClosure();
@@ -113,6 +112,20 @@ TEST_F(WebApkIconHasherTest, DataUriInvalid) {
   EXPECT_EQ("", runner.murmur2_hash());
 }
 
+TEST_F(WebApkIconHasherTest, InvalidUrl) {
+  GURL icon_url("http::google.com");
+  WebApkIconHasherRunner runner;
+  runner.Run(icon_url);
+  EXPECT_EQ("", runner.murmur2_hash());
+}
+
+TEST_F(WebApkIconHasherTest, DownloadTimedOut) {
+  GURL icon_url = test_server()->GetURL("/slow?100");
+  WebApkIconHasherRunner runner;
+  runner.Run(icon_url);
+  EXPECT_EQ("", runner.murmur2_hash());
+}
+
 // Test that the hash callback is called with an empty string if an HTTP error
 // prevents the icon URL from being fetched.
 TEST_F(WebApkIconHasherTest, HTTPError) {

chrome/browser/android/webapk/webapk_installer.cc

@@ -450,27 +450,15 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
 }
 
 void WebApkInstaller::DownloadAppIconAndComputeMurmur2Hash() {
-  // Safeguard. WebApkIconHasher crashes if asked to fetch an invalid URL.
-  if (!shortcut_info_.best_primary_icon_url.is_valid()) {
-    OnResult(WebApkInstallResult::FAILURE);
-    return;
-  }
-
-  timer_.Start(
-      FROM_HERE, base::TimeDelta::FromMilliseconds(download_timeout_ms_),
-      base::Bind(&WebApkInstaller::OnResult, weak_ptr_factory_.GetWeakPtr(),
-                 WebApkInstallResult::FAILURE));
-
-  icon_hasher_.reset(new WebApkIconHasher());
-  icon_hasher_->DownloadAndComputeMurmur2Hash(
+  icon_hasher_.reset(new WebApkIconHasher(
       request_context_getter_, shortcut_info_.best_primary_icon_url,
       base::Bind(&WebApkInstaller::OnGotIconMurmur2Hash,
-                 weak_ptr_factory_.GetWeakPtr()));
+                 weak_ptr_factory_.GetWeakPtr())));
+  icon_hasher_->DownloadAndComputeMurmur2Hash();
 }
 
 void WebApkInstaller::OnGotIconMurmur2Hash(
     const std::string& icon_murmur2_hash) {
-  timer_.Stop();
   icon_hasher_.reset();
 
   // An empty hash indicates that |icon_hasher_| encountered an error.

chrome/browser/android/webapk/webapk_update_data_fetcher.cc

@@ -152,13 +152,14 @@ void WebApkUpdateDataFetcher::OnDidGetInstallableData(
   info_.best_primary_icon_url = data.primary_icon_url;
   best_primary_icon_ = *data.primary_icon;
 
-  icon_hasher_.reset(new WebApkIconHasher());
   Profile* profile =
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-  icon_hasher_->DownloadAndComputeMurmur2Hash(
+
+  icon_hasher_.reset(new WebApkIconHasher(
       profile->GetRequestContext(), data.primary_icon_url,
       base::Bind(&WebApkUpdateDataFetcher::OnGotIconMurmur2Hash,
-                 weak_ptr_factory_.GetWeakPtr()));
+                 weak_ptr_factory_.GetWeakPtr())));
+  icon_hasher_->DownloadAndComputeMurmur2Hash();
 }
 
 void WebApkUpdateDataFetcher::OnGotIconMurmur2Hash(