Autofill: Add contents of CVC unmask prompt dialog on OSX.

chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm

Index: chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm
diff --git a/chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm b/chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm
index cc8897d4e7692583d115c363dd61d96f79105a6e..7aa106d3fe193c7e8bc6dad910d827ee540aee80 100644
--- a/chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm
+++ b/chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm
@@ -2,10 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/ui/autofill/card_unmask_prompt_controller.h"
 #include "chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h"
+#import "chrome/browser/ui/cocoa/autofill/autofill_pop_up_button.h"
+#import "chrome/browser/ui/cocoa/autofill/autofill_textfield.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_button.h"
 #include "chrome/browser/ui/chrome_style.h"
+#import "chrome/browser/ui/cocoa/constrained_window/constrained_window_control_utils.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_sheet.h"
 #import "chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_window.h"
 #import "chrome/browser/ui/cocoa/key_equivalent_constants.h"
@@ -14,7 +18,24 @@
 #include "ui/base/l10n/l10n_util.h"
 
 namespace {
+
 const CGFloat kButtonGap = 6.0f;
+const CGFloat kDialogMinWidth = 250.0f;
+const CGFloat kCvcInputWidth = 64.0f;
+
+AutofillPopUpButton* CreateDatePopup(ui::ComboboxModel& model) {

[groby-ooo-7-16, 2015/02/18 19:01:19]

FWIW, we usually have those builder functions as class methods on the cocoa
object. I.e. 

+ (AutofillPopUpButton*)buildDatePopupWithModel:(ui::ComboboxModel&);

[bondd, 2015/02/26 01:25:08]

Done.

+  AutofillPopUpButton* popup =

[groby-ooo-7-16, 2015/02/18 19:01:19]

why not use menuFromModel:?

[bondd, 2015/02/26 01:25:08]

Views implementation uses autofill::MonthComboboxModel and
autofill::YearComboboxModel, which are derived from ui::ComboboxModel.
menuFromModel requires a ui::MenuModel. I'd like to keep using the same
ComboboxModels as the Views implementation so that the behavior stays as
consistent as possible across platforms. I could convert the data from the
ComboboxModel to a MenuModel, but it seems like it would be at least as much
code as this solution. Maybe I am missing something in your suggestion?

[groby-ooo-7-16, 2015/02/28 00:49:17]

No, you don't. I naively assumed we had a menuFromModel for ComboboxModel as
well :(

+      [[AutofillPopUpButton alloc] initWithFrame:NSZeroRect pullsDown:NO];
+
+  for (int i = 0; i < model.GetItemCount(); ++i) {
+    [popup addItemWithTitle:base::SysUTF16ToNSString(model.GetItemAt(i))];
+  }
+  [popup setDefaultValue:base::SysUTF16ToNSString(
+                             model.GetItemAt(model.GetDefaultIndex()))];
+  [popup sizeToFit];
+  return popup;
+}
+
 }  // namespace
 
 namespace autofill {
@@ -30,12 +51,16 @@ CardUnmaskPromptView* CardUnmaskPromptView::CreateAndShow(
 CardUnmaskPromptViewBridge::CardUnmaskPromptViewBridge(
     CardUnmaskPromptController* controller)
     : controller_(controller) {
-  sheet_controller_.reset([[CardUnmaskPromptViewCocoa alloc]
+  view_controller_.reset([[CardUnmaskPromptViewCocoa alloc]
       initWithWebContents:controller_->GetWebContents()
                    bridge:this]);
+
+  // Setup the constrained window that will show the view.
+  base::scoped_nsobject<NSWindow> window([[ConstrainedWindowCustomWindow alloc]
+      initWithContentRect:[[view_controller_ view] bounds]]);
+  [[window contentView] addSubview:[view_controller_ view]];

[groby-ooo-7-16, 2015/02/18 19:01:19]

Are you sure you don't just want to replace the contentView?

[bondd, 2015/02/26 01:25:08]

Done.

   base::scoped_nsobject<CustomConstrainedWindowSheet> sheet(
-      [[CustomConstrainedWindowSheet alloc]
-          initWithCustomWindow:[sheet_controller_ window]]);
+      [[CustomConstrainedWindowSheet alloc] initWithCustomWindow:window]);
   constrained_window_.reset(
       new ConstrainedWindowMac(this, controller_->GetWebContents(), sheet));
 }
@@ -44,6 +69,8 @@ CardUnmaskPromptViewBridge::~CardUnmaskPromptViewBridge() {
 }
 
 void CardUnmaskPromptViewBridge::ControllerGone() {
+  controller_ = nullptr;
+  PerformClose();
 }
 
 void CardUnmaskPromptViewBridge::DisableAndWaitForVerification() {
@@ -58,6 +85,10 @@ void CardUnmaskPromptViewBridge::OnConstrainedWindowClosed(
   controller_->OnUnmaskDialogClosed();
 }
 
+CardUnmaskPromptController* CardUnmaskPromptViewBridge::GetController() {
+  return controller_;
+}
+
 void CardUnmaskPromptViewBridge::PerformClose() {
   constrained_window_->CloseWebContentsModalDialog();
 }
@@ -73,14 +104,9 @@ void CardUnmaskPromptViewBridge::PerformClose() {
   DCHECK(webContents);
   DCHECK(bridge);
 
-  NSRect frame = NSMakeRect(0, 0, 550, 600);
-  base::scoped_nsobject<ConstrainedWindowCustomWindow> window(
-      [[ConstrainedWindowCustomWindow alloc] initWithContentRect:frame]);
-  if ((self = [super initWithWindow:window])) {
+  if ((self = [super initWithNibName:nil bundle:nil])) {
     webContents_ = webContents;
     bridge_ = bridge;
-
-    [self buildWindowButtons];
   }
   return self;
 }
@@ -89,38 +115,150 @@ void CardUnmaskPromptViewBridge::PerformClose() {
   bridge_->PerformClose();
 }
 
-- (void)buildWindowButtons {
-  base::scoped_nsobject<NSView> buttonContainer(
-      [[NSView alloc] initWithFrame:NSZeroRect]);
+- (void)loadView {
+  self.view = [[[NSView alloc] initWithFrame:NSZeroRect] autorelease];

[groby-ooo-7-16, 2015/02/18 19:01:19]

Ideally, loadView does not reference [self view] - it can introduce circular
dependencies. I suggest fully building and setting at the end of -loadVIew

[bondd, 2015/02/26 01:25:08]

Done.

+
+  autofill::CardUnmaskPromptController* controller = bridge_->GetController();
+  if (!controller)
+    return;

[bondd, 2015/02/18 02:49:28]

Is this okay? Should I expose WeakPtr from CardUnmaskPromptControllerImpl
instead?

[groby-ooo-7-16, 2015/02/18 19:01:19]

I can't see any benefit from exposing the WeakPtr here. I'd prefer if we
reasoned properly about the lifetime of objects (and DCHECK'd those assumptions)
than adding lots of just-in-case early outs.

Under what circumstances can we ever hit loadView with a NULL controller?

[bondd, 2015/02/26 01:25:08]

Okay, assuming all of this code is run on the same thread (is that a valid
assumption?): controller will only be null after ControllerGone() is called.
ControllerGone() calls PerformClose(). loadView should not be called after
close, so loadView should never be called with a NULL controller. Added a DCHECK
to verify this assumption.

Please check my logic for the above paragraph...it's very possible I am missing
something. :)

+
+  // Title label.
+  NSTextField* title = constrained_window::CreateLabel();
+  [title setAttributedStringValue:constrained_window::GetAttributedLabelString(

[groby-ooo-7-16, 2015/02/18 19:01:19]

Is that clang-format? Because it looks horrible :)

[bondd, 2015/02/26 01:25:09]

Added an intermediate variable to make the formatting less grotesque to you. :)

+                                      SysUTF16ToNSString(
+                                          controller->GetWindowTitle()),
+                                      chrome_style::kTitleFontStyle,
+                                      NSNaturalTextAlignment,
+                                      NSLineBreakByWordWrapping)];
+  [title sizeToFit];
+  [[self view] addSubview:title];
+
+  // Instructions label.
+  NSTextField* instructions = constrained_window::CreateLabel();
+  [instructions
+      setAttributedStringValue:constrained_window::GetAttributedLabelString(
+                                   SysUTF16ToNSString(
+                                       controller->GetInstructionsMessage()),
+                                   chrome_style::kTextFontStyle,
+                                   NSNaturalTextAlignment,
+                                   NSLineBreakByWordWrapping)];
+  // No need to call sizeToFit here. Size is calculated later.
+  [[self view] addSubview:instructions];
+
+  base::scoped_nsobject<AutofillPopUpButton> monthPopup;

[groby-ooo-7-16, 2015/02/18 19:01:19]

Do you ever parent these popups to a view?

[bondd, 2015/02/26 01:25:08]

-addSubview is sent 5 lines below this comment. Or do you mean something else?

+  base::scoped_nsobject<AutofillPopUpButton> yearPopup;
+  if (controller->ShouldRequestExpirationDate()) {

[groby-ooo-7-16, 2015/02/18 19:01:19]

There are 3 different places that special-case ShouldRequestExpirationDate. Can
we roll this into one, for better readability?

[bondd, 2015/02/26 01:25:08]

I've reduced it to two places: once for creation and once for layout.

+    // Month.
+    monthPopup.reset(CreateDatePopup(month_combobox_model_));
+    [[self view] addSubview:monthPopup];
+
+    // Year.
+    yearPopup.reset(CreateDatePopup(year_combobox_model_));
+    [[self view] addSubview:yearPopup];
+  }
+
+  // CVC text input.
+  base::scoped_nsobject<AutofillTextField> cvcInput(

[groby-ooo-7-16, 2015/02/18 19:01:19]

Does this need to be an AutofillTextField?

[bondd, 2015/02/26 01:25:09]

Done.

+      [[AutofillTextField alloc] init]);

[groby-ooo-7-16, 2015/02/18 19:01:19]

You should prefer initWithFrame: over init - init is not an initializer on any
NSViews. (Yes, it still works, but please don't)

[bondd, 2015/02/26 01:25:08]

Done.

+  [[cvcInput cell]
+      setPlaceholderString:l10n_util::GetNSString(
+                               IDS_AUTOFILL_DIALOG_PLACEHOLDER_CVC)];
+  [[cvcInput cell] setScrollable:YES];
+  [cvcInput sizeToFit];
+  [cvcInput
+      setFrameSize:NSMakeSize(kCvcInputWidth, NSHeight([cvcInput frame]))];
+  [[self view] addSubview:cvcInput];
+
+  // CVC image.
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  NSImage* image =
+      rb.GetNativeImageNamed(controller->GetCvcImageRid()).ToNSImage();
 
-  base::scoped_nsobject<NSButton> button(
+  base::scoped_nsobject<NSImageView> cvcImage([[NSImageView alloc] init]);
+  [cvcImage setImage:image];
+
+  const CGFloat inputRowHeight = NSHeight([cvcInput frame]);

[bondd, 2015/02/18 02:49:28]

This assumes that cvcInput is the tallest element in the input row, and that the
date popups are the same hieght as cvcInput. That's a bad assumption right?
Should I instead calculate row height without making any assumptions about the
relative sizes of the elements? Just checking with you before I do it, in case
there are OSX or Chrome guarantees of element sizes.

[groby-ooo-7-16, 2015/02/18 19:01:19]

I have no idea what the guarantees are :) In general, don't we want image to
show at its natural height, centered with the text field?

[bondd, 2015/02/26 01:25:08]

Changed it to make no assumptions about element heights. inputRowSize.height is
set to the height of the tallest input row element, and all input row elements
are centered vertically in the row.

+  [cvcImage setFrameSize:NSMakeSize([image size].width, inputRowHeight)];
+  [[self view] addSubview:cvcImage];
+
+  // Cancel button.
+  base::scoped_nsobject<NSButton> cancelButton(
       [[ConstrainedWindowButton alloc] initWithFrame:NSZeroRect]);
-  [button setTitle:l10n_util::GetNSStringWithFixup(IDS_CANCEL)];
-  [button setKeyEquivalent:kKeyEquivalentEscape];
-  [button setTarget:self];
-  [button setAction:@selector(closeSheet:)];
-  [button sizeToFit];
-  [buttonContainer addSubview:button];
-
-  CGFloat nextX = NSMaxX([button frame]) + kButtonGap;
-  button.reset([[ConstrainedWindowButton alloc] initWithFrame:NSZeroRect]);
-  [button setFrameOrigin:NSMakePoint(nextX, 0)];
-  [button setTitle:l10n_util::GetNSStringWithFixup(
-                       IDS_AUTOFILL_DIALOG_SUBMIT_BUTTON)];
-  [button setKeyEquivalent:kKeyEquivalentReturn];
-  [button setTarget:self];
-  [button setAction:@selector(closeSheet:)];
-  [button sizeToFit];
-  [buttonContainer addSubview:button];
-
-  const CGFloat dialogOffset = NSWidth([[self window] frame]) -
-                               chrome_style::kHorizontalPadding -
-                               NSMaxX([button frame]);
-  [buttonContainer
-      setFrame:NSMakeRect(dialogOffset, chrome_style::kClientBottomPadding,
-                          NSMaxX([button frame]), NSMaxY([button frame]))];
-
-  [[[self window] contentView] addSubview:buttonContainer];
+  [cancelButton setTitle:l10n_util::GetNSStringWithFixup(IDS_CANCEL)];
+  [cancelButton setKeyEquivalent:kKeyEquivalentEscape];
+  [cancelButton setTarget:self];
+  [cancelButton setAction:@selector(closeSheet:)];
+  [cancelButton sizeToFit];
+  [[self view] addSubview:cancelButton];
+
+  // Verify button.
+  base::scoped_nsobject<NSButton> verifyButton(
+      [[ConstrainedWindowButton alloc] initWithFrame:NSZeroRect]);
+  // TODO(bondd): use l10n string.
+  [verifyButton setTitle:@"Verify"];
+  [verifyButton setKeyEquivalent:kKeyEquivalentReturn];
+  [verifyButton setTarget:self];
+  [verifyButton setAction:@selector(closeSheet:)];
+  [verifyButton sizeToFit];
+  [[self view] addSubview:verifyButton];
+
+  // Calculate dialog width.
+  CGFloat inputRowWidth =
+      NSWidth([cvcInput frame]) + kButtonGap + NSWidth([cvcImage frame]);
+  if (controller->ShouldRequestExpirationDate()) {
+    inputRowWidth += NSWidth([monthPopup frame]) + NSWidth([yearPopup frame]) +
+                     kButtonGap * 2;
+  }
+  CGFloat dialogWidth = std::max(NSWidth([title frame]), inputRowWidth) +
+                        chrome_style::kHorizontalPadding * 2;
+  dialogWidth = std::max(dialogWidth, kDialogMinWidth);
+
+  // Layout the elements, starting at the bottom and moving up.

[bondd, 2015/02/18 02:49:28]

Doing creation and layout in the same message, per several existing examples
like this one:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[groby-ooo-7-16, 2015/02/18 19:01:19]

Perfectly fine, unless you'll need to re-layout. You can always factor out at
that point.

+
+  CGFloat curX = dialogWidth - chrome_style::kHorizontalPadding;
+  CGFloat curY = chrome_style::kClientBottomPadding;
+
+  // Verify and Cancel buttons.
+  curX -= NSWidth([verifyButton frame]);
+  [verifyButton setFrameOrigin:NSMakePoint(curX, curY)];
+  curX -= kButtonGap + NSWidth([cancelButton frame]);
+  [cancelButton setFrameOrigin:NSMakePoint(curX, curY)];
+  curY += NSHeight([cancelButton frame]);
+
+  // Input row.
+  curX = chrome_style::kHorizontalPadding;
+  curY += chrome_style::kRowPadding;
+
+  if (controller->ShouldRequestExpirationDate()) {
+    [monthPopup setFrameOrigin:NSMakePoint(curX, curY)];
+    curX += NSWidth([monthPopup frame]) + kButtonGap;
+    [yearPopup setFrameOrigin:NSMakePoint(curX, curY)];
+    curX += NSWidth([yearPopup frame]) + kButtonGap;
+  }
+
+  [cvcInput setFrameOrigin:NSMakePoint(curX, curY)];
+  curX += NSWidth([cvcInput frame]) + kButtonGap;
+  [cvcImage setFrameOrigin:NSMakePoint(curX, curY)];
+  curX += NSWidth([cvcImage frame]);
+
+  // Instruction label.
+  curX = chrome_style::kHorizontalPadding;
+  curY += inputRowHeight + chrome_style::kRowPadding;
+  [instructions setFrameOrigin:NSMakePoint(curX, curY)];
+  CGFloat maxTextWidth = dialogWidth - chrome_style::kHorizontalPadding * 2;
+  NSSize instructionsSize = [[instructions cell]

[groby-ooo-7-16, 2015/02/18 19:01:19]

Why use cellsize? This ignores any chrome around the instructions? Also, can we
fix the height to not use CGFLOAT_MAX?

[bondd, 2015/02/26 01:25:08]

Instructions text needs to wrap onto multiple lines. cellSizeForBounds is used
to figure out how tall the instructions will be when the text is wrapped to
maxTextWidth.
CGFLOAT_MAX is used to indicate no maximum height; the cellSizeForBounds result
reduces it to the minimum height that still fits the wrapped text.

[groby-ooo-7-16, 2015/02/28 00:49:17]

Acknowledged.

+      cellSizeForBounds:NSMakeRect(0.0, 0.0, maxTextWidth, CGFLOAT_MAX)];
+  [instructions setFrameSize:instructionsSize];
+
+  // Title label.
+  curY += instructionsSize.height + chrome_style::kRowPadding;
+  [title setFrameOrigin:NSMakePoint(curX, curY)];
+
+  // Dialog size.
+  CGFloat dialogHeight =
+      curY + NSHeight([title frame]) + chrome_style::kTitleTopPadding;
+
+  [[self view] setFrame:NSMakeRect(0, 0, dialogWidth, dialogHeight)];
 }
 
 @end