Update Android card unmasking prompt for the expiration date case.

Update Android card unmasking prompt for the expiration date case.

Also fix up some padding so the demo looks better

BUG=451654
Committed: https://crrev.com/4c240bbb5bbd0d4a7843d5a04de2e78aefbaeb07
Cr-Commit-Position: refs/heads/master@{#313975}

[Evan Stade, 2015-01-28 23:06:06]

estade@chromium.org changed reviewers:
+ brettw@chromium.org, newt@chromium.org

[newt (away), 2015-01-29 04:42:28]

https://codereview.chromium.org/873313011/diff/1/ui/android/java/res/layout/a...
File ui/android/java/res/layout/autofill_card_unmask_prompt.xml (right):

https://codereview.chromium.org/873313011/diff/1/ui/android/java/res/layout/a...
ui/android/java/res/layout/autofill_card_unmask_prompt.xml:33:
android:layout_height="match_parent"
Height should be wrap_content. Same below.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java (right):

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:132: //
TODO(estade): UI decision - should we clear the input?
IMO, no. I'd leave the input so the user can see and correct the error.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:161:
ArrayAdapter<CharSequence> adapter = new ArrayAdapter<CharSequence>(
Reusing this variable makes the code a bit harder for me to read. I'd just use
two separate variables: monthAdapter and yearAdapter.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:165:
adapter.add("MM");
You could follow the UI design used in the autofill profile editor: have a
little label above the spinner

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:197: private
boolean inputsAreValid() {
I'd call this "areInputsValid()". The current name almost sounds like a royal
decree to me :)

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:198: return
mDelegate.checkUserInputValidity(getCardUnmaskInput().getText().toString())
Maybe easier to read:

    if (mShouldRequestExpirationDate) {
        if (getMonthSpinner().getSelectedItemPosition() == 0
            || getYearSpinner().getSelectedItemPosition() == 0)) {
            return false;
        }
    }
    return
mDelegate.checkUserInputValidity(getCardUnmaskInput().getText().toString());

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:205: return
(EditText) mDialog.findViewById(R.id.card_unmask_input);
A more typical pattern is have a member variable for each interesting View, and
initialize those variables as soon as the view is constructed. Later, you can
simply use mMonthSpinner instead of getMonthSpinner().

[Evan Stade, 2015-01-30 02:13:24]

https://codereview.chromium.org/873313011/diff/1/ui/android/java/res/layout/a...
File ui/android/java/res/layout/autofill_card_unmask_prompt.xml (right):

https://codereview.chromium.org/873313011/diff/1/ui/android/java/res/layout/a...
ui/android/java/res/layout/autofill_card_unmask_prompt.xml:33:
android:layout_height="match_parent"
On 2015/01/29 04:42:27, newt wrote:
> Height should be wrap_content. Same below.

Done.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java (right):

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:132: //
TODO(estade): UI decision - should we clear the input?
On 2015/01/29 04:42:28, newt wrote:
> IMO, no. I'd leave the input so the user can see and correct the error.

this is mostly a note to myself to check with ainslie or someone about what
they'd like

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:161:
ArrayAdapter<CharSequence> adapter = new ArrayAdapter<CharSequence>(
On 2015/01/29 04:42:28, newt wrote:
> Reusing this variable makes the code a bit harder for me to read. I'd just use
> two separate variables: monthAdapter and yearAdapter.

Done.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:165:
adapter.add("MM");
On 2015/01/29 04:42:28, newt wrote:
> You could follow the UI design used in the autofill profile editor: have a
> little label above the spinner

I don't think that fits nicely into the mocks that natsant has created. Granted,
she also wanted EditText, but I think Spinner is less problematic. This will all
go through UI review at some point, but blocking each tiny decision like this on
designer feedback (which often lags) would lead to very slow progress.

The general pattern of "dummy value as placeholder text/default option" is used
in chrome://settings/autofill.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:197: private
boolean inputsAreValid() {
On 2015/01/29 04:42:28, newt wrote:
> I'd call this "areInputsValid()". The current name almost sounds like a royal
> decree to me :)

Done.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:198: return
mDelegate.checkUserInputValidity(getCardUnmaskInput().getText().toString())
On 2015/01/29 04:42:28, newt wrote:
> Maybe easier to read:
> 
>     if (mShouldRequestExpirationDate) {
>         if (getMonthSpinner().getSelectedItemPosition() == 0
>             || getYearSpinner().getSelectedItemPosition() == 0)) {
>             return false;
>         }
>     }
>     return
> mDelegate.checkUserInputValidity(getCardUnmaskInput().getText().toString());

not a huge fan of nested ifs in place of &&, but I partially applied this
suggestion.

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:205: return
(EditText) mDialog.findViewById(R.id.card_unmask_input);
On 2015/01/29 04:42:28, newt wrote:
> A more typical pattern is have a member variable for each interesting View,
and
> initialize those variables as soon as the view is constructed. Later, you can
> simply use mMonthSpinner instead of getMonthSpinner().

Done.

[newt (away), 2015-01-30 16:29:00]

lgtm

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
File ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java (right):

https://codereview.chromium.org/873313011/diff/1/ui/android/java/src/org/chro...
ui/android/java/src/org/chromium/ui/autofill/CardUnmaskPrompt.java:132: //
TODO(estade): UI decision - should we clear the input?
On 2015/01/30 02:13:23, Evan Stade wrote:
> On 2015/01/29 04:42:28, newt wrote:
> > IMO, no. I'd leave the input so the user can see and correct the error.
> 
> this is mostly a note to myself to check with ainslie or someone about what
> they'd like

I'm aware. Just providing another opinion in case that matters :)

[brettw, 2015-01-30 18:46:21]

lgtm

[Evan Stade, 2015-01-30 19:08:08]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-01-30 19:09:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873313011/20001

[commit-bot: I haz the power, 2015-01-30 20:18:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-30 20:20:35]

Patchset 2 (id:??) landed as
https://crrev.com/4c240bbb5bbd0d4a7843d5a04de2e78aefbaeb07
Cr-Commit-Position: refs/heads/master@{#313975}