Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1014683007:
Autofill OSX: Add "Verifying card" / "Your card is verified" status overlay. (Closed)
DescriptionAutofill OSX: Add "Verifying card" / "Your card is verified" status overlay.
"Verifying card" status overlay is displayed while the dialog is waiting
for card verification. Once the card has been verified "Your card is
verified" is displayed for one second and then the dialog closes.
If the card was successfully verified then the card data on the form
will be filled.
This CL is not pixel-perfect with the UI mocks. That will come in a
later CL.
No error messages are displayed on card verification failure. Coming
soon in another CL.
Video attached to bug: https://code.google.com/p/chromium/issues/detail?id=448572#c11
BUG=448572
Committed: https://crrev.com/df4f75d1b9f3b1f32f90303283f3724b0c42e5b5
Cr-Commit-Position: refs/heads/master@{#321499}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address groby@ comments on patch set 1. #
Total comments: 6
Patch Set 3 : Delete dialog properly + simplify text height calculation. #
Total comments: 3
Patch Set 4 : Rebase. #
Messages
Total messages: 17 (6 generated)
bondd@chromium.org changed reviewers: + estade@chromium.org, groby@chromium.org
bondd@chromium.org changed required reviewers: + groby@chromium.org
https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:84: base::TimeDelta::FromSeconds(1)); This call to PostDelayedTask is copied from the Views implementation.
https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:56: - (void)setProgressOverlayHidden:(BOOL)hidden; Can we make do with a single entry point, instead of 3? When we have an overlay text, the input should be hidden, the overlay visible, and the text displayed. If the text is nil, the input should be visible and the overlay hidden. All that is achieved with a single setProgressOverlayText:(NSString*) Also: This is an API that's private to the class - why expose it in headers? https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:117: base::scoped_nsobject<NSView> progressOverlay_; Why a separate view and not just a text field? https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:393: [self createProgressOverlay]; In case it becomes a single view, doing this inline might be a good idea.
https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:56: - (void)setProgressOverlayHidden:(BOOL)hidden; On 2015/03/18 07:10:05, groby wrote: > Can we make do with a single entry point, instead of 3? When we have an overlay > text, the input should be hidden, the overlay visible, and the text displayed. > > If the text is nil, the input should be visible and the overlay hidden. All that > is achieved with a single setProgressOverlayText:(NSString*) > > Also: This is an API that's private to the class - why expose it in headers? Reduced it to one entry point as suggested. API is public because it is accessed from CardUnmaskPromptViewBridge. CardUnmaskPromptViewCocoa will be moving to its own .h and .mm files soon. https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:117: base::scoped_nsobject<NSView> progressOverlay_; On 2015/03/18 07:10:05, groby wrote: > Why a separate view and not just a text field? Done. https://codereview.chromium.org/1014683007/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:393: [self createProgressOverlay]; On 2015/03/18 07:10:05, groby wrote: > In case it becomes a single view, doing this inline might be a good idea. Done. I also did an alternate implementation where progressOverlayText_ init is done in setProgressOverlayText once there is a text string to set. Avoids getting font height from NSLayoutManager, but the calculations for the progressOverlayText_ frame dimensions are a bit more complicated, and get done every time the text is changed. I don't think either way has a clear advantage.
https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:81: base::Unretained(this)), Unretained this? What happens if you close the dialog before the task lands? https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:376: base::scoped_nsobject<NSLayoutManager> layoutManager( Whoa, that's a lot of code :) Why not progressFont.GetHeight()? https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:380: [progressOverlayText_ setFrame:NSMakeRect(0, ceil(NSMidY([inputRow_ frame]) - That's an interesting calculation... I'm not sure I follow. why midY - (progressHeight/2)? (Also, what's wrong with dividing by 2? :)
https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:81: base::Unretained(this)), On 2015/03/19 05:15:38, groby wrote: > Unretained this? What happens if you close the dialog before the task lands? Done. Same solution as estade@'s here: https://codereview.chromium.org/1019143003/ https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:376: base::scoped_nsobject<NSLayoutManager> layoutManager( On 2015/03/19 05:15:38, groby wrote: > Whoa, that's a lot of code :) > > Why not progressFont.GetHeight()? Done. https://codereview.chromium.org/1014683007/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:380: [progressOverlayText_ setFrame:NSMakeRect(0, ceil(NSMidY([inputRow_ frame]) - On 2015/03/19 05:15:38, groby wrote: > That's an interesting calculation... I'm not sure I follow. why midY - > (progressHeight/2)? > > (Also, what's wrong with dividing by 2? :) Done. Answered in comment. Is there an easier way to center text vertically within a frame? Or a more standard idiom for doing the calculations? IIRC we've had this discussion on a CL before and there is no nice easy way. This is part of the reason why the original patch set wrapped the NSTextField with an NSView - I could just call +verticallyCenterSubviewsInView and the ugly math would be hidden away in that helper method. https://codereview.chromium.org/1014683007/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:92: base::MessageLoop::current()->DeleteSoon(FROM_HERE, this); CardUnmaskPromptViewBridge was not getting deleted. It owns itself per https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... DeleteSoon() looks to be the standard way that self-owned dialogs delete themselves on OSX.
lgtm https://codereview.chromium.org/1014683007/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1014683007/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:92: base::MessageLoop::current()->DeleteSoon(FROM_HERE, this); On 2015/03/19 21:43:37, bondd wrote: > CardUnmaskPromptViewBridge was not getting deleted. > It owns itself per > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > DeleteSoon() looks to be the standard way that self-owned dialogs delete > themselves on OSX. Dialogs should not be self-owned (controller could hold ownership just fine), but that's religious debate outside scope for this CL :) https://codereview.chromium.org/1014683007/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:378: [progressOverlayText_ setFrame:NSMakeRect(0, ceil(NSMidY([inputRow_ frame]) - One would think we have something like that in base/ One would be wrong, which makes me sad... Oh well.
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/1014683007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/1014683007/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1014683007/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/df4f75d1b9f3b1f32f90303283f3724b0c42e5b5 Cr-Commit-Position: refs/heads/master@{#321499} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
