Commit 3559d207 authored by Collin Baker's avatar Collin Baker Committed by Commit Bot

Add close callback to FeaturePromoController

This allows clients to supply a callback upon starting a promo. When
the bubble closes, it will be called.

No information is given in this CL. In the future, supplying a close
reason is desirable.

Bug: 1143465
Change-Id: I6a1af8d6f314cc268e67406371a5414cbc4feb6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2508395Reviewed-by: default avatarDana Fried <dfried@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822789}
parent af1a3a8c
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#ifndef CHROME_BROWSER_UI_USER_EDUCATION_FEATURE_PROMO_CONTROLLER_H_ #ifndef CHROME_BROWSER_UI_USER_EDUCATION_FEATURE_PROMO_CONTROLLER_H_
#define CHROME_BROWSER_UI_USER_EDUCATION_FEATURE_PROMO_CONTROLLER_H_ #define CHROME_BROWSER_UI_USER_EDUCATION_FEATURE_PROMO_CONTROLLER_H_
#include "base/callback.h"
#include "base/memory/weak_ptr.h" #include "base/memory/weak_ptr.h"
namespace base { namespace base {
...@@ -18,18 +19,26 @@ class FeaturePromoController { ...@@ -18,18 +19,26 @@ class FeaturePromoController {
FeaturePromoController() = default; FeaturePromoController() = default;
virtual ~FeaturePromoController() = default; virtual ~FeaturePromoController() = default;
using BubbleCloseCallback = base::OnceCallback<void()>;
// Starts the promo if possible. Returns whether it started. // Starts the promo if possible. Returns whether it started.
// |iph_feature| must be an IPH feature defined in // |iph_feature| must be an IPH feature defined in
// components/feature_engagement/public/feature_list.cc and registered // components/feature_engagement/public/feature_list.cc and registered
// with |FeaturePromoRegistry|. Note that this is different than the // with |FeaturePromoRegistry|. Note that this is different than the
// feature that the IPH is showing for. // feature that the IPH is showing for.
// //
// If a bubble was shown and |close_callback| was provided, it will be
// called when the bubble closes. |close_callback| must be valid as
// long as the bubble shows.
//
// For users that can't register their parameters with // For users that can't register their parameters with
// FeaturePromoRegistry, see // FeaturePromoRegistry, see
// |FeaturePromoControllerViews::MaybeShowPromoWithParams()|. Prefer // |FeaturePromoControllerViews::MaybeShowPromoWithParams()|. Prefer
// statically registering params with FeaturePromoRegistry and using // statically registering params with FeaturePromoRegistry and using
// this method when possible. // this method when possible.
virtual bool MaybeShowPromo(const base::Feature& iph_feature) = 0; virtual bool MaybeShowPromo(
const base::Feature& iph_feature,
BubbleCloseCallback close_callback = BubbleCloseCallback()) = 0;
// Returns whether a bubble is showing for the given IPH. Note that if // Returns whether a bubble is showing for the given IPH. Note that if
// this is false, a promo might still be in progress; for example, a // this is false, a promo might still be in progress; for example, a
......
...@@ -15,7 +15,10 @@ class MockFeaturePromoController : public FeaturePromoController { ...@@ -15,7 +15,10 @@ class MockFeaturePromoController : public FeaturePromoController {
~MockFeaturePromoController() override; ~MockFeaturePromoController() override;
// FeaturePromoController: // FeaturePromoController:
MOCK_METHOD(bool, MaybeShowPromo, (const base::Feature&), (override)); MOCK_METHOD(bool,
MaybeShowPromo,
(const base::Feature&, BubbleCloseCallback),
(override));
MOCK_METHOD(bool, BubbleIsShowing, (const base::Feature&), (const, override)); MOCK_METHOD(bool, BubbleIsShowing, (const base::Feature&), (const, override));
MOCK_METHOD(bool, CloseBubble, (const base::Feature&), (override)); MOCK_METHOD(bool, CloseBubble, (const base::Feature&), (override));
MOCK_METHOD(PromoHandle, MOCK_METHOD(PromoHandle,
......
...@@ -80,7 +80,7 @@ TEST_F(ReopenTabInProductHelpTest, TriggersIPH) { ...@@ -80,7 +80,7 @@ TEST_F(ReopenTabInProductHelpTest, TriggersIPH) {
ReopenTabInProductHelp reopen_tab_iph(profile(), clock()); ReopenTabInProductHelp reopen_tab_iph(profile(), clock());
EXPECT_CALL(*mock_promo_controller(), EXPECT_CALL(*mock_promo_controller(),
MaybeShowPromo(Ref(feature_engagement::kIPHReopenTabFeature))) MaybeShowPromo(Ref(feature_engagement::kIPHReopenTabFeature), _))
.Times(1) .Times(1)
.WillOnce(Return(true)); .WillOnce(Return(true));
......
...@@ -61,7 +61,8 @@ FeaturePromoControllerViews* FeaturePromoControllerViews::GetForView( ...@@ -61,7 +61,8 @@ FeaturePromoControllerViews* FeaturePromoControllerViews::GetForView(
bool FeaturePromoControllerViews::MaybeShowPromoWithParams( bool FeaturePromoControllerViews::MaybeShowPromoWithParams(
const base::Feature& iph_feature, const base::Feature& iph_feature,
const FeaturePromoBubbleParams& params) { const FeaturePromoBubbleParams& params,
BubbleCloseCallback close_callback) {
if (promos_blocked_for_testing_) if (promos_blocked_for_testing_)
return false; return false;
...@@ -95,6 +96,7 @@ bool FeaturePromoControllerViews::MaybeShowPromoWithParams( ...@@ -95,6 +96,7 @@ bool FeaturePromoControllerViews::MaybeShowPromoWithParams(
snooze_service_->kUmaMaxSnoozeCount); snooze_service_->kUmaMaxSnoozeCount);
ShowPromoBubbleImpl(params); ShowPromoBubbleImpl(params);
close_callback_ = std::move(close_callback);
return true; return true;
} }
...@@ -135,13 +137,15 @@ void FeaturePromoControllerViews::CloseBubbleForCriticalPromo( ...@@ -135,13 +137,15 @@ void FeaturePromoControllerViews::CloseBubbleForCriticalPromo(
} }
bool FeaturePromoControllerViews::MaybeShowPromo( bool FeaturePromoControllerViews::MaybeShowPromo(
const base::Feature& iph_feature) { const base::Feature& iph_feature,
BubbleCloseCallback close_callback) {
base::Optional<FeaturePromoBubbleParams> params = base::Optional<FeaturePromoBubbleParams> params =
FeaturePromoRegistry::GetInstance()->GetParamsForFeature(iph_feature, FeaturePromoRegistry::GetInstance()->GetParamsForFeature(iph_feature,
browser_view_); browser_view_);
if (!params) if (!params)
return false; return false;
return MaybeShowPromoWithParams(iph_feature, *params); return MaybeShowPromoWithParams(iph_feature, *params,
std::move(close_callback));
} }
void FeaturePromoControllerViews::OnUserSnooze( void FeaturePromoControllerViews::OnUserSnooze(
...@@ -186,6 +190,9 @@ FeaturePromoControllerViews::CloseBubbleAndContinuePromo( ...@@ -186,6 +190,9 @@ FeaturePromoControllerViews::CloseBubbleAndContinuePromo(
if (anchor_view_tracker_.view()) if (anchor_view_tracker_.view())
anchor_view_tracker_.view()->SetProperty(kHasInProductHelpPromoKey, false); anchor_view_tracker_.view()->SetProperty(kHasInProductHelpPromoKey, false);
if (close_callback_)
std::move(close_callback_).Run();
// Record count of previous snoozes when the IPH gets dismissed by user // Record count of previous snoozes when the IPH gets dismissed by user
// following the promo. e.g. clicking on relevant controls. // following the promo. e.g. clicking on relevant controls.
int snooze_count = snooze_service_->GetSnoozeCount(iph_feature); int snooze_count = snooze_service_->GetSnoozeCount(iph_feature);
...@@ -260,6 +267,9 @@ void FeaturePromoControllerViews::HandleBubbleClosed() { ...@@ -260,6 +267,9 @@ void FeaturePromoControllerViews::HandleBubbleClosed() {
if (anchor_view_tracker_.view()) if (anchor_view_tracker_.view())
anchor_view_tracker_.view()->SetProperty(kHasInProductHelpPromoKey, false); anchor_view_tracker_.view()->SetProperty(kHasInProductHelpPromoKey, false);
if (close_callback_)
std::move(close_callback_).Run();
if (current_iph_feature_) { if (current_iph_feature_) {
tracker_->Dismissed(*current_iph_feature_); tracker_->Dismissed(*current_iph_feature_);
current_iph_feature_ = nullptr; current_iph_feature_ = nullptr;
......
...@@ -51,8 +51,10 @@ class FeaturePromoControllerViews : public FeaturePromoController, ...@@ -51,8 +51,10 @@ class FeaturePromoControllerViews : public FeaturePromoController,
// For IPH not registered with |FeaturePromoRegistry|. Only use this // For IPH not registered with |FeaturePromoRegistry|. Only use this
// if it is infeasible to pre-register your IPH. // if it is infeasible to pre-register your IPH.
bool MaybeShowPromoWithParams(const base::Feature& iph_feature, bool MaybeShowPromoWithParams(
const FeaturePromoBubbleParams& params); const base::Feature& iph_feature,
const FeaturePromoBubbleParams& params,
BubbleCloseCallback close_callback = BubbleCloseCallback());
// Only for security or privacy critical promos. Immedialy shows a // Only for security or privacy critical promos. Immedialy shows a
// promo with |params|, cancelling any normal promo and blocking any // promo with |params|, cancelling any normal promo and blocking any
...@@ -67,7 +69,9 @@ class FeaturePromoControllerViews : public FeaturePromoController, ...@@ -67,7 +69,9 @@ class FeaturePromoControllerViews : public FeaturePromoController,
void CloseBubbleForCriticalPromo(const base::Token& critical_promo_id); void CloseBubbleForCriticalPromo(const base::Token& critical_promo_id);
// FeaturePromoController: // FeaturePromoController:
bool MaybeShowPromo(const base::Feature& iph_feature) override; bool MaybeShowPromo(
const base::Feature& iph_feature,
BubbleCloseCallback close_callback = BubbleCloseCallback()) override;
bool BubbleIsShowing(const base::Feature& iph_feature) const override; bool BubbleIsShowing(const base::Feature& iph_feature) const override;
bool CloseBubble(const base::Feature& iph_feature) override; bool CloseBubble(const base::Feature& iph_feature) override;
PromoHandle CloseBubbleAndContinuePromo( PromoHandle CloseBubbleAndContinuePromo(
...@@ -131,6 +135,11 @@ class FeaturePromoControllerViews : public FeaturePromoController, ...@@ -131,6 +135,11 @@ class FeaturePromoControllerViews : public FeaturePromoController,
// The bubble currently showing, if any. // The bubble currently showing, if any.
FeaturePromoBubbleView* promo_bubble_ = nullptr; FeaturePromoBubbleView* promo_bubble_ = nullptr;
// If present, called when |current_iph_feature_|'s bubble stops
// showing. Only valid if |current_iph_feature_| and |promo_bubble_|
// are both non-null.
BubbleCloseCallback close_callback_;
// Stores the bubble anchor view so we can set/unset a highlight on // Stores the bubble anchor view so we can set/unset a highlight on
// it. // it.
views::ViewTracker anchor_view_tracker_; views::ViewTracker anchor_view_tracker_;
......
...@@ -8,6 +8,8 @@ ...@@ -8,6 +8,8 @@
#include "base/feature_list.h" #include "base/feature_list.h"
#include "base/optional.h" #include "base/optional.h"
#include "base/test/bind_test_util.h" #include "base/test/bind_test_util.h"
#include "base/test/mock_callback.h"
#include "base/test/task_environment.h"
#include "chrome/browser/feature_engagement/tracker_factory.h" #include "chrome/browser/feature_engagement/tracker_factory.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/user_education/feature_promo_snooze_service.h" #include "chrome/browser/ui/user_education/feature_promo_snooze_service.h"
...@@ -99,6 +101,8 @@ class FeaturePromoControllerViewsTest : public TestWithBrowserView { ...@@ -99,6 +101,8 @@ class FeaturePromoControllerViewsTest : public TestWithBrowserView {
} }
}; };
using BubbleCloseCallback = FeaturePromoControllerViews::BubbleCloseCallback;
TEST_F(FeaturePromoControllerViewsTest, GetForView) { TEST_F(FeaturePromoControllerViewsTest, GetForView) {
EXPECT_EQ(controller_, EXPECT_EQ(controller_,
FeaturePromoControllerViews::GetForView(GetAnchorView())); FeaturePromoControllerViews::GetForView(GetAnchorView()));
...@@ -112,8 +116,12 @@ TEST_F(FeaturePromoControllerViewsTest, AsksBackendToShowPromo) { ...@@ -112,8 +116,12 @@ TEST_F(FeaturePromoControllerViewsTest, AsksBackendToShowPromo) {
EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature))) EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature)))
.Times(1) .Times(1)
.WillOnce(Return(false)); .WillOnce(Return(false));
EXPECT_FALSE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
DefaultBubbleParams())); base::MockCallback<BubbleCloseCallback> close_callback;
EXPECT_CALL(close_callback, Run()).Times(0);
EXPECT_FALSE(controller_->MaybeShowPromoWithParams(
kTestIPHFeature, DefaultBubbleParams(), close_callback.Get()));
EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature)); EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
EXPECT_FALSE(controller_->promo_bubble_for_testing()); EXPECT_FALSE(controller_->promo_bubble_for_testing());
} }
...@@ -144,8 +152,10 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsWhenRequested) { ...@@ -144,8 +152,10 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsWhenRequested) {
.Times(1) .Times(1)
.WillOnce(Return(true)); .WillOnce(Return(true));
EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(0); EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(0);
ASSERT_TRUE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
DefaultBubbleParams())); base::MockCallback<BubbleCloseCallback> close_callback;
ASSERT_TRUE(controller_->MaybeShowPromoWithParams(
kTestIPHFeature, DefaultBubbleParams(), close_callback.Get()));
// Only valid before the widget is closed. // Only valid before the widget is closed.
FeaturePromoBubbleView* const bubble = FeaturePromoBubbleView* const bubble =
...@@ -156,6 +166,8 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsWhenRequested) { ...@@ -156,6 +166,8 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsWhenRequested) {
views::test::WidgetClosingObserver widget_observer(bubble->GetWidget()); views::test::WidgetClosingObserver widget_observer(bubble->GetWidget());
EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1); EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1);
EXPECT_CALL(close_callback, Run()).Times(1);
EXPECT_TRUE(controller_->CloseBubble(kTestIPHFeature)); EXPECT_TRUE(controller_->CloseBubble(kTestIPHFeature));
EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature)); EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
EXPECT_FALSE(controller_->promo_bubble_for_testing()); EXPECT_FALSE(controller_->promo_bubble_for_testing());
...@@ -187,8 +199,10 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsOnBubbleClosure) { ...@@ -187,8 +199,10 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsOnBubbleClosure) {
.Times(1) .Times(1)
.WillOnce(Return(true)); .WillOnce(Return(true));
EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(0); EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(0);
ASSERT_TRUE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
DefaultBubbleParams())); base::MockCallback<BubbleCloseCallback> close_callback;
ASSERT_TRUE(controller_->MaybeShowPromoWithParams(
kTestIPHFeature, DefaultBubbleParams(), close_callback.Get()));
// Only valid before the widget is closed. // Only valid before the widget is closed.
FeaturePromoBubbleView* const bubble = FeaturePromoBubbleView* const bubble =
...@@ -199,6 +213,7 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsOnBubbleClosure) { ...@@ -199,6 +213,7 @@ TEST_F(FeaturePromoControllerViewsTest, PromoEndsOnBubbleClosure) {
views::test::WidgetClosingObserver widget_observer(bubble->GetWidget()); views::test::WidgetClosingObserver widget_observer(bubble->GetWidget());
EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1); EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1);
EXPECT_CALL(close_callback, Run());
bubble->GetWidget()->Close(); bubble->GetWidget()->Close();
widget_observer.Wait(); widget_observer.Wait();
...@@ -211,8 +226,10 @@ TEST_F(FeaturePromoControllerViewsTest, ContinuedPromoDefersBackendDismissed) { ...@@ -211,8 +226,10 @@ TEST_F(FeaturePromoControllerViewsTest, ContinuedPromoDefersBackendDismissed) {
.Times(1) .Times(1)
.WillOnce(Return(true)); .WillOnce(Return(true));
EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(0); EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(0);
ASSERT_TRUE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
DefaultBubbleParams())); base::MockCallback<BubbleCloseCallback> close_callback;
ASSERT_TRUE(controller_->MaybeShowPromoWithParams(
kTestIPHFeature, DefaultBubbleParams(), close_callback.Get()));
// Only valid before the widget is closed. // Only valid before the widget is closed.
FeaturePromoBubbleView* const bubble = FeaturePromoBubbleView* const bubble =
...@@ -225,6 +242,7 @@ TEST_F(FeaturePromoControllerViewsTest, ContinuedPromoDefersBackendDismissed) { ...@@ -225,6 +242,7 @@ TEST_F(FeaturePromoControllerViewsTest, ContinuedPromoDefersBackendDismissed) {
// First check that CloseBubbleAndContinuePromo() actually closes the // First check that CloseBubbleAndContinuePromo() actually closes the
// bubble, but doesn't yet tell the backend the promo finished. // bubble, but doesn't yet tell the backend the promo finished.
EXPECT_CALL(close_callback, Run()).Times(1);
base::Optional<FeaturePromoController::PromoHandle> promo_handle = base::Optional<FeaturePromoController::PromoHandle> promo_handle =
controller_->CloseBubbleAndContinuePromo(kTestIPHFeature); controller_->CloseBubbleAndContinuePromo(kTestIPHFeature);
EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature)); EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
...@@ -315,12 +333,16 @@ TEST_F(FeaturePromoControllerViewsTest, CriticalPromoPreemptsNormalPromo) { ...@@ -315,12 +333,16 @@ TEST_F(FeaturePromoControllerViewsTest, CriticalPromoPreemptsNormalPromo) {
EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature))) EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature)))
.Times(1) .Times(1)
.WillOnce(Return(true)); .WillOnce(Return(true));
EXPECT_TRUE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
DefaultBubbleParams())); base::MockCallback<BubbleCloseCallback> close_callback;
EXPECT_TRUE(controller_->MaybeShowPromoWithParams(
kTestIPHFeature, DefaultBubbleParams(), close_callback.Get()));
EXPECT_TRUE(controller_->BubbleIsShowing(kTestIPHFeature)); EXPECT_TRUE(controller_->BubbleIsShowing(kTestIPHFeature));
EXPECT_TRUE(controller_->promo_bubble_for_testing()); EXPECT_TRUE(controller_->promo_bubble_for_testing());
EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1); EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1);
EXPECT_CALL(close_callback, Run()).Times(1);
EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams())); EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature)); EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
EXPECT_TRUE(controller_->promo_bubble_for_testing()); EXPECT_TRUE(controller_->promo_bubble_for_testing());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment