Update WebAPK if homescreen icon changed.

chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java

Index: chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
index c953bb05423117af96098b6a7de3c0300229db20..60490c6b101ed7969b98a781c7e5dfaa6e7dfcdf 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/webapps/ManifestUpgradeDetectorTest.java
@@ -7,10 +7,13 @@ package org.chromium.chrome.browser.webapps;
 import android.content.Context;
 import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageInfo;
+import android.graphics.Bitmap;
+import android.graphics.Color;
 import android.os.Bundle;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.blink_public.platform.WebDisplayMode;
+import org.chromium.chrome.browser.ShortcutHelper;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.content_public.common.ScreenOrientationValues;
 import org.chromium.testing.local.LocalRobolectricTestRunner;
@@ -25,6 +28,7 @@ import org.mockito.stubbing.Answer;
 import org.robolectric.Robolectric;
 import org.robolectric.annotation.Config;
 import org.robolectric.res.builder.RobolectricPackageManager;
+import org.robolectric.shadows.ShadowBitmap;
 
 /**
  * Tests the ManifestUpgradeDetector.
@@ -37,6 +41,8 @@ public class ManifestUpgradeDetectorTest {
     private static final String WEBAPK_SCOPE_URL = "/";
     private static final String WEBAPK_NAME = "Long Name";
     private static final String WEBAPK_SHORT_NAME = "Short Name";
+    private static final String WEBAPK_ICON_URL = "/icon.png";
+    private static final long WEBAPK_ICON_MURMUR2_HASH = 3L;
     private static final int WEBAPK_DISPLAY_MODE = WebDisplayMode.Standalone;
     private static final int WEBAPK_ORIENTATION = ScreenOrientationValues.LANDSCAPE;
     private static final long WEBAPK_THEME_COLOR = 1L;
@@ -54,6 +60,9 @@ public class ManifestUpgradeDetectorTest {
         public String scopeUrl = WEBAPK_SCOPE_URL;
         public String name = WEBAPK_NAME;
         public String shortName = WEBAPK_SHORT_NAME;
+        public String iconUrl = WEBAPK_ICON_URL;
+        public long iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH;
+        public Bitmap icon = createBitmap(Color.GREEN);
         public int displayMode = WEBAPK_DISPLAY_MODE;
         public int orientation = WEBAPK_ORIENTATION;
         public long themeColor = WEBAPK_THEME_COLOR;
@@ -69,6 +78,7 @@ public class ManifestUpgradeDetectorTest {
      */
     private static class TestManifestUpgradeDetector extends ManifestUpgradeDetector {
         private FetchedData mFetchedData;
+        public String mRequestedIconUrl;
         public boolean mIsUpgraded;
         public boolean mCompleted;
 
@@ -87,9 +97,9 @@ public class ManifestUpgradeDetectorTest {
                     ManifestUpgradeDetectorFetcher.Callback callback =
                             (ManifestUpgradeDetectorFetcher.Callback) invocation.getArguments()[0];
                     callback.onGotManifestData(mFetchedData.startUrl, mFetchedData.scopeUrl,
-                            mFetchedData.name, mFetchedData.shortName, mFetchedData.displayMode,
-                            mFetchedData.orientation, mFetchedData.themeColor,
-                            mFetchedData.backgroundColor);
+                            mFetchedData.name, mFetchedData.shortName, mFetchedData.iconUrl,
+                            mFetchedData.displayMode, mFetchedData.orientation,
+                            mFetchedData.themeColor, mFetchedData.backgroundColor);
                     return null;
                 }
             };
@@ -99,6 +109,13 @@ public class ManifestUpgradeDetectorTest {
         }
 
         @Override
+        protected void fetchHomescreenImage(
+                String imageUrl, ShortcutHelper.FetchHomescreenImageCallback callback) {
+            mRequestedIconUrl = imageUrl;
+            callback.onFetchedHomescreenImage(mFetchedData.icon, mFetchedData.iconMurmur2Hash);
+        }
+
+        @Override
         protected void upgrade() {
             mIsUpgraded = true;
         }
@@ -109,13 +126,22 @@ public class ManifestUpgradeDetectorTest {
         }
     }
 
