Add static method WebApkIconHasher::DownloadAndComputeMurmur2Hash

chrome/browser/android/webapk/webapk_icon_hasher.h

Index: chrome/browser/android/webapk/webapk_icon_hasher.h
diff --git a/chrome/browser/android/webapk/webapk_icon_hasher.h b/chrome/browser/android/webapk/webapk_icon_hasher.h
index 4a337106990f5d977c70017b18143d053a1906c2..9e8a242a4d51f354b28b96ccc1fb8ab2af0c20f6 100644
--- a/chrome/browser/android/webapk/webapk_icon_hasher.h
+++ b/chrome/browser/android/webapk/webapk_icon_hasher.h
@@ -25,11 +25,23 @@ class WebApkIconHasher : public net::URLFetcherDelegate {
   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
@@ -37,7 +49,6 @@ class WebApkIconHasher : public net::URLFetcherDelegate {
   // 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;