Move reconcileVisualState to be called when the TabModel is swapped

The current logic is setup in such a way that it's called along with
#setShowIconsWhenUrlFocused. The result of this is it's called more
often then it needs to be. Some calls are setup where status view icon
is the first focusable view. When the status view icon is set to GONE,
Android goes looking for the next focusable view and focused it. This
just happens to be the urlbar, which focuses the entire omnibox.

Moving this to the tab model changed callback guarantees that when
it's called, the status view icon won't have focus (either on the NTP or
the tab switcher).

Bug: 1063203
Change-Id: Ie9501da027b23133a4a4f2d78ed5c11225147d0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2138263
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760195}

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

@@ -18,6 +18,7 @@ import org.chromium.chrome.browser.ntp.NewTabPage;
 import org.chromium.chrome.browser.omnibox.UrlBar.UrlBarDelegate;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.toolbar.IncognitoStateProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarDataProvider;
 import org.chromium.chrome.browser.toolbar.top.Toolbar;
 import org.chromium.chrome.browser.toolbar.top.ToolbarActionModeCallback;
@@ -135,10 +136,13 @@ public interface LocationBar extends UrlBarDelegate, FakeboxDelegate {
      * Initialize controls that will act as hooks to various functions.
      * @param windowDelegate {@link WindowDelegate} that will provide {@link Window} related info.
      * @param windowAndroid {@link WindowAndroid} that is used by the owning {@link Activity}.
-     * @param provider An {@link ActivityTabProvider} to access the activity's current tab.
+     * @param activityTabProvider An {@link ActivityTabProvider} to access the activity's current
+     *         tab.
+     * @param incognitoStateProvider An {@link IncognitoStateProvider} to access the current
+     *         incognito state.
      */
     void initializeControls(WindowDelegate windowDelegate, WindowAndroid windowAndroid,
-            ActivityTabProvider provider);
+            ActivityTabProvider activityTabProvider, IncognitoStateProvider incognitoStateProvider);
 
     /**
      * Triggers the cursor to be visible in the UrlBar without triggering any of the focus animation

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

@@ -60,6 +60,7 @@ import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tasks.ReturnToChromeExperimentsUtil;
+import org.chromium.chrome.browser.toolbar.IncognitoStateProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarDataProvider;
 import org.chromium.chrome.browser.toolbar.top.ToolbarActionModeCallback;
 import org.chromium.chrome.browser.ui.native_page.NativePage;
@@ -282,13 +283,15 @@ public class LocationBarLayout extends FrameLayout
 
     @Override
     public void initializeControls(WindowDelegate windowDelegate, WindowAndroid windowAndroid,
-            ActivityTabProvider provider) {
+            ActivityTabProvider activityTabProvider,
+            IncognitoStateProvider incognitoStateProvider) {
         mWindowDelegate = windowDelegate;
         mWindowAndroid = windowAndroid;
 
         mUrlCoordinator.setWindowDelegate(windowDelegate);
         mAutocompleteCoordinator.setWindowAndroid(windowAndroid);
-        mAutocompleteCoordinator.setActivityTabProvider(provider);
+        mAutocompleteCoordinator.setActivityTabProvider(activityTabProvider);
+        mStatusViewCoordinator.setIncognitoStateProvider(incognitoStateProvider);
     }
 
     /**

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

@@ -23,6 +23,7 @@ import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
 import org.chromium.chrome.browser.omnibox.status.StatusProperties.StatusIconResource;
 import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinatorFactory;
 import org.chromium.chrome.browser.profiles.Profile;
+import org.chromium.chrome.browser.toolbar.IncognitoStateProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarColors;
 import org.chromium.chrome.browser.toolbar.ToolbarCommonPropertiesModel;
 import org.chromium.components.security_state.ConnectionSecurityLevel;
@@ -32,7 +33,7 @@ import org.chromium.ui.modelutil.PropertyModel;
 /**
  * Contains the controller logic of the Status component.
  */
