Toolbar: HomeButton without HomepageManager dependency

Made HomeButton work without dependency on chrome.browser.homepage
classes. All the deps are now deferred to ToolbarManager.
The dependencies are injected not through ctor but init method
introduced in this CL since the home button and the enclosing view
(ToolbarLayout) are instantiated from xml.

Menu handling logic was moved to HomepageManager to make it test-
friendly.

Also removed a bunch of dead code in HomeButton. Several public setters
(ActivityTabObserver, ThemeColorProvider, StartSurfaceSupplier) were
used by BrowsingModeBottomToolbarCoordinator only (see
https://crrev.com/c/1981099). There's no callsite left any more as
the class was already gone.

Bug: 1127732
Change-Id: I56b8cc200d79a2790f0fd81b515bbe0d41fe5d99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2459428Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Wenyu Fu <wenyufu@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819659}

chrome/android/java/src/org/chromium/chrome/browser/homepage/HomepageManager.java

@@ -4,6 +4,7 @@
 
 package org.chromium.chrome.browser.homepage;
 
+import android.content.Context;
 import android.text.TextUtils;
 
 import androidx.annotation.NonNull;
@@ -12,10 +13,14 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.base.ObserverList;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.homepage.settings.HomepageMetricsEnums.HomepageLocationType;
+import org.chromium.chrome.browser.homepage.settings.HomepageSettings;
 import org.chromium.chrome.browser.partnercustomizations.PartnerBrowserCustomizations;
 import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
 import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
+import org.chromium.chrome.browser.settings.SettingsLauncher;
+import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
 import org.chromium.components.embedder_support.util.UrlConstants;
 import org.chromium.components.embedder_support.util.UrlUtilities;
 
@@ -40,12 +45,14 @@ public class HomepageManager implements HomepagePolicyManager.HomepagePolicyStat
 
     private final SharedPreferencesManager mSharedPreferencesManager;
     private final ObserverList<HomepageStateListener> mHomepageStateListeners;
+    private SettingsLauncher mSettingsLauncher;
 
     private HomepageManager() {
         mSharedPreferencesManager = SharedPreferencesManager.getInstance();
         mHomepageStateListeners = new ObserverList<>();
         HomepagePolicyManager.getInstance().addListener(this);
         PartnerBrowserCustomizations.getInstance().setPartnerHomepageListener(this);
+        mSettingsLauncher = new SettingsLauncherImpl();
     }
 
     /**
@@ -73,6 +80,19 @@ public class HomepageManager implements HomepagePolicyManager.HomepagePolicyStat
         mHomepageStateListeners.removeObserver(listener);
     }
 
+    /**
+     * Menu click handler on home button.
+     * @param context {@link Context} used for launching a settings activity.
+     */
+    public void onMenuClick(Context context) {
+        assert ChromeFeatureList.isInitialized();
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.HOMEPAGE_SETTINGS_UI_CONVERSION)) {
+            mSettingsLauncher.launchSettingsActivity(context, HomepageSettings.class);
+        } else {
+            setPrefHomepageEnabled(false);
+        }
+    }
+
     /**
      * Notify any listeners about a homepage state change.
      */
@@ -295,4 +315,9 @@ public class HomepageManager implements HomepagePolicyManager.HomepagePolicyStat
     public void onHomepageUpdate() {
         notifyHomepageUpdated();
     }
+
+    @VisibleForTesting
+    public void setSettingsLauncherForTesting(SettingsLauncher launcher) {
+        mSettingsLauncher = launcher;
+    }
 }

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

@@ -5,7 +5,6 @@
 package org.chromium.chrome.browser.toolbar;
 
 import android.content.Context;
-import android.content.res.ColorStateList;
 import android.util.AttributeSet;
 import android.view.ContextMenu;
 import android.view.ContextMenu.ContextMenuInfo;
@@ -17,121 +16,51 @@ import android.view.View.OnCreateContextMenuListener;
 import androidx.annotation.VisibleForTesting;
 import androidx.core.content.ContextCompat;
 
-import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.Callback;
 import org.chromium.base.TraceEvent;
 import org.chromium.base.supplier.ObservableSupplier;
