Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1024553004:
Autofill OSX: Add RetriableErrorMessage text label. (Closed)
DescriptionAutofill OSX: Add RetriableErrorMessage text label.
Add error message text label between CVC input row and Verify/Cancel
buttons. Error messages passed to
CardUnmaskPromptViewBridge::GotVerificationResult() are displayed.
Dialog will re-layout to fit entire message if a multiline message is
set. Previous to this CL the layout was done exactly once per dialog,
upon dialog creation. This CL refactors the layout code out into
-performLayout and calls it whenever the error message text changes.
Screenshot attached to bug: https://code.google.com/p/chromium/issues/detail?id=448572#c14
BUG=448572
Committed: https://crrev.com/a8b89aa94a3320ee053d0f24869835914742c860
Cr-Commit-Position: refs/heads/master@{#322262}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address groby@ comments on patch set 1. #
Total comments: 2
Patch Set 3 : Remove -sizeTextField TODO comment. #
Messages
Total messages: 15 (4 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/1024553004/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:93: [view_controller_ setRetriableErrorMessage:error_message]; setRetriableErrorMessage is same method name as in Views implementation, for consistency.
https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:58: - (void)setRetriableErrorMessage:(const base::string16&)text; Retryable. (Ideally, just setErrorMessage - if you can't retry it, probably setFatalErrorMessage, if we must distinguish) https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:93: [view_controller_ setRetriableErrorMessage:error_message]; On 2015/03/23 21:27:55, bondd wrote: > setRetriableErrorMessage is same method name as in Views implementation, for > consistency. Can we fix the views version too, then? https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:117: base::scoped_nsobject<NSView> expirationView_; This is never used outside -loadView, so no need to make it an ivar. https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:281: contentWidth = std::max(contentWidth, kDialogContentMinWidth); This is weird to me - we only invoke this after init, or when the error message has changed. And yet, the frame size is not dependent on the error message? https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:283: [storageView_ At this point, the layout is sufficiently complex that a small ASCII diagram would be awesome. (Disregard if subject to frequent changes for now, but please let's make sure that's in here before we're done. TODO, if you must) https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:295: // Error message label. Please do use a complete sentence, here and elsewhere. Even if it's just "Lay out error message label." https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:299: cellSizeForBounds:NSMakeRect(0, 0, contentWidth, CGFLOAT_MAX)]; We do this in several places - probably worth hoisting into base https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:473: [self performLayout]; At this point -loadView will automatically display the window. Is that intentional?
https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:58: - (void)setRetriableErrorMessage:(const base::string16&)text; On 2015/03/24 01:26:49, groby wrote: > Retryable. (Ideally, just setErrorMessage - if you can't retry it, probably > setFatalErrorMessage, if we must distinguish) what's wrong with retriable? Neither retriable nor retryable is in my spellcheck dictionary but both of them show up with decent frequency in a google search.
https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h (right): https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.h:58: - (void)setRetriableErrorMessage:(const base::string16&)text; On 2015/03/24 16:07:40, Evan Stade wrote: > On 2015/03/24 01:26:49, groby wrote: > > Retryable. (Ideally, just setErrorMessage - if you can't retry it, probably > > setFatalErrorMessage, if we must distinguish) > > what's wrong with retriable? Neither retriable nor retryable is in my spellcheck > dictionary but both of them show up with decent frequency in a google search. Maybe just my preference? For my searches, retryable outnumbers retriable 3:1, though. (And triable refers to legal trials, AIUI). But if everybody is attached to retriable, fine. I think this particular shed is fine in multiple colors :)
https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:93: [view_controller_ setRetriableErrorMessage:error_message]; On 2015/03/24 01:26:49, groby wrote: > On 2015/03/23 21:27:55, bondd wrote: > > setRetriableErrorMessage is same method name as in Views implementation, for > > consistency. > > Can we fix the views version too, then? Leaving shed this color per discussion elsewhere in this patch set's review comments. :) https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:117: base::scoped_nsobject<NSView> expirationView_; On 2015/03/24 01:26:49, groby wrote: > This is never used outside -loadView, so no need to make it an ivar. Done. https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:281: contentWidth = std::max(contentWidth, kDialogContentMinWidth); On 2015/03/24 01:26:49, groby wrote: > This is weird to me - we only invoke this after init, or when the error message > has changed. And yet, the frame size is not dependent on the error message? Dialog width stays the same, but dialog height changes depending on error message. See [[self view] setFrameSize:...] near the bottom of this method. I could calculate contentWidth in -loadView, but it makes more logical sense to put it in -performLayout, IMO. Saves keeping it in an ivar too. https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:283: [storageView_ On 2015/03/24 01:26:49, groby wrote: > At this point, the layout is sufficiently complex that a small ASCII diagram > would be awesome. (Disregard if subject to frequent changes for now, but please > let's make sure that's in here before we're done. TODO, if you must) Done. https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:295: // Error message label. On 2015/03/24 01:26:49, groby wrote: > Please do use a complete sentence, here and elsewhere. Even if it's just "Lay > out error message label." Done. Removed some of these comments because code is easier to read and self-explanatory now that -cellSizeForBounds has been pulled out into its own method. Turned remaining comments into sentences. https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:299: cellSizeForBounds:NSMakeRect(0, 0, contentWidth, CGFLOAT_MAX)]; On 2015/03/24 01:26:50, groby wrote: > We do this in several places - probably worth hoisting into base I hoisted it to the top of this class, with a TODO to move it to base. IMO moving it to base right now (and writing associated unit test) has too much potential to distract from M43 branch deadline target. https://codereview.chromium.org/1024553004/diff/1/chrome/browser/ui/cocoa/aut... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:473: [self performLayout]; On 2015/03/24 01:26:49, groby wrote: > At this point -loadView will automatically display the window. Is that > intentional? Done.
lgtm % nit https://codereview.chromium.org/1024553004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1024553004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:154: // TODO(bondd): Hoist this to ui/base/cocoa. You can kill the TODO - see other CL re l10::SizeToFitView()
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/1024553004/#ps40001 (title: "Remove -sizeTextField TODO comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1024553004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a8b89aa94a3320ee053d0f24869835914742c860 Cr-Commit-Position: refs/heads/master@{#322262}
Message was sent while issue was closed.
https://codereview.chromium.org/1024553004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): https://codereview.chromium.org/1024553004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:154: // TODO(bondd): Hoist this to ui/base/cocoa. On 2015/03/25 20:51:53, groby wrote: > You can kill the TODO - see other CL re l10::SizeToFitView() Done.
Message was sent while issue was closed.
On 2015/03/26 00:05:11, bondd wrote: > https://codereview.chromium.org/1024553004/diff/20001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm (right): > > https://codereview.chromium.org/1024553004/diff/20001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/autofill/card_unmask_prompt_view_bridge.mm:154: // > TODO(bondd): Hoist this to ui/base/cocoa. > On 2015/03/25 20:51:53, groby wrote: > > You can kill the TODO - see other CL re l10::SizeToFitView() > > Done. BTW, next patch set to https://codereview.chromium.org/1038503003/ removes +sizeTextField: completely and replaces it with WrapOrSizeToFit(). I will be uploading it momentarily. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
