From: csagan5 <32685696+csagan5@users.noreply.github.com> Date: Wed, 7 Dec 2022 20:32:15 +0100 Subject: Restore adaptive-button-in-top-toolbar-customization This reverts commit 18d03b9cca4e90d2a446ea28266876d8c5fdc4f0. Voice button and legacy share/voice functionality is not restored. License: GPL-3.0-only - https://spdx.org/licenses/GPL-3.0-only.html --- .../chrome/browser/settings/MainSettings.java | 5 +- .../browser/share/ShareButtonController.java | 37 +++++++++++ chrome/browser/about_flags.cc | 6 -- chrome/browser/flag-metadata.json | 3 +- .../AdaptiveToolbarButtonController.java | 6 +- .../adaptive/AdaptiveToolbarFeatures.java | 63 +++++++++++++++++++ .../AdaptiveToolbarStatePredictor.java | 7 ++- .../AdaptiveToolbarStatePredictorTest.java | 15 +++++ 8 files changed, 132 insertions(+), 10 deletions(-) diff --git a/chrome/android/java/src/org/chromium/chrome/browser/settings/MainSettings.java b/chrome/android/java/src/org/chromium/chrome/browser/settings/MainSettings.java --- a/chrome/android/java/src/org/chromium/chrome/browser/settings/MainSettings.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/settings/MainSettings.java @@ -43,6 +43,7 @@ import org.chromium.chrome.browser.sync.settings.ManageSyncSettings; import org.chromium.chrome.browser.sync.settings.SignInPreference; import org.chromium.chrome.browser.sync.settings.SyncPromoPreference; import org.chromium.chrome.browser.sync.settings.SyncSettingsUtils; +import org.chromium.chrome.browser.toolbar.adaptive.AdaptiveToolbarFeatures; import org.chromium.chrome.browser.toolbar.adaptive.AdaptiveToolbarStatePredictor; import org.chromium.chrome.browser.tracing.settings.DeveloperSettings; import org.chromium.components.browser_ui.settings.ChromeBasePreference; @@ -225,7 +226,9 @@ public class MainSettings extends ChromeBaseSettingsFragment new AdaptiveToolbarStatePredictor(null).recomputeUiState(uiState -> { // We don't show the toolbar shortcut settings page if disabled from finch. - if (uiState.canShowUi) return; + // Note, we can still have the old data collection experiment running for which + // |canShowUi| might be true. In that case, just hide the settings page. + if (uiState.canShowUi && !AdaptiveToolbarFeatures.isSingleVariantModeEnabled()) return; getPreferenceScreen().removePreference(findPreference(PREF_TOOLBAR_SHORTCUT)); }); diff --git a/chrome/android/java/src/org/chromium/chrome/browser/share/ShareButtonController.java b/chrome/android/java/src/org/chromium/chrome/browser/share/ShareButtonController.java --- a/chrome/android/java/src/org/chromium/chrome/browser/share/ShareButtonController.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/share/ShareButtonController.java @@ -100,6 +100,43 @@ public class ShareButtonController extends BaseButtonDataProvider { return mShareUtils.shouldEnableShare(tab); } +/* @Override + public ButtonData get(Tab tab) { + updateButtonVisibility(tab); + maybeSetIphCommandBuilder(tab); + return mButtonData; + } + + private void updateButtonVisibility(Tab tab) { + if (tab == null || tab.getWebContents() == null || mTabProvider == null + || mTabProvider.get() == null || !isFeatureEnabled()) { + mButtonData.setCanShow(false); + return; + } + + final boolean isDeviceWideEnough = + mScreenWidthDp >= AdaptiveToolbarFeatures.getDeviceMinimumWidthForShowingButton(); + if (mShareDelegateSupplier.get() == null || !isDeviceWideEnough) { + mButtonData.setCanShow(false); + return; + } + + mButtonData.setCanShow(mShareUtils.shouldEnableShare(tab)); + } + + private static boolean isFeatureEnabled() { + return (AdaptiveToolbarFeatures.isSingleVariantModeEnabled() + && AdaptiveToolbarFeatures.getSingleVariantMode() + == AdaptiveToolbarButtonVariant.SHARE) + || AdaptiveToolbarFeatures.isCustomizationEnabled(); + } + + private void notifyObservers(boolean hint) { + for (ButtonDataObserver observer : mObservers) { + observer.buttonDataChanged(hint); + } + } */ + /** * Returns an IPH for this button. Only called once native is initialized and when {@code * AdaptiveToolbarFeatures.isCustomizationEnabled()} is true. diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc --- a/chrome/browser/about_flags.cc +++ b/chrome/browser/about_flags.cc @@ -685,18 +685,12 @@ const FeatureEntry::FeatureParam kAdaptiveButtonCustomization_Share[] = { {"default_segment", "share"}, {"show_ui_only_after_ready", "false"}, {"ignore_segmentation_results", "true"}}; -const FeatureEntry::FeatureParam kAdaptiveButtonCustomization_Voice[] = { - {"default_segment", "voice"}, - {"show_ui_only_after_ready", "false"}, - {"ignore_segmentation_results", "true"}}; const FeatureEntry::FeatureVariation kAdaptiveButtonInTopToolbarCustomizationVariations[] = { {"New Tab", kAdaptiveButtonCustomization_NewTab, std::size(kAdaptiveButtonCustomization_NewTab), nullptr}, {"Share", kAdaptiveButtonCustomization_Share, std::size(kAdaptiveButtonCustomization_Share), nullptr}, - {"Voice", kAdaptiveButtonCustomization_Voice, - std::size(kAdaptiveButtonCustomization_Voice), nullptr}, }; const FeatureEntry::FeatureParam kContextualPageActionsUiParams_Quiet[] = { diff --git a/chrome/browser/flag-metadata.json b/chrome/browser/flag-metadata.json --- a/chrome/browser/flag-metadata.json +++ b/chrome/browser/flag-metadata.json @@ -49,9 +49,10 @@ "expiry_milestone": 120 }, { + // restored in Bromite "name": "adaptive-button-in-top-toolbar-customization", "owners": [ "shaktisahu", "chrome-segmentation-platform@google.com" ], - "expiry_milestone": 120 + "expiry_milestone": -1 }, { "name": "adaptive-button-in-top-toolbar-translate", diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java --- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java +++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java @@ -249,7 +249,11 @@ public class AdaptiveToolbarButtonController @Override public void onFinishNativeInitialization() { - if (AdaptiveToolbarFeatures.isCustomizationEnabled()) { + if (AdaptiveToolbarFeatures.isSingleVariantModeEnabled()) { + @AdaptiveToolbarButtonVariant + int variant = AdaptiveToolbarFeatures.getSingleVariantMode(); + setSingleProvider(variant); + } else if (AdaptiveToolbarFeatures.isCustomizationEnabled()) { mAdaptiveToolbarStatePredictor.recomputeUiState(uiState -> { mSessionButtonVariant = uiState.canShowUi ? uiState.toolbarButtonState : AdaptiveToolbarButtonVariant.UNKNOWN; diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarFeatures.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarFeatures.java --- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarFeatures.java +++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarFeatures.java @@ -16,8 +16,18 @@ import org.chromium.chrome.browser.signin.services.UnifiedConsentServiceBridge; /** * A utility class for handling feature flags used by {@link AdaptiveToolbarButtonController}. + * + *

