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..76906481a55b3cfaf8957dce6c6fa6e8c93b3d2e 100644
--- a/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
+++ b/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
@@ -11,6 +11,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/views/autofill/view_util.h"
+#include "chrome/browser/ui/views/payments/view_stack.h"

[Mathieu, 2017/05/17 23:53:01]

no need

[Jared Saul, 2017/05/18 17:01:23]

Done.

 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill/core/browser/legal_message_line.h"
 #include "components/autofill/core/browser/ui/save_card_bubble_controller.h"
@@ -55,8 +56,8 @@ SaveCardBubbleViews::SaveCardBubbleViews(views::View* anchor_view,
                                          SaveCardBubbleController* controller)
     : LocationBarBubbleDelegateView(anchor_view, web_contents),
       controller_(controller),
-      cvc_textfield_(nullptr),
-      learn_more_link_(nullptr) {
+      learn_more_link_(nullptr),

[Mathieu, 2017/05/17 23:53:01]

feel free to initialize this and cvc_textfield_ = nullptr; in the .h instead

[Jared Saul, 2017/05/18 17:01:23]

Done.

+      cvc_textfield_(nullptr) {
   DCHECK(controller);
   views::BubbleDialogDelegateView::CreateBubble(this);
   chrome::RecordDialogCreation(chrome::DialogIdentifier::SAVE_CARD);
@@ -98,6 +99,15 @@ views::View* SaveCardBubbleViews::CreateFootnoteView() {
 }
 
 bool SaveCardBubbleViews::Accept() {
+  // If upload save and more info is needed, push the next view onto the stack.
+  if (controller_->ShouldRequestCvcFromUser() && view_stack_->size() == 1) {

[anthonyvd, 2017/05/18 12:01:01]

Can this be reached if IsUploading is false? If so, view_stack_ will be null
here. Note that Math's suggestion of always having the view stack exist fixes
this.

[Jared Saul, 2017/05/18 17:01:23]

Always having the view stack exist fixes this, thanks.

+    CreateAndAddRequestCvcViewToViewStack();
+    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());
@@ -182,6 +192,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());
+  view->set_background(views::Background::CreateSolidBackground(SK_ColorWHITE));
   view->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
                            views::kUnrelatedControlVerticalSpacing));
@@ -208,10 +219,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()) {
@@ -224,8 +231,16 @@ std::unique_ptr<views::View> SaveCardBubbleViews::CreateMainContentView() {
   return view;
 }
 
-std::unique_ptr<views::View> SaveCardBubbleViews::CreateRequestCvcView() {
-  std::unique_ptr<View> request_cvc_view = base::MakeUnique<views::View>();
+void SaveCardBubbleViews::CreateAndAddRequestCvcViewToViewStack() {

[Mathieu, 2017/05/17 23:53:01]

I would have this return std::unique_ptr<views::View> and in Accept you can do

view_stack_->Push(CreateRequestCvcView());

[Jared Saul, 2017/05/18 17:01:23]

Done.

+  std::unique_ptr<View> overall_view(new View());

[Mathieu, 2017/05/17 23:53:01]

 = base::MakeUnique<views::View>();

[Jared Saul, 2017/05/18 17:01:23]

Done.

+  overall_view->set_background(
+      views::Background::CreateSolidBackground(SK_ColorWHITE));
+  overall_view->SetLayoutManager(
+      new views::BoxLayout(views::BoxLayout::kVertical, 0, 0,
+                           views::kUnrelatedControlVerticalSpacing));
+  views::View* request_cvc_view = new views::View();

[Mathieu, 2017/05/17 23:53:01]

same base::MakeUnique<...>() and .release() later

[Jared Saul, 2017/05/18 17:01:23]

Done.

+  request_cvc_view->set_background(

[Mathieu, 2017/05/17 23:53:01]

need?

[Jared Saul, 2017/05/18 17:01:23]

Seems so--without it, upon switching to ViewStack the views have a cyan
background and I get warnings for "Ancestor view has a non-opaque layer".

[anthonyvd, 2017/05/18 17:22:59]

Ah yeah, this is probably because the ViewStack paints to a layer (we were using
it with Harmony buttons, which paint to a layer themselves and were encountering
bugs where clipping didn't happen correctly).

In any case setting the background should be fine if it works for you but
deciding whether to paint the stack to a layer or not should probably be changed
to be the caller's decision.

[Jared Saul, 2017/05/18 17:34:34]

You mean in ViewStack's code?  Or do you have a suggested alternative for this?

[anthonyvd, 2017/05/18 21:08:41]

Sorry, yes I meant in ViewStack's code :)

+      views::Background::CreateSolidBackground(SK_ColorWHITE));
   request_cvc_view->SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kHorizontal, 0, 0, views::kRelatedButtonHSpacing));
 
@@ -242,7 +257,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);
+  view_stack_->Push(std::move(overall_view), true);
 }
 
 bool SaveCardBubbleViews::IsDialogButtonEnabled(ui::DialogButton button) const {
@@ -260,9 +277,19 @@ void SaveCardBubbleViews::ContentsChanged(views::Textfield* sender,
   GetDialogClientView()->UpdateDialogButtons();
 }
 
+bool SaveCardBubbleViews::IsUploading() {
+  return !controller_->GetLegalMessageLines().empty();
+}
+
 void SaveCardBubbleViews::Init() {
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
-  AddChildView(CreateMainContentView().release());
+  if (IsUploading()) {
+    view_stack_ = base::MakeUnique<ViewStack>();

[anthonyvd, 2017/05/18 12:01:01]

If you own the view, you must call set_owned_by_client on it (otherwise it'll be
deleted twice).

It's not immediately clear that you need to own it explicitly (and
set_owned_by_client is discouraged). Provided you implement Math's change that
always adds the view stack, you could also do AddChildView(new ViewStack) in the
ctor, and have a ViewStack* view_stack() { return
static_cast<ViewStack*>(child_at(0)); } function to get it back (or even keep a
raw pointer to it).

Anyways I'm not sure there's a nice solution to this, so I'll let you guys pick
whichever you feel is the lesser evil as owners of this code.

[Jared Saul, 2017/05/18 17:01:23]

Thanks for the heads-up.  I tried the raw pointer approach, but I think the
bubble was taking into account the empty ViewStack when initially deciding its
size and hiding the whole thing even after adding to it.  I opted for
set_owned_by_client for now, but Mathieu please let me know if you have a
preference.

+    view_stack_->Push(CreateMainContentView(), false);

[Mathieu, 2017/05/17 23:53:01]

Why not do this in all cases? Would simplify the code.

[Mathieu, 2017/05/17 23:53:01]

/*animate=*/false

[anthonyvd, 2017/05/18 12:01:01]

Yep, this and what's in the else block should be equivalent visually.

[Jared Saul, 2017/05/18 17:01:23]

Good idea!  Done.

+    AddChildView(view_stack_.get());
+  } else {
+    AddChildView(CreateMainContentView().release());
+  }
 }
 
 }  // namespace autofill