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

chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java

Index: chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
index 1f6480ffcaabcd9ffc4df66adf6589ed1347414f..bb9a00a123fa5084a41cd943f7fcc1465b772d8c 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorFetcherTest.java
@@ -45,17 +45,29 @@ public class ManifestUpgradeDetectorFetcherTest extends ChromeTabbedActivityTest
     // callback is called.
     private static class CallbackWaiter
             extends CallbackHelper implements ManifestUpgradeDetectorFetcher.Callback {
+        private boolean mIsWebApkCompatible;
         private String mName;
 
         @Override
         public void onGotManifestData(String startUrl, String scopeUrl, String name,
-                String shortName, String iconUrl, long iconMurmur2Hash, Bitmap iconBitmap,
+                String shortName, String iconUrl, String iconMurmur2Hash, Bitmap iconBitmap,
                 int displayMode, int orientation, long themeColor, long backgroundColor) {
             assertNull(mName);
             mName = name;
+            mIsWebApkCompatible = true;
             notifyCalled();
         }
 
+        @Override
+        public void onGotNonWebApkCompatibleManifest() {
+            mIsWebApkCompatible = false;
+            notifyCalled();
+        }
+
+        public boolean isWebApkCompatible() {
+            return mIsWebApkCompatible;
+        }
+
         public String name() {
             return mName;
         }
@@ -112,6 +124,7 @@ public class ManifestUpgradeDetectorFetcherTest extends ChromeTabbedActivityTest
         waiter.waitForCallback(0);
 
         assertEquals(WEB_MANIFEST_NAME1, waiter.name());
+        assertTrue(waiter.isWebApkCompatible());
     }
 
     /**
@@ -132,5 +145,44 @@ public class ManifestUpgradeDetectorFetcherTest extends ChromeTabbedActivityTest
         waiter.waitForCallback(0);
 
         assertEquals(WEB_MANIFEST_NAME2, waiter.name());
+        assertTrue(waiter.isWebApkCompatible());
+    }
+
+    /**
+     * Test that ManifestUpgradeDetectorFetcher classifies a page as "not WebAPK compatible" if it
+     * does not have a service worker.
+     */
+    @MediumTest
+    @Feature({"Webapps"})
+    public void testNoServiceWorkerNotInstallable() throws Exception {

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

This test doesn't seem to be related to the changes here, it is more likely a
test for InstallableManager. If you want to test whether the
OnGotNonWebApkCompatibleManifest() is called, why not try the case of an empty
murmur2Hash or Manifest URL has a passwords?

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

Yes, there are many error cases. The service worker error case is one of many.
It is convenient because there is already test data for this error case. (I
could write many more tests)

There are already browser tests for InstallableManager such as
InstallableManagerBrowserTest.CheckManifestCorruptedIcon

I did not pick a different scenario to test (such as a URL in the manifest using
a password because I am lazy)

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

I understand since you added the remaining parts of CL
(https://codereview.chromium.org/2262583002/) into this CL, and the tests are
more likely for that code, rather than the murmur2Hash used by
ManifestUpgradeDetector. Maybe add that part into the CL description as well. I
think it helps to reduce confusion why the tests doesn't seem to be related to
the CL title.

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

I'd prefer if you split these tests into a separate C, since they are
orthogonal. It also protects the rest of the CL from having to be reverted in
the case where something goes wrong with one of the unrelated tests (or
vice-versa) :)

+        CallbackWaiter waiter = new CallbackWaiter();
+        startManifestUpgradeDetectorFetcher(mTestServer.getURL(WEB_MANIFEST_SCOPE),
+                mTestServer.getURL("/chrome/test/data/banners/manifest.json"), waiter);
+
+        TabLoadObserver tabLoadObserver = new TabLoadObserver(mTab);
+        tabLoadObserver.fullyLoadUrl(
+                mTestServer.getURL("/chrome/test/data/banners/manifest_no_service_worker.html"));
+        waiter.waitForCallback(0);
+
+        assertFalse(waiter.isWebApkCompatible());
+    }
+
+    /**
+     * Test that ManifestUpgradeDetectorFetcher classifies a Web Manifest as "not WebAPK compatible"
+     * if the Web Manifest points to a corrupted icon.
+     */
+    @MediumTest
+    @Feature({"Webapps"})
+    public void testCorruptedIconNotInstallable() throws Exception {

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

This sounds like another test for data.is_installable check. I am confused when
trying to understand the purpose of these two tests. Do you want to test the
logic in ManifestUpgradeDetectorFetcher::OnDidGetInstallableData() and
ManifestUpgradeDetectorFetcher::OnGotIconMurmur2Hash() for the failure cases
that calling OnGotNonWebApkCompatibleManifest()? These two seems are both
covered by the check: if (data.error_code != NO_ERROR_DETECTED ||...) {
  OnGotNonWebApkCompatibleManifest();
}

Sounds like the second one is good enough for this check. If adding more tests,
it might be more straight forwards to mock the value of the murmur2Hash for the
check: if (icon_murmur2_hash.empty()); or to cover a case of
!AreWebManifestUrlsWebApkCompatible(). Any thought?

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

The reason that I have two tests is that the corrupted icon case is very
special. When the icon is corrupted

InstallableData::is_installable == true but
InstallableData::error_code != NO_ERROR_DETECTED

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

Agree, this one is definitely needs to be kept.

+        CallbackWaiter waiter = new CallbackWaiter();
+        startManifestUpgradeDetectorFetcher(mTestServer.getURL(WEB_MANIFEST_SCOPE),
+                mTestServer.getURL("/chrome/test/data/banners/manifest_bad_icon.json"), waiter);
+
+        TabLoadObserver tabLoadObserver = new TabLoadObserver(mTab);
+        tabLoadObserver.fullyLoadUrl(
+                mTestServer.getURL("/chrome/test/data/banners/manifest_bad_icon_test_page.html"));
+        waiter.waitForCallback(0);
+
+        assertFalse(waiter.isWebApkCompatible());
     }
 }