cbui: remove CardUnmaskPromptViews CreateFootnoteView

Whether this control creates a footnote view or not (and the state of the
footnote view if it does create one) depends on several pieces of state in the
controller, some of which are only present after the controller's ShowPrompt
method is called. This change therefore:

1) Changes ShowPrompt to take a factory callback that manufactures the concrete
   View class
2) Invokes that factory callback once all the state needed for the View is ready
3) Moves creation of the View's footnote view into the View's constructor, now
   that all the needed state is guaranteed to be available.

This change exposed a bug in the tests: the production code passed ownership of
a raw pointer in for the View, but the tests instead retain ownership and pass
in a weak raw pointer. This happens to work because View tolerates being
destroyed while still parented, but this behavior of View is deprecated. I have
not fixed this test behavior in this CL to keep the size down.

Bug: 1011446
Change-Id: I1ad6f12cc59b2f77c75ac81b13f41eae67738205
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1907069
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714625}

chrome/browser/ui/autofill/chrome_autofill_client.cc

@@ -221,7 +221,9 @@ void ChromeAutofillClient::ShowUnmaskPrompt(
     UnmaskCardReason reason,
     base::WeakPtr<CardUnmaskDelegate> delegate) {
   unmask_controller_.ShowPrompt(
-      CreateCardUnmaskPromptView(&unmask_controller_, web_contents()),
+      base::Bind(&CreateCardUnmaskPromptView,
+                 base::Unretained(&unmask_controller_),
+                 base::Unretained(web_contents())),
       card, reason, delegate);
 }
 

chrome/browser/ui/autofill/payments/card_unmask_prompt_view_browsertest.cc

