Take Murmur2 hash of untransformed icon when creating WebAPK part 1

chrome/browser/android/webapk/webapk_installer.cc

Index: chrome/browser/android/webapk/webapk_installer.cc
diff --git a/chrome/browser/android/webapk/webapk_installer.cc b/chrome/browser/android/webapk/webapk_installer.cc
index 417ffc62f21aa1485d682f37c1e023cc807f0770..5b6b4b2ba2fc81df9bf5436b09493cbd5fb2ace7 100644
--- a/chrome/browser/android/webapk/webapk_installer.cc
+++ b/chrome/browser/android/webapk/webapk_installer.cc
@@ -13,7 +13,6 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -21,15 +20,14 @@
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/android/shortcut_helper.h"
 #include "chrome/browser/android/webapk/webapk.pb.h"
+#include "chrome/browser/android/webapk/webapk_icon_hasher.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/common/manifest_util.h"
 #include "jni/WebApkInstaller_jni.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
-#include "third_party/smhasher/src/MurmurHash2.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
 
@@ -42,9 +40,6 @@ const char kDefaultWebApkServerUrl[] =
 // The MIME type of the POST data sent to the server.
 const char kProtoMimeType[] = "application/x-protobuf";
 
-// The seed to use the murmur2 hash of the app icon.
-const uint32_t kMurmur2HashSeed = 0;
-
 // The default number of milliseconds to wait for the WebAPK download URL from
 // the WebAPK server.
 const int kWebApkDownloadUrlTimeoutMs = 60000;
@@ -61,15 +56,6 @@ GURL GetScope(const ShortcutInfo& info) {
       : ShortcutHelper::GetScopeFromURL(info.url);
 }
 
