From c261c379ecc73c5f4ae9f67efb64da4b41e3ea78 Mon Sep 17 00:00:00 2001 From: Shakti Sahu Date: Tue, 6 Jun 2023 14:09:52 +0000 Subject: [PATCH] [CSC] Triggering logic for region search IPH This CL 1. Sets up IPH config for region search IPH. Default config is to show the very first time. 2. Sends a boolean in the companion url params proto to indicate whether or not to show the IPH. 2. Notifies IPH system immediately that the bubble was dismissed so that any subsequent IPH can be shown. This behavior will be modified as needed further. (cherry picked from commit 0744b0efbde0cc6ff48b949b29c50e5a1ab468c8) Bug: b/278267814, 1449021 Change-Id: I80fbfcb33934b5ea2b5e5110a56941baeff8bfa1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4577645 Reviewed-by: Michael Crouse Reviewed-by: Alex Gough Reviewed-by: Ali Stanfield Commit-Queue: Shakti Sahu Cr-Original-Commit-Position: refs/heads/main@{#1152699} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4591117 Auto-Submit: Michael Crouse Reviewed-by: Shakti Sahu Commit-Queue: Alex Gough Cr-Commit-Position: refs/branch-heads/5790@{#403} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../companion/core/companion_url_builder.cc | 4 ++++ .../core/companion_url_builder_unittest.cc | 4 ++++ .../browser/companion/core/mojom/companion.mojom | 3 +++ chrome/browser/companion/core/promo_handler.cc | 2 ++ .../companion/core/promo_handler_unittest.cc | 1 + .../core/proto/companion_url_params.proto | 3 ++- chrome/browser/companion/core/signin_delegate.h | 3 +++ .../companion_page_browsertest.cc | 4 ++++ .../companion/companion_page_handler.cc | 16 ++++++++++++++++ .../side_panel/companion/signin_delegate_impl.cc | 11 +++++++++++ .../side_panel/companion/signin_delegate_impl.h | 1 + .../public/feature_configurations.cc | 15 +++++++++++++++ .../public/feature_constants.cc | 3 +++ .../public/feature_constants.h | 1 + .../feature_engagement/public/feature_list.cc | 1 + .../feature_engagement/public/feature_list.h | 3 +++ tools/metrics/actions/actions.xml | 2 ++ .../metadata/feature_engagement/histograms.xml | 3 +++ 18 files changed, 79 insertions(+), 1 deletion(-) diff --git a/chrome/browser/companion/core/companion_url_builder.cc b/chrome/browser/companion/core/companion_url_builder.cc index b8c6af8c34055..377eb984a6e72 100644 --- a/chrome/browser/companion/core/companion_url_builder.cc +++ b/chrome/browser/companion/core/companion_url_builder.cc @@ -128,6 +128,10 @@ std::string CompanionUrlBuilder::BuildCompanionUrlParamProto(GURL page_url) { promo_state->set_exps_promo_shown_count( pref_service_->GetInteger(kExpsPromoShownCountPref)); + // Set region search IPH state. + promo_state->set_should_show_region_search_iph( + signin_delegate_->ShouldShowRegionSearchIPH()); + std::string base64_encoded_proto; base::Base64Encode(url_params.SerializeAsString(), &base64_encoded_proto); return base64_encoded_proto; diff --git a/chrome/browser/companion/core/companion_url_builder_unittest.cc b/chrome/browser/companion/core/companion_url_builder_unittest.cc index 42216afe918f3..af5c2ce614439 100644 --- a/chrome/browser/companion/core/companion_url_builder_unittest.cc +++ b/chrome/browser/companion/core/companion_url_builder_unittest.cc @@ -34,6 +34,7 @@ class MockSigninDelegate : public SigninDelegate { MOCK_METHOD0(StartSigninFlow, void()); MOCK_METHOD1(EnableMsbb, void(bool)); MOCK_METHOD1(LoadUrlInNewTab, void(const GURL&)); + MOCK_METHOD0(ShouldShowRegionSearchIPH, bool()); }; } // namespace @@ -54,6 +55,8 @@ class CompanionUrlBuilderTest : public testing::Test { SetSignInAndMsbbExpectations(/*is_sign_in_allowed=*/true, /*is_signed_in=*/true, /*msbb_pref_enabled=*/true); + EXPECT_CALL(signin_delegate_, ShouldShowRegionSearchIPH()) + .WillRepeatedly(testing::Return(true)); url_builder_ = std::make_unique(&pref_service_, &signin_delegate_); } @@ -212,6 +215,7 @@ TEST_F(CompanionUrlBuilderTest, MsbbOn) { EXPECT_EQ(0, proto.promo_state().msbb_promo_denial_count()); EXPECT_EQ(0, proto.promo_state().exps_promo_denial_count()); EXPECT_EQ(2, proto.promo_state().exps_promo_shown_count()); + EXPECT_TRUE(proto.promo_state().should_show_region_search_iph()); } TEST_F(CompanionUrlBuilderTest, NonProtobufParams) { diff --git a/chrome/browser/companion/core/mojom/companion.mojom b/chrome/browser/companion/core/mojom/companion.mojom index fd6af80fe158e..04f68ded9f375 100644 --- a/chrome/browser/companion/core/mojom/companion.mojom +++ b/chrome/browser/companion/core/mojom/companion.mojom @@ -59,6 +59,9 @@ enum PromoType { // Promo to opt into experience api. kExps = 3, + + // IPH about region search capability. + kRegionSearchIPH = 4, }; // User actions taken on a promo. diff --git a/chrome/browser/companion/core/promo_handler.cc b/chrome/browser/companion/core/promo_handler.cc index 10e2e6b725655..b800d05eabeaa 100644 --- a/chrome/browser/companion/core/promo_handler.cc +++ b/chrome/browser/companion/core/promo_handler.cc @@ -41,6 +41,8 @@ void PromoHandler::OnPromoAction(PromoType promo_type, case PromoType::kExps: OnExpsPromo(promo_action, exps_promo_url); return; + default: + return; } } diff --git a/chrome/browser/companion/core/promo_handler_unittest.cc b/chrome/browser/companion/core/promo_handler_unittest.cc index 5ffa5882b0a16..7908a660c6a07 100644 --- a/chrome/browser/companion/core/promo_handler_unittest.cc +++ b/chrome/browser/companion/core/promo_handler_unittest.cc @@ -23,6 +23,7 @@ class MockSigninDelegate : public SigninDelegate { MOCK_METHOD0(StartSigninFlow, void()); MOCK_METHOD1(EnableMsbb, void(bool)); MOCK_METHOD1(LoadUrlInNewTab, void(const GURL&)); + MOCK_METHOD0(ShouldShowRegionSearchIPH, bool()); }; } // namespace diff --git a/chrome/browser/companion/core/proto/companion_url_params.proto b/chrome/browser/companion/core/proto/companion_url_params.proto index f0394b7374adb..7e7c7284208a1 100644 --- a/chrome/browser/companion/core/proto/companion_url_params.proto +++ b/chrome/browser/companion/core/proto/companion_url_params.proto @@ -12,13 +12,14 @@ package companion.proto; // Keep this file in sync with // google3/lens/web/standalone/companion/proto/companion_url_params.proto -// Next ID: 5 +// Next ID: 6 message PromoState { // Number of times the user has denied various promo requests in past. int32 signin_promo_denial_count = 1; int32 msbb_promo_denial_count = 2; int32 exps_promo_denial_count = 3; int32 exps_promo_shown_count = 4; + bool should_show_region_search_iph = 5; } // This proto file is shared between Chrome and Server side and used to pack the diff --git a/chrome/browser/companion/core/signin_delegate.h b/chrome/browser/companion/core/signin_delegate.h index aee21bed1fdd6..8b80a509ed26c 100644 --- a/chrome/browser/companion/core/signin_delegate.h +++ b/chrome/browser/companion/core/signin_delegate.h @@ -34,6 +34,9 @@ class SigninDelegate { // Loads URL in the browser in a new tab. virtual void LoadUrlInNewTab(const GURL& url) = 0; + + // Returns whether region search IPH should be shown. + virtual bool ShouldShowRegionSearchIPH() = 0; }; } // namespace companion diff --git a/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc b/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc index eab9b1a8aace2..a0c54e75822f9 100644 --- a/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc +++ b/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc @@ -34,6 +34,7 @@ #include "chrome/common/pref_names.h" #include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/ui_test_utils.h" +#include "components/feature_engagement/public/feature_constants.h" #include "components/prefs/pref_service.h" #include "components/signin/public/identity_manager/identity_manager.h" #include "components/signin/public/identity_manager/identity_test_utils.h" @@ -361,6 +362,8 @@ class CompanionPageBrowserTest : public InProcessBrowserTest { } virtual void SetUpFeatureList() { + iph_feature_list_.InitAndEnableFeatures( + {feature_engagement::kIPHCompanionSidePanelRegionSearchFeature}); base::FieldTrialParams params; params["companion-homepage-url"] = companion_server_.GetURL("/companion_iframe.html").spec(); @@ -431,6 +434,7 @@ class CompanionPageBrowserTest : public InProcessBrowserTest { } protected: + feature_engagement::test::ScopedIphFeatureList iph_feature_list_; base::test::ScopedFeatureList feature_list_; net::EmbeddedTestServer page_url_server_{net::EmbeddedTestServer::TYPE_HTTPS}; net::EmbeddedTestServer companion_server_{ diff --git a/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc b/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc index 3820039b4fd84..7362e670d9a14 100644 --- a/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc +++ b/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc @@ -12,6 +12,7 @@ #include "chrome/browser/companion/core/promo_handler.h" #include "chrome/browser/companion/text_finder/text_finder_manager.h" #include "chrome/browser/companion/text_finder/text_highlighter_manager.h" +#include "chrome/browser/feature_engagement/tracker_factory.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/search/search.h" #include "chrome/browser/signin/identity_manager_factory.h" @@ -25,6 +26,9 @@ #include "chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.h" #include "chrome/browser/unified_consent/unified_consent_service_factory.h" #include "chrome/common/webui_url_constants.h" +#include "components/feature_engagement/public/event_constants.h" +#include "components/feature_engagement/public/feature_constants.h" +#include "components/feature_engagement/public/tracker.h" #include "components/lens/buildflags.h" #include "components/prefs/pref_service.h" #include "components/unified_consent/pref_names.h" @@ -198,6 +202,16 @@ void CompanionPageHandler::OnPromoAction( side_panel::mojom::PromoType promo_type, side_panel::mojom::PromoAction promo_action, const absl::optional& exps_promo_url) { + if (promo_type == side_panel::mojom::PromoType::kRegionSearchIPH) { + if (promo_action == side_panel::mojom::PromoAction::kRejected) { + auto* tracker = feature_engagement::TrackerFactory::GetForBrowserContext( + GetProfile()); + tracker->Dismissed( + feature_engagement::kIPHCompanionSidePanelRegionSearchFeature); + } + return; + } + promo_handler_->OnPromoAction(promo_type, promo_action, exps_promo_url); metrics_logger_->OnPromoAction(promo_type, promo_action); } @@ -208,6 +222,8 @@ void CompanionPageHandler::OnRegionSearchClicked() { helper->StartRegionSearch(web_contents(), /*use_fullscreen_capture=*/false); metrics_logger_->RecordUiSurfaceClicked( side_panel::mojom::UiSurface::kRegionSearch, kInvalidPosition); + feature_engagement::TrackerFactory::GetForBrowserContext(GetProfile()) + ->NotifyEvent("companion_side_panel_region_search_button_clicked"); } void CompanionPageHandler::OnExpsOptInStatusAvailable(bool is_exps_opted_in) { diff --git a/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.cc b/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.cc index 56f75abb02618..56d4c8df548b2 100644 --- a/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.cc +++ b/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.cc @@ -5,6 +5,7 @@ #include "chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.h" #include "base/functional/callback.h" +#include "chrome/browser/feature_engagement/tracker_factory.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/identity_manager_factory.h" #include "chrome/browser/signin/signin_ui_util.h" @@ -12,6 +13,9 @@ #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/side_panel/companion/companion_side_panel_controller_utils.h" #include "chrome/browser/unified_consent/unified_consent_service_factory.h" +#include "components/feature_engagement/public/event_constants.h" +#include "components/feature_engagement/public/feature_constants.h" +#include "components/feature_engagement/public/tracker.h" #include "components/prefs/pref_service.h" #include "components/signin/public/base/consent_level.h" #include "components/signin/public/base/signin_metrics.h" @@ -75,6 +79,13 @@ void SigninDelegateImpl::LoadUrlInNewTab(const GURL& url) { browser->OpenURL(params); } +bool SigninDelegateImpl::ShouldShowRegionSearchIPH() { + auto* tracker = + feature_engagement::TrackerFactory::GetForBrowserContext(GetProfile()); + return tracker->ShouldTriggerHelpUI( + feature_engagement::kIPHCompanionSidePanelRegionSearchFeature); +} + Profile* SigninDelegateImpl::GetProfile() { return Profile::FromBrowserContext(webui_contents_->GetBrowserContext()); } diff --git a/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.h b/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.h index 2e605776abff0..082643aaa28d6 100644 --- a/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.h +++ b/chrome/browser/ui/webui/side_panel/companion/signin_delegate_impl.h @@ -31,6 +31,7 @@ class SigninDelegateImpl : public SigninDelegate { void StartSigninFlow() override; void EnableMsbb(bool enable_msbb) override; void LoadUrlInNewTab(const GURL& url) override; + bool ShouldShowRegionSearchIPH() override; private: Profile* GetProfile(); diff --git a/components/feature_engagement/public/feature_configurations.cc b/components/feature_engagement/public/feature_configurations.cc index 28bccdf203db6..80c736d540ce8 100644 --- a/components/feature_engagement/public/feature_configurations.cc +++ b/components/feature_engagement/public/feature_configurations.cc @@ -265,6 +265,21 @@ absl::optional GetClientSideFeatureConfig( return config; } + if (kIPHCompanionSidePanelRegionSearchFeature.name == feature->name) { + absl::optional config = FeatureConfig(); + config->valid = true; + config->availability = Comparator(ANY, 0); + config->session_rate = Comparator(EQUAL, 0); + // Show the promo up to 1 times a year. + config->trigger = + EventConfig("iph_companion_side_panel_region_search_trigger", + Comparator(LESS_THAN, 1), 360, 360); + config->used = + EventConfig("companion_side_panel_region_search_button_clicked", + Comparator(EQUAL, 0), 360, 360); + return config; + } + if (kIPHDesktopCustomizeChromeFeature.name == feature->name) { absl::optional config = FeatureConfig(); config->valid = true; diff --git a/components/feature_engagement/public/feature_constants.cc b/components/feature_engagement/public/feature_constants.cc index 30dc05b8f333d..64d0ea7404e67 100644 --- a/components/feature_engagement/public/feature_constants.cc +++ b/components/feature_engagement/public/feature_constants.cc @@ -30,6 +30,9 @@ BASE_FEATURE(kIPHBatterySaverModeFeature, BASE_FEATURE(kIPHCompanionSidePanelFeature, "IPH_CompanionSidePanel", base::FEATURE_DISABLED_BY_DEFAULT); +BASE_FEATURE(kIPHCompanionSidePanelRegionSearchFeature, + "IPH_CompanionSidePanelRegionSearch", + base::FEATURE_ENABLED_BY_DEFAULT); BASE_FEATURE(kIPHDesktopSharedHighlightingFeature, "IPH_DesktopSharedHighlighting", base::FEATURE_DISABLED_BY_DEFAULT); diff --git a/components/feature_engagement/public/feature_constants.h b/components/feature_engagement/public/feature_constants.h index dab77a71c5321..a150f91d88d82 100644 --- a/components/feature_engagement/public/feature_constants.h +++ b/components/feature_engagement/public/feature_constants.h @@ -26,6 +26,7 @@ BASE_DECLARE_FEATURE(kIPHDummyFeature); BASE_DECLARE_FEATURE(kIPHAutofillFeedbackNewBadgeFeature); BASE_DECLARE_FEATURE(kIPHBatterySaverModeFeature); BASE_DECLARE_FEATURE(kIPHCompanionSidePanelFeature); +BASE_DECLARE_FEATURE(kIPHCompanionSidePanelRegionSearchFeature); BASE_DECLARE_FEATURE(kIPHDesktopSharedHighlightingFeature); BASE_DECLARE_FEATURE(kIPHDesktopTabGroupsNewGroupFeature); BASE_DECLARE_FEATURE(kIPHDesktopCustomizeChromeFeature); diff --git a/components/feature_engagement/public/feature_list.cc b/components/feature_engagement/public/feature_list.cc index 7dfc2e78c80f8..fc150531af2bc 100644 --- a/components/feature_engagement/public/feature_list.cc +++ b/components/feature_engagement/public/feature_list.cc @@ -142,6 +142,7 @@ const base::Feature* const kAllFeatures[] = { &kIPHAutofillFeedbackNewBadgeFeature, &kIPHBatterySaverModeFeature, &kIPHCompanionSidePanelFeature, + &kIPHCompanionSidePanelRegionSearchFeature, &kIPHDesktopTabGroupsNewGroupFeature, &kIPHDesktopCustomizeChromeFeature, &kIPHDownloadToolbarButtonFeature, diff --git a/components/feature_engagement/public/feature_list.h b/components/feature_engagement/public/feature_list.h index 85a5ba1d97a28..40d79607bb5ae 100644 --- a/components/feature_engagement/public/feature_list.h +++ b/components/feature_engagement/public/feature_list.h @@ -260,6 +260,8 @@ DEFINE_VARIATION_PARAM(kIPHAutofillFeedbackNewBadgeFeature, "IPH_AutofillFeedbackNewBadge"); DEFINE_VARIATION_PARAM(kIPHBatterySaverModeFeature, "IPH_BatterySaverMode"); DEFINE_VARIATION_PARAM(kIPHCompanionSidePanelFeature, "IPH_CompanionSidePanel"); +DEFINE_VARIATION_PARAM(kIPHCompanionSidePanelRegionSearchFeature, + "IPH_CompanionSidePanelRegionSearch"); DEFINE_VARIATION_PARAM(kIPHDesktopCustomizeChromeFeature, "IPH_DesktopCustomizeChrome"); DEFINE_VARIATION_PARAM(kIPHDesktopTabGroupsNewGroupFeature, @@ -455,6 +457,7 @@ constexpr flags_ui::FeatureEntry::FeatureVariation VARIATION_ENTRY(kIPHAutofillFeedbackNewBadgeFeature), VARIATION_ENTRY(kIPHBatterySaverModeFeature), VARIATION_ENTRY(kIPHCompanionSidePanelFeature), + VARIATION_ENTRY(kIPHCompanionSidePanelRegionSearchFeature), VARIATION_ENTRY(kIPHDesktopCustomizeChromeFeature), VARIATION_ENTRY(kIPHDesktopTabGroupsNewGroupFeature), VARIATION_ENTRY(kIPHDownloadToolbarButtonFeature), diff --git a/tools/metrics/actions/actions.xml b/tools/metrics/actions/actions.xml index 2f25119ba2917..ef166bfccfb47 100644 --- a/tools/metrics/actions/actions.xml +++ b/tools/metrics/actions/actions.xml @@ -37679,6 +37679,8 @@ should be able to be added at any place in this file. + +