+    /**
+     * Creates 1x1 bitmap.
+     * @param color The bitmap color.
+     */
+    private static Bitmap createBitmap(int color) {
+        int colors[] = { color };
+        return ShadowBitmap.createBitmap(colors, 1, 1, Bitmap.Config.ALPHA_8);
+    }
+
     @Before
     public void setUp() {
         Context context = Robolectric.application;
         ContextUtils.initApplicationContextForTests(context);
         mPackageManager = (RobolectricPackageManager) context.getPackageManager();
 
-        setMetaData(WEBAPK_START_URL);
+        setMetaData(WEBAPK_START_URL, WEBAPK_ICON_URL, WEBAPK_ICON_MURMUR2_HASH);
     }
 
     private TestManifestUpgradeDetector createDetector(FetchedData fetchedData) {
@@ -126,9 +152,12 @@ public class ManifestUpgradeDetectorTest {
         return new TestManifestUpgradeDetector(null, webappInfo, fetchedData);
     }
 
-    private void setMetaData(String startUrl) {
+    private void setMetaData(String startUrl, String iconUrl, long iconMurmur2Hash) {
         Bundle bundle = new Bundle();
         bundle.putString(ManifestUpgradeDetector.META_DATA_START_URL, startUrl);
+        bundle.putString(ManifestUpgradeDetector.META_DATA_ICON_URL, iconUrl);
+        bundle.putString(
+                ManifestUpgradeDetector.META_DATA_ICON_MURMUR2_HASH, iconMurmur2Hash + "L");
 
         ApplicationInfo appInfo = new ApplicationInfo();
         appInfo.metaData = bundle;
@@ -156,4 +185,138 @@ public class ManifestUpgradeDetectorTest {
         Assert.assertTrue(detector.mCompleted);
         Assert.assertTrue(detector.mIsUpgraded);
     }
+
+    /**
+     * Test that an upgrade is requested when:
+     * - WebAPK is generated using icon at {@link WEBAPK_ICON_URL} from Web Manifest.
+     * - Bitmap at {@link WEBAPK_ICON_URL} has changed.
+     */
+    @Test
+    public void testHomescreenIconChangeShouldUpgrade() {
+        FetchedData fetchedData = new FetchedData();
+        fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1;
+        fetchedData.icon = createBitmap(Color.BLUE);
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        Assert.assertEquals(WEBAPK_ICON_URL, detector.mRequestedIconUrl);
+        Assert.assertTrue(detector.mIsUpgraded);
+    }
+
+    /**
+     * Test that an upgrade is requested when:
+     * - WebAPK is generated using icon at {@link WEBAPK_ICON_URL} from Web Manifest.
+     * - Web Manifest is updated to refer to different icon.
+     */
+    @Test
+    public void testHomescreenIconUrlChangeShouldUpgrade() {
+        FetchedData fetchedData = new FetchedData();
+        fetchedData.iconUrl = "/icon2.png";
+        fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1;

[Xi Han, 2016/08/10 15:49:19]

As long as the |iconUrl| changes, the changes of |iconMurmur2Hash| and |icon|
aren't necessary here.

[pkotwicz, 2016/08/10 19:03:47]

I know that the other changes are unnecessary. I put in the other changes to
emulate as accurately as possible the Web Manifest pointing to a new icon URL
(It is unlikely that when the Web Manifest points to a new icon URL that the
bitmap at the new icon URL would be identical to the bitmap at the old icon URL)

[Xi Han, 2016/08/11 15:09:33]

We should avoid to add unnecessary changes to the test. If the test fails due to
these changes, the test name won't reflect the realy reason it fails.

[pkotwicz, 2016/08/11 20:02:01]

Ok, removed the unnecessary changes

+        fetchedData.icon = createBitmap(Color.YELLOW);
+
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        Assert.assertEquals("/icon2.png", detector.mRequestedIconUrl);
+        Assert.assertTrue(detector.mIsUpgraded);
+    }
+
+    /**
+     * Test that an upgrade is requested when:
+     * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at

[Xi Han, 2016/08/10 15:49:19]

If I understand correctly, the |WEBAPK_ICON_URL| was used when the WebAPK was
generated, while there are no icons in the manifest now. "There are no icons in
Web Manifest when WebAPK is generated." is confusing.

How about: "There are no icons in Web Manifest anymore. Page favicon at ..."

[pkotwicz, 2016/08/10 19:03:47]

I am actually trying to test the scenario of:
- Web Manifest did not have any icon URLs when the WebAPK was generated and
still does not have any icon URLs. When the WebAPK was generated we fell back to
using the favicon which is located at |WEBAPK_ICON_URL|. The bitmap at
|WEBAPK_ICON_URL| has changed.

You are right that if I were to write a test for the scenario of:
- Web Manifest had an icon when WebAPK was generated. The icon was located at
|WEBAPK_ICON_URL|.
- Web Manifest was changed and no longer has any icon URLs
- The bitmap at |WEBAPK_ICON_URL| has changed since the WebAPK was generated

that the test would look exactly the same (lines 234 - 245 would be the same)

I fixed up my grammar in the comments. I now use the past tense to refer to
things which have occurred in the past. I hope that this makes the comments
clearer.

[Xi Han, 2016/08/11 15:09:33]

Hmmm, you still use the default data for the old WebAPK, but give it a different
interpretation. It is not easy to understand that from the test description. It
would be more clear if the test has a data for old WebAPK, and we can set like:
icon_url = webapk_favicon_url; icon = favicon_icon; and then initializes the
fetchedData. I think then the tests will look like how you designed. At least,
add a TODO to add "mutable" data for old WebAPK.

[pkotwicz, 2016/08/11 20:02:02]

I have made the old data "mutable"
I have made the old data and the new data use a variable named "faviconUrl"
instead of a variable named "WEBAPK_ICON_URL"

+     *   {@link WEBAPK_ICON_URL} is used as the homescreen icon.
+     * - Bitmap at {@link WEBAPK_ICON_URL} has changed.
+     */
+    @Test
+    public void testFaviconHomescreenIconChangeShouldUpgrade() {
+        FetchedData fetchedData = new FetchedData();
+        // Still no icon specified in Web Manifest.
+        fetchedData.iconUrl = "";
+        // Updated data at {@link WEBAPK_ICON_URL}.
+        fetchedData.icon = createBitmap(Color.BLUE);
+        fetchedData.iconMurmur2Hash = WEBAPK_ICON_MURMUR2_HASH + 1;
+
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        Assert.assertEquals(WEBAPK_ICON_URL, detector.mRequestedIconUrl);
+        Assert.assertTrue(detector.mIsUpgraded);
+    }
+
+    /**
+     * Test that an upgrade is not requested when:
+     * - There are no icons in Web Manifest when WebAPK is generated. Page favicon at

[Xi Han, 2016/08/10 15:49:19]

Same here.

+     *   {@link WEBAPK_ICON_URL} is used as the homescreen icon.
+     * - Bitmap at {@link WEBAPK_ICON_URL} has not changed.
+     */
+    @Test
+    public void testFaviconHomescreenIconShouldNotUpgrade() {
+        FetchedData fetchedData = new FetchedData();
+        // Still no icon specified in Web Manifest.
+        fetchedData.iconUrl = "";
+
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        Assert.assertEquals(WEBAPK_ICON_URL, detector.mRequestedIconUrl);
+        Assert.assertFalse(detector.mIsUpgraded);
+    }
+
+    /**
+     * Test that an upgrade is not requested when:
+     * - WebAPK is generated using icon at {@link WEBAPK_ICON_URL} from Web Manifest.
+     * - Request for {@link WEBAPK_ICON_URL} returns HTTP 404 error.
+     */
+    @Test
+    public void test404HomescreenIconShouldNotUpgrade() {
+        FetchedData fetchedData = new FetchedData();
+        fetchedData.icon = null;
+        fetchedData.iconMurmur2Hash = 0L;
+
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        Assert.assertEquals(WEBAPK_ICON_URL, detector.mRequestedIconUrl);
+        Assert.assertFalse(detector.mIsUpgraded);
+    }
+
+    /**
+     * Test that an upgrade is requested when:
+     * - There are no icons in Web Manifest when WebAPK is generated. Icon is generated for use as
+     *   the homescreen icon.
+     * - Web Manifest is updated to refer to {@link WEBAPK_ICON_URL}.
+     */
+    @Test
+    public void testNoIconToCustomIconShouldUpgrade() {
+        setMetaData(WEBAPK_START_URL, "", 0L);
+        FetchedData fetchedData = new FetchedData();
+
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        Assert.assertEquals(WEBAPK_ICON_URL, detector.mRequestedIconUrl);
+        Assert.assertTrue(detector.mIsUpgraded);
+    }
+
+    /**
+     * Test that an upgrade is not requested when:
+     * - There are no icons in Web Manifest when WebAPK is generated. Icon is generated for use as
+     *   the homescreen icon.
+     * - Web Manifest remains unchanged.
+     */
+    @Test
+    public void testNoIconShouldNotUpgrade() {
+        setMetaData(WEBAPK_START_URL, "", 0L);
+        FetchedData fetchedData = new FetchedData();
+        fetchedData.iconUrl = "";
+
+        TestManifestUpgradeDetector detector = createDetector(fetchedData);
+        detector.start();
+        Assert.assertTrue(detector.mCompleted);
+        // No icon should have been requested.
+        Assert.assertNull(detector.mRequestedIconUrl);
+        Assert.assertFalse(detector.mIsUpgraded);
+    }
 }