[Payments] Upload card UI now has a CVC prompt

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 2549a1759d90632881233199c593c8ef9d5f3ae3..3bb8b819c41eb1c7415284e39a4597890d80409e 100644
--- a/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
+++ b/chrome/browser/ui/views/autofill/save_card_bubble_views.cc
@@ -8,9 +8,9 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
-#include "chrome/browser/ui/autofill/save_card_bubble_controller.h"
 #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"
@@ -21,8 +21,10 @@
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/styled_label.h"
+#include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/layout_constants.h"
+#include "ui/views/window/dialog_client_view.h"
 
 namespace autofill {
 
@@ -30,6 +32,8 @@ namespace {
 
 // Fixed width of the bubble.

[Evan Stade, 2017/04/07 02:14:39]

[1/2] this is in dip

[Jared Saul, 2017/04/08 01:05:36]

Acknowledged.

 const int kBubbleWidth = 395;
+// Fixed width of the CVC TextField.

[Evan Stade, 2017/04/07 02:14:39]

nit: TextField has odd capitalization

[Jared Saul, 2017/04/08 01:05:36]

Whoops you're right, it's "Textfield".  Thanks.

+const int kDefaultWidthForCvc = 8;

[Evan Stade, 2017/04/07 02:14:39]

[2/2] but this is in characters. This distinction should be documented. Although
I think it's even better to share this constant with the 8 in
card_unmask_prompt_view, or heck share the whole textfield init stanza.

[Jared Saul, 2017/04/08 01:05:36]

I'm all for sharing the entire textfield init stanza.  Where do you think it
should be placed?  (In the meantime, documented the distinction.)

[Evan Stade, 2017/04/10 18:32:09]

probably a static fn in chrome/browser/ui/views/autofill/view_util.h

[Jared Saul, 2017/04/11 00:53:06]

Done; created view_util.h, PTAL.

 
 std::unique_ptr<views::StyledLabel> CreateLegalMessageLineLabel(
     const LegalMessageLine& line,
@@ -50,6 +54,7 @@ SaveCardBubbleViews::SaveCardBubbleViews(views::View* anchor_view,
                                          SaveCardBubbleController* controller)
     : LocationBarBubbleDelegateView(anchor_view, web_contents),
       controller_(controller),
+      cvc_textfield_(nullptr),
       learn_more_link_(nullptr) {
   DCHECK(controller);
   views::BubbleDialogDelegateView::CreateBubble(this);
@@ -91,8 +96,12 @@ views::View* SaveCardBubbleViews::CreateFootnoteView() {
 }
 
 bool SaveCardBubbleViews::Accept() {
-  if (controller_)
+  if (controller_) {
+    if (controller_->ShouldRequestCvcFromUser()) {

[Evan Stade, 2017/04/07 02:14:39]

nit: no curlies

also I would slightly prefer this to check the null-ness of cvc_textfield_
instead of asking the controller again.

[Jared Saul, 2017/04/08 01:05:36]

Done and done.

+      controller_->SetCvcEnteredByUser(cvc_textfield_->text());
+    }
     controller_->OnSaveButton();
+  }
   return true;
 }
 
@@ -200,6 +209,11 @@ 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()) {

[Evan Stade, 2017/04/07 02:14:39]

nit: no curlies

[Jared Saul, 2017/04/08 01:05:36]

Done.

+    view->AddChildView(CreateRequestCvcView().release());
+  }
+
   // Optionally add label that will contain an explanation for upload.
   base::string16 explanation = controller_->GetExplanatoryMessage();
   if (!explanation.empty()) {
@@ -212,6 +226,47 @@ 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>();
+  request_cvc_view->SetLayoutManager(
+      base::MakeUnique<views::BoxLayout>(views::BoxLayout::kHorizontal, 0, 0,
+                                         views::kRelatedButtonHSpacing)
+          .release());
+
+  DCHECK(!cvc_textfield_);
+  cvc_textfield_ = new views::Textfield();

[Evan Stade, 2017/04/07 02:14:39]

aside: I wish we had a way to set the limit on number of characters input

[Jared Saul, 2017/04/08 01:05:36]

Acknowledged (agreed).

+  cvc_textfield_->set_placeholder_text(
+      l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PLACEHOLDER_CVC));
+  cvc_textfield_->set_controller(this);
+  cvc_textfield_->set_default_width_in_chars(kDefaultWidthForCvc);
+  request_cvc_view->AddChildView(cvc_textfield_);
+
+  std::unique_ptr<views::ImageView> cvc_image =

[Evan Stade, 2017/04/07 02:14:39]

nit: we don't typically use unique_ptr for views like this

[Jared Saul, 2017/04/08 01:05:36]

Done, changed to match card_unmask_prompt_views.cc.

+      base::MakeUnique<views::ImageView>();
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  cvc_image->SetImage(
+      rb.GetImageSkiaNamed(controller_->GetCvcImageResourceId()));
+  request_cvc_view->AddChildView(cvc_image.release());
+
+  request_cvc_view->AddChildView(new views::Label(
+      l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC)));
+  return request_cvc_view;
+}
+
+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());

[Evan Stade, 2017/04/07 02:14:39]

for future consideration: it would be nice if there were some indication the
input cvc were invalid when users can't figure out why the ok button is disabled

[Jared Saul, 2017/04/08 01:05:36]

csashi@ said the same thing, both for this and for card_unmask_prompt_view. 
It's something we should definitely consider for an upcoming improvement.  Just
spitballing ideas here--any idea if there's precedent for disabled buttons to
have tooltip text explaining why they're disabled?

[Evan Stade, 2017/04/10 18:32:09]

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/card_un...

[Jared Saul, 2017/04/11 00:53:06]

Acknowledged.  Though for the record, that happens *after* the CVC is entered
and the button is clicked (and auth fails).  Looks like that view still doesn't
have anything beforehand that implies CVC is what's preventing the button from
being clicked, hmm.

[Evan Stade, 2017/04/11 15:28:05]

yes. Since this CVC validity check is local and not done on the server you can
do the check whenever you feel is appropriate --- like when CVC loses focus, or
when the user presses "enter" or hovers the mouse over the button or something.

[Jared Saul, 2017/04/11 22:57:47]

Acknowledged.  Wrote myself a note to look into this for here and the unmask
view in the future; I like the loses-focus-or-user-presses-enter idea.

+}
+
+void SaveCardBubbleViews::ContentsChanged(views::Textfield* sender,
+                                          const base::string16& new_contents) {
+  GetDialogClientView()->UpdateDialogButtons();
+}
+
 void SaveCardBubbleViews::Init() {
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
   AddChildView(CreateMainContentView().release());