Invoke Password storage IPH through FeaturePromoControllerViews

Previously these two IPH directly created a FeaturePromoBubbleView.
One of them would potentially display at the same time as other IPH
since it isn't registered with the IPH backend. This specific promo is
important to show to users immediately, hence why it is treated
differently.

To support this use case a notion of a "critical promo" is added to
FeaturePromoControllerViews. These promos will preempt any normal IPH
promo. This is reserved for very important promos that must be shown
immediately.

With this support, both promos are invoked through
FeaturePromoControllerViews.

Bug: 1133016
Change-Id: Ifb3b2273c108bc342698faa61adb4b54d5f7072d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2487874
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Dana Fried <dfried@chromium.org>
Reviewed-by: Dana Fried <dfried@google.com>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820410}

chrome/browser/ui/views/in_product_help/feature_promo_controller_views.cc

@@ -10,6 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
+#include "base/token.h"
 #include "chrome/browser/feature_engagement/tracker_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/in_product_help/feature_promo_snooze_service.h"
@@ -48,6 +49,11 @@ bool FeaturePromoControllerViews::MaybeShowPromoWithParams(
   if (promos_blocked_for_testing_)
     return false;
 
+  // A normal promo cannot show if a critical promo is displayed. These
+  // are not registered with |tracker_| so check here.
+  if (current_critical_promo_)
+    return false;
+
   // Temporarily turn off IPH in incognito as a concern was raised that
   // the IPH backend ignores incognito and writes to the parent profile.
   // See https://bugs.chromium.org/p/chromium/issues/detail?id=1128728#c30
@@ -63,18 +69,7 @@ bool FeaturePromoControllerViews::MaybeShowPromoWithParams(
   // If the tracker says we should trigger, but we have a promo
   // currently showing, there is a bug somewhere in here.
   DCHECK(!current_iph_feature_);
-
-  params.anchor_view->SetProperty(kHasInProductHelpPromoKey, true);
-  anchor_view_tracker_.SetView(params.anchor_view);
-
   current_iph_feature_ = &iph_feature;
-  promo_bubble_ = FeaturePromoBubbleView::Create(
-      std::move(params),
-      base::BindRepeating(&FeaturePromoControllerViews::OnUserSnooze,
-                          base::Unretained(this), iph_feature),
-      base::BindRepeating(&FeaturePromoControllerViews::OnUserDismiss,
-                          base::Unretained(this), iph_feature));
-  widget_observer_.Add(promo_bubble_->GetWidget());
 
   // Record count of previous snoozes when an IPH triggers.
   int snooze_count = snooze_service_->GetSnoozeCount(iph_feature);
@@ -83,9 +78,46 @@ bool FeaturePromoControllerViews::MaybeShowPromoWithParams(
                                 snooze_count,
                                 snooze_service_->kUmaMaxSnoozeCount);
 
+  ShowPromoBubbleImpl(params);
+
   return true;
 }
 
+base::Optional<base::Token> FeaturePromoControllerViews::ShowCriticalPromo(
+    const FeaturePromoBubbleParams& params) {
+  if (promos_blocked_for_testing_)
+    return base::nullopt;
+
+  // Don't preempt an existing critical promo.
+  if (current_critical_promo_)
+    return base::nullopt;
+
+  // If a normal bubble is showing, close it. If the promo is has
+  // continued after a CloseBubbleAndContinuePromo() call, we can't stop
+  // it. However we will show the critical promo anyway.
+  if (current_iph_feature_ && promo_bubble_)
+    CloseBubble(*current_iph_feature_);
+
+  // Snooze is not supported for critical promos.
+  DCHECK(!params.allow_snooze);
+
+  DCHECK(!promo_bubble_);
+
+  current_critical_promo_ = base::Token::CreateRandom();
+  ShowPromoBubbleImpl(params);
+
+  return current_critical_promo_;
+}
+
+void FeaturePromoControllerViews::CloseBubbleForCriticalPromo(
+    const base::Token& critical_promo_id) {
+  if (current_critical_promo_ != critical_promo_id)
+    return;
+
+  DCHECK(promo_bubble_);
+  promo_bubble_->GetWidget()->Close();
+}
+
 bool FeaturePromoControllerViews::MaybeShowPromo(
     const base::Feature& iph_feature) {
   base::Optional<FeaturePromoBubbleParams> params =
@@ -176,15 +208,46 @@ void FeaturePromoControllerViews::FinishContinuedPromo() {
   current_iph_feature_ = nullptr;
 }
 
+void FeaturePromoControllerViews::ShowPromoBubbleImpl(
+    const FeaturePromoBubbleParams& params) {
+  params.anchor_view->SetProperty(kHasInProductHelpPromoKey, true);
+  anchor_view_tracker_.SetView(params.anchor_view);
+
+  if (current_iph_feature_) {
+    // For normal promos:
+    promo_bubble_ = FeaturePromoBubbleView::Create(
+        params,
+        base::BindRepeating(&FeaturePromoControllerViews::OnUserSnooze,
+                            base::Unretained(this), *current_iph_feature_),
+        base::BindRepeating(&FeaturePromoControllerViews::OnUserDismiss,
+                            base::Unretained(this), *current_iph_feature_));
+  } else {
+    // For critical promos, since they aren't snoozable:
+    promo_bubble_ = FeaturePromoBubbleView::Create(params);
+  }
+
+  widget_observer_.Add(promo_bubble_->GetWidget());
+}
+
 void FeaturePromoControllerViews::HandleBubbleClosed() {
-  DCHECK(current_iph_feature_);
+  // A bubble should be showing.
+  DCHECK(promo_bubble_);
 
-  tracker_->Dismissed(*current_iph_feature_);
-  widget_observer_.Remove(promo_bubble_->GetWidget());
+  // Exactly one of current_iph_feature_ or current_critical_promo_ should have
+  // a value.
+  DCHECK_NE(current_iph_feature_ != nullptr,
+            current_critical_promo_.has_value());
 
-  current_iph_feature_ = nullptr;
+  widget_observer_.Remove(promo_bubble_->GetWidget());
   promo_bubble_ = nullptr;
 
   if (anchor_view_tracker_.view())
     anchor_view_tracker_.view()->SetProperty(kHasInProductHelpPromoKey, false);
+
+  if (current_iph_feature_) {
+    tracker_->Dismissed(*current_iph_feature_);
+    current_iph_feature_ = nullptr;
+  } else {
+    current_critical_promo_ = base::nullopt;
+  }
 }

chrome/browser/ui/views/in_product_help/feature_promo_controller_views.h

@@ -8,7 +8,9 @@
 #include <memory>
 
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/scoped_observer.h"
+#include "base/token.h"
 #include "chrome/browser/ui/in_product_help/feature_promo_controller.h"
 #include "ui/views/view_tracker.h"
 #include "ui/views/widget/widget.h"
@@ -21,6 +23,7 @@ class FeaturePromoSnoozeService;
 
 namespace base {
 struct Feature;
+class Token;
 }
 
 namespace feature_engagement {
@@ -46,6 +49,18 @@ class FeaturePromoControllerViews : public FeaturePromoController,
   bool MaybeShowPromoWithParams(const base::Feature& iph_feature,
                                 const FeaturePromoBubbleParams& params);
 
+  // Only for security or privacy critical promos. Immedialy shows a
+  // promo with |params|, cancelling any normal promo and blocking any
+  // further promos until it's done.
+  //
+  // Returns an ID that can be passed to CloseBubbleForCriticalPromo()
+  // if successful. This can fail if another critical promo is showing.
+  base::Optional<base::Token> ShowCriticalPromo(
+      const FeaturePromoBubbleParams& params);
+
+  // Ends a promo started by ShowCriticalPromo() if it's still showing.
+  void CloseBubbleForCriticalPromo(const base::Token& critical_promo_id);
+
   // FeaturePromoController:
   bool MaybeShowPromo(const base::Feature& iph_feature) override;
   bool BubbleIsShowing(const base::Feature& iph_feature) const override;
@@ -78,6 +93,8 @@ class FeaturePromoControllerViews : public FeaturePromoController,
   // Called when PromoHandle is destroyed to finish the promo.
   void FinishContinuedPromo() override;
 
+  void ShowPromoBubbleImpl(const FeaturePromoBubbleParams& params);
+
   void HandleBubbleClosed();
 
   // Call these methods when the user actively snooze or dismiss the IPH.
@@ -99,6 +116,13 @@ class FeaturePromoControllerViews : public FeaturePromoController,
   // feature registered with |tracker_|.
   const base::Feature* current_iph_feature_ = nullptr;
 
+  // Has a value if a critical promo is showing. If this has a value,
+  // |current_iph_feature_| will usually be null. There is one edge case
+  // where this may not be true: when a critical promo is requested
+  // between a normal promo's CloseBubbleAndContinuePromo() call and its
+  // end.
+  base::Optional<base::Token> current_critical_promo_;
+
   // The bubble currently showing, if any.
   FeaturePromoBubbleView* promo_bubble_ = nullptr;
 

chrome/browser/ui/views/in_product_help/feature_promo_controller_views_unittest.cc

@@ -267,8 +267,7 @@ TEST_F(FeaturePromoControllerViewsTest, GetsParamsFromRegistry) {
 
 TEST_F(FeaturePromoControllerViewsTest, TestCanBlockPromos) {
   EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature)))
-      .Times(0)
-      .WillOnce(Return(true));
+      .Times(0);
 
   controller_->BlockPromosForTesting();
   EXPECT_FALSE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
@@ -289,3 +288,86 @@ TEST_F(FeaturePromoControllerViewsTest, TestCanStopCurrentPromo) {
   EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
   EXPECT_FALSE(controller_->promo_bubble_for_testing());
 }
+
+TEST_F(FeaturePromoControllerViewsTest, CriticalPromoBlocksNormalPromo) {
+  EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+
+  EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature)))
+      .Times(0);
+  EXPECT_FALSE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
+                                                     DefaultBubbleParams()));
+
+  EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+}
+
+TEST_F(FeaturePromoControllerViewsTest, CriticalPromoPreemptsNormalPromo) {
+  EXPECT_CALL(*mock_tracker_, ShouldTriggerHelpUI(Ref(kTestIPHFeature)))
+      .Times(1)
+      .WillOnce(Return(true));
+  EXPECT_TRUE(controller_->MaybeShowPromoWithParams(kTestIPHFeature,
+                                                    DefaultBubbleParams()));
+  EXPECT_TRUE(controller_->BubbleIsShowing(kTestIPHFeature));
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+
+  EXPECT_CALL(*mock_tracker_, Dismissed(Ref(kTestIPHFeature))).Times(1);
+  EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
+  EXPECT_FALSE(controller_->BubbleIsShowing(kTestIPHFeature));
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+}
+
+TEST_F(FeaturePromoControllerViewsTest, FirstCriticalPromoHasPrecedence) {
+  EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
+
+  const auto* first_bubble = controller_->promo_bubble_for_testing();
+  EXPECT_TRUE(first_bubble);
+
+  EXPECT_FALSE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
+  EXPECT_EQ(controller_->promo_bubble_for_testing(), first_bubble);
+}
+
+TEST_F(FeaturePromoControllerViewsTest, CloseBubbleForCriticalPromo) {
+  base::Optional<base::Token> maybe_id =
+      controller_->ShowCriticalPromo(DefaultBubbleParams());
+  ASSERT_TRUE(maybe_id);
+  base::Token id = maybe_id.value();
+
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+  controller_->CloseBubbleForCriticalPromo(id);
+  EXPECT_FALSE(controller_->promo_bubble_for_testing());
+}
+
+TEST_F(FeaturePromoControllerViewsTest,
+       CloseBubbleForCriticalPromoDoesNothingAfterClose) {
+  base::Optional<base::Token> maybe_id =
+      controller_->ShowCriticalPromo(DefaultBubbleParams());
+  ASSERT_TRUE(maybe_id);
+  base::Token id = maybe_id.value();
+
+  auto* bubble = controller_->promo_bubble_for_testing();
+  ASSERT_TRUE(bubble);
+  bubble->GetWidget()->Close();
+  EXPECT_FALSE(controller_->promo_bubble_for_testing());
+
+  EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+
+  // Since |id| has expired, this should do nothing.
+  controller_->CloseBubbleForCriticalPromo(id);
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+}
+
+TEST_F(FeaturePromoControllerViewsTest, ShowNewCriticalPromoAfterClose) {
+  base::Optional<base::Token> maybe_id =
+      controller_->ShowCriticalPromo(DefaultBubbleParams());
+  ASSERT_TRUE(maybe_id);
+  base::Token id = maybe_id.value();
+
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+  controller_->CloseBubbleForCriticalPromo(id);
+  EXPECT_FALSE(controller_->promo_bubble_for_testing());
+
+  EXPECT_TRUE(controller_->ShowCriticalPromo(DefaultBubbleParams()));
+  EXPECT_TRUE(controller_->promo_bubble_for_testing());
+}