-class StatusMediator {
+class StatusMediator implements IncognitoStateProvider.IncognitoStateObserver {
     @VisibleForTesting
     class StatusMediatorDelegate {
         /** @see {@link AutocompleteCoordinatorFactory#qualifyPartialURLQuery} */
@@ -103,6 +104,9 @@ class StatusMediator {
     private float mUrlFocusPercent;
     private String mSearchEngineLogoUrl;
 
+    private boolean mIsIncognito;
+    private Runnable mForceModelViewReconciliationRunnable;
+
     // Factors used to offset the animation of the status icon's alpha adjustment. The full formula
     // used: alpha = (focusAnimationProgress - mTextOffsetThreshold) / (1 - mTextOffsetThreshold)
     // mTextOffsetThreshold will be the % space that the icon takes up during the focus animation.
@@ -112,7 +116,8 @@ class StatusMediator {
     private final float mTextOffsetAdjustedScale;
 
     StatusMediator(PropertyModel model, Resources resources, Context context,
-            UrlBarEditingTextStateProvider urlBarEditingTextStateProvider, boolean isTablet) {
+            UrlBarEditingTextStateProvider urlBarEditingTextStateProvider, boolean isTablet,
+            Runnable forceModelViewReconciliationRunnable) {
         mModel = model;
         mDelegate = new StatusMediatorDelegate();
         updateColorTheme();
@@ -131,6 +136,7 @@ class StatusMediator {
         mTextOffsetAdjustedScale = mTextOffsetThreshold == 1 ? 1 : (1 - mTextOffsetThreshold);
 
         mIsTablet = isTablet;
+        mForceModelViewReconciliationRunnable = forceModelViewReconciliationRunnable;
     }
 
     /**
@@ -271,7 +277,7 @@ class StatusMediator {
         // On tablets, the status icon should always be shown so the following logic doesn't apply.
         assert !mIsTablet : "This logic shouldn't be called on tablets";
 
-        if (!mDelegate.shouldShowSearchEngineLogo(mToolbarCommonPropertiesModel.isIncognito())) {
+        if (!mDelegate.shouldShowSearchEngineLogo(mIsIncognito)) {
             return;
         }
 
@@ -297,7 +303,7 @@ class StatusMediator {
         // On tablets, the status icon should always be shown so the following logic doesn't apply.
         assert !mIsTablet : "This logic shouldn't be called on tablets";
 
-        if (!mDelegate.shouldShowSearchEngineLogo(mToolbarCommonPropertiesModel.isIncognito())) {
+        if (!mDelegate.shouldShowSearchEngineLogo(mIsIncognito)) {
             return;
         }
 
@@ -511,12 +517,10 @@ class StatusMediator {
                 && mToolbarCommonPropertiesModel.getDisplaySearchTerms() != null
                 && mDelegate.doesUrlMatchDefaultSearchEngine(
                         mToolbarCommonPropertiesModel.getCurrentUrl());
-        boolean isIncognito = mToolbarCommonPropertiesModel != null
-                && mToolbarCommonPropertiesModel.isIncognito();
-        if (mDelegate.shouldShowSearchEngineLogo(isIncognito) && mIsSearchEngineStateSetup
+        if (mDelegate.shouldShowSearchEngineLogo(mIsIncognito) && mIsSearchEngineStateSetup
                 && (showIconWhenFocused || showIconWhenScrollingOnNTP
                         || showUnfocusedSearchResultsPage)) {
-            getStatusIconResourceForSearchEngineIcon(isIncognito, (statusIconRes) -> {
+            getStatusIconResourceForSearchEngineIcon(mIsIncognito, (statusIconRes) -> {
                 mModel.set(StatusProperties.STATUS_ICON_RESOURCE, statusIconRes);
             });
             return true;
@@ -594,8 +598,7 @@ class StatusMediator {
     /** Return the resource id for the accessibility description or 0 if none apply. */
     private int getAccessibilityDescriptionRes() {
         if (mUrlHasFocus) {
-            if (SearchEngineLogoUtils.shouldShowSearchEngineLogo(
-                        mToolbarCommonPropertiesModel.isIncognito())) {
+            if (SearchEngineLogoUtils.shouldShowSearchEngineLogo(mIsIncognito)) {
                 return 0;
             } else if (mShowStatusIconWhenUrlFocused) {
                 return R.string.accessibility_toolbar_btn_site_info;
@@ -648,6 +651,42 @@ class StatusMediator {
         return urlTextWithAutocomplete;
     }
 
+    public void setIncognitoStateProvider(IncognitoStateProvider incognitoStateProvider) {
+        if (incognitoStateProvider == null) return;
+        incognitoStateProvider.addIncognitoStateObserverAndTrigger(this);
+    }
+
+    @Override
+    public void onIncognitoStateChanged(boolean isIncognito) {
+        boolean previousIsIcognito = mIsIncognito;
+        mIsIncognito = isIncognito;
+        if (previousIsIcognito != isIncognito) reconcileVisualState();
+    }
+
+    /**
+     * Temporary workaround for the divergent logic for status icon visibility changes for the dse
+     * icon experiment. Should be removed when the dse icon launches (crbug.com/1019488).
+     *
+     * When transitioning to incognito, the first visible view when focused will be assigned to
+     * UrlBar. When the UrlBar is the first visible view when focused, the StatusView's alpha
+     * will be set to 0 in LocationBarPhone#populateFadeAnimations. When transitioning back from
+     * incognito, StatusView's state needs to be reset to match the current state of the status view
+     * {@link org.chromium.chrome.browser.omnibox.LocationBarPhone#updateVisualsForState}.
+     * property model.
+     **/
+    private void reconcileVisualState() {
+        // No reconciliation is needed on tablet because the status icon is always shown.
+        if (mIsTablet) return;
+
+        if (!mShowStatusIconWhenUrlFocused || mIsIncognito
+                || !mDelegate.shouldShowSearchEngineLogo(mIsIncognito)) {
+            return;
+        }
+
+        assert mForceModelViewReconciliationRunnable != null;
+        mForceModelViewReconciliationRunnable.run();
+    }
+
     void setDelegateForTesting(StatusMediatorDelegate delegate) {
         mDelegate = delegate;
     }

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

@@ -13,13 +13,13 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.offlinepages.OfflinePageUtils;
-import org.chromium.chrome.browser.omnibox.SearchEngineLogoUtils;
 import org.chromium.chrome.browser.omnibox.UrlBar.UrlTextChangeListener;
 import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
 import org.chromium.chrome.browser.page_info.ChromePageInfoControllerDelegate;
 import org.chromium.chrome.browser.page_info.PageInfoController;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.TabImpl;
+import org.chromium.chrome.browser.toolbar.IncognitoStateProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarDataProvider;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.modelutil.PropertyModel;
@@ -51,8 +51,16 @@ public class StatusViewCoordinator implements View.OnClickListener, UrlTextChang
         mModel = new PropertyModel(StatusProperties.ALL_KEYS);
 
         PropertyModelChangeProcessor.create(mModel, mStatusView, new StatusViewBinder());
+
+        Runnable forceModelViewReconciliationRunnable = () -> {
+            final View securityIconView = getSecurityIconView();
+            mStatusView.setAlpha(1f);
+            securityIconView.setAlpha(mModel.get(StatusProperties.STATUS_ICON_ALPHA));
+            securityIconView.setVisibility(
+                    mModel.get(StatusProperties.SHOW_STATUS_ICON) ? View.VISIBLE : View.GONE);
+        };
         mMediator = new StatusMediator(mModel, mStatusView.getResources(), mStatusView.getContext(),
-                urlBarEditingTextStateProvider, isTablet);
+                urlBarEditingTextStateProvider, isTablet, forceModelViewReconciliationRunnable);
 
         Resources res = mStatusView.getResources();
         mMediator.setUrlMinWidth(res.getDimensionPixelSize(R.dimen.location_bar_min_url_width)
@@ -228,7 +236,6 @@ public class StatusViewCoordinator implements View.OnClickListener, UrlTextChang
      */
     public void setShowIconsWhenUrlFocused(boolean showIconsWithUrlFocused) {
         mMediator.setShowIconsWhenUrlFocused(showIconsWithUrlFocused);
-        reconcileVisualState(showIconsWithUrlFocused);
     }
 
     /**
@@ -238,6 +245,10 @@ public class StatusViewCoordinator implements View.OnClickListener, UrlTextChang
         mMediator.setFirstSuggestionIsSearchType(firstSuggestionIsSearchQuery);
     }
 
+    public void setIncognitoStateProvider(IncognitoStateProvider incognitoStateProvider) {
+        mMediator.setIncognitoStateProvider(incognitoStateProvider);
+    }
+
     /**
      * Update information required to display the search engine icon.
      */
@@ -247,40 +258,6 @@ public class StatusViewCoordinator implements View.OnClickListener, UrlTextChang
                 shouldShowSearchEngineLogo, isSearchEngineGoogle, searchEngineUrl);
     }
 
-    /**
-     * Temporary workaround for the divergent logic for status icon visibility changes for the dse
-     * icon experiment. Should be removed when the dse icon launches (crbug.com/1019488).
-     *
-     * When transitioning to incognito, the first visible view when focused will be assigned to
-     * UrlBar. When the UrlBar is the first visible view when focused, the StatusView's alpha
-     * will be set to 0 in LocationBarPhone#populateFadeAnimations. When transitioning back from
-     * incognito, StatusView's state needs to be reset to match the current state of the status view
-     * {@link org.chromium.chrome.browser.omnibox.LocationBarPhone#updateVisualsForState}.
-     * property model.
-     **/
-    private void reconcileVisualState(boolean showStatusIconWhenFocused) {
-        // No reconciliation is needed on tablet because the status icon is always shown.
-        if (mIsTablet) return;
-
-        // State requirements:
-        // - The ToolbarDataProvider and views are not null.
-        // - The status icon will be shown when focused.
-        // - Incognito isn't active.
-        // - The search engine logo should be shown.
-        if (mToolbarDataProvider == null || mStatusView == null || getSecurityIconView() == null
-                || !showStatusIconWhenFocused || mToolbarDataProvider.isIncognito()
-                || !SearchEngineLogoUtils.shouldShowSearchEngineLogo(
-                        mToolbarDataProvider.isIncognito())) {
-            return;
-        }
-        View securityIconView = getSecurityIconView();
-
-        mStatusView.setAlpha(1f);
-        securityIconView.setAlpha(mModel.get(StatusProperties.STATUS_ICON_ALPHA));
-        securityIconView.setVisibility(
-                mModel.get(StatusProperties.SHOW_STATUS_ICON) ? View.VISIBLE : View.GONE);
-    }
-
     /**
      * @return Width of the status icon including start/end margins.
      */

chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java

@@ -158,7 +158,8 @@ public class SearchActivity extends AsyncInitializationActivity
                 R.id.search_location_bar);
         mSearchBox.setDelegate(this);
         mSearchBox.setToolbarDataProvider(mSearchBoxDataProvider);
-        mSearchBox.initializeControls(new WindowDelegate(getWindow()), getWindowAndroid(), null);
+        mSearchBox.initializeControls(
+                new WindowDelegate(getWindow()), getWindowAndroid(), null, null);
 
         // Kick off everything needed for the user to type into the box.
         beginQuery();

chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java

@@ -404,7 +404,7 @@ public class ToolbarManager implements ToolbarTabController, UrlFocusChangeListe
         mLocationBar.setDefaultTextEditActionModeCallback(
                 mActionModeController.getActionModeCallback());
         mLocationBar.initializeControls(new WindowDelegate(mActivity.getWindow()),
-                mActivity.getWindowAndroid(), mActivityTabProvider);
+                mActivity.getWindowAndroid(), mActivityTabProvider, mIncognitoStateProvider);
         mLocationBar.addUrlFocusChangeListener(mLocationBarFocusObserver);
         mProgressBarCoordinator =
                 new LoadProgressCoordinator(mActivityTabProvider, mToolbar.getProgressBar());

chrome/android/java/src/org/chromium/chrome/browser/toolbar/top/CustomTabToolbar.java

@@ -60,6 +60,7 @@ import org.chromium.chrome.browser.page_info.PageInfoController;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.TrustedCdn;
+import org.chromium.chrome.browser.toolbar.IncognitoStateProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarColors;
 import org.chromium.chrome.browser.toolbar.ToolbarDataProvider;
 import org.chromium.chrome.browser.toolbar.ToolbarTabController;
@@ -765,7 +766,7 @@ public class CustomTabToolbar extends ToolbarLayout implements View.OnLongClickL
 
         @Override
         public void initializeControls(WindowDelegate windowDelegate, WindowAndroid windowAndroid,
-                ActivityTabProvider provider) {}
+                ActivityTabProvider provider, IncognitoStateProvider incognitoStateProvider) {}
 
         @Override
         public void updateStatusIcon() {

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

@@ -7,7 +7,9 @@ package org.chromium.chrome.browser.omnibox.status;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -50,6 +52,8 @@ public final class StatusMediatorUnitTest {
     UrlBarEditingTextStateProvider mUrlBarEditingTextStateProvider;
     @Mock
     StatusMediator.StatusMediatorDelegate mDelegate;
+    @Mock
+    Runnable mMockForceModelViewReconciliationRunnable;
     @Captor
     ArgumentCaptor<Callback<Bitmap>> mCallbackCaptor;
     @Captor
@@ -72,8 +76,9 @@ public final class StatusMediatorUnitTest {
         mResources = mContext.getResources();
 
         mModel = new PropertyModel(StatusProperties.ALL_KEYS);
-        mMediator = new StatusMediator(mModel, mResources, mContext,
-                mUrlBarEditingTextStateProvider, /* isTablet */ false);
+        mMediator =
+                new StatusMediator(mModel, mResources, mContext, mUrlBarEditingTextStateProvider,
+                        /* isTablet */ false, mMockForceModelViewReconciliationRunnable);
         mMediator.setToolbarCommonPropertiesModel(mToolbarCommonPropertiesModel);
         mMediator.setDelegateForTesting(mDelegate);
         mBitmap = Bitmap.createBitmap(10, 10, Bitmap.Config.ARGB_8888);
@@ -378,13 +383,45 @@ public final class StatusMediatorUnitTest {
         Assert.assertEquals(msg, "bar.com", mMediator.resolveUrlBarTextWithAutocomplete("bar.com"));
     }
 
+    @CalledByNativeJavaTest
+    @NativeJavaTestFeatures.Enable(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void testIncognitoStateChange_goingToIncognito() {
+        mMediator.setShowIconsWhenUrlFocused(true);
+
+        setupSearchEngineLogoForTesting(
+                /* shouldShowLogo= */ true, /* showGoogle= */ true, /* loupeEverywhere= */ false);
+        mMediator.onIncognitoStateChanged(true);
+        verify(mMockForceModelViewReconciliationRunnable, times(0)).run();
+    }
+
+    @CalledByNativeJavaTest
+    @NativeJavaTestFeatures.Enable(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void testIncognitoStateChange_backFromIncognito() {
+        mMediator.setShowIconsWhenUrlFocused(true);
+
+        setupSearchEngineLogoForTesting(
+                /* shouldShowLogo= */ true, /* showGoogle= */ true, /* loupeEverywhere= */ false);
+        mMediator.onIncognitoStateChanged(true);
+        mMediator.onIncognitoStateChanged(false);
+        verify(mMockForceModelViewReconciliationRunnable).run();
+    }
+
+    @CalledByNativeJavaTest
+    @NativeJavaTestFeatures.Enable(ChromeFeatureList.OMNIBOX_SEARCH_ENGINE_LOGO)
+    public void testIncognitoStateChange_shouldShowStatusIcon() {
+        mMediator.setShowIconsWhenUrlFocused(true);
+
+        mMediator.onIncognitoStateChanged(true);
+        mMediator.onIncognitoStateChanged(false);
+        verify(mMockForceModelViewReconciliationRunnable, times(0)).run();
+    }
+
     private void setupSearchEngineLogoForTesting(
             boolean shouldShowLogo, boolean showGoogle, boolean loupeEverywhere) {
         doReturn(shouldShowLogo).when(mDelegate).shouldShowSearchEngineLogo(false);
         doReturn(false).when(mDelegate).shouldShowSearchEngineLogo(true);
         doReturn(loupeEverywhere).when(mDelegate).shouldShowSearchLoupeEverywhere(anyBoolean());
-        Mockito.doNothing().when(mDelegate).getSearchEngineLogoFavicon(
-                any(), mCallbackCaptor.capture());
+        doNothing().when(mDelegate).getSearchEngineLogoFavicon(any(), mCallbackCaptor.capture());
 
         mMediator.updateSearchEngineStatusIcon(shouldShowLogo, showGoogle, TEST_SEARCH_URL);
     }