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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/autofill/save_card_bubble_views.h"
 
 #include <stddef.h>
 #include <memory>
 
 #include "base/strings/utf_string_conversions.h"
 #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"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/views/border.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/controls/button/blue_button.h"
 #include "ui/views/controls/button/label_button.h"
@@ skipping 24 matching lines @@
   return label;
 }
 
 }  // namespace
 
 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) {
+      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);
 }
 
 SaveCardBubbleViews::~SaveCardBubbleViews() {}
 
 void SaveCardBubbleViews::Show(DisplayReason reason) {
   ShowForReason(reason);
 }
@@ skipping 21 matching lines @@
       new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
 
   // Add a StyledLabel for each line of the legal message.
   for (const LegalMessageLine& line : controller_->GetLegalMessageLines())
     view->AddChildView(CreateLegalMessageLineLabel(line, this).release());
 
   return view;
 }
 
 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());
   return true;
 }
 
 bool SaveCardBubbleViews::Cancel() {
   if (controller_)
     controller_->OnCancelButton();
   return true;
@@ skipping 64 matching lines @@
     }
   }
 
   // |range| was not found.
   NOTREACHED();
 }
 
 // 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));
 
   // Add the card type icon, last four digits and expiration date.
   views::View* description_view = new views::View();
   description_view->SetLayoutManager(new views::BoxLayout(
       views::BoxLayout::kHorizontal, 0, 0, views::kRelatedButtonHSpacing));
   view->AddChildView(description_view);
 
   const CreditCard& card = controller_->GetCard();
   views::ImageView* card_type_icon = new views::ImageView();
   card_type_icon->SetImage(
       ResourceBundle::GetSharedInstance()
           .GetImageNamed(CreditCard::IconResourceId(card.network()))
           .AsImageSkia());
   card_type_icon->SetTooltipText(card.NetworkForDisplay());
   card_type_icon->SetBorder(
       views::CreateSolidBorder(1, SkColorSetA(SK_ColorBLACK, 10)));
   description_view->AddChildView(card_type_icon);
 
   description_view->AddChildView(new views::Label(
       base::string16(kMidlineEllipsis) + card.LastFourDigits()));
   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()) {
     views::Label* explanation_label = new views::Label(explanation);
     explanation_label->SetMultiLine(true);
     explanation_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     view->AddChildView(explanation_label);
   }
 
   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));
 
   DCHECK(!cvc_textfield_);
   cvc_textfield_ = CreateCvcTextfield();
   cvc_textfield_->set_controller(this);
   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);
 
   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 {
   if (button == ui::DIALOG_BUTTON_CANCEL)
     return true;
 
   DCHECK_EQ(ui::DIALOG_BUTTON_OK, button);
   return !cvc_textfield_ ||
          controller_->InputCvcIsValid(cvc_textfield_->text());
 }
 
 void SaveCardBubbleViews::ContentsChanged(views::Textfield* sender,
                                           const base::string16& new_contents) {
   DCHECK_EQ(cvc_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