chrome/browser/ui/views/passwords/password_bubble_view_base.cc

@@ -47,7 +47,8 @@ void PasswordBubbleViewBase::ShowBubble(content::WebContents* web_contents,
       button_provider->GetAnchorView(PageActionIconType::kManagePasswords);
 
   PasswordBubbleViewBase* bubble =
-      CreateBubble(web_contents, anchor_view, reason);
+      CreateBubble(web_contents, anchor_view, reason,
+                   browser_view->feature_promo_controller());
   DCHECK(bubble);
   DCHECK_EQ(bubble, g_manage_passwords_bubble_);
 
@@ -64,7 +65,8 @@ void PasswordBubbleViewBase::ShowBubble(content::WebContents* web_contents,
 PasswordBubbleViewBase* PasswordBubbleViewBase::CreateBubble(
     content::WebContents* web_contents,
     views::View* anchor_view,
-    DisplayReason reason) {
+    DisplayReason reason,
+    FeaturePromoControllerViews* promo_controller) {
   PasswordBubbleViewBase* view = nullptr;
   password_manager::ui::State model_state =
       PasswordsModelDelegateFromWebContents(web_contents)->GetState();
@@ -80,8 +82,8 @@ PasswordBubbleViewBase* PasswordBubbleViewBase::CreateBubble(
              model_state == password_manager::ui::PENDING_PASSWORD_STATE) {
     if (base::FeatureList::IsEnabled(
             password_manager::features::kEnablePasswordsAccountStorage)) {
-      view = new PasswordSaveUpdateWithAccountStoreView(web_contents,
-                                                        anchor_view, reason);
+      view = new PasswordSaveUpdateWithAccountStoreView(
+          web_contents, anchor_view, reason, promo_controller);
     } else {
       view = new PasswordSaveUpdateView(web_contents, anchor_view, reason);
     }

chrome/browser/ui/views/passwords/password_bubble_view_base.h

@@ -23,6 +23,7 @@ namespace views {
 class Label;
 }
 
+class FeaturePromoControllerViews;
 class PasswordBubbleControllerBase;
 
 // Base class for all manage-passwords bubbles. Provides static methods for
@@ -50,7 +51,8 @@ class PasswordBubbleViewBase : public LocationBarBubbleDelegateView {
   static PasswordBubbleViewBase* CreateBubble(
       content::WebContents* web_contents,
       views::View* anchor_view,
-      DisplayReason reason);
+      DisplayReason reason,
+      FeaturePromoControllerViews* promo_controller);
 
   // Closes the existing bubble.
   static void CloseCurrentBubble();

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.cc

@@ -22,7 +22,7 @@
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "chrome/browser/ui/views/in_product_help/feature_promo_bubble_params.h"
-#include "chrome/browser/ui/views/in_product_help/feature_promo_bubble_view.h"
+#include "chrome/browser/ui/views/in_product_help/feature_promo_controller_views.h"
 #include "chrome/browser/ui/views/passwords/credentials_item_view.h"
 #include "chrome/browser/ui/views/passwords/password_items_view.h"
 #include "chrome/grit/generated_resources.h"
@@ -350,7 +350,8 @@ class PasswordSaveUpdateWithAccountStoreView::AutoResizingLayout
 PasswordSaveUpdateWithAccountStoreView::PasswordSaveUpdateWithAccountStoreView(
     content::WebContents* web_contents,
     views::View* anchor_view,
-    DisplayReason reason)
+    DisplayReason reason,
+    FeaturePromoControllerViews* promo_controller)
     : PasswordBubbleViewBase(web_contents,
                              anchor_view,
                              /*auto_dismissable=*/false),
@@ -362,7 +363,8 @@ PasswordSaveUpdateWithAccountStoreView::PasswordSaveUpdateWithAccountStoreView(
       is_update_bubble_(controller_.state() ==
                         password_manager::ui::PENDING_PASSWORD_UPDATE_STATE),
       are_passwords_revealed_(
-          controller_.are_passwords_revealed_when_bubble_is_opened()) {
+          controller_.are_passwords_revealed_when_bubble_is_opened()),
+      promo_controller_(promo_controller) {
   // If kEnablePasswordsAccountStorage is disabled, then PasswordSaveUpdateView
   // should be used instead of this class.
   DCHECK(base::FeatureList::IsEnabled(
@@ -527,39 +529,20 @@ void PasswordSaveUpdateWithAccountStoreView::DestinationChanged() {
   controller_.OnToggleAccountStore(is_account_store_selected);
   // Saving in account and local stores have different header images.
   UpdateHeaderImage();
-  // If the user explicitly switched to "save on this device only", record this
-  // with the IPH tracker (so it can decide not to show the IPH again).
-  if (!is_account_store_selected) {
-    if (!iph_tracker_) {
-      iph_tracker_ = feature_engagement::TrackerFactory::GetForBrowserContext(
-          controller_.GetProfile());
-    }
-    iph_tracker_->NotifyEvent("passwords_account_storage_unselected");
+  // If the user explicitly switched to "save on this device only",
+  // record this with the IPH tracker (so it can decide not to show the
+  // IPH again). It may be null in tests, so handle that case.
+  if (!is_account_store_selected && promo_controller_) {
+    promo_controller_->feature_engagement_tracker()->NotifyEvent(
+        "passwords_account_storage_unselected");
   }
   // The IPH shown upon failure in reauth is used to informs the user that the
   // password will be stored on device. This is why it's important to close it
   // if the user changes the destination to account.
-  if (currenly_shown_iph_type_ == IPHType::kFailedReauth)
+  if (failed_reauth_promo_id_)
     CloseIPHBubbleIfOpen();
 }
 
-void PasswordSaveUpdateWithAccountStoreView::OnWidgetDestroying(
-    views::Widget* widget) {
-  // IPH bubble is getting closed.
-  if (account_storage_promo_ && account_storage_promo_->GetWidget() == widget) {
-    observed_account_storage_promo_.Remove(widget);
-    // If the reauth failed, we have shown the IPH unconditionally. No need to
-    // inform the tracker. Only regular IPH's are tracked
-    if (currenly_shown_iph_type_ == IPHType::kRegular) {
-      DCHECK(iph_tracker_);
-      iph_tracker_->Dismissed(
-          feature_engagement::kIPHPasswordsAccountStorageFeature);
-    }
-    currenly_shown_iph_type_ = IPHType::kNone;
-    account_storage_promo_ = nullptr;
-  }
-}
-
 views::View* PasswordSaveUpdateWithAccountStoreView::GetInitiallyFocusedView() {
   if (username_dropdown_ && username_dropdown_->GetText().empty())
     return username_dropdown_;
@@ -588,9 +571,9 @@ void PasswordSaveUpdateWithAccountStoreView::AddedToWidget() {
       ->SetAllowCharacterBreak(true);
 
   if (ShouldShowFailedReauthIPH())
-    ShowIPH(IPHType::kFailedReauth);
-  else if (ShouldShowRegularIPH())
-    ShowIPH(IPHType::kRegular);
+    MaybeShowIPH(IPHType::kFailedReauth);
+  else
+    MaybeShowIPH(IPHType::kRegular);
 }
 
 void PasswordSaveUpdateWithAccountStoreView::OnThemeChanged() {
@@ -615,8 +598,8 @@ void PasswordSaveUpdateWithAccountStoreView::OnThemeChanged() {
 void PasswordSaveUpdateWithAccountStoreView::OnLayoutIsAnimatingChanged(
     views::AnimatingLayoutManager* source,
     bool is_animating) {
-  if (!is_animating && ShouldShowRegularIPH())
-    ShowIPH(IPHType::kRegular);
+  if (!is_animating)
+    MaybeShowIPH(IPHType::kRegular);
 }
 
 void PasswordSaveUpdateWithAccountStoreView::TogglePasswordVisibility() {
@@ -703,27 +686,7 @@ void PasswordSaveUpdateWithAccountStoreView::UpdateHeaderImage() {
   GetBubbleFrameView()->SetHeaderView(CreateHeaderImage(id));
 }
 
-bool PasswordSaveUpdateWithAccountStoreView::ShouldShowRegularIPH() {
-  // IPH is shown only where the destination dropdown is shown (i.e. only for
-  // Save bubble).
-  if (!destination_dropdown_ || controller_.IsCurrentStateUpdate())
-    return false;
-
-  if (!iph_tracker_) {
-    iph_tracker_ = feature_engagement::TrackerFactory::GetForBrowserContext(
-        controller_.GetProfile());
-  }
-
-  return iph_tracker_->ShouldTriggerHelpUI(
-      feature_engagement::kIPHPasswordsAccountStorageFeature);
-}
-
 bool PasswordSaveUpdateWithAccountStoreView::ShouldShowFailedReauthIPH() {
-  // IPH is shown only where the destination dropdown is shown (i.e. only for
-  // Save bubble).
-  if (!destination_dropdown_ || controller_.IsCurrentStateUpdate())
-    return false;
-
   // If the reauth failed, we should have automatically switched to local mdoe,
   // and we should show the reauth failed IPH unconditionally as long as the
   // user didn't change the save location.
@@ -731,21 +694,17 @@ bool PasswordSaveUpdateWithAccountStoreView::ShouldShowFailedReauthIPH() {
          !controller_.IsUsingAccountStore();
 }
 
-void PasswordSaveUpdateWithAccountStoreView::ShowIPH(IPHType type) {
+void PasswordSaveUpdateWithAccountStoreView::MaybeShowIPH(IPHType type) {
   DCHECK_NE(IPHType::kNone, type);
-  DCHECK(destination_dropdown_);
-  DCHECK(destination_dropdown_->GetVisible());
 
-  base::Optional<int> title_string_specificer;
-  if (type == IPHType::kRegular) {
-    // IPH when reauth fails has no title.
-    title_string_specificer = IDS_PASSWORD_MANAGER_IPH_TITLE_SAVE_TO_ACCOUNT;
-  }
+  // IPH is shown only where the destination dropdown is shown (i.e. only for
+  // Save bubble).
+  if (!destination_dropdown_ || controller_.IsCurrentStateUpdate())
+    return;
 
-  int body_string_specificer =
-      type == IPHType::kRegular
-          ? IDS_PASSWORD_MANAGER_IPH_BODY_SAVE_TO_ACCOUNT
-          : IDS_PASSWORD_MANAGER_IPH_BODY_SAVE_REAUTH_FAIL;
+  // The promo controller may not exist in tests.
+  if (!promo_controller_)
+    return;
 
   // Make sure the Save/Update bubble doesn't get closed when the IPH bubble is
   // opened.
@@ -753,8 +712,6 @@ void PasswordSaveUpdateWithAccountStoreView::ShowIPH(IPHType type) {
   set_close_on_deactivate(false);
 
   FeaturePromoBubbleParams bubble_params;
-  bubble_params.body_string_specifier = body_string_specificer;
-  bubble_params.title_string_specifier = title_string_specificer;
   bubble_params.anchor_view = destination_dropdown_;
   bubble_params.arrow = views::BubbleBorder::RIGHT_CENTER;
   bubble_params.preferred_width = kAccountStoragePromoWidth;
@@ -763,18 +720,48 @@ void PasswordSaveUpdateWithAccountStoreView::ShowIPH(IPHType type) {
   bubble_params.timeout_default = GetRegularIPHTimeout();
   bubble_params.timeout_short = GetShortIPHTimeout();
 
-  account_storage_promo_ =
-      FeaturePromoBubbleView::Create(std::move(bubble_params));
-  set_close_on_deactivate(close_save_bubble_on_deactivate_original_value);
-  observed_account_storage_promo_.Add(account_storage_promo_->GetWidget());
+  if (type == IPHType::kRegular) {
+    bubble_params.body_string_specifier =
+        IDS_PASSWORD_MANAGER_IPH_BODY_SAVE_TO_ACCOUNT;
+    bubble_params.title_string_specifier =
+        IDS_PASSWORD_MANAGER_IPH_TITLE_SAVE_TO_ACCOUNT;
+
+    if (promo_controller_->MaybeShowPromoWithParams(
+            feature_engagement::kIPHPasswordsAccountStorageFeature,
+            bubble_params)) {
+      // If the regular promo was shown, the failed reauth promo is
+      // definitely finished. If not, we can't be confident it hasn't
+      // finished.
+      failed_reauth_promo_id_ = base::nullopt;
+    }
+  } else {
+    bubble_params.body_string_specifier =
+        IDS_PASSWORD_MANAGER_IPH_BODY_SAVE_REAUTH_FAIL;
 
-  currenly_shown_iph_type_ = type;
+    failed_reauth_promo_id_ =
+        promo_controller_->ShowCriticalPromo(bubble_params);
+  }
+
+  set_close_on_deactivate(close_save_bubble_on_deactivate_original_value);
 }
 
 void PasswordSaveUpdateWithAccountStoreView::CloseIPHBubbleIfOpen() {
-  if (!account_storage_promo_)
+  // The promo controller may not exist in tests.
+  if (!promo_controller_)
     return;
-  account_storage_promo_->CloseBubble();
+
+  if (!failed_reauth_promo_id_) {
+    promo_controller_->CloseBubble(
+        feature_engagement::kIPHPasswordsAccountStorageFeature);
+    return;
+  }
+
+  // |failed_reauth_promo_id_| may have a value if it closed on its
+  // own. This is fine; CloseBubbleForCriticalPromo() handles expired
+  // IDs, and we reset ours when showing a normal IPH bubble.
+  promo_controller_->CloseBubbleForCriticalPromo(
+      failed_reauth_promo_id_.value());
+  failed_reauth_promo_id_ = base::nullopt;
 }
 
 void PasswordSaveUpdateWithAccountStoreView::AnnounceSaveUpdateChange() {

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_PASSWORDS_PASSWORD_SAVE_UPDATE_WITH_ACCOUNT_STORE_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_PASSWORDS_PASSWORD_SAVE_UPDATE_WITH_ACCOUNT_STORE_VIEW_H_
 
+#include "base/optional.h"
+#include "base/token.h"
 #include "chrome/browser/ui/passwords/bubble_controllers/save_update_with_account_store_bubble_controller.h"
 #include "chrome/browser/ui/views/passwords/password_bubble_view_base.h"
 #include "ui/views/layout/animating_layout_manager.h"
@@ -17,11 +19,7 @@ class EditableCombobox;
 class ToggleImageButton;
 }  // namespace views
 
-namespace feature_engagement {
-class Tracker;
-}
-
-class FeaturePromoBubbleView;
+class FeaturePromoControllerViews;
 
 // A view offering the user the ability to save or update credentials (depending
 // on |is_update_bubble|) either in the profile and/or account stores. Contains
@@ -33,9 +31,11 @@ class PasswordSaveUpdateWithAccountStoreView
       public views::WidgetObserver,
       public views::AnimatingLayoutManager::Observer {
  public:
-  PasswordSaveUpdateWithAccountStoreView(content::WebContents* web_contents,
-                                         views::View* anchor_view,
-                                         DisplayReason reason);
+  PasswordSaveUpdateWithAccountStoreView(
+      content::WebContents* web_contents,
+      views::View* anchor_view,
+      DisplayReason reason,
+      FeaturePromoControllerViews* promo_controller);
 
   views::Combobox* DestinationDropdownForTesting() {
     return destination_dropdown_;
@@ -58,9 +58,6 @@ class PasswordSaveUpdateWithAccountStoreView
   PasswordBubbleControllerBase* GetController() override;
   const PasswordBubbleControllerBase* GetController() const override;
 
-  // views::WidgetObserver:
-  void OnWidgetDestroying(views::Widget* widget) override;
-
   // PasswordBubbleViewBase:
   views::View* GetInitiallyFocusedView() override;
   bool IsDialogButtonEnabled(ui::DialogButton button) const override;
@@ -81,19 +78,13 @@ class PasswordSaveUpdateWithAccountStoreView
 
   void DestinationChanged();
 
-  // Whether we should show the IPH informing the user about the destination
-  // picker and that they can now select where to store the passwords. It
-  // creates (if needed) and queries the |iph_tracker_|
-  bool ShouldShowRegularIPH();
-
   // Whether we should shown an IPH upon account reauth failure that informs the
   // user that the destination has been automatically switched to device.
   bool ShouldShowFailedReauthIPH();
 
-  // Opens an IPH bubble of |type|. Callers should make sure the
-  // pre-conditions are satisfied by calling the corresponding ShouldShow*IPH()
-  // methods before invoking this method.
-  void ShowIPH(IPHType type);
+  // Tries to show an IPH bubble of |type|. For kFailedReauth,
+  // ShouldShowFailedReauthIPH() should be checked first.
+  void MaybeShowIPH(IPHType type);
 
   void CloseIPHBubbleIfOpen();
 
@@ -119,18 +110,12 @@ class PasswordSaveUpdateWithAccountStoreView
   views::EditableCombobox* password_dropdown_ = nullptr;
   bool are_passwords_revealed_;
 
-  feature_engagement::Tracker* iph_tracker_ = nullptr;
-
-  // Promotional UI that appears next to the |destination_dropdown_|. Owned by
-  // its NativeWidget.
-  FeaturePromoBubbleView* account_storage_promo_ = nullptr;
-
-  IPHType currenly_shown_iph_type_ = IPHType::kNone;
+  // Used to display IPH. May be null in tests.
+  FeaturePromoControllerViews* const promo_controller_;
 
-  // Observes the |account_storage_promo_|'s Widget.  Used to tell whether the
-  // promo is open and get called back when it closes.
-  ScopedObserver<views::Widget, views::WidgetObserver>
-      observed_account_storage_promo_{this};
+  // When showing kReauthFailure IPH, |promo_controller_| gives back an
+  // ID. This is used to close the bubble later.
+  base::Optional<base::Token> failed_reauth_promo_id_;
 
   // Hidden view that will contain status text for immediate output by
   // screen readers when the bubble changes state between Save and Update.

chrome/browser/ui/views/passwords/password_save_update_with_account_store_view_unittest.cc

@@ -100,7 +100,8 @@ void PasswordSaveUpdateWithAccountStoreViewTest::CreateViewAndShow() {
   CreateAnchorViewAndShow();
 
   view_ = new PasswordSaveUpdateWithAccountStoreView(
-      web_contents(), anchor_view(), LocationBarBubbleDelegateView::AUTOMATIC);
+      web_contents(), anchor_view(), LocationBarBubbleDelegateView::AUTOMATIC,
+      /*promo_controller=*/nullptr);
   views::BubbleDialogDelegateView::CreateBubble(view_)->Show();
 }