Edit contacts UI for PaymentRequest

Edit contacts UI for PaymentRequest

BUG=617195, 621586
Committed: https://crrev.com/6c669e362933a62f3a3bf3714dbb405c1dc14277
Cr-Commit-Position: refs/heads/master@{#401350}

[please use gerrit instead, 2016-06-19 19:39:01]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-06-19 19:39:06]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2016-06-19 19:39:14]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2016-06-19 20:02:12]

Patchset #1 (id:60001) has been deleted

[please use gerrit instead, 2016-06-19 20:02:57]

Patchset #1 (id:80001) has been deleted

[please use gerrit instead, 2016-06-19 22:12:44]

Patchset #1 (id:100001) has been deleted

[please use gerrit instead, 2016-06-19 23:38:31]

Patchset #1 (id:120001) has been deleted

[please use gerrit instead, 2016-06-20 01:26:25]

Patchset #2 (id:160001) has been deleted

[please use gerrit instead, 2016-06-20 01:26:32]

Patchset #1 (id:140001) has been deleted

[please use gerrit instead, 2016-06-20 03:26:28]

Patchset #1 (id:180001) has been deleted

[please use gerrit instead, 2016-06-20 03:26:55]

Description was changed from

==========
Edit contacts UI

BUG=617195
==========

to

==========
Edit contacts UI for PaymentRequest

BUG=617195
==========

[please use gerrit instead, 2016-06-20 14:35:52]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[please use gerrit instead, 2016-06-20 14:35:53]

dfalcantara@, ptal.

