Fix fallback favicons in Chrome site settings

crrev.com/c/2171450 introduced a bug in Chrome's Site Settings UI that
caused fallback favicons to not load. That CL used the iconUrl parameter
of the onFaviconAvailable callback method to create the fallback favicon
when a real one wasn't available, but that parameter actually stores the
original url of the saved favicon, not the url of the page we're looking
up a favicon for, so it was empty when one wasn't found. This CL makes
the callback use the original url when generating the fallback favicon.

Bug: 1077716
Change-Id: I5e9d047994f0ec71a83930e97d410636d9e3134e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226297Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775217}

chrome/android/chrome_test_java_sources.gni

@@ -469,6 +469,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/signin/SigninFragmentTest.java",
   "javatests/src/org/chromium/chrome/browser/signin/SigninHelperTest.java",
   "javatests/src/org/chromium/chrome/browser/signin/SigninSignoutIntegrationTest.java",
+  "javatests/src/org/chromium/chrome/browser/site_settings/ChromeSiteSettingsClientTest.java",
   "javatests/src/org/chromium/chrome/browser/site_settings/CookieControlsBridgeTest.java",
   "javatests/src/org/chromium/chrome/browser/site_settings/CookieControlsServiceBridgeTest.java",
   "javatests/src/org/chromium/chrome/browser/site_settings/ManageSpaceActivityTest.java",

chrome/android/java/src/org/chromium/chrome/browser/site_settings/ChromeSiteSettingsClient.java

@@ -106,23 +106,25 @@ public class ChromeSiteSettingsClient implements SiteSettingsClient {
      * has been called.
      */
     private class FaviconLoader implements FaviconImageCallback {
+        private final String mFaviconUrl;
+        private final Callback<Bitmap> mCallback;
         private final int mFaviconSizePx;
         // Loads the favicons asynchronously.
         private final FaviconHelper mFaviconHelper;
-        private final Callback<Bitmap> mCallback;
 
         private FaviconLoader(String faviconUrl, Callback<Bitmap> callback) {
+            mFaviconUrl = faviconUrl;
+            mCallback = callback;
             mFaviconSizePx =
                     mContext.getResources().getDimensionPixelSize(R.dimen.default_favicon_size);
-            mCallback = callback;
             mFaviconHelper = new FaviconHelper();
 
             // TODO(https://crbug.com/1048632): Use the current profile (i.e., regular profile or
             // incognito profile) instead of always using regular profile. It works correctly now,
             // but it is not safe.
             if (!mFaviconHelper.getLocalFaviconImageForURL(
-                        Profile.getLastUsedRegularProfile(), faviconUrl, mFaviconSizePx, this)) {
-                onFaviconAvailable(/*image=*/null, faviconUrl);
+                        Profile.getLastUsedRegularProfile(), mFaviconUrl, mFaviconSizePx, this)) {
+                onFaviconAvailable(/*image=*/null, mFaviconUrl);
             }
         }
 
@@ -140,7 +142,7 @@ public class ChromeSiteSettingsClient implements SiteSettingsClient {
                                 Math.round(FAVICON_CORNER_RADIUS_FRACTION * faviconSizeDp),
                                 FAVICON_BACKGROUND_COLOR,
                                 Math.round(FAVICON_TEXT_SIZE_FRACTION * faviconSizeDp));
-                image = faviconGenerator.generateIconForUrl(iconUrl);
+                image = faviconGenerator.generateIconForUrl(mFaviconUrl);
             }
             mCallback.onResult(image);
         }

chrome/android/javatests/src/org/chromium/chrome/browser/site_settings/ChromeSiteSettingsClientTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.site_settings;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import android.graphics.Bitmap;
+import android.support.test.filters.SmallTest;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.task.PostTask;
+import org.chromium.base.test.util.CallbackHelper;
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.chrome.browser.ChromeActivity;
+import org.chromium.chrome.browser.flags.ChromeSwitches;
+import org.chromium.chrome.test.ChromeActivityTestRule;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.content_public.browser.UiThreadTaskTraits;
+
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Tests for Chrome's SiteSettingsClient implementation.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class ChromeSiteSettingsClientTest {
+    @Rule
+    public ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
+            new ChromeActivityTestRule<>(ChromeActivity.class);
+
+    @Before
+    public void setUp() throws Exception {
+        mActivityTestRule.startMainActivityOnBlankPage();
+    }
+
+    // Tests that a fallback favicon is generated when a real one isn't found locally.
+    // This is a regression test for crbug.com/1077716.
+    @Test
+    @SmallTest
+    public void testFallbackFaviconLoads() throws TimeoutException {
+        ChromeSiteSettingsClient client =
+                new ChromeSiteSettingsClient(mActivityTestRule.getActivity());
+
+        Bitmap[] holder = new Bitmap[1];
+        CallbackHelper helper = new CallbackHelper();
+        PostTask.postTask(UiThreadTaskTraits.DEFAULT, () -> {
+            client.getFaviconImageForURL("url.with.no.favicon", favicon -> {
+                holder[0] = favicon;
+                helper.notifyCalled();
+            });
+        });
+        helper.waitForCallback(helper.getCallCount());
+
+        Bitmap favicon = holder[0];
+        assertThat(favicon).isNotNull();
+        assertThat(favicon.getWidth()).isGreaterThan(0);
+        assertThat(favicon.getHeight()).isGreaterThan(0);
+    }
+}