TODO(shaktisahu): This class supports both the data collection and the customization + * experiment. Cleanup once the former is no longer needed. */ public class AdaptiveToolbarFeatures { + /** Adaptive toolbar button is always empty. */ + public static final String ALWAYS_NONE = "always-none"; + /** Adaptive toolbar button opens a new tab. */ + public static final String ALWAYS_NEW_TAB = "always-new-tab"; + /** Adaptive toolbar button shares the current tab. */ + public static final String ALWAYS_SHARE = "always-share"; + /** Finch default group for new tab variation. */ static final String NEW_TAB = "new-tab"; /** Finch default group for share variation. */ @@ -30,6 +40,7 @@ public class AdaptiveToolbarFeatures { private static final String VARIATION_PARAM_DISABLE_UI = "disable_ui"; private static final String VARIATION_PARAM_IGNORE_SEGMENTATION_RESULTS = "ignore_segmentation_results"; + private static final String VARIATION_PARAM_SINGLE_VARIANT_MODE = "mode"; private static final String VARIATION_PARAM_SHOW_UI_ONLY_AFTER_READY = "show_ui_only_after_ready"; @VisibleForTesting @@ -100,6 +111,21 @@ public class AdaptiveToolbarFeatures { } } + /** + * Returns whether the adaptive toolbar is enabled in single variant mode. Returns true also to + * provide legacy support for feature flags {@code ShareButtonInTopToolbar} and {@code + * VoiceButtonInTopToolbar}. + * + *

Must be called with the {@link FeatureList} initialized. + */ + public static boolean isSingleVariantModeEnabled() { + if (isCustomizationEnabled()) return false; + if (ChromeFeatureList.isEnabled(ChromeFeatureList.ADAPTIVE_BUTTON_IN_TOP_TOOLBAR)) { + return true; + } + return false; + } + /** * Returns whether the adaptive toolbar is enabled with segmentation and customization. * @@ -220,11 +246,48 @@ public class AdaptiveToolbarFeatures { ChromeFeatureList.CONTEXTUAL_PAGE_ACTIONS, "enable_ui", true); } + /** + * When the adaptive toolbar is configured in a single button variant mode, returns the {@link + * AdaptiveToolbarButtonVariant} being used. + * + *

This methods avoids parsing param strings more than once. Tests need to call {@link + * #clearParsedParamsForTesting()} to clear the cached values. + * + *

Must be called with the {@link FeatureList} initialized. + * + *

TODO(shaktisahu): Have a similar method for segmentation. + */ + @AdaptiveToolbarButtonVariant + public static int getSingleVariantMode() { + assert isSingleVariantModeEnabled(); + if (sButtonVariant != null) return sButtonVariant; + + String mode = ChromeFeatureList.getFieldTrialParamByFeature( + ChromeFeatureList.ADAPTIVE_BUTTON_IN_TOP_TOOLBAR, + VARIATION_PARAM_SINGLE_VARIANT_MODE); + switch (mode) { + case ALWAYS_NONE: + sButtonVariant = AdaptiveToolbarButtonVariant.NONE; + break; + case ALWAYS_NEW_TAB: + sButtonVariant = AdaptiveToolbarButtonVariant.NEW_TAB; + break; + case ALWAYS_SHARE: + sButtonVariant = AdaptiveToolbarButtonVariant.SHARE; + break; + default: + sButtonVariant = AdaptiveToolbarButtonVariant.UNKNOWN; + break; + } + return sButtonVariant; + } + /** * Returns the default variant to be shown in segmentation experiment when the backend results * are unavailable or not configured. */ static @AdaptiveToolbarButtonVariant int getSegmentationDefault() { + assert !isSingleVariantModeEnabled(); assert isCustomizationEnabled(); if (sButtonVariant != null) return sButtonVariant; String defaultSegment = getDefaultSegment(); diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictor.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictor.java --- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictor.java +++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictor.java @@ -93,7 +93,12 @@ public class AdaptiveToolbarStatePredictor { // Early return if the feature isn't enabled. if (!AdaptiveToolbarFeatures.isCustomizationEnabled()) { - callback.onResult(new UiState(false, AdaptiveToolbarButtonVariant.UNKNOWN, + boolean canShowUi = AdaptiveToolbarFeatures.isSingleVariantModeEnabled(); + @AdaptiveToolbarButtonVariant + int toolbarButtonState = AdaptiveToolbarFeatures.isSingleVariantModeEnabled() + ? AdaptiveToolbarFeatures.getSingleVariantMode() + : AdaptiveToolbarButtonVariant.UNKNOWN; + callback.onResult(new UiState(canShowUi, toolbarButtonState, AdaptiveToolbarButtonVariant.UNKNOWN, AdaptiveToolbarButtonVariant.UNKNOWN)); return; } diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictorTest.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictorTest.java --- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictorTest.java +++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarStatePredictorTest.java @@ -68,6 +68,21 @@ public class AdaptiveToolbarStatePredictorTest { statePredictor.recomputeUiState(verifyResultCallback(expected)); } + @Test + @SmallTest + @EnableFeatures({ChromeFeatureList.ADAPTIVE_BUTTON_IN_TOP_TOOLBAR, + ChromeFeatureList.VOICE_SEARCH_AUDIO_CAPTURE_POLICY}) + @DisableFeatures({ChromeFeatureList.ADAPTIVE_BUTTON_IN_TOP_TOOLBAR_CUSTOMIZATION_V2}) + public void + testWorksWithDataCollectionFeatureFlag() { + ShadowChromeFeatureList.sParamValues.put("mode", "always-voice"); + AdaptiveToolbarStatePredictor statePredictor = buildStatePredictor( + true, AdaptiveToolbarButtonVariant.VOICE, true, AdaptiveToolbarButtonVariant.SHARE); + UiState expected = new UiState(true, AdaptiveToolbarButtonVariant.VOICE, + AdaptiveToolbarButtonVariant.UNKNOWN, AdaptiveToolbarButtonVariant.UNKNOWN); + statePredictor.recomputeUiState(verifyResultCallback(expected)); + } + @Test @SmallTest public void testManualOverride() { -- 2.25.1