From d20d1b34b293296164f01e6cd2a06ee3c838a11f Mon Sep 17 00:00:00 2001 From: mcrouse Date: Mon, 5 Jun 2023 21:40:09 +0000 Subject: [PATCH] Reland "Reland "[CSC] Create chrome flags for Companion UXR."" This reverts commit f91001b8b99de1e7cedb7f98d014ba38a88e7ca6. Reason for revert: Re-land to fix the enums and histogram checks. Original change's description: > Revert "Reland "[CSC] Create chrome flags for Companion UXR."" > > This reverts commit 86f8ba8b8b3d310487a87a55c1c495d896a6cc80. > > Reason for revert: Tests still failing for feature: > https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/33141/test-results?sortby=&groupby= > > Bug: 1449587 > > Original change's description: > > Reland "[CSC] Create chrome flags for Companion UXR." > > > > This reverts commit d90aaa6b3b7ae492ee25c84f7119a3c781517f57. > > > > Reason for revert: Fixing the enums generation issue. > > > > Original change's description: > > > Revert "[CSC] Create chrome flags for Companion UXR." > > > > > > This reverts commit 8b1641ff7a22d69b668ab256cf48c57edb9391df. > > > > > > Reason for revert: Failing AboutFlagsHistogram on linux-chromeos-chrome. > > > https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/33089/overview > > > PTAL: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#flag-histograms > > > > > > Original change's description: > > > > [CSC] Create chrome flags for Companion UXR. > > > > > > > > This requires us to be able to explicitly control the clobber vs new > > > > tab behavior. We also need to be able to control the pin AND ignore > > > > the labs experiment state (e.g., when unpinned and labs is on, > > > > the UI will attempt to force the state to pinned, this overrides > > > > that behavior). > > > > > > > > Bug: b:283109243 > > > > Change-Id: I981ad573ee38cb4b69bb39f008c134c4e14bbf56 > > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4553173 > > > > Reviewed-by: Caroline Rising > > > > Commit-Queue: Michael Crouse > > > > Reviewed-by: Tarun Bansal > > > > Cr-Commit-Position: refs/heads/main@{#1148554} > > > > > > Bug: b:283109243 > > > Change-Id: Ic0fcdcc8512a5e38ffbaef7b5492c766b1961294 > > > No-Presubmit: true > > > No-Tree-Checks: true > > > No-Try: true > > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4564057 > > > Owners-Override: Ana Salazar Maldonado > > > Reviewed-by: Ana Salazar Maldonado > > > Commit-Queue: Rubber Stamper > > > Bot-Commit: Rubber Stamper > > > Auto-Submit: Ana Salazar Maldonado > > > Cr-Commit-Position: refs/heads/main@{#1148779} > > > > Bug: b:283109243 > > Change-Id: I1785c9de3aee67a87a19ce51cc8591fa99f7f40a > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4569767 > > Reviewed-by: Tarun Bansal > > Commit-Queue: Michael Crouse > > Cr-Commit-Position: refs/heads/main@{#1149978} > > Bug: b:283109243 > Change-Id: I43be3f4a73c604e601d40bcf7a16745e2cc04f79 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4571762 > Owners-Override: Ana Salazar Maldonado > Auto-Submit: Ana Salazar Maldonado > Commit-Queue: Rubber Stamper > Bot-Commit: Rubber Stamper > Cr-Commit-Position: refs/heads/main@{#1150204} (cherry picked from commit ff4a6cf43a85f3c59d50f6ef26385d9fc1a4f182) Bug: 1449587 Bug: b:283109243 Change-Id: I60082380b29022de4ca58417020c74a695b00772 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4567442 Commit-Queue: Michael Crouse Reviewed-by: Tarun Bansal Auto-Submit: Michael Crouse Cr-Original-Commit-Position: refs/heads/main@{#1151070} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4590411 Cr-Commit-Position: refs/branch-heads/5790@{#375} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- chrome/browser/about_flags.cc | 32 ++++++- chrome/browser/companion/core/BUILD.gn | 2 +- chrome/browser/companion/core/features.cc | 22 +++++ chrome/browser/companion/core/features.h | 6 ++ chrome/browser/flag-metadata.json | 10 ++ chrome/browser/flag_descriptions.cc | 6 ++ chrome/browser/flag_descriptions.h | 6 ++ .../side_panel/companion/companion_utils.cc | 8 ++ .../companion/companion_utils_unittest.cc | 96 +++++++++++++++++++ chrome/test/BUILD.gn | 1 + tools/metrics/histograms/enums.xml | 3 + 11 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 chrome/browser/ui/side_panel/companion/companion_utils_unittest.cc diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc index 4b29b841c9a2e..4b0adecc90db5 100644 --- a/chrome/browser/about_flags.cc +++ b/chrome/browser/about_flags.cc @@ -3240,14 +3240,34 @@ constexpr FeatureEntry::FeatureVariation kLensPingVariations[] = { }; #if BUILDFLAG(ENABLE_LENS_DESKTOP_GOOGLE_BRANDED_FEATURES) -constexpr FeatureEntry::FeatureParam kCscVariation[] = { +constexpr FeatureEntry::FeatureParam kCscStagingEnvVariation[] = { {"companion-homepage-url", "https://lens-staging.corp.google.com/companion"}, {"companion-image-upload-url", "https://lens-staging.corp.google.com/v2/upload"}}; +constexpr FeatureEntry::FeatureParam kCscClobberVariation[] = { + {"open-links-in-current-tab", "true"}, +}; +constexpr FeatureEntry::FeatureParam kCscNewTabVariation[] = { + {"open-links-in-current-tab", "false"}, +}; constexpr FeatureEntry::FeatureVariation kCscVariations[] = { - {"with staging URL", kCscVariation, std::size(kCscVariation), nullptr}}; + {"with staging URL", kCscStagingEnvVariation, + std::size(kCscStagingEnvVariation), nullptr}, + {"with clobber", kCscClobberVariation, std::size(kCscClobberVariation), + nullptr}, + {"with new tab", kCscNewTabVariation, std::size(kCscNewTabVariation), + nullptr}, +}; + +const FeatureEntry::Choice kSidePanelPinnedStateChoices[] = { + {flags_ui::kGenericExperimentChoiceDefault, "", ""}, + {"Forced Pinned", companion::switches::kForceCompanionPinnedState, + "pinned"}, + {"Forced Unpinned", companion::switches::kForceCompanionPinnedState, + "unpinned"}, +}; #endif // BUILDFLAG(ENABLE_LENS_DESKTOP_GOOGLE_BRANDED_FEATURES) #if BUILDFLAG(IS_CHROMEOS_ASH) @@ -8838,6 +8858,14 @@ const FeatureEntry kFeatureEntries[] = { kCscVariations, "CSC")}, + {"csc-pinned-state", flag_descriptions::kCscPinnedName, + flag_descriptions::kCscPinnedDescription, kOsDesktop, + MULTI_VALUE_TYPE(kSidePanelPinnedStateChoices)}, + + {"csc-vss", flag_descriptions::kCscVssName, + flag_descriptions::kCscVssDescription, kOsDesktop, + FEATURE_VALUE_TYPE(visual_search::features::kVisualSearchSuggestions)}, + {"enable-lens-region-search-static-page", flag_descriptions::kLensRegionSearchStaticPageName, flag_descriptions::kLensRegionSearchStaticPageDescription, kOsDesktop, diff --git a/chrome/browser/companion/core/BUILD.gn b/chrome/browser/companion/core/BUILD.gn index 0a14098a7f920..777fc4959c582 100644 --- a/chrome/browser/companion/core/BUILD.gn +++ b/chrome/browser/companion/core/BUILD.gn @@ -17,7 +17,7 @@ static_library("core") { "promo_handler.h", "signin_delegate.h", ] - + public_deps = [ "//third_party/abseil-cpp:absl" ] deps = [ "mojom:mojo_bindings", "proto", diff --git a/chrome/browser/companion/core/features.cc b/chrome/browser/companion/core/features.cc index 5459415d867be..5480c386284bc 100644 --- a/chrome/browser/companion/core/features.cc +++ b/chrome/browser/companion/core/features.cc @@ -39,10 +39,32 @@ namespace switches { const char kDisableCheckUserPermissionsForCompanion[] = "disable-checking-companion-user-permissions"; +const char kForceCompanionPinnedState[] = "force-companion-pinned-state"; + bool ShouldOverrideCheckingUserPermissionsForCompanion() { base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); return command_line->HasSwitch(kDisableCheckUserPermissionsForCompanion); } +absl::optional ShouldForceOverrideCompanionPinState() { + base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); + if (!command_line->HasSwitch(kForceCompanionPinnedState)) { + return absl::nullopt; + } + + std::string pinned_state = + command_line->GetSwitchValueASCII(kForceCompanionPinnedState); + if (pinned_state == "pinned") { + return true; + } + if (pinned_state == "unpinned") { + return false; + } + + NOTREACHED() << "Invalid Companion pin state command line switch value: " + << pinned_state; + return absl::nullopt; +} + } // namespace switches } // namespace companion diff --git a/chrome/browser/companion/core/features.h b/chrome/browser/companion/core/features.h index 974e795df8567..7d28b3b06d884 100644 --- a/chrome/browser/companion/core/features.h +++ b/chrome/browser/companion/core/features.h @@ -8,6 +8,7 @@ #include "base/feature_list.h" #include "base/metrics/field_trial_params.h" #include "build/build_config.h" +#include "third_party/abseil-cpp/absl/types/optional.h" namespace companion { namespace features { @@ -23,11 +24,16 @@ extern const base::FeatureParam kOpenLinksInCurrentTab; namespace switches { extern const char kDisableCheckUserPermissionsForCompanion[]; +extern const char kForceCompanionPinnedState[]; // Returns true if checking of the user's permissions to share page information // with the Companion server should be ignored. Returns true only in tests. bool ShouldOverrideCheckingUserPermissionsForCompanion(); +// Returns whether the Companion pin state should force overridden, regardless +// of prefs or labs state. +absl::optional ShouldForceOverrideCompanionPinState(); + } // namespace switches } // namespace companion diff --git a/chrome/browser/flag-metadata.json b/chrome/browser/flag-metadata.json index a67c0fb5af8fc..97397f89d3f8d 100644 --- a/chrome/browser/flag-metadata.json +++ b/chrome/browser/flag-metadata.json @@ -1327,6 +1327,16 @@ "owners": ["stanfield@google.com", "mcrouse@google.com", "lens-chrome@google.com"], "expiry_milestone": 120 }, + { + "name": "csc-pinned-state", + "owners": ["stanfield@google.com", "mcrouse@google.com", "lens-chrome@google.com"], + "expiry_milestone": 120 + }, + { + "name": "csc-vss", + "owners": ["pstjuste@google.com", "stanfield@google.com", "mcrouse@google.com", "lens-chrome@google.com"], + "expiry_milestone": 120 + }, { "name": "cups-ipp-printing-backend", "owners": [ "awscreen", "//printing/OWNERS" ], diff --git a/chrome/browser/flag_descriptions.cc b/chrome/browser/flag_descriptions.cc index fdff9d982b53d..eb37a988b124c 100644 --- a/chrome/browser/flag_descriptions.cc +++ b/chrome/browser/flag_descriptions.cc @@ -1986,6 +1986,12 @@ const char kEnableLensPingDescription[] = const char kCscName[] = "CSC"; const char kCscDescription[] = ""; +const char kCscPinnedName[] = "CSC Pin State"; +const char kCscPinnedDescription[] = ""; + +const char kCscVssName[] = "CSC-VSS"; +const char kCscVssDescription[] = ""; + const char kLogJsConsoleMessagesName[] = "Log JS console messages in system logs"; const char kLogJsConsoleMessagesDescription[] = diff --git a/chrome/browser/flag_descriptions.h b/chrome/browser/flag_descriptions.h index 84b5aa3a40bed..72099372bed66 100644 --- a/chrome/browser/flag_descriptions.h +++ b/chrome/browser/flag_descriptions.h @@ -1107,6 +1107,12 @@ extern const char kEnableLensPingDescription[]; extern const char kCscName[]; extern const char kCscDescription[]; +extern const char kCscPinnedName[]; +extern const char kCscPinnedDescription[]; + +extern const char kCscVssName[]; +extern const char kCscVssDescription[]; + extern const char kLensOnQuickActionSearchWidgetName[]; extern const char kLensOnQuickActionSearchWidgetDescription[]; diff --git a/chrome/browser/ui/side_panel/companion/companion_utils.cc b/chrome/browser/ui/side_panel/companion/companion_utils.cc index b8f541e1f3e05..ea135fee05ebc 100644 --- a/chrome/browser/ui/side_panel/companion/companion_utils.cc +++ b/chrome/browser/ui/side_panel/companion/companion_utils.cc @@ -71,6 +71,14 @@ bool IsSearchImageInCompanionSidePanelSupported(const Browser* browser) { } void UpdateCompanionDefaultPinnedToToolbarState(PrefService* pref_service) { + absl::optional should_force_pin = + switches::ShouldForceOverrideCompanionPinState(); + if (should_force_pin) { + pref_service->SetBoolean(prefs::kSidePanelCompanionEntryPinnedToToolbar, + *should_force_pin); + return; + } + bool companion_should_be_default_pinned = base::FeatureList::IsEnabled( ::features::kSidePanelCompanionDefaultPinned) || diff --git a/chrome/browser/ui/side_panel/companion/companion_utils_unittest.cc b/chrome/browser/ui/side_panel/companion/companion_utils_unittest.cc new file mode 100644 index 0000000000000..2531bd6914a5a --- /dev/null +++ b/chrome/browser/ui/side_panel/companion/companion_utils_unittest.cc @@ -0,0 +1,96 @@ +// Copyright 2023 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/ui/side_panel/companion/companion_utils.h" + +#include "base/command_line.h" +#include "base/feature_list.h" +#include "base/test/scoped_feature_list.h" +#include "chrome/browser/companion/core/constants.h" +#include "chrome/browser/companion/core/features.h" +#include "chrome/browser/ui/ui_features.h" +#include "chrome/common/pref_names.h" +#include "components/prefs/pref_registry_simple.h" +#include "components/prefs/testing_pref_service.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace companion { + +void RegisterPrefs(TestingPrefServiceSimple* pref_service) { + pref_service->registry()->RegisterBooleanPref( + prefs::kSidePanelCompanionEntryPinnedToToolbar, false); + pref_service->registry()->RegisterBooleanPref( + companion::kExpsOptInStatusGrantedPref, false); +} + +TEST(CompanionUtilsTest, PinnedStateCommandlineOverridePinned) { + TestingPrefServiceSimple pref_service; + RegisterPrefs(&pref_service); + base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( + switches::kForceCompanionPinnedState, "pinned"); + + UpdateCompanionDefaultPinnedToToolbarState(&pref_service); + EXPECT_EQ( + pref_service.GetBoolean(prefs::kSidePanelCompanionEntryPinnedToToolbar), + true); +} + +TEST(CompanionUtilsTest, PinnedStateCommandlineOverrideUnpinned) { + TestingPrefServiceSimple pref_service; + RegisterPrefs(&pref_service); + base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( + switches::kForceCompanionPinnedState, "unpinned"); + + UpdateCompanionDefaultPinnedToToolbarState(&pref_service); + EXPECT_EQ( + pref_service.GetBoolean(prefs::kSidePanelCompanionEntryPinnedToToolbar), + false); +} + +TEST(CompanionUtilsTest, UpdatePinnedStateDefaultUnpinnedLabsOverride) { + base::test::ScopedFeatureList scoped_feature_list; + TestingPrefServiceSimple pref_service; + RegisterPrefs(&pref_service); + + scoped_feature_list.InitAndDisableFeature( + ::features::kSidePanelCompanionDefaultPinned); + pref_service.SetBoolean(companion::kExpsOptInStatusGrantedPref, true); + + UpdateCompanionDefaultPinnedToToolbarState(&pref_service); + EXPECT_EQ( + pref_service.GetBoolean(prefs::kSidePanelCompanionEntryPinnedToToolbar), + true); +} + +TEST(CompanionUtilsTest, UpdatePinnedStateDefaultPinned) { + base::test::ScopedFeatureList scoped_feature_list; + TestingPrefServiceSimple pref_service; + RegisterPrefs(&pref_service); + + scoped_feature_list.InitAndEnableFeature( + ::features::kSidePanelCompanionDefaultPinned); + pref_service.SetBoolean(companion::kExpsOptInStatusGrantedPref, false); + + UpdateCompanionDefaultPinnedToToolbarState(&pref_service); + EXPECT_EQ( + pref_service.GetBoolean(prefs::kSidePanelCompanionEntryPinnedToToolbar), + true); +} + +TEST(CompanionUtilsTest, UpdatePinnedStateDefaultUnPinned) { + base::test::ScopedFeatureList scoped_feature_list; + TestingPrefServiceSimple pref_service; + RegisterPrefs(&pref_service); + + scoped_feature_list.InitAndDisableFeature( + ::features::kSidePanelCompanionDefaultPinned); + pref_service.SetBoolean(companion::kExpsOptInStatusGrantedPref, false); + + UpdateCompanionDefaultPinnedToToolbarState(&pref_service); + EXPECT_EQ( + pref_service.GetBoolean(prefs::kSidePanelCompanionEntryPinnedToToolbar), + false); +} + +} // namespace companion diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn index 36215397c85e4..5b8d337594503 100644 --- a/chrome/test/BUILD.gn +++ b/chrome/test/BUILD.gn @@ -7265,6 +7265,7 @@ test("unit_tests") { "../browser/ui/search/search_ipc_router_policy_unittest.cc", "../browser/ui/search/search_ipc_router_unittest.cc", "../browser/ui/serial/serial_chooser_controller_unittest.cc", + "../browser/ui/side_panel/companion/companion_utils_unittest.cc", "../browser/ui/singleton_tabs_unittest.cc", "../browser/ui/startup/launch_mode_recorder_unittest.cc", "../browser/ui/sync/sync_promo_ui_unittest.cc", diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml index 8b1d61799b8d2..3236d95b69cd4 100644 --- a/tools/metrics/histograms/enums.xml +++ b/tools/metrics/histograms/enums.xml @@ -60699,6 +60699,7 @@ from previous Chrome versions. + @@ -61522,6 +61523,7 @@ from previous Chrome versions. + @@ -65003,6 +65005,7 @@ from previous Chrome versions. +