@@ -158,14 +158,14 @@ class CardUnmaskPromptViewBrowserTest : public DialogBrowserTest {
   }
 
   void ShowUi(const std::string& name) override {
-    CardUnmaskPromptView* dialog =
-        CreateCardUnmaskPromptView(controller(), contents());
     CreditCard card = test::GetMaskedServerCard();
     if (name == kExpiryExpired)
       card.SetExpirationYear(2016);
 
-    controller()->ShowPrompt(dialog, card, AutofillClient::UNMASK_FOR_AUTOFILL,
-                             delegate()->GetWeakPtr());
+    controller()->ShowPrompt(
+        base::Bind(&CreateCardUnmaskPromptView, base::Unretained(controller()),
+                   base::Unretained(contents())),
+        card, AutofillClient::UNMASK_FOR_AUTOFILL, delegate()->GetWeakPtr());
     // Setting error expectations and confirming the dialogs for some test
     // cases.
     if (name == kExpiryValidPermanentError ||

chrome/browser/ui/views/autofill/payments/card_unmask_prompt_views.cc

@@ -64,6 +64,19 @@ static views::GridLayout* ResetOverlayLayout(views::View* overlay) {
   return overlay_layout;
 }
 
+std::unique_ptr<views::Checkbox> CreateSaveCheckbox(bool start_state) {
+  auto storage_checkbox =
+      std::make_unique<views::Checkbox>(l10n_util::GetStringUTF16(
+          IDS_AUTOFILL_CARD_UNMASK_PROMPT_STORAGE_CHECKBOX));
+  storage_checkbox->SetBorder(views::CreateEmptyBorder(gfx::Insets()));
+  storage_checkbox->SetChecked(start_state);
+  storage_checkbox->SetEnabledTextColors(views::style::GetColor(
+      *storage_checkbox.get(), ChromeTextContext::CONTEXT_BODY_TEXT_SMALL,
+      views::style::STYLE_SECONDARY));
+
+  return storage_checkbox;
+}
+
 }  // namespace
 
 CardUnmaskPromptViews::CardUnmaskPromptViews(
@@ -71,6 +84,11 @@ CardUnmaskPromptViews::CardUnmaskPromptViews(
     content::WebContents* web_contents)
     : controller_(controller), web_contents_(web_contents) {
   chrome::RecordDialogCreation(chrome::DialogIdentifier::CARD_UNMASK);
+  if (controller_->CanStoreLocally()) {
+    storage_checkbox_ = DialogDelegate::SetFootnoteView(
+        CreateSaveCheckbox(controller_->GetStoreLocallyStartState()));
+  }
+
   UpdateButtonLabels();
 }
 
@@ -224,23 +242,6 @@ views::View* CardUnmaskPromptViews::GetContentsView() {
   return this;
 }
 
-std::unique_ptr<views::View> CardUnmaskPromptViews::CreateFootnoteView() {
-  if (!controller_->CanStoreLocally())
-    return nullptr;
-
-  auto storage_checkbox =
-      std::make_unique<views::Checkbox>(l10n_util::GetStringUTF16(
-          IDS_AUTOFILL_CARD_UNMASK_PROMPT_STORAGE_CHECKBOX));
-  storage_checkbox->SetBorder(views::CreateEmptyBorder(gfx::Insets()));
-  storage_checkbox->SetChecked(controller_->GetStoreLocallyStartState());
-  storage_checkbox->SetEnabledTextColors(views::style::GetColor(
-      *storage_checkbox.get(), ChromeTextContext::CONTEXT_BODY_TEXT_SMALL,
-      views::style::STYLE_SECONDARY));
-  storage_checkbox_ = storage_checkbox.get();
-
-  return storage_checkbox;
-}
-
 gfx::Size CardUnmaskPromptViews::CalculatePreferredSize() const {
   // If the margins width is not discounted here, the bubble border will be
   // taken into consideration in the frame width size. Because of that, the

chrome/browser/ui/views/autofill/payments/card_unmask_prompt_views.h

@@ -51,7 +51,6 @@ class CardUnmaskPromptViews : public CardUnmaskPromptView,
 
   // views::DialogDelegateView
   View* GetContentsView() override;
-  std::unique_ptr<View> CreateFootnoteView() override;
 
   // views::View
   gfx::Size CalculatePreferredSize() const override;

components/autofill/core/browser/ui/payments/card_unmask_prompt_controller_impl.cc

@@ -37,7 +37,7 @@ CardUnmaskPromptControllerImpl::~CardUnmaskPromptControllerImpl() {
 }
 
 void CardUnmaskPromptControllerImpl::ShowPrompt(
-    CardUnmaskPromptView* card_unmask_view,
+    CardUnmaskPromptViewFactory card_unmask_view_factory,
     const CreditCard& card,
     AutofillClient::UnmaskCardReason reason,
     base::WeakPtr<CardUnmaskDelegate> delegate) {
@@ -47,10 +47,10 @@ void CardUnmaskPromptControllerImpl::ShowPrompt(
   new_card_link_clicked_ = false;
   shown_timestamp_ = AutofillClock::Now();
   pending_details_ = CardUnmaskDelegate::UserProvidedUnmaskDetails();
-  card_unmask_view_ = card_unmask_view;
   card_ = card;
   reason_ = reason;
   delegate_ = delegate;
+  card_unmask_view_ = std::move(card_unmask_view_factory).Run();
   card_unmask_view_->Show();
   unmasking_result_ = AutofillClient::NONE;
   unmasking_number_of_attempts_ = 0;

components/autofill/core/browser/ui/payments/card_unmask_prompt_controller_impl.h

@@ -25,8 +25,15 @@ class CardUnmaskPromptControllerImpl : public CardUnmaskPromptController {
                                  bool is_off_the_record);
   virtual ~CardUnmaskPromptControllerImpl();
 
+  // This should be OnceCallback<unique_ptr<CardUnmaskPromptView>> but there are
+  // tests which don't do the ownership correctly.
+  using CardUnmaskPromptViewFactory =
+      base::OnceCallback<CardUnmaskPromptView*()>;
+
   // Functions called by ChromeAutofillClient.
-  void ShowPrompt(CardUnmaskPromptView* view,
+  // It is guaranteed that |view_factory| is called before this function
+  // returns, i.e., the callback will not outlive the stack frame of ShowPrompt.
+  void ShowPrompt(CardUnmaskPromptViewFactory view_factory,
                   const CreditCard& card,
                   AutofillClient::UnmaskCardReason reason,
                   base::WeakPtr<CardUnmaskDelegate> delegate);

components/autofill/core/browser/ui/payments/card_unmask_prompt_controller_impl_unittest.cc

@@ -96,14 +96,20 @@ class CardUnmaskPromptControllerImplGenericTest {
 
   void ShowPrompt() {
     controller_->ShowPrompt(
-        test_unmask_prompt_view_.get(), test::GetMaskedServerCard(),
-        AutofillClient::UNMASK_FOR_AUTOFILL, delegate_->GetWeakPtr());
+        base::Bind(
+            &CardUnmaskPromptControllerImplGenericTest::GetCardUnmaskPromptView,
+            base::Unretained(this)),
+        test::GetMaskedServerCard(), AutofillClient::UNMASK_FOR_AUTOFILL,
+        delegate_->GetWeakPtr());
   }
 
   void ShowPromptAmex() {
     controller_->ShowPrompt(
-        test_unmask_prompt_view_.get(), test::GetMaskedServerCardAmex(),
-        AutofillClient::UNMASK_FOR_AUTOFILL, delegate_->GetWeakPtr());
+        base::Bind(
+            &CardUnmaskPromptControllerImplGenericTest::GetCardUnmaskPromptView,
+            base::Unretained(this)),
+        test::GetMaskedServerCardAmex(), AutofillClient::UNMASK_FOR_AUTOFILL,
+        delegate_->GetWeakPtr());
   }
 
   void ShowPromptAndSimulateResponse(bool should_store_pan,
@@ -130,6 +136,10 @@ class CardUnmaskPromptControllerImplGenericTest {
   std::unique_ptr<TestCardUnmaskDelegate> delegate_;
 
  private:
+  CardUnmaskPromptView* GetCardUnmaskPromptView() {
+    return test_unmask_prompt_view_.get();
+  }
+
   DISALLOW_COPY_AND_ASSIGN(CardUnmaskPromptControllerImplGenericTest);
 };
 

ios/chrome/browser/ui/autofill/chrome_autofill_client_ios.mm

@@ -71,6 +71,13 @@ std::unique_ptr<infobars::InfoBar> CreateSaveCardInfoBarMobile(
   }
 }
 
+autofill::CardUnmaskPromptView* CreateCardUnmaskPromptViewBridge(
+    autofill::CardUnmaskPromptControllerImpl* unmask_controller,
+    UIViewController* base_view_controller) {
+  return new autofill::CardUnmaskPromptViewBridge(unmask_controller,
+                                                  base_view_controller);
+}
+
 }  // namespace
 
 namespace autofill {
@@ -202,12 +209,10 @@ void ChromeAutofillClientIOS::ShowUnmaskPrompt(
     const CreditCard& card,
     UnmaskCardReason reason,
     base::WeakPtr<CardUnmaskDelegate> delegate) {
-  unmask_controller_.ShowPrompt(
-      // autofill::CardUnmaskPromptViewBridge manages its own lifetime, so
-      // do not use std::unique_ptr<> here.
-      new autofill::CardUnmaskPromptViewBridge(&unmask_controller_,
-                                               base_view_controller_),
-      card, reason, delegate);
+  unmask_controller_.ShowPrompt(base::Bind(&CreateCardUnmaskPromptViewBridge,
+                                    base::Unretained(&unmask_controller_),
+                                    base::Unretained(base_view_controller_)),
+                                card, reason, delegate);
 }
 
 void ChromeAutofillClientIOS::OnUnmaskVerificationResult(

ios/chrome/browser/ui/payments/full_card_requester.mm

@@ -12,6 +12,17 @@
 #error "This file requires ARC support."
 #endif
 
+namespace {
+
+autofill::CardUnmaskPromptView* CreateCardUnmaskPromptViewBridge(
+    autofill::CardUnmaskPromptControllerImpl* controller,
+    UIViewController* base_view_controller) {
+  return new autofill::CardUnmaskPromptViewBridge(controller,
+                                                  base_view_controller);
+}
+
+}
+
 FullCardRequester::FullCardRequester(UIViewController* base_view_controller,
                                      ios::ChromeBrowserState* browser_state)
     : base_view_controller_(base_view_controller),
@@ -34,9 +45,11 @@ void FullCardRequester::ShowUnmaskPrompt(
     const autofill::CreditCard& card,
     autofill::AutofillClient::UnmaskCardReason reason,
     base::WeakPtr<autofill::CardUnmaskDelegate> delegate) {
-  unmask_controller_.ShowPrompt(new autofill::CardUnmaskPromptViewBridge(
-                                    &unmask_controller_, base_view_controller_),
-                                card, reason, delegate);
+  unmask_controller_.ShowPrompt(
+      base::Bind(&CreateCardUnmaskPromptViewBridge,
+                 base::Unretained(&unmask_controller_),
+                 base::Unretained(base_view_controller_)),
+      card, reason, delegate);
 }
 
 void FullCardRequester::OnUnmaskVerificationResult(

ios/web_view/internal/autofill/cwv_credit_card_verifier.mm

@@ -109,8 +109,11 @@ class WebViewCardUnmaskPromptView : public autofill::CardUnmaskPromptView {
     _unmaskingController =
         std::make_unique<autofill::CardUnmaskPromptControllerImpl>(
             prefs, isOffTheRecord);
-    _unmaskingController->ShowPrompt(_unmaskingView.get(), creditCard, reason,
-                                     delegate);
+    _unmaskingController->ShowPrompt(
+        base::BindOnce(^autofill::CardUnmaskPromptView*() {
+          return _unmaskingView.get();
+        }),
+        creditCard, reason, delegate);
   }
   return self;
 }