Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
936293002:
Change expiration date spinners to text inputs, as per mocks. (Closed)
|
Created:
5 years, 10 months ago by Evan Stade Modified:
5 years, 10 months ago Reviewers:
newt (away) CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncard unmasking prompt - Change expiration date spinners to text inputs, as per mocks.
In the context of a dialog, inputs are preferred over spinners, because it's bad to show popups on top of popups.
BUG=437116
Committed: https://crrev.com/c9161dcf6c0618d8c6bf7741609e19b4ddb84fc8
Cr-Commit-Position: refs/heads/master@{#317167}
Patch Set 1 #
Total comments: 7
Patch Set 2 : remove setInputError(null) #Patch Set 3 : newt review #
Total comments: 6
Patch Set 4 : einliner #Patch Set 5 : work for year=2098 (I hope I live to see the day) #
Messages
Total messages: 17 (5 generated)
estade@chromium.org changed reviewers: + newt@chromium.org
https://codereview.chromium.org/936293002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:272: input.getBackground().mutate().setColorFilter(filter); So I flashed my device to a newer version of L and now setting filter to null works as expected.
https://codereview.chromium.org/936293002/diff/1/chrome/android/java/res/layo... File chrome/android/java/res/layout/autofill_card_unmask_prompt.xml (right): https://codereview.chromium.org/936293002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:49: android:gravity="center_horizontal" How does center_horizontal look when typing? Do the characters moves as you type? https://codereview.chromium.org/936293002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:57: android:text="/" /> I think we might want to localize this string. https://codereview.chromium.org/936293002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:209: Calendar calendar = Calendar.getInstance(); Creating a Calendar is rather expensive (yes, Calendar.getInstance() creates a new Calendar). I'd save the calendar as a member variable and reuse it. Or just save the initialYear as a member variable.
https://codereview.chromium.org/936293002/diff/1/chrome/android/java/res/layo... File chrome/android/java/res/layout/autofill_card_unmask_prompt.xml (right): https://codereview.chromium.org/936293002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:49: android:gravity="center_horizontal" On 2015/02/19 19:28:37, newt wrote: > How does center_horizontal look when typing? Do the characters moves as you > type? Yes, they do, but it doesn't look bad to me. https://codereview.chromium.org/936293002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/autofill_card_unmask_prompt.xml:57: android:text="/" /> On 2015/02/19 19:28:37, newt wrote: > I think we might want to localize this string. We might --- I'm not sure. But we don't do so in autofill currently. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/936293002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:209: Calendar calendar = Calendar.getInstance(); On 2015/02/19 19:28:37, newt wrote: > Creating a Calendar is rather expensive (yes, Calendar.getInstance() creates a > new Calendar). I'd save the calendar as a member variable and reuse it. Or just > save the initialYear as a member variable. Done.
lgtm https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:40: private final int mThisYear; It seems a little cleaner to let mThisYear hold the current year (e.g. 2015), then add 2000 to the value the user inputted (e.g. 17) before checking if it's within 10 of the current year. https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:110: mThisYear = calendar.get(Calendar.YEAR) % 100; could be a one-liner
New patchsets have been uploaded after l-g-t-m from newt@chromium.org
https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:40: private final int mThisYear; On 2015/02/19 21:55:50, newt wrote: > It seems a little cleaner to let mThisYear hold the current year (e.g. 2015), > then add 2000 to the value the user inputted (e.g. 17) before checking if it's > within 10 of the current year. The only difference I can see is that the current code works for any century, and the suggested code only works for the 21st century. I don't think this code needs to work in 100 years' time, but I'm not sure what the advantage is to changing it. https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:110: mThisYear = calendar.get(Calendar.YEAR) % 100; On 2015/02/19 21:55:50, newt wrote: > could be a one-liner Done.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/936293002/60001
https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:40: private final int mThisYear; On 2015/02/19 22:13:07, Evan Stade wrote: > On 2015/02/19 21:55:50, newt wrote: > > It seems a little cleaner to let mThisYear hold the current year (e.g. 2015), > > then add 2000 to the value the user inputted (e.g. 17) before checking if it's > > within 10 of the current year. > > The only difference I can see is that the current code works for any century, > and the suggested code only works for the 21st century. I don't think this code > needs to work in 100 years' time, but I'm not sure what the advantage is to > changing it. Mainly that mThisYear has the value that one might guess based on its name. And the logic that makes assumptions about 2-digit years is centralized in one place: in areInputsValid().
Also, neither option works around the turn of a century. But my suggestion would be easier to adapt to that scenario. Doesn't really matter, though; it's just a suggestion.
The CQ bit was unchecked by estade@chromium.org
https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/936293002/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:40: private final int mThisYear; On 2015/02/19 22:18:24, newt wrote: > On 2015/02/19 22:13:07, Evan Stade wrote: > > On 2015/02/19 21:55:50, newt wrote: > > > It seems a little cleaner to let mThisYear hold the current year (e.g. > 2015), > > > then add 2000 to the value the user inputted (e.g. 17) before checking if > it's > > > within 10 of the current year. > > > > The only difference I can see is that the current code works for any century, > > and the suggested code only works for the 21st century. I don't think this > code > > needs to work in 100 years' time, but I'm not sure what the advantage is to > > changing it. > > Mainly that mThisYear has the value that one might guess based on its name. And > the logic that makes assumptions about 2-digit years is centralized in one > place: in areInputsValid(). updated to work around the turn of the century and to make mThisYear the full year.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/936293002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c9161dcf6c0618d8c6bf7741609e19b4ddb84fc8 Cr-Commit-Position: refs/heads/master@{#317167} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