-// Computes a murmur2 hash of |bitmap|'s PNG encoded bytes.
-std::string ComputeBitmapHash(const SkBitmap& bitmap) {
-  std::vector<unsigned char> png_bytes;
-  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &png_bytes);
-  uint64_t hash =
-      MurmurHash64B(&png_bytes.front(), png_bytes.size(), kMurmur2HashSeed);
-  return base::Uint64ToString(hash);
-}
-
 // Converts a color from the format specified in content::Manifest to a CSS
 // string.
 std::string ColorToString(int64_t color) {
@@ -84,6 +70,46 @@ std::string ColorToString(int64_t color) {
   return base::StringPrintf("rgba(%d,%d,%d,%.2f)", r, g, b, a);
 }
 
+// Populates webapk::WebApk and returns it.
+// Must be called on a worker thread because it encodes an SkBitmap.
+std::unique_ptr<webapk::WebApk> BuildWebApkProtoInBackground(
+    const ShortcutInfo& shortcut_info,
+    const SkBitmap& shortcut_icon,
+    const std::string& shortcut_icon_murmur2_hash) {
+  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
+
+  std::unique_ptr<webapk::WebApk> webapk(new webapk::WebApk);
+  webapk->set_manifest_url(shortcut_info.manifest_url.spec());
+  webapk->set_requester_application_package(
+      base::android::BuildInfo::GetInstance()->package_name());
+  webapk->set_requester_application_version(version_info::GetVersionNumber());
+
+  webapk::WebAppManifest* web_app_manifest = webapk->mutable_manifest();
+  web_app_manifest->set_name(base::UTF16ToUTF8(shortcut_info.name));
+  web_app_manifest->set_short_name(
+      base::UTF16ToUTF8(shortcut_info.short_name));
+  web_app_manifest->set_start_url(shortcut_info.url.spec());
+  web_app_manifest->set_orientation(
+      content::WebScreenOrientationLockTypeToString(
+          shortcut_info.orientation));
+  web_app_manifest->set_display_mode(
+      content::WebDisplayModeToString(shortcut_info.display));
+  web_app_manifest->set_background_color(
+      ColorToString(shortcut_info.background_color));
+  web_app_manifest->set_theme_color(ColorToString(shortcut_info.theme_color));
+
+  std::string* scope = web_app_manifest->add_scopes();
+  scope->assign(GetScope(shortcut_info).spec());
+  webapk::Image* image = web_app_manifest->add_icons();
+  image->set_src(shortcut_info.icon_url.spec());
+  image->set_hash(shortcut_icon_murmur2_hash);
+  std::vector<unsigned char> png_bytes;
+  gfx::PNGCodec::EncodeBGRASkBitmap(shortcut_icon, false, &png_bytes);
+  image->set_image_data(&png_bytes.front(), png_bytes.size());
+
+  return webapk;
+}
+
 // Returns task runner for running background tasks.
 scoped_refptr<base::TaskRunner> GetBackgroundTaskRunner() {
   return content::BrowserThread::GetBlockingPool()
@@ -122,18 +148,20 @@ void WebApkInstaller::InstallAsyncWithURLRequestContextGetter(
   request_context_getter_ = request_context_getter;
   finish_callback_ = finish_callback;
 
-  // base::Unretained() is safe because WebApkInstaller owns itself and does not
-  // start the timeout timer till after SendCreateWebApkRequest() is called.
-  scoped_refptr<base::TaskRunner> background_task_runner =
-      content::BrowserThread::GetBlockingPool()
-          ->GetTaskRunnerWithShutdownBehavior(
-              base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
-  base::PostTaskAndReplyWithResult(
-      background_task_runner.get(), FROM_HERE,
-      base::Bind(&WebApkInstaller::BuildWebApkProtoInBackground,
-                 base::Unretained(this)),
-      base::Bind(&WebApkInstaller::SendCreateWebApkRequest,
-                 base::Unretained(this)));
+  if (shortcut_info_.icon_url.is_valid()) {
+    // We need to take the hash of the bitmap at the icon URL prior to any
+    // transformations being applied to the bitmap (such as encoding/decoding
+    // the bitmap). The WebAPK server crawls the internet and generates WebAPKs

[Yaron, 2016/08/12 21:24:05]

it doesn't actually crawl the internet per-say and it's an implementation
detail. I would simplify it and just say the hash is used for caching. Please
also update CL description accordingly

+    // for any Web Manifests that it finds. The icon hash is used to determine
+    // whether the icon that the user sees matches the icon of a WebAPK that the
+    // WebAPK server generated while crawling the internet.
+    //
+    // We redownload the icon in order to take the Murmur2 hash. The redownload
+    // should be fast because the icon should be in the HTTP cache.

[Yaron, 2016/08/12 21:24:05]

have you tested that locally? is there a way to set flags so that it only loads
from cache?

[pkotwicz, 2016/08/15 17:19:26]

Yes, I have tested locally and the icon is loaded from the cache. (I checked
URLFetcher::IsCached())

There is a flag so that URLFetcher loads only from the cache:
net::ONLY_FROM_CACHE. I don't want to use it though. (I don't know whether it is
possible for the icon data to be evicted from the cache)

+    DownloadAppIconAndComputeMurmur2Hash();
+    return;
+  }
+  SendCreateWebApkRequest();
 }
 
 void WebApkInstaller::SetTimeoutMs(int timeout_ms) {
@@ -176,7 +204,44 @@ void WebApkInstaller::OnURLFetchComplete(const net::URLFetcher* source) {
   OnGotWebApkDownloadUrl(signed_download_url, response->package_name());
 }
 
-void WebApkInstaller::SendCreateWebApkRequest(
+void WebApkInstaller::DownloadAppIconAndComputeMurmur2Hash() {
+  timer_.Start(
+      FROM_HERE, base::TimeDelta::FromMilliseconds(download_timeout_ms_),
+      base::Bind(&WebApkInstaller::OnTimeout, weak_ptr_factory_.GetWeakPtr()));
+
+  icon_hasher_.reset(new WebApkIconHasher());
+  icon_hasher_->DownloadAndGetMurmur2Hash(
+      request_context_getter_, shortcut_info_.icon_url,
+      base::Bind(&WebApkInstaller::OnGotIconMurmur2Hash,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void WebApkInstaller::OnGotIconMurmur2Hash(
+    const std::string& icon_murmur2_hash) {
+  timer_.Stop();
+  icon_hasher_.reset();
+
+  shortcut_icon_murmur2_hash_ = icon_murmur2_hash;
+
+  // An empty hash indicates that |icon_hasher_| encountered an error.
+  if (icon_murmur2_hash.empty()) {
+    OnFailure();
+    return;
+  }
+
+  SendCreateWebApkRequest();
+}
+
+void WebApkInstaller::SendCreateWebApkRequest() {
+  base::PostTaskAndReplyWithResult(
+      GetBackgroundTaskRunner().get(), FROM_HERE,
+      base::Bind(&BuildWebApkProtoInBackground, shortcut_info_, shortcut_icon_,
+                 shortcut_icon_murmur2_hash_),
+      base::Bind(&WebApkInstaller::SendCreateWebApkRequestWithProto,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void WebApkInstaller::SendCreateWebApkRequestWithProto(
     std::unique_ptr<webapk::WebApk> webapk_proto) {
   timer_.Start(
       FROM_HERE,
@@ -254,54 +319,18 @@ void WebApkInstaller::OnWebApkMadeWorldReadable(
     OnFailure();
 }
 
-std::unique_ptr<webapk::WebApk>
-WebApkInstaller::BuildWebApkProtoInBackground() {
-  DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
-
-  std::unique_ptr<webapk::WebApk> webapk(new webapk::WebApk);
-  webapk->set_manifest_url(shortcut_info_.manifest_url.spec());
-  webapk->set_requester_application_package(
-      base::android::BuildInfo::GetInstance()->package_name());
-  webapk->set_requester_application_version(version_info::GetVersionNumber());
-
-  webapk::WebAppManifest* web_app_manifest = webapk->mutable_manifest();
-  web_app_manifest->set_name(base::UTF16ToUTF8(shortcut_info_.name));
-  web_app_manifest->set_short_name(
-      base::UTF16ToUTF8(shortcut_info_.short_name));
-  web_app_manifest->set_start_url(shortcut_info_.url.spec());
-  web_app_manifest->set_orientation(
-      content::WebScreenOrientationLockTypeToString(
-          shortcut_info_.orientation));
-  web_app_manifest->set_display_mode(
-      content::WebDisplayModeToString(shortcut_info_.display));
-  web_app_manifest->set_background_color(
-      ColorToString(shortcut_info_.background_color));
-  web_app_manifest->set_theme_color(ColorToString(shortcut_info_.theme_color));
-
-  std::string* scope = web_app_manifest->add_scopes();
-  scope->assign(GetScope(shortcut_info_).spec());
-  webapk::Image* image = web_app_manifest->add_icons();
-  image->set_src(shortcut_info_.icon_url.spec());
-  // TODO(pkotwicz): Get Murmur2 hash of untransformed icon's bytes (with no
-  // encoding/decoding).
-  image->set_hash(ComputeBitmapHash(shortcut_icon_));
-  std::vector<unsigned char> png_bytes;
-  gfx::PNGCodec::EncodeBGRASkBitmap(shortcut_icon_, false, &png_bytes);
-  image->set_image_data(&png_bytes.front(), png_bytes.size());
-
-  return webapk;
-}
-
 void WebApkInstaller::OnTimeout() {
   OnFailure();
 }
 
 void WebApkInstaller::OnSuccess() {
-  finish_callback_.Run(true);
+  FinishCallback callback = finish_callback_;
   delete this;
+  callback.Run(true);
 }
 
 void WebApkInstaller::OnFailure() {
-  finish_callback_.Run(false);
+  FinishCallback callback = finish_callback_;
   delete this;
+  callback.Run(false);
 }