Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

chrome/browser/android/webapk/webapk_icon_hasher.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_ANDROID_WEBAPK_WEBAPK_ICON_HASHER_H_
 #define CHROME_BROWSER_ANDROID_WEBAPK_WEBAPK_ICON_HASHER_H_
 
 #include <memory>
 #include <string>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/timer/timer.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
 namespace net {
 class URLFetcher;
 class URLRequestContextGetter;
 }
 
 // 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() override;

[dominickn, 2017/03/29 23:27:00]

Can this be declared private? Or is it called by the URLFetcherDelegate?

[F, 2017/03/30 00:20:13]

Done. This can be declared private.

+
+  // Creates a self-owned WebApkIconHasher instance. The instance downloads
+  // |icon_url| and 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).
+  static void DownloadAndComputeMurmur2Hash(
+      net::URLRequestContextGetter* request_context_getter,
+      const GURL& icon_url,
+      const Murmur2HashCallback& callback);
+
+ private:
   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();

[dominickn, 2017/03/29 23:27:00]

You could remove this method and do all of its work in the constructor. Then you
don't need to store either icon_url_ or the net::URLRequestContextGetter

[F, 2017/03/30 00:20:13]

After experimenting with it, I think maybe we should keep this function. In
order to "delete" itself in the constructor, WebApkIconHasher needs to call a
new static function that calls the callback and then deletes a WebApkIconHasher
instance. Additionally,  the unused WebApkIconHasher instance would trigger
"unused variable" error during compilation.

WDUT?

[dominickn, 2017/03/30 00:26:46]

That's a good point. You could get around it by moving the icon_url_ validity
and data URL checks into the static
WebApkIconHasher::DownloadAndComputeMurmur2Hash(). Then you never have to
allocate a WebApkIconHasher for those cases. How does that sound?

[pkotwicz, 2017/03/30 14:47:25]

In my opinion, we should try to avoid having constructors doing a a lot of work.
I like the way the code is currently structured.

We could move some of the processing from
WebApkIconHasher::DownloadAndComputeMurmur2Hash() to the static
WebApkIconHasher::DownloadAndComputeMurmur2Hash() but I don't see much point

We should have a new method RunCallback() which is responsible for both running
the callback and deleting the WebApkIconHasher (so that none of the callsites
forget to delete the WebApkIconHasher)

[F, 2017/03/30 15:52:46]

For now I kept the current code structure and added RunCallback. WDUT?

If we move everything into the constructor, we'll split the logic into static
function and constructor. Also we'll "new" a WebApkIconHasher without assigning
it to any variable. I'd prefer keeping the logic together and call a member
function, similar to what WebApkInstaller does now.

 
- private:
   // net::URLFetcherDelegate:
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
   // 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);
 };
 
 #endif  // CHROME_BROWSER_ANDROID_WEBAPK_WEBAPK_ICON_HASHER_H_