Use ViewStack to convert card upload request CVC experiment to 2-step flow

chrome/browser/ui/views/autofill/save_card_bubble_views.cc

Index: chrome/browser/ui/views/autofill/save_card_bubble_views.cc
diff --git a/chrome/browser/ui/views/autofill/save_card_bubble_views.cc b/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
index 56b4018f702a2f715a871771167ce74207939e36..c16875e19b28539bde380322f169127da97c0d66 100644
--- a/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
+++ b/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
@@ -54,9 +54,7 @@ SaveCardBubbleViews::SaveCardBubbleViews(views::View* anchor_view,
                                          content::WebContents* web_contents,
                                          SaveCardBubbleController* controller)
     : LocationBarBubbleDelegateView(anchor_view, web_contents),
-      controller_(controller),
-      cvc_textfield_(nullptr),
-      learn_more_link_(nullptr) {
+      controller_(controller) {
   DCHECK(controller);
   views::BubbleDialogDelegateView::CreateBubble(this);
   chrome::RecordDialogCreation(chrome::DialogIdentifier::SAVE_CARD);
@@ -98,6 +96,19 @@ views::View* SaveCardBubbleViews::CreateFootnoteView() {
 }
 
 bool SaveCardBubbleViews::Accept() {
+  CHECK_LE(view_stack_->size(), static_cast<size_t>(2));

[Evan Stade, 2017/05/19 19:01:28]

nit: 2U rather than casting

[Evan Stade, 2017/05/19 19:01:29]

nit: make these DCHECKs

[Jared Saul, 2017/05/19 19:20:55]

Done.

[Jared Saul, 2017/05/19 19:20:55]

Done.

+  CHECK(view_stack_->size() == 1 || controller_->ShouldRequestCvcFromUser());

[Evan Stade, 2017/05/19 19:01:29]

I don't think this check is super self-explanatory, consider adding a comment.

[Jared Saul, 2017/05/19 19:20:55]

Done; how's this?

+  if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) {
+    // If user accepted upload but more info is needed, push the next view onto
+    // the stack.
+    view_stack_->Push(CreateRequestCvcView(), /*animate=*/true);
+    // Disable the Save button until a valid CVC is entered:
+    GetDialogClientView()->UpdateDialogButtons();
+    // Resize the bubble if it's grown larger:
+    SizeToContents();
+    return false;
+  }
+  // Otherwise, close the bubble as normal.
   if (controller_)
     controller_->OnSaveButton(cvc_textfield_ ? cvc_textfield_->text()
                                              : base::string16());
@@ -181,7 +192,8 @@ void SaveCardBubbleViews::StyledLabelLinkClicked(views::StyledLabel* label,
 
 // Create view containing everything except for the footnote.
 std::unique_ptr<views::View> SaveCardBubbleViews::CreateMainContentView() {
-  std::unique_ptr<View> view(new View());
+  std::unique_ptr<View> view = base::MakeUnique<views::View>();

[Evan Stade, 2017/05/19 22:18:16]

nit: great place to use auto

[Jared Saul, 2017/05/23 18:53:02]

Done; replaced the other two "std::unique_ptr<View>" as well.

+  view->set_background(views::Background::CreateSolidBackground(SK_ColorWHITE));
   view->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
                            views::kUnrelatedControlVerticalSpacing));
@@ -208,10 +220,6 @@ std::unique_ptr<views::View> SaveCardBubbleViews::CreateMainContentView() {
   description_view->AddChildView(
       new views::Label(card.AbbreviatedExpirationDateForDisplay()));
 
-  // Optionally add CVC request field if CVC was missing.
-  if (controller_->ShouldRequestCvcFromUser())
-    view->AddChildView(CreateRequestCvcView().release());
-
   // Optionally add label that will contain an explanation for upload.
   base::string16 explanation = controller_->GetExplanatoryMessage();
   if (!explanation.empty()) {
@@ -226,22 +234,33 @@ std::unique_ptr<views::View> SaveCardBubbleViews::CreateMainContentView() {
 
 std::unique_ptr<views::View> SaveCardBubbleViews::CreateRequestCvcView() {
   std::unique_ptr<View> request_cvc_view = base::MakeUnique<views::View>();
-  request_cvc_view->SetLayoutManager(new views::BoxLayout(
+  request_cvc_view->set_background(
+      views::Background::CreateSolidBackground(SK_ColorWHITE));

[Evan Stade, 2017/05/19 19:01:28]

Why do you need to set this color? Why aren't we getting the correct bg color by
default, as per the theme? What platform are you developing on?

[Jared Saul, 2017/05/19 19:20:55]

It's a limitation of ViewStack, see mathp@/anthonyvd@'s discussion in this CL:
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut...
Sounds like Anthony has an idea on how to modify ViewStack to choose what it
should paint.  (Also, Linux.)

[Evan Stade, 2017/05/19 22:18:17]

What you need both here and there is CreateThemedSolidBackground(), probably
with kColorId_BubbleBackground as the id, and you should probably call it on the
ViewStack. Hardcoding an SkColor value is nearly always not what you want.

[Jared Saul, 2017/05/23 18:53:03]

Thanks for the tip; that worked!  I also did have to set it on the 2nd view as
well, otherwise when it slides in from the right without a background, both
views overlap.

+  request_cvc_view->SetLayoutManager(
+      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
+                           views::kUnrelatedControlVerticalSpacing));
+  std::unique_ptr<View> inner_request_cvc_view =
+      base::MakeUnique<views::View>();
+  inner_request_cvc_view->set_background(
+      views::Background::CreateSolidBackground(SK_ColorWHITE));
+  inner_request_cvc_view->SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kHorizontal, 0, 0, views::kRelatedButtonHSpacing));
 
   DCHECK(!cvc_textfield_);
   cvc_textfield_ = CreateCvcTextfield();
   cvc_textfield_->set_controller(this);
-  request_cvc_view->AddChildView(cvc_textfield_);
+  inner_request_cvc_view->AddChildView(cvc_textfield_);
 
   views::ImageView* cvc_image = new views::ImageView();
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   cvc_image->SetImage(
       rb.GetImageSkiaNamed(controller_->GetCvcImageResourceId()));
-  request_cvc_view->AddChildView(cvc_image);
+  inner_request_cvc_view->AddChildView(cvc_image);
 
-  request_cvc_view->AddChildView(new views::Label(
+  inner_request_cvc_view->AddChildView(new views::Label(
       l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC)));
+
+  request_cvc_view->AddChildView(inner_request_cvc_view.release());
   return request_cvc_view;
 }
 
@@ -262,7 +281,10 @@ void SaveCardBubbleViews::ContentsChanged(views::Textfield* sender,
 
 void SaveCardBubbleViews::Init() {
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
-  AddChildView(CreateMainContentView().release());
+  view_stack_ = base::MakeUnique<ViewStack>();
+  view_stack_->set_owned_by_client();  // Required else it'll be deleted twice.

[Evan Stade, 2017/05/19 19:01:28]

are you sure this is required? Why not make view_stack_ a weak/dumb pointer like
the vast majority of views that are members?

[Jared Saul, 2017/05/19 19:20:55]

See comment thread:
https://codereview.chromium.org/2883273005/diff/1/chrome/browser/ui/views/aut...
anthonyvd@ called out that it's required.  I tried making it a raw pointer, but
the bubble then only accounted for an empty ViewStack when deciding its
dimensions and the actual content of the bubble was hidden.

[Evan Stade, 2017/05/19 22:18:17]

I don't understand how making this a raw pointer not owned by client would cause
that problem. The ownership/destruction sequence is almost exactly the same
either way.

[Jared Saul, 2017/05/23 18:53:03]

Ah, you're right.  I believe those problems stemmed from Anthony's suggestion of
adding the ViewStack in the ctor instead of Init().  Making this a raw pointer
and keeping it here works after all.

+  view_stack_->Push(CreateMainContentView(), /*animate=*/false);
+  AddChildView(view_stack_.get());
 }
 
 }  // namespace autofill