+import org.chromium.base.supplier.Supplier;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ActivityTabProvider;
-import org.chromium.chrome.browser.ActivityTabProvider.ActivityTabTabObserver;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
-import org.chromium.chrome.browser.homepage.HomepageManager;
-import org.chromium.chrome.browser.homepage.HomepagePolicyManager;
-import org.chromium.chrome.browser.homepage.settings.HomepageSettings;
-import org.chromium.chrome.browser.settings.SettingsLauncher;
-import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
-import org.chromium.chrome.browser.tab.Tab;
-import org.chromium.chrome.browser.tasks.ReturnToChromeExperimentsUtil;
-import org.chromium.chrome.browser.toolbar.ThemeColorProvider.TintObserver;
-import org.chromium.chrome.features.start_surface.StartSurface;
-import org.chromium.chrome.features.start_surface.StartSurfaceState;
-import org.chromium.components.embedder_support.util.UrlUtilities;
 import org.chromium.ui.widget.ChromeImageButton;
 
 /**
  * The home button.
+ * TODO(crbug.com/1056422): Fix the visibility bug on NTP.
  */
 public class HomeButton extends ChromeImageButton
-        implements TintObserver, OnCreateContextMenuListener, MenuItem.OnMenuItemClickListener,
-                   HomepageManager.HomepageStateListener {
+        implements OnCreateContextMenuListener, MenuItem.OnMenuItemClickListener {
     @VisibleForTesting
     public static final int ID_REMOVE = 0;
     @VisibleForTesting
     public static final int ID_SETTINGS = 1;
 
-    /** A provider that notifies components when the theme color changes.*/
-    private ThemeColorProvider mThemeColorProvider;
-
-    /** The {@link ActivityTabTabObserver} used to know when the active page changed. */
-    private ActivityTabTabObserver mActivityTabTabObserver;
-
-    /** The {@link ActivityTabProvider} used to know if the active tab is on the NTP. */
-    private ActivityTabProvider mActivityTabProvider;
+    private Callback<Context> mOnMenuClickCallback;
+    private Supplier<Boolean> mIsManagedByPolicySupplier;
 
     // Test related members
     private static boolean sSaveContextMenuForTests;
     private ContextMenu mMenuForTests;
 
-    private SettingsLauncher mSettingsLauncher;
-
-    private ObservableSupplier<StartSurface> mStartSurfaceSupplier;
-    private Callback<StartSurface> mStartSurfaceSupplierObserver;
-    private StartSurface mStartSurface;
-    private StartSurface.StateObserver mStartSurfaceStateObserver;
-
     public HomeButton(Context context, AttributeSet attrs) {
         super(context, attrs);
 
         final int homeButtonIcon = R.drawable.btn_toolbar_home;
         setImageDrawable(ContextCompat.getDrawable(context, homeButtonIcon));
-        HomepageManager.getInstance().addListener(this);
-        updateContextMenuListener();
-
-        mSettingsLauncher = new SettingsLauncherImpl();
-    }
-
-    public void destroy() {
-        if (mThemeColorProvider != null) {
-            mThemeColorProvider.removeTintObserver(this);
-            mThemeColorProvider = null;
-        }
-
-        if (mActivityTabTabObserver != null) {
-            mActivityTabTabObserver.destroy();
-            mActivityTabTabObserver = null;
-        }
-
-        HomepageManager.getInstance().removeListener(this);
-
-        if (mStartSurfaceSupplier != null) {
-            mStartSurfaceSupplier.removeObserver(mStartSurfaceSupplierObserver);
-        }
-
-        if (mStartSurface != null) {
-            mStartSurface.removeStateChangeObserver(mStartSurfaceStateObserver);
-        }
     }
 
-    public void setThemeColorProvider(ThemeColorProvider themeColorProvider) {
-        mThemeColorProvider = themeColorProvider;
-        mThemeColorProvider.addTintObserver(this);
-    }
-
-    public void setStartSurfaceSupplier(ObservableSupplier<StartSurface> startSurfaceSupplier) {
-        assert ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePage();
-
-        mStartSurfaceSupplier = startSurfaceSupplier;
-        mStartSurfaceSupplierObserver = (startSurface) -> {
-            // TODO(crbug.com/1084528): Replace with OneShotSupplier when it is available.
-            assert startSurface != null;
-            assert mStartSurface == null : "The StartSurface should be set at most once.";
-
-            mStartSurface = startSurface;
-            mStartSurfaceStateObserver = (newState, shouldShowToolbar) -> {
-                if (newState == StartSurfaceState.SHOWN_HOMEPAGE) {
-                    updateButtonEnabledState(false);
-                } else {
-                    updateButtonEnabledState(null);
-                }
-            };
-            startSurface.addStateChangeObserver(mStartSurfaceStateObserver);
-        };
-        mStartSurfaceSupplier.addObserver(mStartSurfaceSupplierObserver);
-    }
-
-    @Override
-    public void onTintChanged(ColorStateList tint, boolean useLight) {
-        ApiCompatibilityUtils.setImageTintList(this, tint);
+    /**
+     * Initialize home button.
+     * @param homepageVisibility Observable used to react on homepage visibility change.
+     * @param onHomepageMenuClickCallback Callback for menu click event on homepage.
+     * @param isHomepageManagedByPolicy Supplier that tells if homepage is managed by policy.
+     */
+    public void init(ObservableSupplier<Boolean> homepageVisibility,
+            Callback<Context> onMenuClickCallback, Supplier<Boolean> isHomepageManagedByPolicy) {
+        homepageVisibility.addObserver((visible) -> updateContextMenuListener());
+        mOnMenuClickCallback = onMenuClickCallback;
+        mIsManagedByPolicySupplier = isHomepageManagedByPolicy;
+        updateContextMenuListener();
     }
 
     @Override
@@ -152,40 +81,17 @@ public class HomeButton extends ChromeImageButton
 
     @Override
     public boolean onMenuItemClick(MenuItem item) {
-        assert !isManagedByPolicy();
+        assert !mIsManagedByPolicySupplier.get();
         if (isHomepageSettingsUIConversionEnabled()) {
             assert item.getItemId() == ID_SETTINGS;
-            mSettingsLauncher.launchSettingsActivity(getContext(), HomepageSettings.class);
         } else {
             assert item.getItemId() == ID_REMOVE;
-            HomepageManager.getInstance().setPrefHomepageEnabled(false);
         }
+        mOnMenuClickCallback.onResult(getContext());
 
         return true;
     }
 
-    @Override
-    public void onHomepageStateUpdated() {
-        updateButtonEnabledState(null);
-    }
-
-    public void setActivityTabProvider(ActivityTabProvider activityTabProvider) {
-        mActivityTabProvider = activityTabProvider;
-        mActivityTabTabObserver = new ActivityTabTabObserver(activityTabProvider) {
-            @Override
-            public void onObservingDifferentTab(Tab tab, boolean hint) {
-                if (tab == null) return;
-                updateButtonEnabledState(tab);
-            }
-
-            @Override
-            public void onUpdateUrl(Tab tab, String url) {
-                if (tab == null) return;
-                updateButtonEnabledState(tab);
-            }
-        };
-    }
-
     @Override
     protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
         try (TraceEvent e = TraceEvent.scoped("HomeButton.onMeasure")) {
@@ -200,60 +106,8 @@ public class HomeButton extends ChromeImageButton
         }
     }
 
-    /**
-     * Menu button is enabled when not in NTP or if in NTP and homepage is enabled and set to
-     * somewhere other than the NTP.
-     * @param tab The notifying {@link Tab} that might be selected soon, this is a hint that a tab
-     *         change is likely.
-     */
-    public void updateButtonEnabledState(Tab tab) {
-        // New tab page button takes precedence over homepage.
-        final boolean isHomepageEnabled = HomepageManager.isHomepageEnabled();
-
-        boolean isEnabled;
-        if (getActiveTab() != null) {
-            // Now tab shows a webpage, let's check if the webpage is not the NTP, or the webpage is
-            // NTP but homepage is not NTP.
-            isEnabled = !isTabNTP(getActiveTab())
-                    || (isHomepageEnabled
-                            && !UrlUtilities.isNTPUrl(HomepageManager.getHomepageUri()));
-        } else {
-            // There is no active tab, which means tab is in transition, ex tab swither view to tab
-            // view, or from one tab to another tab.
-            isEnabled = !isTabNTP(tab);
-        }
-        updateButtonEnabledState(isEnabled);
-    }
-
-    private void updateButtonEnabledState(boolean isEnabled) {
-        setEnabled(isEnabled);
-        updateContextMenuListener();
-    }
-
-    /**
-     * Check if the provided tab is NTP. The tab is a hint that
-     * @param tab The notifying {@link Tab} that might be selected soon, this is a hint that a tab
-     *         change is likely.
-     */
-    private boolean isTabNTP(Tab tab) {
-        return tab != null && UrlUtilities.isNTPUrl(tab.getUrlString());
-    }
-
-    /**
-     * Return the active tab. If no active tab is shown, return null.
-     */
-    private Tab getActiveTab() {
-        if (mActivityTabProvider == null) return null;
-
-        return mActivityTabProvider.get();
-    }
-
-    private boolean isManagedByPolicy() {
-        return HomepagePolicyManager.isHomepageManagedByPolicy();
-    }
-
     private void updateContextMenuListener() {
-        if (!isManagedByPolicy()) {
+        if (!mIsManagedByPolicySupplier.get()) {
             setOnCreateContextMenuListener(this);
         } else {
             setOnCreateContextMenuListener(null);
@@ -281,9 +135,4 @@ public class HomeButton extends ChromeImageButton
     public ContextMenu getMenuForTests() {
         return mMenuForTests;
     }
-
-    @VisibleForTesting
-    public void setSettingsLauncherForTests(SettingsLauncher settingsLauncher) {
-        mSettingsLauncher = settingsLauncher;
-    }
 }

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

@@ -53,6 +53,7 @@ import org.chromium.chrome.browser.findinpage.FindToolbarObserver;
 import org.chromium.chrome.browser.fullscreen.FullscreenManager;
 import org.chromium.chrome.browser.fullscreen.FullscreenOptions;
 import org.chromium.chrome.browser.homepage.HomepageManager;
+import org.chromium.chrome.browser.homepage.HomepagePolicyManager;
 import org.chromium.chrome.browser.identity_disc.IdentityDiscController;
 import org.chromium.chrome.browser.ntp.FakeboxDelegate;
 import org.chromium.chrome.browser.ntp.IncognitoNewTabPage;
@@ -683,6 +684,12 @@ public class ToolbarManager implements UrlFocusChangeListener, ThemeColorObserve
             identityDiscController.addObserver(
                     (canShowHint) -> mIdentityDiscStateSupplier.set(canShowHint));
         }
+        HomeButton homeButton = toolbarLayout.getHomeButton();
+        if (homeButton != null) {
+            homeButton.init(mHomeButtonVisibilitySupplier,
+                    HomepageManager.getInstance()::onMenuClick,
+                    HomepagePolicyManager::isHomepageManagedByPolicy);
+        }
         return toolbar;
     }
 

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

@@ -365,10 +365,6 @@ public class ToolbarPhone extends ToolbarLayout implements OnClickListener, TabC
 
     @Override
     void destroy() {
-        if (mHomeButton != null) {
-            mHomeButton.destroy();
-            mHomeButton = null;
-        }
         if (mLocationBar != null) {
             mLocationBar.destroy();
             mLocationBar = null;

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

@@ -151,10 +151,6 @@ public class ToolbarTablet extends ToolbarLayout
 
     @Override
     void destroy() {
-        if (mHomeButton != null) {
-            mHomeButton.destroy();
-            mHomeButton = null;
-        }
         if (mLocationBar != null) {
             mLocationBar.destroy();
             mLocationBar = null;

chrome/android/javatests/src/org/chromium/chrome/browser/toolbar/HomeButtonTest.java

@@ -26,9 +26,11 @@ import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 
+import org.chromium.base.supplier.ObservableSupplierImpl;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.homepage.HomepageManager;
+import org.chromium.chrome.browser.homepage.HomepagePolicyManager;
 import org.chromium.chrome.browser.homepage.HomepageTestRule;
 import org.chromium.chrome.browser.homepage.settings.HomepageSettings;
 import org.chromium.chrome.browser.settings.SettingsLauncher;
@@ -83,8 +85,11 @@ public class HomeButtonTest extends DummyUiActivityTestCase {
 
             mIdHomeButton = View.generateViewId();
             mHomeButton = new HomeButton(getActivity(), null);
+            mHomeButton.init(new ObservableSupplierImpl<Boolean>(),
+                    HomepageManager.getInstance()::onMenuClick,
+                    HomepagePolicyManager::isHomepageManagedByPolicy);
             mHomeButton.setId(mIdHomeButton);
-            mHomeButton.setSettingsLauncherForTests(mSettingsLauncher);
+            HomepageManager.getInstance().setSettingsLauncherForTesting(mSettingsLauncher);
             HomeButton.setSaveContextMenuForTests(true);
 
             content.addView(mHomeButton);