Take Murmur2 hash of untransformed icon when creating WebAPK part 2/2

chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc

Index: chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc
diff --git a/chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc b/chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc
index 447b0a57ee2f2144d18732dc16fbad06a17af986..c3ceef956e96f0db6e55860f9e4f43cafefd92b4 100644
--- a/chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc
+++ b/chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc
@@ -9,9 +9,10 @@
 
 #include "base/android/jni_string.h"
 #include "chrome/browser/android/shortcut_helper.h"
-#include "chrome/browser/android/shortcut_info.h"
+#include "chrome/browser/android/webapk/webapk_icon_hasher.h"
 #include "chrome/browser/android/webapk/webapk_web_manifest_checker.h"
 #include "chrome/browser/installable/installable_manager.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/manifest.h"
@@ -26,16 +27,6 @@ using base::android::ScopedJavaLocalRef;
 
 namespace {
 
-// The seed to use for the murmur2 hash.
-const uint64_t kMurmur2HashSeed = 0;
-
-// Computes a murmur2 hash of |bitmap|'s PNG encoded bytes.
-uint64_t ComputeBitmapHash(const SkBitmap& bitmap) {
-  std::vector<unsigned char> png_bytes;
-  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &png_bytes);
-  return MurmurHash64B(&png_bytes.front(), png_bytes.size(), kMurmur2HashSeed);
-}
-
 // Returns whether the given |url| is within the scope of the |scope| url.
 bool IsInScope(const GURL& url, const GURL& scope) {
   return base::StartsWith(url.spec(), scope.spec(),
@@ -64,6 +55,7 @@ ManifestUpgradeDetectorFetcher::ManifestUpgradeDetectorFetcher(
     : content::WebContentsObserver(nullptr),
       scope_(scope),
       web_manifest_url_(web_manifest_url),
+      info_(GURL()),
       weak_ptr_factory_(this) {
   java_ref_.Reset(env, obj);
 }
@@ -134,22 +126,42 @@ void ManifestUpgradeDetectorFetcher::OnDidGetInstallableData(
   if (data.manifest.IsEmpty() || web_manifest_url_ != data.manifest_url)
     return;
 
-  // TODO(pkotwicz): Tell Java side that the Web Manifest was fetched but the
-  // Web Manifest is not WebAPK-compatible. (http://crbug.com/639536)
-  if (!data.is_installable ||
+  if (data.error_code != NO_ERROR_DETECTED ||

[Xi Han, 2016/09/12 19:19:05]

Why don't check data.is_installable?

[pkotwicz, 2016/09/12 19:43:59]

Because data.is_installable is true if there is a problem with the icon

AppBannerManager::OnDidPerformInstallableCheck() also checks the error code

[Xi Han, 2016/09/12 19:58:49]

Got it, thanks!

       !AreWebManifestUrlsWebApkCompatible(data.manifest)) {
+    OnGotNonWebApkCompatibleManifest();
+    return;
+  }
+
+  info_.UpdateFromManifest(data.manifest);
+  info_.manifest_url = data.manifest_url;
+  info_.icon_url = data.icon_url;
+  icon_ = *data.icon;
+
+  icon_hasher_.reset(new WebApkIconHasher());
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  icon_hasher_->DownloadAndComputeMurmur2Hash(

[dominickn, 2016/09/13 01:09:35]

We already have the parsed icon in data.icon and icon_. It seems quite wasteful
to download it again. Is it possible to split up downloading and computing the
hash so you can just pass the SkBitmap and use its bytes? You would only be
accessing bytes, not decoding anything, so I don't think there's a security
issue.

That may also have the advantage of making this synchronous, so you could just
stack-initialise icon_hasher_ rather than having it as a member.

[pkotwicz, 2016/09/13 22:23:33]

An SkBitmap is a decoded icon. The SkBitmap does not know about the image's
source bytes.

I considered modifying WebContents::DownloadImage() and ImageDownloadCallback to
return a Murmur2 hash of the icon's unmodified bytes. I decided against this
because WebAPKs are an experimental feature.

The icon downloaded by WebApkIconHasher should be in the HTTP cache so the
download should be cheap (no additional network traffic).

I agree that this is wasteful but I do not have a better idea. Dominick, do you
have any ideas?

[dominickn, 2016/09/14 06:58:43]

Acknowledged. I was worried that would be the case. Making
WebContents::DownloadImage return a hash is also suboptimal because then there
would be redundant work for a bunch of other places which use that method.

Instead, would it be sufficient to change InstallableManager and have it run
ManifestIconSelector::FindBestMatchingIcon when
InstallableParams::check_installable is true? Then the upgrade detector would
not fetch an icon, but it would have the URL which it could then pass off to the
hasher. If the hash comes back different we'd need to run InstallableManager
again to get the icon.

This doesn't solve the redownloading when you install the WebAPK initially to
compute the hash for the first time, but it'll make each subsequent upgrade
request more efficient (I'm imagining that WebAPKs won't need to be upgraded too
often, so this will halve the number of image bytes each upgrade request has to
fetch). WDYT?

By the way, this proposal shouldn't be in this CL - we should have metrics for
how often the upgrade detection runs, and how often it says that an upgrade is
needed (and what the reason for the upgrade is). If we find that upgrades hardly
ever happen, it'll be worth optimizing the data they consume. Are there plans to
add upgrade request metrics?

+      profile->GetRequestContext(),
+      data.icon_url,
+      base::Bind(&ManifestUpgradeDetectorFetcher::OnGotIconMurmur2Hash,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ManifestUpgradeDetectorFetcher::OnGotIconMurmur2Hash(
+    const std::string& icon_murmur2_hash) {
+  icon_hasher_.reset();
+
+  if (icon_murmur2_hash.empty()) {
+    OnGotNonWebApkCompatibleManifest();
     return;
   }
 
-  ShortcutInfo info(GURL::EmptyGURL());
-  info.UpdateFromManifest(data.manifest);
-  info.manifest_url = data.manifest_url;
-  info.icon_url = data.icon_url;
-  OnDataAvailable(info, (data.icon ? *data.icon : SkBitmap()));
+  OnGotWebApkCompatibleManifest(info_, icon_murmur2_hash, icon_);
 }
 
-void ManifestUpgradeDetectorFetcher::OnDataAvailable(
+void ManifestUpgradeDetectorFetcher::OnGotWebApkCompatibleManifest(
     const ShortcutInfo& info,
+    const std::string& icon_murmur2_hash,
     const SkBitmap& icon_bitmap) {
   JNIEnv* env = base::android::AttachCurrentThread();
 
@@ -163,17 +175,19 @@ void ManifestUpgradeDetectorFetcher::OnDataAvailable(
       base::android::ConvertUTF16ToJavaString(env, info.short_name);
   ScopedJavaLocalRef<jstring> java_icon_url =
       base::android::ConvertUTF8ToJavaString(env, info.icon_url.spec());
-  ScopedJavaLocalRef<jobject> java_bitmap;
-  uint64_t icon_murmur2_hash = 0;
-  if (icon_bitmap.getSize()) {
-    java_bitmap = gfx::ConvertToJavaBitmap(&icon_bitmap);
-    // TODO(pkotwicz): Get hash of untransformed icon's bytes (with no
-    // encoding/decoding).
-    icon_murmur2_hash = ComputeBitmapHash(icon_bitmap);
-  }
+  ScopedJavaLocalRef<jstring> java_icon_murmur2_hash =
+      base::android::ConvertUTF8ToJavaString(env, icon_murmur2_hash);
+  ScopedJavaLocalRef<jobject> java_bitmap =
+      gfx::ConvertToJavaBitmap(&icon_bitmap);
 
-  Java_ManifestUpgradeDetectorFetcher_onDataAvailable(
+  Java_ManifestUpgradeDetectorFetcher_onGotWebApkCompatibleManifest(
       env, java_ref_, java_url, java_scope, java_name, java_short_name,
-      java_icon_url, icon_murmur2_hash, java_bitmap, info.display,
+      java_icon_url, java_icon_murmur2_hash, java_bitmap, info.display,
       info.orientation, info.theme_color, info.background_color);
 }
+
+void ManifestUpgradeDetectorFetcher::OnGotNonWebApkCompatibleManifest() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_ManifestUpgradeDetectorFetcher_onGotNonWebApkCompatibleManifest(
+      env, java_ref_);
+}