Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
929293005:
Autofill: Add contents of CVC unmask prompt dialog on OSX. (Closed)
|
Created:
5 years, 10 months ago by bondd Modified:
5 years, 9 months ago CC:
chromium-reviews, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cup_01_initial_add Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutofill: Add contents of CVC unmask prompt dialog on OSX.
Dialog contents now match the Views implementation.
Dialog UI elements are not hooked up to CardUnmaskPromptController yet.
Screenshots and video attached to bug: https://code.google.com/p/chromium/issues/detail?id=448572#c3
BUG=448572
Committed: https://crrev.com/b297f8ae256ddc2c8740e18f1d48229eb7f16449
Cr-Commit-Position: refs/heads/master@{#319562}
Patch Set 1 #
Total comments: 35
Patch Set 2 : Address groby@ comments for patch set 1. #
Total comments: 23
Patch Set 3 : Address groby@ comments for patch set 2. #
Total comments: 23
Patch Set 4 : Address groby@ comments for patch set 3 - make layout code less verbose. #
Total comments: 6
Patch Set 5 : Fix failing browser test by calling test::SetCreditCardInfo. #
Total comments: 1
Messages
Total messages: 25 (8 generated)
bondd@chromium.org changed reviewers: + estade@chromium.org, groby@chromium.org
bondd@chromium.org changed required reviewers: + groby@chromium.org
estade@ indicated that the dialog contents and layout are not final yet and are likely to change, so I didn't spend much time polishing the layout. I'd like to get to functional parity with the Views dialog first, and then do another pass on the layout after feedback from UX designers. I'll separate CardUnmaskPromptViewCocoa into its own file in the next CL. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:123: return; Is this okay? Should I expose WeakPtr from CardUnmaskPromptControllerImpl instead? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:180: const CGFloat inputRowHeight = NSHeight([cvcInput frame]); 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. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:216: // Layout the elements, starting at the bottom and moving up. 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/...
https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:49: @interface CardUnmaskPromptViewCocoa : NSViewController<NSWindowDelegate> { Why is this a view controller now? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:56: autofill::MonthComboboxModel month_combobox_model_; Why are these members? Do they need to persist ag https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:26: AutofillPopUpButton* CreateDatePopup(ui::ComboboxModel& model) { FWIW, we usually have those builder functions as class methods on the cocoa object. I.e. + (AutofillPopUpButton*)buildDatePopupWithModel:(ui::ComboboxModel&); https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:27: AutofillPopUpButton* popup = why not use menuFromModel:? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:61: [[window contentView] addSubview:[view_controller_ view]]; Are you sure you don't just want to replace the contentView? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:119: self.view = [[[NSView alloc] initWithFrame:NSZeroRect] autorelease]; Ideally, loadView does not reference [self view] - it can introduce circular dependencies. I suggest fully building and setting at the end of -loadVIew https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:123: return; On 2015/02/18 02:49:28, bondd wrote: > Is this okay? Should I expose WeakPtr from CardUnmaskPromptControllerImpl > instead? 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? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:127: [title setAttributedStringValue:constrained_window::GetAttributedLabelString( Is that clang-format? Because it looks horrible :) https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:148: base::scoped_nsobject<AutofillPopUpButton> monthPopup; Do you ever parent these popups to a view? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:150: if (controller->ShouldRequestExpirationDate()) { There are 3 different places that special-case ShouldRequestExpirationDate. Can we roll this into one, for better readability? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:161: base::scoped_nsobject<AutofillTextField> cvcInput( Does this need to be an AutofillTextField? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:162: [[AutofillTextField alloc] init]); You should prefer initWithFrame: over init - init is not an initializer on any NSViews. (Yes, it still works, but please don't) https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:180: const CGFloat inputRowHeight = NSHeight([cvcInput frame]); On 2015/02/18 02:49:28, bondd wrote: > 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. 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? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:216: // Layout the elements, starting at the bottom and moving up. On 2015/02/18 02:49:28, bondd wrote: > 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/... Perfectly fine, unless you'll need to re-layout. You can always factor out at that point. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:249: NSSize instructionsSize = [[instructions cell] Why use cellsize? This ignores any chrome around the instructions? Also, can we fix the height to not use CGFLOAT_MAX?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:49: @interface CardUnmaskPromptViewCocoa : NSViewController<NSWindowDelegate> { On 2015/02/18 19:01:18, groby wrote: > Why is this a view controller now? You had previously suggested (over IM) that I should build everything in -loadView. NSWindowController doesn't have -loadView so I figured you meant to use NSViewController. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:56: autofill::MonthComboboxModel month_combobox_model_; On 2015/02/18 19:01:18, groby wrote: > Why are these members? Do they need to persist ag Done. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:26: AutofillPopUpButton* CreateDatePopup(ui::ComboboxModel& model) { On 2015/02/18 19:01:19, groby wrote: > FWIW, we usually have those builder functions as class methods on the cocoa > object. I.e. > > + (AutofillPopUpButton*)buildDatePopupWithModel:(ui::ComboboxModel&); Done. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:27: AutofillPopUpButton* popup = On 2015/02/18 19:01:19, groby wrote: > why not use menuFromModel:? 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? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:61: [[window contentView] addSubview:[view_controller_ view]]; On 2015/02/18 19:01:19, groby wrote: > Are you sure you don't just want to replace the contentView? Done. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:119: self.view = [[[NSView alloc] initWithFrame:NSZeroRect] autorelease]; On 2015/02/18 19:01:19, groby wrote: > Ideally, loadView does not reference [self view] - it can introduce circular > dependencies. I suggest fully building and setting at the end of -loadVIew Done. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:123: return; On 2015/02/18 19:01:19, groby wrote: > On 2015/02/18 02:49:28, bondd wrote: > > Is this okay? Should I expose WeakPtr from CardUnmaskPromptControllerImpl > > instead? > > 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? 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. :) https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:127: [title setAttributedStringValue:constrained_window::GetAttributedLabelString( On 2015/02/18 19:01:19, groby wrote: > Is that clang-format? Because it looks horrible :) Added an intermediate variable to make the formatting less grotesque to you. :) https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:148: base::scoped_nsobject<AutofillPopUpButton> monthPopup; On 2015/02/18 19:01:19, groby wrote: > Do you ever parent these popups to a view? -addSubview is sent 5 lines below this comment. Or do you mean something else? https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:150: if (controller->ShouldRequestExpirationDate()) { On 2015/02/18 19:01:19, groby wrote: > There are 3 different places that special-case ShouldRequestExpirationDate. Can > we roll this into one, for better readability? I've reduced it to two places: once for creation and once for layout. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:161: base::scoped_nsobject<AutofillTextField> cvcInput( On 2015/02/18 19:01:19, groby wrote: > Does this need to be an AutofillTextField? Done. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:162: [[AutofillTextField alloc] init]); On 2015/02/18 19:01:19, groby wrote: > You should prefer initWithFrame: over init - init is not an initializer on any > NSViews. (Yes, it still works, but please don't) Done. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:180: const CGFloat inputRowHeight = NSHeight([cvcInput frame]); On 2015/02/18 19:01:19, groby wrote: > On 2015/02/18 02:49:28, bondd wrote: > > 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. > > 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? 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. https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:249: NSSize instructionsSize = [[instructions cell] On 2015/02/18 19:01:19, groby wrote: > Why use cellsize? This ignores any chrome around the instructions? Also, can we > fix the height to not use CGFLOAT_MAX? 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.
https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:49: @interface CardUnmaskPromptViewCocoa : NSViewController<NSWindowDelegate> { On 2015/02/26 01:25:08, bondd wrote: > On 2015/02/18 19:01:18, groby wrote: > > Why is this a view controller now? > > You had previously suggested (over IM) that I should build everything in > -loadView. NSWindowController doesn't have -loadView so I figured you meant to > use NSViewController. Ah, my apologies. In general, if reality conflicts with me, reality is usually right :) https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:27: AutofillPopUpButton* popup = On 2015/02/26 01:25:08, bondd wrote: > On 2015/02/18 19:01:19, groby wrote: > > why not use menuFromModel:? > > 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? No, you don't. I naively assumed we had a menuFromModel for ComboboxModel as well :( https://codereview.chromium.org/929293005/diff/1/chrome/browser/ui/cocoa/auto... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:249: NSSize instructionsSize = [[instructions cell] On 2015/02/26 01:25:08, bondd wrote: > On 2015/02/18 19:01:19, groby wrote: > > Why use cellsize? This ignores any chrome around the instructions? Also, can > we > > fix the height to not use CGFLOAT_MAX? > > 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. Acknowledged. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:10: #include "chrome/browser/ui/autofill/autofill_dialog_models.h" Why include this? https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:152: // No need to call sizeToFit here. Size is calculated later. Feel free to kill this comment :) https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:229: chrome_style::kHorizontalPadding * 2; If you want to save yourself a whole lot of padding calculation, you could use an NSBoxView as the main view - it allows specifying a contentInset, which automatically maintains outer padding. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:232: // Layout the elements, starting at the bottom and moving up. "Starting at the bottom" is pretty much implied in this being Cocoa :) https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:234: CGFloat curX = dialogWidth - chrome_style::kHorizontalPadding; Since you keep converting this to a point anyways - any point making this NSPoint? https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:263: // Center cvcInput vertically in the input row. Does that ensure centering of popups as well? https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:265: curY + round((inputRowSize.height - NSHeight([cvcInput frame])) * 0.5); Why round, instead of ceil? https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:266: [cvcInput setFrameOrigin:NSMakePoint(curX, cvcInputY)]; setFrame:NSMakeRect(... https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:274: setFrameSize:NSMakeSize([cvcImage size].width, inputRowSize.height)]; setFrame:NSMakeRect(... https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:292: curY + NSHeight([title frame]) + chrome_style::kTitleTopPadding; That's NSMaxY([title frame]) + chrome_style::kTitleTopPadding https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:294: [mainView setFrame:NSMakeRect(0, 0, dialogWidth, dialogHeight)]; You can probably just to -setFrameSize Ther origin should already be 0
https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:10: #include "chrome/browser/ui/autofill/autofill_dialog_models.h" On 2015/02/28 00:49:17, groby wrote: > Why include this? Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:107: [popup setFrameSize:NSMakeSize(NSWidth([popup frame]), height)]; After combining setFrameOrigin + setFrameSize -> setFrame, this message becomes a one-liner. It's not much shorter than the caller just sending - setFrame itself, so I got rid of this message. This way the reader doesn't need to come find this definition to see what's happening. Easier to understand the code at a glance. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:152: // No need to call sizeToFit here. Size is calculated later. On 2015/02/28 00:49:17, groby wrote: > Feel free to kill this comment :) Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:229: chrome_style::kHorizontalPadding * 2; On 2015/02/28 00:49:17, groby wrote: > If you want to save yourself a whole lot of padding calculation, you could use > an NSBoxView as the main view - it allows specifying a contentInset, which > automatically maintains outer padding. Done. Now I use NSBox contentViewMargins for horizontal padding. Vertical padding is still done same as before, because AFAICT NSBox does not allow top and bottom padding to be different from each other. As it is now kTitleTopPadding and kClientBottomPadding are referenced exactly once each, so using NSBox for that would not get us much anyway. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:232: // Layout the elements, starting at the bottom and moving up. On 2015/02/28 00:49:17, groby wrote: > "Starting at the bottom" is pretty much implied in this being Cocoa :) Acknowledged. I saw it in profile_signin_confirmation_view_controller.mm and found it useful. It indicates to future authors that (a) things don't happen to be laid out all willy-nilly here for some reason, and that (b) they should keep doing it that way when they add stuff. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:234: CGFloat curX = dialogWidth - chrome_style::kHorizontalPadding; On 2015/02/28 00:49:17, groby wrote: > Since you keep converting this to a point anyways - any point making this > NSPoint? Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:263: // Center cvcInput vertically in the input row. On 2015/02/28 00:49:17, groby wrote: > Does that ensure centering of popups as well? Yes, popups will be centered vertically. Popups get centered vertically within their frame. Frame height is set to inputRowSize.height for both popups. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:265: curY + round((inputRowSize.height - NSHeight([cvcInput frame])) * 0.5); On 2015/02/28 00:49:17, groby wrote: > Why round, instead of ceil? Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:266: [cvcInput setFrameOrigin:NSMakePoint(curX, cvcInputY)]; On 2015/02/28 00:49:17, groby wrote: > setFrame:NSMakeRect(... Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:274: setFrameSize:NSMakeSize([cvcImage size].width, inputRowSize.height)]; On 2015/02/28 00:49:17, groby wrote: > setFrame:NSMakeRect(... Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:292: curY + NSHeight([title frame]) + chrome_style::kTitleTopPadding; On 2015/02/28 00:49:17, groby wrote: > That's NSMaxY([title frame]) + chrome_style::kTitleTopPadding Done. https://codereview.chromium.org/929293005/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:294: [mainView setFrame:NSMakeRect(0, 0, dialogWidth, dialogHeight)]; On 2015/02/28 00:49:17, groby wrote: > You can probably just to -setFrameSize > > Ther origin should already be 0 Done.
https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:169: // Dimensions. That is kind of awkward. Wrap the comboboxes in a separate view, then expirationDateSize == [expirationDateView frame].size Also: [monthPopup setFrameOrigin:NSMakePoint(0,0)] [yearPopup setFrameOrigin:NSMakePoint(NSMaxX([monthPopup frame]) + kButtonGap, 0); NSRect expirationSize = NSUnionRect([monthPopup frame], [yearPopup frame]; expirationSize.width += kButtonGap; [expirationView setFrameSize:expirationSize) This should do a way with a bunch of calculations further down. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:182: [[cvcInput cell] setScrollable:YES]; We know the maximum size of a CVC - can we just pick a fixed size instead of making this scrollable? https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:208: // TODO(bondd): use l10n string. Please do so :) https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:218: inputRowSize.width = kCvcInputWidth + kButtonGap + [cvcImage size].width + You can get that width by just positioning the elements and then getting NSMaxX - do you really need to pre-compute it before you laid out? https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:243: // Expiration date. This entire thing becomes [expirationView setFrameOrigin:pos]; pos.x += NSWidth([expirationView frame]); (You can send messages to nil objects. The result is nil. Or NSZeroRect for NSRect) https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:263: inputRowSize.height)]; Why not just use the original image size? And do a setFrameOrigin? https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:267: pos.x = 0.0; pos = NSMakePoint(0.0, NSMaxY([cvcImage frame]) + chrome_style::kRowPadding) This expresses the relationship to the previous element a bit more directly. In fact, you might even just pass this to setFrameOrigin. Here and elsewhere. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:275: pos.y += instructionsSize.height + chrome_style::kRowPadding; NSMakePoint(0.0, NSMaxY([instructions frame] + chrome_style::kRowPadding); https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:279: CGFloat dialogHeight = NSMaxY([title frame]) + chrome_style::kTitleTopPadding; Shouldn't the bottom have the same padding? In which case, the inset does this? https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:282: setFrameSize:NSMakeSize( [mainView sizeToFit] should do.
In general, the layout code is very verbose - can you compact that a bit?
Compacted layout code and made it easier to read and understand. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:169: // Dimensions. On 2015/03/03 18:06:54, groby wrote: > That is kind of awkward. Wrap the comboboxes in a separate view, then > expirationDateSize == [expirationDateView frame].size > > Also: > [monthPopup setFrameOrigin:NSMakePoint(0,0)] > [yearPopup setFrameOrigin:NSMakePoint(NSMaxX([monthPopup frame]) + kButtonGap, > 0); > NSRect expirationSize = NSUnionRect([monthPopup frame], [yearPopup frame]; > expirationSize.width += kButtonGap; > [expirationView setFrameSize:expirationSize) > > This should do a way with a bunch of calculations further down. Done. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:182: [[cvcInput cell] setScrollable:YES]; On 2015/03/03 18:06:54, groby wrote: > We know the maximum size of a CVC - can we just pick a fixed size instead of > making this scrollable? What I've got here is the same functionality as Views implementation. I'd rather have the two versions as similar as possible until the UI designers weigh in. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:208: // TODO(bondd): use l10n string. On 2015/03/03 18:06:54, groby wrote: > Please do so :) Same TODO exists in the Views implementation: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I believe we are waiting on UI designers before we add the l10n string. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:218: inputRowSize.width = kCvcInputWidth + kButtonGap + [cvcImage size].width + On 2015/03/03 18:06:54, groby wrote: > You can get that width by just positioning the elements and then getting NSMaxX > - do you really need to pre-compute it before you laid out? Done. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:243: // Expiration date. On 2015/03/03 18:06:54, groby wrote: > This entire thing becomes > > [expirationView setFrameOrigin:pos]; > pos.x += NSWidth([expirationView frame]); > > (You can send messages to nil objects. The result is nil. Or NSZeroRect for > NSRect) Done. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:263: inputRowSize.height)]; On 2015/03/03 18:06:54, groby wrote: > Why not just use the original image size? And do a setFrameOrigin? Done. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:267: pos.x = 0.0; On 2015/03/03 18:06:54, groby wrote: > > pos = NSMakePoint(0.0, NSMaxY([cvcImage frame]) + chrome_style::kRowPadding) > > This expresses the relationship to the previous element a bit more directly. In > fact, you might even just pass this to setFrameOrigin. > > Here and elsewhere. Done. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:275: pos.y += instructionsSize.height + chrome_style::kRowPadding; On 2015/03/03 18:06:54, groby wrote: > NSMakePoint(0.0, NSMaxY([instructions frame] + chrome_style::kRowPadding); Done. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:279: CGFloat dialogHeight = NSMaxY([title frame]) + chrome_style::kTitleTopPadding; On 2015/03/03 18:06:54, groby wrote: > Shouldn't the bottom have the same padding? In which case, the inset does this? AFAICT the padding is different for top and bottom: kTitleTopPadding == 15 kClientBottomPadding == 20 I'm using it the same way as this code: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... That's why I'm not using the NSBox inset for vertical spacing. Please let me know if I'm missing something here, or if the code I'm basing this on is not the way I should be doing this. https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:282: setFrameSize:NSMakeSize( On 2015/03/03 18:06:54, groby wrote: > [mainView sizeToFit] should do. See above comment - top and bottom padding are different. I tried doing [mainView sizeToFit] and then tweaking the vertical dimensions, but IMO it was harder to understand than what I've got here. https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:105: + (void)sizeToFitView:(NSView*)view { Moved inputRowView layout to these helper methods (-sizeToFitView and -verticallyCenterSubviewsInView). Added -sizeToFitView because there is no -sizeToFit for NSView. NSBox has -sizeToFit but it inserts extra view(s) between the NSBox and its contents, which makes verticallyCenterSubviewsInView a bit more complicated. Decided to err on the sides of using a simple class (NSView) instead of slightly heavier one (NSBox), since the # lines of code would probably be the same (NSBox requires more setup code). https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:113: + (void)verticallyCenterSubviewsInView:(NSView*)view { Is there an easier way to center elements vertically in a parent frame that I am missing? I looked in Apple docs and codesearch and didn't find anything. https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:150: base::scoped_nsobject<NSView> inputRowView( Gave input row its own NSView. I think it makes the code easier to read and understand.
https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:182: [[cvcInput cell] setScrollable:YES]; On 2015/03/04 20:45:30, bondd wrote: > On 2015/03/03 18:06:54, groby wrote: > > We know the maximum size of a CVC - can we just pick a fixed size instead of > > making this scrollable? > > What I've got here is the same functionality as Views implementation. I'd rather > have the two versions as similar as possible until the UI designers weigh in. I'm with Rachel here. UX designers aren't going to weigh in on every detail of this granularity. We should do the best we can on every platform. (I don't think Views has a way of limiting the length of the input.) https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:208: // TODO(bondd): use l10n string. On 2015/03/04 20:45:30, bondd wrote: > On 2015/03/03 18:06:54, groby wrote: > > Please do so :) > > Same TODO exists in the Views implementation: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > I believe we are waiting on UI designers before we add the l10n string. right. It's less a matter of "the value of this string may change" and more a matter of "the set of strings may change". So in many places I've punted on l10n for now.
lgtm https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:105: + (void)sizeToFitView:(NSView*)view { On 2015/03/04 20:45:30, bondd wrote: > Moved inputRowView layout to these helper methods (-sizeToFitView and > -verticallyCenterSubviewsInView). > > Added -sizeToFitView because there is no -sizeToFit for NSView. NSBox has > -sizeToFit but it inserts extra view(s) between the NSBox and its contents, > which makes verticallyCenterSubviewsInView a bit more complicated. Decided to > err on the sides of using a simple class (NSView) instead of slightly heavier > one (NSBox), since the # lines of code would probably be the same (NSBox > requires more setup code). Acknowledged. https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:113: + (void)verticallyCenterSubviewsInView:(NSView*)view { On 2015/03/04 20:45:30, bondd wrote: > Is there an easier way to center elements vertically in a parent frame that I am > missing? I looked in Apple docs and codesearch and didn't find anything. I don't think there is. https://codereview.chromium.org/929293005/diff/100001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:150: base::scoped_nsobject<NSView> inputRowView( On 2015/03/04 20:45:31, bondd wrote: > Gave input row its own NSView. I think it makes the code easier to read and > understand. Acknowledged.
https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/929293005/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:182: [[cvcInput cell] setScrollable:YES]; On 2015/03/05 02:38:02, Evan Stade wrote: > On 2015/03/04 20:45:30, bondd wrote: > > On 2015/03/03 18:06:54, groby wrote: > > > We know the maximum size of a CVC - can we just pick a fixed size instead of > > > making this scrollable? > > > > What I've got here is the same functionality as Views implementation. I'd > rather > > have the two versions as similar as possible until the UI designers weigh in. > > I'm with Rachel here. UX designers aren't going to weigh in on every detail of > this granularity. We should do the best we can on every platform. (I don't think > Views has a way of limiting the length of the input.) Apparently it is not trivial to do in OSX either: http://lists.apple.com/archives/cocoa-dev/2013/Oct/msg00615.html Unless someone objects I am going to leave this for a future CL. It doesn't make sense to me that we should hold up this CL and other basic dialog functionality to write a custom NSFormatter at this point. There are bigger fish to fry. :) https://codereview.chromium.org/929293005/diff/120001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc (right): https://codereview.chromium.org/929293005/diff/120001/chrome/browser/ui/autof... chrome/browser/ui/autofill/card_unmask_prompt_view_browsertest.cc:105: credit_card.SetTypeForMaskedCard(kMasterCard); estade@ emailed me this as a fix for the broken browser test. Looked fine to me so I copied it in verbatim.
lgtm
The CQ bit was checked by bondd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/929293005/#ps120001 (title: "Fix failing browser test by calling test::SetCreditCardInfo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929293005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by bondd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929293005/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b297f8ae256ddc2c8740e18f1d48229eb7f16449 Cr-Commit-Position: refs/heads/master@{#319562} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
