Move search engine icon status icon logic to separate functions

Reverting crrev/c/1818604, which caused a crash. Replacing the logic
introduced in that change here in a way that doesn't rely on the the
ordering of the callback.

Bug: 1004903, 1009832
Change-Id: Ie8e34325648704aa7068113e60aac74d8d685d66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1835177Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Ender <ender@google.com>
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702970}

chrome/android/java/src/org/chromium/chrome/browser/omnibox/SearchEngineLogoUtils.java

@@ -15,8 +15,6 @@ import androidx.annotation.Nullable;
 import org.chromium.base.Callback;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.library_loader.LibraryProcessType;
-import org.chromium.base.task.PostTask;
-import org.chromium.base.task.TaskTraits;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.favicon.FaviconHelper;
@@ -26,7 +24,6 @@ import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
 import org.chromium.chrome.browser.ui.widget.RoundedIconGenerator;
 import org.chromium.chrome.browser.util.UrlUtilities;
 import org.chromium.content_public.browser.BrowserStartupController;
-import org.chromium.content_public.browser.UiThreadTaskTraits;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -170,25 +167,18 @@ public class SearchEngineLogoUtils {
      */
     public static void getSearchEngineLogoFavicon(
             Profile profile, Resources resources, Callback<Bitmap> callback) {
-        PostTask.postTask(TaskTraits.USER_VISIBLE,
-                () -> getSearchEngineLogoFaviconInternal(profile, resources, callback));
-    }
-
-    /** @see {@link #getSearchEngineLogoFavicon}. */
-    private static void getSearchEngineLogoFaviconInternal(
-            Profile profile, Resources resources, Callback<Bitmap> callback) {
         if (sFaviconHelper == null) sFaviconHelper = new FaviconHelper();
 
         String logoUrl = getSearchLogoUrl();
         if (logoUrl == null) {
-            returnLogoOnUiThread(null, callback);
+            callback.onResult(null);
             return;
         }
 
         // Return a cached copy if it's available.
         if (sCachedComposedBackground != null
                 && sCachedComposedBackgroundLogoUrl.equals(getSearchLogoUrl())) {
-            returnLogoOnUiThread(sCachedComposedBackground, callback);
+            callback.onResult(sCachedComposedBackground);
             return;
         }
 
@@ -196,15 +186,13 @@ public class SearchEngineLogoUtils {
         boolean willReturn = sFaviconHelper.getLocalFaviconImageForURL(
                 profile, logoUrl, logoSizePixels, (image, iconUrl) -> {
                     if (image == null) {
-                        returnLogoOnUiThread(image, callback);
+                        callback.onResult(image);
                         return;
                     }
 
                     processReturnedLogo(logoUrl, image, resources, callback);
                 });
-        if (!willReturn) {
-            returnLogoOnUiThread(null, callback);
-        }
+        if (!willReturn) callback.onResult(null);
     }
 
     /**
@@ -248,15 +236,7 @@ public class SearchEngineLogoUtils {
         sCachedComposedBackground = composedIcon;
         sCachedComposedBackgroundLogoUrl = logoUrl;
 
-        returnLogoOnUiThread(sCachedComposedBackground, callback);
-    }
-
-    /**
-     * @param logo The logo to return to callback.
-     * @param callback The callback for the calling client.
-     */
-    private static void returnLogoOnUiThread(Bitmap logo, Callback<Bitmap> callback) {
-        PostTask.postTask(UiThreadTaskTraits.USER_VISIBLE, () -> callback.onResult(logo));
+        callback.onResult(sCachedComposedBackground);
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/omnibox/status/StatusMediator.java

@@ -62,11 +62,6 @@ class StatusMediator {
         }
     }
 
-    // The size that the icon should be displayed as.
-    private static final int SEARCH_ENGINE_LOGO_ICON_TARGET_SIZE_DP = 24;
-    // The size given to LargeIconBridge to increase the probability that we'll get an icon back.
-    private static final int SEARCH_ENGINE_LOGO_ICON_DOWNLOAD_SIZE_DP = 16;
-
     private final PropertyModel mModel;
     private boolean mDarkTheme;
     private boolean mUrlHasFocus;
@@ -368,13 +363,43 @@ class StatusMediator {
      *     - not shown if URL is focused.
      */
     private void updateLocationBarIcon() {
+        // Update the accessibility description before continuing since we need it either way.
+        mModel.set(StatusProperties.STATUS_ICON_DESCRIPTION_RES, getAccessibilityDescriptionRes());
+
+        // No need to proceed further if we've already updated it for the search engine icon.
+        if (maybeUpdateStatusIconForSearchEngineIcon()) return;
+
         int icon = 0;
         int tint = 0;
         int toast = 0;
 
-        // Update the accessibility description before continuing since we need it either way.
-        mModel.set(StatusProperties.STATUS_ICON_DESCRIPTION_RES, getAccessibilityDescriptionRes());
+        mIsSecurityButtonShown = false;
+        if (mUrlHasFocus) {
+            if (mShowStatusIconWhenUrlFocused) {
+                icon = mFirstSuggestionIsSearchQuery ? R.drawable.omnibox_search
+                                                     : R.drawable.ic_omnibox_page;
+                tint = mNavigationIconTintRes;
+            }
+        } else if (mSecurityIconRes != 0) {
+            mIsSecurityButtonShown = true;
+            icon = mSecurityIconRes;
+            tint = mSecurityIconTintRes;
+            toast = R.string.menu_page_info;
+        }
 
+        if (mPageIsPreview) {
+            tint = mDarkTheme ? R.color.locationbar_status_preview_color
+                              : R.color.locationbar_status_preview_color_light;
+        }
+
+        mModel.set(StatusProperties.STATUS_ICON_RES, icon);
+        mModel.set(StatusProperties.STATUS_ICON_TINT_RES, tint);
+        mModel.set(StatusProperties.STATUS_ICON_ACCESSIBILITY_TOAST_RES, toast);
+    }
+
+    /** @return True if the security icon has been set for the search engine icon. */
+    @VisibleForTesting
+    boolean maybeUpdateStatusIconForSearchEngineIcon() {
         // When the search engine logo should be shown, but the engine isn't Google. In this case,
         // we download the icon on the fly.
         boolean showFocused = mUrlHasFocus && mShowStatusIconWhenUrlFocused;
@@ -393,56 +418,67 @@ class StatusMediator {
                 && mToolbarCommonPropertiesModel.isIncognito();
         if (mDelegate.shouldShowSearchEngineLogo(isIncognito) && mIsSearchEngineStateSetup
                 && (showFocused || showUnfocusedSearchResultsPage)) {
-            mShouldCancelCustomFavicon = false;
-            // If the current url text is a valid url, then swap the dse icon for a globe.
-            if (mUrlBarTextIsValidUrl) {
-                icon = R.drawable.ic_globe_24dp;
-            } else if (mIsSearchEngineGoogle) {
-                if (mDelegate.shouldShowSearchLoupeEverywhere(isIncognito)) {
-                    icon = R.drawable.ic_search;
-                } else {
-                    icon = R.drawable.ic_logo_googleg_24dp;
-                }
-            } else {
-                icon = R.drawable.ic_search;
-                if (!mDelegate.shouldShowSearchLoupeEverywhere(isIncognito)) {
-                    mDelegate.getSearchEngineLogoFavicon(mResources, (favicon) -> {
-                        if (favicon == null || mShouldCancelCustomFavicon) return;
-                        mModel.set(StatusProperties.STATUS_ICON, favicon);
-                        mModel.set(StatusProperties.STATUS_ICON_TINT_RES, 0);
-                    });
-                }
-            }
-            tint = icon == R.drawable.ic_logo_googleg_24dp ? 0 : mSecurityIconTintRes;
-            mModel.set(StatusProperties.STATUS_ICON_RES, icon);
-            mModel.set(StatusProperties.STATUS_ICON_TINT_RES, tint);
-            return;
+            setSecurityIconResourceForSearchEngineIcon(isIncognito, (icon) -> {
+                mModel.set(StatusProperties.STATUS_ICON_TINT_RES,
+                        getSecurityIconTintForSearchEngineIcon(icon));
+            });
+            return true;
         } else {
             mShouldCancelCustomFavicon = true;
+            return false;
         }
+    }
 
-        mIsSecurityButtonShown = false;
-        if (mUrlHasFocus) {
-            if (mShowStatusIconWhenUrlFocused) {
-                icon = mFirstSuggestionIsSearchQuery ? R.drawable.omnibox_search
-                                                     : R.drawable.ic_omnibox_page;
-                tint = mNavigationIconTintRes;
+    /**
+     * Set the security icon resource for the search engine icon and invoke the callback to inform
+     * the caller which resource has been set.
+     *
+     * @param isIncognito True if the user is incognito.
+     * @param callback Called when the final value is set for the security icon resource. Meant to
+     *                 give the caller a chance to set the tint for the given resource.
+     */
+    private void setSecurityIconResourceForSearchEngineIcon(
+            boolean isIncognito, Callback<Integer> callback) {
+        mShouldCancelCustomFavicon = false;
+        // If the current url text is a valid url, then swap the dse icon for a globe.
+        if (mUrlBarTextIsValidUrl) {
+            mModel.set(StatusProperties.STATUS_ICON_RES, R.drawable.ic_globe_24dp);
+            callback.onResult(R.drawable.ic_globe_24dp);
+        } else if (mIsSearchEngineGoogle) {
+            int icon = mDelegate.shouldShowSearchLoupeEverywhere(isIncognito)
+                    ? R.drawable.ic_search
+                    : R.drawable.ic_logo_googleg_24dp;
+            mModel.set(StatusProperties.STATUS_ICON_RES, icon);
+            callback.onResult(icon);
+        } else {
+            mModel.set(StatusProperties.STATUS_ICON_RES, R.drawable.ic_search);
+            callback.onResult(R.drawable.ic_search);
+            if (!mDelegate.shouldShowSearchLoupeEverywhere(isIncognito)) {
+                mDelegate.getSearchEngineLogoFavicon(mResources, (favicon) -> {
+                    if (favicon == null || mShouldCancelCustomFavicon) return;
+                    mModel.set(StatusProperties.STATUS_ICON, favicon);
+                    callback.onResult(0);
+                });
             }
-        } else if (mSecurityIconRes != 0) {
-            mIsSecurityButtonShown = true;
-            icon = mSecurityIconRes;
-            tint = mSecurityIconTintRes;
-            toast = R.string.menu_page_info;
         }
+    }
 
-        if (mPageIsPreview) {
-            tint = mDarkTheme ? R.color.locationbar_status_preview_color
-                              : R.color.locationbar_status_preview_color_light;
+    /**
+     * Get the icon tint for the given search engine icon resource.
+     * @param icon The icon resource for the search engine icon.
+     * @return The tint resource for the given parameters.
+     */
+    @VisibleForTesting
+    int getSecurityIconTintForSearchEngineIcon(int icon) {
+        int tint;
+        if (icon == 0 || icon == R.drawable.ic_logo_googleg_24dp) {
+            tint = 0;
+        } else {
+            tint = mDarkTheme ? R.color.default_icon_color_secondary_list
+                              : ColorUtils.getThemedToolbarIconTintRes(!mDarkTheme);
         }
 
-        mModel.set(StatusProperties.STATUS_ICON_RES, icon);
-        mModel.set(StatusProperties.STATUS_ICON_TINT_RES, tint);
-        mModel.set(StatusProperties.STATUS_ICON_ACCESSIBILITY_TOAST_RES, toast);
+        return tint;
     }
 
     /** Return the resource id for the accessibility description or 0 if none apply. */

chrome/android/junit/src/org/chromium/chrome/browser/omnibox/status/StatusMediatorUnitTest.java

@@ -143,14 +143,63 @@ public final class StatusMediatorUnitTest {
 
     @Test
     @Features.EnableFeatures(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
-    public void searchEngineLogo_tintAppliedToLoupe() {
-        setupSearchEngineLogoForTesting(true, true, true, false);
-        Mockito.doReturn(false).when(mToolbarCommonPropertiesModel).isIncognito();
+    public void searchEngineLogo_maybeUpdateStatusIconForSearchEngineIconChanges() {
+        setupSearchEngineLogoForTesting(true, true, false, false);
 
         mMediator.setUrlHasFocus(true);
         mMediator.setShowIconsWhenUrlFocused(true);
-        mMediator.setSecurityIconTint(11);
-        Assert.assertEquals(11, mModel.get(StatusProperties.STATUS_ICON_TINT_RES));
+        mMediator.setSecurityIconResource(0);
+        mMediator.updateSearchEngineStatusIcon(true, true, TEST_SEARCH_URL);
+
+        Assert.assertTrue(mMediator.maybeUpdateStatusIconForSearchEngineIcon());
+        Assert.assertEquals(
+                R.drawable.ic_logo_googleg_24dp, mModel.get(StatusProperties.STATUS_ICON_RES));
+    }
+
+    @Test
+    @Features.EnableFeatures(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void searchEngineLogo_maybeUpdateStatusIconForSearchEngineIconNoChanges() {
+        setupSearchEngineLogoForTesting(true, true, false, false);
+
+        mMediator.setUrlHasFocus(true);
+        mMediator.setShowIconsWhenUrlFocused(false);
+        mMediator.setSecurityIconResource(0);
+        mMediator.updateSearchEngineStatusIcon(true, true, TEST_SEARCH_URL);
+
+        Assert.assertFalse(mMediator.maybeUpdateStatusIconForSearchEngineIcon());
+    }
+
+    @Test
+    @Features.EnableFeatures(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void setSecurityIconTintForSearchEngineIcon_zeroForGoogleAndNoIcon() {
+        mMediator.setUseDarkColors(false);
+        Assert.assertEquals(0, mMediator.getSecurityIconTintForSearchEngineIcon(0));
+        Assert.assertEquals(0,
+                mMediator.getSecurityIconTintForSearchEngineIcon(R.drawable.ic_logo_googleg_24dp));
+        mMediator.setUseDarkColors(true);
+        Assert.assertEquals(0, mMediator.getSecurityIconTintForSearchEngineIcon(0));
+        Assert.assertEquals(0,
+                mMediator.getSecurityIconTintForSearchEngineIcon(R.drawable.ic_logo_googleg_24dp));
+    }
+
+    @Test
+    @Features.EnableFeatures(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void setSecurityIconTintForSearchEngineIcon_correctForDarkColors() {
+        mMediator.setUseDarkColors(true);
+        Assert.assertEquals(R.color.default_icon_color_secondary_list,
+                mMediator.getSecurityIconTintForSearchEngineIcon(R.drawable.ic_globe_24dp));
+        Assert.assertEquals(R.color.default_icon_color_secondary_list,
+                mMediator.getSecurityIconTintForSearchEngineIcon(R.drawable.ic_search));
+    }
+
+    @Test
+    @Features.EnableFeatures(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void setSecurityIconTintForSearchEngineIcon_correctForLightColors() {
+        mMediator.setUseDarkColors(false);
+        Assert.assertEquals(R.color.tint_on_dark_bg,
+                mMediator.getSecurityIconTintForSearchEngineIcon(R.drawable.ic_globe_24dp));
+        Assert.assertEquals(R.color.tint_on_dark_bg,
+                mMediator.getSecurityIconTintForSearchEngineIcon(R.drawable.ic_search));
     }
 
     private void setupSearchEngineLogoForTesting(