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..1289e3e7b377779bea4acd7317793280c6131ab1 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,16 @@ views::View* SaveCardBubbleViews::CreateFootnoteView() {
 }
 
 bool SaveCardBubbleViews::Accept() {
+  // If upload save and more info is needed, push the next view onto the stack.

[csashi, 2017/05/18 17:42:25]

s/upload save and/user accepted upload but/

Move comment to inside if?

[Jared Saul, 2017/05/18 18:03:07]

Done.

+  if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) {

[csashi, 2017/05/18 17:42:25]

DCHECK_LE(view_stack_->size(), 1)?
When can we be here if a view_stack_ size != 1?

[Jared Saul, 2017/05/18 18:03:07]

1) Done.
2) If missing CVC but on the second step of the bubble where CVC is being
entered.  The button that is clicked doesn't change.

[csashi, 2017/05/18 19:29:48]

For (2). If stack_size() > 1, should we
DCHECK(conroller_->ShouldRequestCvcFromUser());

[Jared Saul, 2017/05/18 19:51:36]

Added check for <= 2 stack size at all times, as well as a check for "must be
stack size 1 or ShouldRequestCvcFromUser()"

+    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;

[csashi, 2017/05/18 17:42:25]

Do you need to update the documentation for Accept now? You are returning false
in some cases.

[Jared Saul, 2017/05/18 18:03:07]

Hmm, I don't think so.  The current documentation says "This function should
return true if the window can be closed after it returns, or false if it must
remain open." which seems sufficient.

+  }
+  // Otherwise, close the bubble as normal.
   if (controller_)
     controller_->OnSaveButton(cvc_textfield_ ? cvc_textfield_->text()
                                              : base::string16());
@@ -182,6 +190,7 @@ 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());

[csashi, 2017/05/18 17:42:25]

MakeUnique?

[Jared Saul, 2017/05/18 18:03:07]

Done.

+  view->set_background(views::Background::CreateSolidBackground(SK_ColorWHITE));
   view->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
                            views::kUnrelatedControlVerticalSpacing));
@@ -208,10 +217,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()) {
@@ -225,7 +230,15 @@ std::unique_ptr<views::View> SaveCardBubbleViews::CreateMainContentView() {
 }
 
 std::unique_ptr<views::View> SaveCardBubbleViews::CreateRequestCvcView() {

[csashi, 2017/05/18 17:42:25]

Do you want to rename this function? We are returning the 'overall' view now.

[Jared Saul, 2017/05/18 18:03:07]

I feel the name is still applicable as it's doing the same thing, but I had to
wrap the horizontal view in a vertical view or it had weird effects.  I'll
rename the variables to make it clearer.

+  std::unique_ptr<View> overall_view = base::MakeUnique<views::View>();
+  overall_view->set_background(
+      views::Background::CreateSolidBackground(SK_ColorWHITE));

[csashi, 2017/05/18 17:42:25]

Do you need this? Does the stack not inherit from the background from the view
on top of the stack?

[Jared Saul, 2017/05/18 18:03:07]

Without it, it doesn't cover up the item below it on the stack and you can see
through to the previous layer.

+  overall_view->SetLayoutManager(
+      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
+                           views::kUnrelatedControlVerticalSpacing));
   std::unique_ptr<View> request_cvc_view = base::MakeUnique<views::View>();
+  request_cvc_view->set_background(
+      views::Background::CreateSolidBackground(SK_ColorWHITE));
   request_cvc_view->SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kHorizontal, 0, 0, views::kRelatedButtonHSpacing));
 
@@ -242,7 +255,9 @@ std::unique_ptr<views::View> SaveCardBubbleViews::CreateRequestCvcView() {
 
   request_cvc_view->AddChildView(new views::Label(
       l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC)));
-  return request_cvc_view;
+
+  overall_view->AddChildView(request_cvc_view.release());
+  return overall_view;
 }
 
 bool SaveCardBubbleViews::IsDialogButtonEnabled(ui::DialogButton button) const {
@@ -262,7 +277,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.
+  view_stack_->Push(CreateMainContentView(), /*animate=*/false);
+  AddChildView(view_stack_.get());
 }
 
 }  // namespace autofill