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 | 3 +- .../browser/share/ShareButtonController.java | 37 +++++++++++ chrome/browser/about_flags.cc | 6 -- chrome/browser/flag-metadata.json | 3 +- .../flags/android/chrome_feature_list.cc | 5 ++ .../browser/flags/ChromeFeatureList.java | 1 + .../AdaptiveToolbarButtonController.java | 6 +- .../adaptive/AdaptiveToolbarFeatures.java | 65 ++++++++++++++++++- .../AdaptiveToolbarStatePredictor.java | 9 ++- .../AdaptiveToolbarStatePredictorTest.java | 15 +++++ 10 files changed, 138 insertions(+), 12 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 @@ -46,6 +46,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.chrome.browser.ui.signin.SyncPromoController; @@ -247,7 +248,7 @@ public class MainSettings extends ChromeBaseSettingsFragment uiState -> { // We don't show the toolbar shortcut settings page if disabled from // finch. - if (uiState.canShowUi) return; + 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 @@ -111,6 +111,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 @@ -658,18 +658,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 @@ -42,9 +42,10 @@ "expiry_milestone": 125 }, { + // restored in Bromite "name": "adaptive-button-in-top-toolbar-customization", "owners": [ "salg@google.com", "chrome-segmentation-platform@google.com" ], - "expiry_milestone": 125 + "expiry_milestone": -1 }, { "name": "adaptive-button-in-top-toolbar-translate", diff --git a/chrome/browser/flags/android/chrome_feature_list.cc b/chrome/browser/flags/android/chrome_feature_list.cc --- a/chrome/browser/flags/android/chrome_feature_list.cc +++ b/chrome/browser/flags/android/chrome_feature_list.cc @@ -150,6 +150,7 @@ const base::Feature* const kFeaturesExposedToJava[] = { &history_clusters::internal::kJourneys, &history_clusters::internal::kOmniboxAction, &history_clusters::internal::kOmniboxHistoryClusterProvider, + &kAdaptiveButtonInTopToolbar, &kAdaptiveButtonInTopToolbarTranslate, &kAdaptiveButtonInTopToolbarAddToBookmarks, &kAdaptiveButtonInTopToolbarCustomizationV2, @@ -388,6 +389,10 @@ static jlong JNI_ChromeFeatureMap_GetNativeMap(JNIEnv* env) { // Alphabetical: +CROMITE_FEATURE(kAdaptiveButtonInTopToolbar, + "AdaptiveButtonInTopToolbar", + base::FEATURE_DISABLED_BY_DEFAULT); + BASE_FEATURE(kAdaptiveButtonInTopToolbarTranslate, "AdaptiveButtonInTopToolbarTranslate", base::FEATURE_DISABLED_BY_DEFAULT); diff --git a/chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java b/chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java --- a/chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java +++ b/chrome/browser/flags/android/java/src/org/chromium/chrome/browser/flags/ChromeFeatureList.java @@ -142,6 +142,7 @@ public abstract class ChromeFeatureList { } public static final String ALLOW_USER_CERTIFICATES = "AllowUserCertificates"; + public static final String ADAPTIVE_BUTTON_IN_TOP_TOOLBAR = "AdaptiveButtonInTopToolbar"; public static final String ADAPTIVE_BUTTON_IN_TOP_TOOLBAR_TRANSLATE = "AdaptiveButtonInTopToolbarTranslate"; public static final String ADAPTIVE_BUTTON_IN_TOP_TOOLBAR_ADD_TO_BOOKMARKS = 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 @@ -253,7 +253,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 = 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 @@ -17,8 +17,18 @@ import org.chromium.chrome.browser.readaloud.ReadAloudFeatures; import java.util.HashMap; -/** A utility class for handling feature flags used by {@link AdaptiveToolbarButtonController}. */ +/** 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"; @@ -34,6 +44,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 static final String VARIATION_PARAM_MIN_VERSION = "min_version_adaptive"; @@ -102,6 +113,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. * @@ -242,11 +268,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 @@ -87,10 +87,15 @@ public class AdaptiveToolbarStatePredictor { // Early return if the feature isn't enabled. if (!AdaptiveToolbarFeatures.isCustomizationEnabled()) { + boolean canShowUi = AdaptiveToolbarFeatures.isSingleVariantModeEnabled(); + @AdaptiveToolbarButtonVariant + int toolbarButtonState = AdaptiveToolbarFeatures.isSingleVariantModeEnabled() + ? AdaptiveToolbarFeatures.getSingleVariantMode() + : AdaptiveToolbarButtonVariant.UNKNOWN; callback.onResult( new UiState( - false, - AdaptiveToolbarButtonVariant.UNKNOWN, + 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 @@ -72,6 +72,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() { --