Video of this implementation in
https://drive.google.com/open?id=0BwnAxYoFTeYPWXF4bTg3SldJMkU

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:703:
PhoneNumberUtils.stripSeparators(value.toString()));
Is there a better way to validate phone numbers? This is too permissive in my
experience.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:699:
for (int i = 0; i < mOptionRows.size(); i++) {
The split of the single for-loop into two for-loops is to improve radio button
UX when adding contact information. I am not too attached to this approach. If
you think it looked better before, we can change it back.

[please use gerrit instead, 2016-06-20 14:36:04]

Description was changed from

==========
Edit contacts UI for PaymentRequest

BUG=617195
==========

to

==========
Edit contacts UI for PaymentRequest

BUG=617195
==========

[please use gerrit instead, 2016-06-20 17:46:41]

Description was changed from

==========
Edit contacts UI for PaymentRequest

BUG=617195
==========

to

==========
Edit contacts UI for PaymentRequest

BUG=617195, 621586
==========

[gone, 2016-06-21 06:04:39]

Was fairly lax on the UI review because it's an intermediary handoff point.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:51:
* Updates the email address. This influences the label and sublabel.
nit: Say more specifically that the phone number is displayed as the main label?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:57:
setLabel(TextUtils.isEmpty(mPayerPhone) ? mPayerEmail : mPayerPhone);
Pull this out into an updateLabels() function and make setPayerPhone() call it?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:93:
public void setComplete() {
nit: setIsComplete?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:241:
int topCompleteContactIndex = SectionInformation.NO_SELECTION;
nit: firstCompleteContactIndex?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:246:
? profile.getEmailAddress()
nit: I don't think we're supposed to break on the colons

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:272:

// The contact section automatically selects the first complete entry.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:340:
private boolean isCompleteContactInformation(String email, String phone) {
isContactInformationComplete?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:596:
public boolean onSectionOptionChanged(@PaymentRequestUI.DataType int optionType,
We should probably rename this function now.  In this case, the option hasn't
been changed, but a new one has been selected.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:664:
: new AutofillProfile("" /* guid */, AutofillPreferences.SETTINGS_ORIGIN,
You should just have an empty AutofillProfile constructor.  Passing in a bunch
of empty strings and then describing what each empty string is seems kind of
silly.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:703:
PhoneNumberUtils.stripSeparators(value.toString()));
On 2016/06/20 14:35:53, Rouslan (ツ) wrote:
> Is there a better way to validate phone numbers? This is too permissive in my
> experience.

Haven't had to do it before, so I wouldn't know :/

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:30:
* Indicates a phone field.
Save some space:

/** Indicates a phone field. */

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:54:
* @param suggestions          Optional list of value to suggest to the user.
values

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:63:
EditorFieldValidator validator, CharSequence requiredErrorMessage,
If requiredErrorMessage and invalidErrorMessage are optional, add @Nullable?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:114:
return mErrorMessage == null;
Little obtuse.  Gain some readability by making it more verbose and symmetric:

if (TextUtils.isEmpty(mValue) || TextUtils.getTrimmedLength(mValue) == 0) {
    mErrorMessage = mRequiredErrorMessage;
    return false;
}

if (mValidator != null && !mValidator.isValid(mValue)) {
    mErrorMessage = mInvalidErrorMessage;
    return false;
}

mErrorMessage = null;
return true;

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:120:
* @return The error message for the last validation. Can be null.
Can be null if no error was reported.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorModel.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorModel.java:32:
* @return The title of the editor window.
For these simple accessors, you might want to just use the less correct but less
repetitive version of the Javadoc:

/** @return The title of the editor window. */

Entirely up to you.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:558:
public void setId(int stringId) {
Don't use stringId -- that implies a string resource.  Just use int id.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:699:
for (int i = 0; i < mOptionRows.size(); i++) {
On 2016/06/20 14:35:53, Rouslan (ツ) wrote:
> The split of the single for-loop into two for-loops is to improve radio button
> UX when adding contact information. I am not too attached to this approach. If
> you think it looked better before, we can change it back.

This is fine, but it didn't early return before.  If this is more correct, go
for it.  Add a comment about what each block does though.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:852:
if (i == 0) addRow.setId(R.id.payments_first_radio_button);
Looks weird because it's only added for testing and only for the first item. 
You should pull it out of the for loop.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:179:
* Called when edit dialog is showing.
the Edit dialog?  Add the in more places?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:591:
* Displays the editor user interface for the given model.
Everything new you've added that is editor related would ideally go into another
class.  PaymentRequestUI is kind of huge already.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:602:
final Button done = new Button(mContext);
Rearrange this code so that the button initializing code is next to it instead
of just having the done button dangle here.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:614:
}
newlines between the blocks

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:641:
input.setOnEditorActionListener(new TextView.OnEditorActionListener() {
This listener looks to be the same for all elements.  Pre-create it and just use
the same one.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:728:
focusInputFieldAndShowSoftKeyboard(invalidViews.get(0));
You really shouldn't be forcing a keyboard to show up.  What happens if the user
has a hardware keyboard?

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:737:
private class PhoneFormatterTask
Geez, do we actually get StrictMode violations here?  How do you feel about
kicking off the AsyncTask when the class is constructed, then getting the result
when it's needed?

private final mPhoneFormatterTask = new AsyncTask<Void, Void,
PhoneNumberFormattingTextWatcher>() {
    @Override
    protected PhoneNumberFormattingTextWatcher doInBackground(Void... unused) {
        return new PhoneNumberFormattingTextWatcher();
    }
}

When you need it:
input.addTextChangedListener(mPhoneFormatterTask.get());

When you're done with it:
input.removeTextChangedListener(mPhoneFormatterTask.get());

This (1) fixes a leak and (2) maybe avoids having to save mPhoneInput.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:751:
private static List<AutoCompleteTextView> getInvalidViews(ViewGroup container) {
getViewsWithInvalidInformation

"invalid" and "invalidate" have different meanings with Views

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:151:
* Adds and selects the given item.
Indicate that it's added at the head of the list.

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/strings/android_chrome_strings.grd:143: </message>
omg thank you

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:135:
protected void typeInEditDialog(final int resourceId, final String input)
I'd rename this something else because it's directly setting the text instead of
simulating typing which we do in other places.  setTextInEditDialog?

[please use gerrit instead, 2016-06-22 02:12:31]

dfalcantara@, ptal patch 2.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:51:
* Updates the email address. This influences the label and sublabel.
On 2016/06/21 06:04:38, dfalcantara wrote:
> nit: Say more specifically that the phone number is displayed as the main
label?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:57:
setLabel(TextUtils.isEmpty(mPayerPhone) ? mPayerEmail : mPayerPhone);
On 2016/06/21 06:04:38, dfalcantara wrote:
> Pull this out into an updateLabels() function and make setPayerPhone() call
it?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:93:
public void setComplete() {
On 2016/06/21 06:04:38, dfalcantara wrote:
> nit: setIsComplete?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:241:
int topCompleteContactIndex = SectionInformation.NO_SELECTION;
On 2016/06/21 06:04:38, dfalcantara wrote:
> nit: firstCompleteContactIndex?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:246:
? profile.getEmailAddress()
On 2016/06/21 06:04:38, dfalcantara wrote:
> nit: I don't think we're supposed to break on the colons

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:272:

On 2016/06/21 06:04:38, dfalcantara wrote:
> // The contact section automatically selects the first complete entry.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:340:
private boolean isCompleteContactInformation(String email, String phone) {
On 2016/06/21 06:04:38, dfalcantara wrote:
> isContactInformationComplete?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:596:
public boolean onSectionOptionChanged(@PaymentRequestUI.DataType int optionType,
On 2016/06/21 06:04:38, dfalcantara wrote:
> We should probably rename this function now.  In this case, the option hasn't
> been changed, but a new one has been selected.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:664:
: new AutofillProfile("" /* guid */, AutofillPreferences.SETTINGS_ORIGIN,
On 2016/06/21 06:04:38, dfalcantara wrote:
> You should just have an empty AutofillProfile constructor.  Passing in a bunch
> of empty strings and then describing what each empty string is seems kind of
> silly.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:703:
PhoneNumberUtils.stripSeparators(value.toString()));
On 2016/06/21 06:04:38, dfalcantara wrote:
> On 2016/06/20 14:35:53, Rouslan (ツ) wrote:
> > Is there a better way to validate phone numbers? This is too permissive in
my
> > experience.
> 
> Haven't had to do it before, so I wouldn't know :/

Acknowledged.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:30:
* Indicates a phone field.
On 2016/06/21 06:04:38, dfalcantara wrote:
> Save some space:
> 
> /** Indicates a phone field. */

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:54:
* @param suggestions          Optional list of value to suggest to the user.
On 2016/06/21 06:04:38, dfalcantara wrote:
> values

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:63:
EditorFieldValidator validator, CharSequence requiredErrorMessage,
On 2016/06/21 06:04:39, dfalcantara wrote:
> If requiredErrorMessage and invalidErrorMessage are optional, add @Nullable?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:114:
return mErrorMessage == null;
On 2016/06/21 06:04:38, dfalcantara wrote:
> Little obtuse.  Gain some readability by making it more verbose and symmetric:
> 
> if (TextUtils.isEmpty(mValue) || TextUtils.getTrimmedLength(mValue) == 0) {
>     mErrorMessage = mRequiredErrorMessage;
>     return false;
> }
> 
> if (mValidator != null && !mValidator.isValid(mValue)) {
>     mErrorMessage = mInvalidErrorMessage;
>     return false;
> }
> 
> mErrorMessage = null;
> return true;

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:120:
* @return The error message for the last validation. Can be null.
On 2016/06/21 06:04:39, dfalcantara wrote:
> Can be null if no error was reported.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorModel.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorModel.java:32:
* @return The title of the editor window.
On 2016/06/21 06:04:39, dfalcantara wrote:
> For these simple accessors, you might want to just use the less correct but
less
> repetitive version of the Javadoc:
> 
> /** @return The title of the editor window. */
> 
> Entirely up to you.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:558:
public void setId(int stringId) {
On 2016/06/21 06:04:39, dfalcantara wrote:
> Don't use stringId -- that implies a string resource.  Just use int id.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:699:
for (int i = 0; i < mOptionRows.size(); i++) {
On 2016/06/21 06:04:39, dfalcantara wrote:
> On 2016/06/20 14:35:53, Rouslan (ツ) wrote:
> > The split of the single for-loop into two for-loops is to improve radio
button
> > UX when adding contact information. I am not too attached to this approach.
If
> > you think it looked better before, we can change it back.
> 
> This is fine, but it didn't early return before.  If this is more correct, go
> for it.  Add a comment about what each block does though.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:852:
if (i == 0) addRow.setId(R.id.payments_first_radio_button);
On 2016/06/21 06:04:39, dfalcantara wrote:
> Looks weird because it's only added for testing and only for the first item. 
> You should pull it out of the for loop.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:179:
* Called when edit dialog is showing.
On 2016/06/21 06:04:39, dfalcantara wrote:
> the Edit dialog?  Add the in more places?

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:591:
* Displays the editor user interface for the given model.
On 2016/06/21 06:04:39, dfalcantara wrote:
> Everything new you've added that is editor related would ideally go into
another
> class.  PaymentRequestUI is kind of huge already.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:602:
final Button done = new Button(mContext);
On 2016/06/21 06:04:39, dfalcantara wrote:
> Rearrange this code so that the button initializing code is next to it instead
> of just having the done button dangle here.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:614:
}
On 2016/06/21 06:04:39, dfalcantara wrote:
> newlines between the blocks

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:641:
input.setOnEditorActionListener(new TextView.OnEditorActionListener() {
On 2016/06/21 06:04:39, dfalcantara wrote:
> This listener looks to be the same for all elements.  Pre-create it and just
use
> the same one.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:728:
focusInputFieldAndShowSoftKeyboard(invalidViews.get(0));
On 2016/06/21 06:04:39, dfalcantara wrote:
> You really shouldn't be forcing a keyboard to show up.  What happens if the
user
> has a hardware keyboard?

This is what we do in CVV prompt.

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

I assumed that hardware keyboard makes this code no-op. I've renamed it into
"focusInputField" to make it more clear.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:737:
private class PhoneFormatterTask
On 2016/06/21 06:04:39, dfalcantara wrote:
> Geez, do we actually get StrictMode violations here?

Yes.

> How do you feel about
> kicking off the AsyncTask when the class is constructed, then getting the
result
> when it's needed?
> 
> private final mPhoneFormatterTask = new AsyncTask<Void, Void,
> PhoneNumberFormattingTextWatcher>() {
>     @Override
>     protected PhoneNumberFormattingTextWatcher doInBackground(Void... unused)
{
>         return new PhoneNumberFormattingTextWatcher();
>     }
> }
> 
> When you need it:
> input.addTextChangedListener(mPhoneFormatterTask.get());
> 
> When you're done with it:
> input.removeTextChangedListener(mPhoneFormatterTask.get());
> 
> This (1) fixes a leak and (2) maybe avoids having to save mPhoneInput.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:751:
private static List<AutoCompleteTextView> getInvalidViews(ViewGroup container) {
On 2016/06/21 06:04:39, dfalcantara wrote:
> getViewsWithInvalidInformation
> 
> "invalid" and "invalidate" have different meanings with Views

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:151:
* Adds and selects the given item.
On 2016/06/21 06:04:39, dfalcantara wrote:
> Indicate that it's added at the head of the list.

Done.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:143: </message>
On 2016/06/21 06:04:39, dfalcantara wrote:
> omg thank you

Acknowledged.

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
(right):

https://codereview.chromium.org/2081533002/diff/200001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:135:
protected void typeInEditDialog(final int resourceId, final String input)
On 2016/06/21 06:04:39, dfalcantara wrote:
> I'd rename this something else because it's directly setting the text instead
of
> simulating typing which we do in other places.  setTextInEditDialog?

Done.

[gone, 2016-06-22 05:24:13]

Definitely feels a lot cleaner.  lgtm % comments

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/200001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:728:
focusInputFieldAndShowSoftKeyboard(invalidViews.get(0));
On 2016/06/22 02:12:30, Rouslan (ツ) wrote:
> On 2016/06/21 06:04:39, dfalcantara wrote:
> > You really shouldn't be forcing a keyboard to show up.  What happens if the
> user
> > has a hardware keyboard?
> 
> This is what we do in CVV prompt.
> 
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> I assumed that hardware keyboard makes this code no-op. I've renamed it into
> "focusInputField" to make it more clear.

Acknowledged.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:20:
@Nullable private String mPayerPhone;
Huh.  I guess I should have expected that fields could be annotated as @Nullable
but it'd never occurred to me to actually do it :)

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:42:
public String getPayerEmail() {
nit: I'd keep @Nullable on the same line as the rest of the signature for
brevity.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:55:
private final AsyncTask<Void, Void, PhoneNumberFormattingTextWatcher>
mPhoneFormatterTask =
Should put the execute inside the constructor.  It's kind of weird to call a
function from outside of there because ordering isn't obvious.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:225:
/** Immediately returns the phone formatter or null if it has not initialized
yet. */
You're cool with not validating the phone number at all when this happens?

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:699:
// Handle click on the "ADD THING" button.
I regret nothing about using ADD THING as the example button.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java:32:
/** If the user has not clicked "Pay" yet, then merchant's abort will succeed.
*/
Heh these make it so much more obvious what they're supposed to be doing...

[please use gerrit instead, 2016-06-22 16:42:38]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-06-22 16:42:39]

Sending to cq.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:20:
@Nullable private String mPayerPhone;
On 2016/06/22 at 05:24:13, dfalcantara wrote:
> Huh.  I guess I should have expected that fields could be annotated as
@Nullable but it'd never occurred to me to actually do it :)

I've seen some Chrome code like this and decided to follow the practice.
FindBugs should use this annotation to... find bugs, right? In retrospect,
putting @Nullable everywhere feels like an overkill. Maybe I'm not used to it
yet.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:42:
public String getPayerEmail() {
On 2016/06/22 at 05:24:13, dfalcantara wrote:
> nit: I'd keep @Nullable on the same line as the rest of the signature for
brevity.

Done.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:55:
private final AsyncTask<Void, Void, PhoneNumberFormattingTextWatcher>
mPhoneFormatterTask =
On 2016/06/22 at 05:24:13, dfalcantara wrote:
> Should put the execute inside the constructor.  It's kind of weird to call a
function from outside of there because ordering isn't obvious.

Done.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:225:
/** Immediately returns the phone formatter or null if it has not initialized
yet. */
On 2016/06/22 at 05:24:13, dfalcantara wrote:
> You're cool with not validating the phone number at all when this happens?

When this happens, the phone number is not auto-formatted. I am cool with that.

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:699:
// Handle click on the "ADD THING" button.
On 2016/06/22 at 05:24:13, dfalcantara wrote:
> I regret nothing about using ADD THING as the example button.

+1

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java
(right):

https://chromiumcodereview.appspot.com/2081533002/diff/220001/chrome/android/...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestAbortTest.java:32:
/** If the user has not clicked "Pay" yet, then merchant's abort will succeed.
*/
On 2016/06/22 at 05:24:13, dfalcantara wrote:
> Heh these make it so much more obvious what they're supposed to be doing...

I am starting to see the value in test comments again.

[please use gerrit instead, 2016-06-22 16:42:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2081533002/#ps240001 (title: "Addressed
comments")

[commit-bot: I haz the power, 2016-06-22 16:43:04]

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

[commit-bot: I haz the power, 2016-06-22 18:15:56]

Description was changed from

==========
Edit contacts UI for PaymentRequest

BUG=617195, 621586
==========

to

==========
Edit contacts UI for PaymentRequest

BUG=617195, 621586
==========

[commit-bot: I haz the power, 2016-06-22 18:15:58]

Committed patchset #3 (id:240001)

[commit-bot: I haz the power, 2016-06-22 18:19:36]

Description was changed from

==========
Edit contacts UI for PaymentRequest

BUG=617195, 621586
==========

to

==========
Edit contacts UI for PaymentRequest

BUG=617195, 621586

Committed: https://crrev.com/6c669e362933a62f3a3bf3714dbb405c1dc14277
Cr-Commit-Position: refs/heads/master@{#401350}
==========

[commit-bot: I haz the power, 2016-06-22 18:19:38]

Patchset 3 (id:??) landed as
https://crrev.com/6c669e362933a62f3a3bf3714dbb405c1dc14277
Cr-Commit-Position: refs/heads/master@{#401350}