Move timeout timer into WebApkIconHasher

chrome/browser/android/webapk/webapk_icon_hasher.cc

Index: chrome/browser/android/webapk/webapk_icon_hasher.cc
diff --git a/chrome/browser/android/webapk/webapk_icon_hasher.cc b/chrome/browser/android/webapk/webapk_icon_hasher.cc
index c685e08496ba5099e3fe8ac550af9eca5317ab21..6e1b519164b0d3ed9dc00236e51c7444a0e420a2 100644
--- a/chrome/browser/android/webapk/webapk_icon_hasher.cc
+++ b/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,50 @@ std::string ComputeMurmur2Hash(const std::string& raw_image_data) {
 
 }  // anonymous namespace
 
-WebApkIconHasher::WebApkIconHasher() {}
+WebApkIconHasher::WebApkIconHasher(
+    net::URLRequestContextGetter* url_request_context_getter,

[dominickn, 2017/03/27 23:27:33]

It looks like everywhere this object is used, it's immediately created and then
DownloadAndComputeMurmur2Hash() is called.

A suggestion for future refactoring (either here or in a follow up):

 * a static method
WebApkIconHasher::DownloadAndComputeMurmur2Hash(context_getter, url, callback)
 * which new-allocates a WebApkIconHasher (which now has a private constructor)
 * which creates and starts the url_fetcher_ in its constructor
 * and calls delete this in either OnURLFetchComplete or OnDownloadTimedOut once
callback is posted

This is the same pattern used in WebApkInstaller, so it means that the object
entirely manages its own lifetime.

Not blocking this CL on this suggestion though.

[F, 2017/03/29 20:00:49]

Thanks for the suggestion! I'll save it for future refactoring.

+    const GURL& icon_url,
+    const Murmur2HashCallback& callback)
+    : url_request_context_getter_(url_request_context_getter),
+      icon_url_(icon_url),
+      callback_(callback),
+      weak_ptr_factory_(this) {}
 
 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,
+                 weak_ptr_factory_.GetWeakPtr()));
+
+  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 +92,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_, ""));
+}