Autofill address editor in PaymentRequest UI.

Autofill address editor in PaymentRequest UI.

BUG=603635
Committed: https://crrev.com/deafd574419e38a08a9f566242ebe20ad986e06e
Cr-Commit-Position: refs/heads/master@{#402930}

[please use gerrit instead, 2016-06-26 22:57:14]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-06-26 22:57:20]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2016-06-27 00:28:48]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2016-06-27 01:30:36]

Patchset #1 (id:60001) has been deleted

[please use gerrit instead, 2016-06-27 02:11:41]

Patchset #1 (id:80001) has been deleted

[please use gerrit instead, 2016-06-27 03:07:54]

Patchset #2 (id:120001) has been deleted

[please use gerrit instead, 2016-06-27 03:08:11]

Patchset #1 (id:100001) has been deleted

[please use gerrit instead, 2016-06-27 16:46:31]

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

[please use gerrit instead, 2016-06-27 16:46:32]

dfalcantara@, ptal android.

mathp@, ptal autofill.

I am looking into the try bot failures.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java
(right):

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillCreditCardEditor.java:122:
profilesAdapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item);
Makes the billing address selector drop downs taller, identical to the other
drop down items in  Chrome.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java
(right):

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/preferences/autofill/AutofillProfileBridge.java:152:
* @return A list of address UI components. The ordering in the list of pairs
specifies the
Note to self: s/of pairs//

[Mathieu, 2016-06-27 17:45:20]

browser autofill code lgtm

[gone, 2016-06-28 17:30:29]

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java
(right):

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:39:
private final List<CharSequence> mAllPhoneNumbers;
Same comment about "All" as the other CL

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:109:
mCountryField.setSpinnerCallback(new Callback<SpinnerChangedEventData>() {
You really, really need to add comments all over this thing.  While I get the
gist of what's going on, I really have to stare at it.

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:122:
if (mAddressFields.isEmpty()) {
Adding a bunch of default fields?

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:162:
profile.setCountryCode(mCountryField.getValue().toString());
Pull all this out into a function called "commitChanges()" or something.

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

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:235:
if (isComplete && firstCompleteAddressIndex < 0 &&
!mMerchantNeedsShippingAddress) {
firstCompleteAddressIndex == SectionInformation.NO_SELECTION

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

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:38:
public static class SpinnerChangedEventData {
Can you just use a Pair<String, Runnable>, or do you want to be more explicit?

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:92:
mInputTypeHint = 0;
Why didn't you make a new InputTypeHint here?

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

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:410:
* Create the visual representation of the EditorModel.
* Create the visual representation of the EditorModel.
*
* Fields are added to the layout at position |getChildCount() - 1| to account
for the additional TextView that says "* indicates required field" at the bottom
of the layout.

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

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:120:
* if this section should be disabled and show a spinner while the added option
is being
"progress spinny".  Now that you have actual spinners in the view, that
distinction is more important.

/**
 * Called when the user clicks on the "Add" button for a section.
 *
 * If this method returns true, then:
 * + The added option should be asynchronously verified.
 * + The section should be disabled and a progress spinny should be shown while
the option is being verified.
 * + The checkedCallback will be invoked with the results of the check and
updated information.
 *
 * etc
 */

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:215:
private final Callback<PaymentInformation> mUpdateSectionsCallback;
mUpdateSectionCallback?

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/browser/...
File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc
(right):

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/browser/...
chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:83: for
(int i = COUNTRY; i <= RECIPIENT; ++i) {
This seems frought with peril.  Is there any way to guarantee that this always
goes through the entire list (i.e. a < MAX instead of <= RECIPIENT)?

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/browser/...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/browser/...
chrome/browser/autofill/android/personal_data_manager_android.h:72: // include
in the label.
included

[please use gerrit instead, 2016-06-29 00:26:46]

Patchset #2 (id:160001) has been deleted

[please use gerrit instead, 2016-06-29 00:28:45]

dfalcantara@, ptal patch 2.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java
(right):

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:39:
private final List<CharSequence> mAllPhoneNumbers;
On 2016/06/28 17:30:28, dfalcantara wrote:
> Same comment about "All" as the other CL

Done.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:109:
mCountryField.setSpinnerCallback(new Callback<SpinnerChangedEventData>() {
On 2016/06/28 17:30:28, dfalcantara wrote:
> You really, really need to add comments all over this thing.  While I get the
> gist of what's going on, I really have to stare at it. 

Done.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:122:
if (mAddressFields.isEmpty()) {
On 2016/06/28 17:30:28, dfalcantara wrote:
> Adding a bunch of default fields?

Added a comment:

// There's a finite number of fields for address editing.
// Changing the country will re-order and relabel the
// fields. The meaning of each field remains the same.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:162:
profile.setCountryCode(mCountryField.getValue().toString());
On 2016/06/28 17:30:28, dfalcantara wrote:
> Pull all this out into a function called "commitChanges()" or something.

Done.

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

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:235:
if (isComplete && firstCompleteAddressIndex < 0 &&
!mMerchantNeedsShippingAddress) {
On 2016/06/28 17:30:29, dfalcantara wrote:
> firstCompleteAddressIndex == SectionInformation.NO_SELECTION

Done.

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

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:38:
public static class SpinnerChangedEventData {
On 2016/06/28 17:30:29, dfalcantara wrote:
> Can you just use a Pair<String, Runnable>, or do you want to be more explicit?

Using Pair.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:92:
mInputTypeHint = 0;
On 2016/06/28 17:30:29, dfalcantara wrote:
> Why didn't you make a new InputTypeHint here?

Done.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java
(right):

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:410:
* Create the visual representation of the EditorModel.
On 2016/06/28 17:30:29, dfalcantara wrote:
> * Create the visual representation of the EditorModel.
> *
> * Fields are added to the layout at position |getChildCount() - 1| to account
> for the additional TextView that says "* indicates required field" at the
bottom
> of the layout.

Done.

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

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:120:
* if this section should be disabled and show a spinner while the added option
is being
On 2016/06/28 17:30:29, dfalcantara wrote:
> "progress spinny".  Now that you have actual spinners in the view, that
> distinction is more important.
> 
> /**
>  * Called when the user clicks on the "Add" button for a section.
>  *
>  * If this method returns true, then:
>  * + The added option should be asynchronously verified.
>  * + The section should be disabled and a progress spinny should be shown
while
> the option is being verified.
>  * + The checkedCallback will be invoked with the results of the check and
> updated information.
>  *
>  * etc
>  */

Done.

https://codereview.chromium.org/2093363002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:215:
private final Callback<PaymentInformation> mUpdateSectionsCallback;
On 2016/06/28 17:30:29, dfalcantara wrote:
> mUpdateSectionCallback?

It updates all sections, so plural sounds right to my ear.

Are you sure you want to change "mUpdateSectionsCallback" to
"mUpdateSectionCallback"?

[yes] [no] [abort]

https://codereview.chromium.org/2093363002/diff/140001/chrome/browser/android...
File chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc
(right):

https://codereview.chromium.org/2093363002/diff/140001/chrome/browser/android...
chrome/browser/android/preferences/autofill/autofill_profile_bridge.cc:83: for
(int i = COUNTRY; i <= RECIPIENT; ++i) {
On 2016/06/28 17:30:29, dfalcantara wrote:
> This seems frought with peril.  Is there any way to guarantee that this always
> goes through the entire list (i.e. a < MAX instead of <= RECIPIENT)?

https://cs.chromium.org/chromium/src/third_party/libaddressinput/src/cpp/incl...
does not define a MAX, sorry. I've added a comment to keep these in sync.

https://codereview.chromium.org/2093363002/diff/140001/chrome/browser/autofil...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://codereview.chromium.org/2093363002/diff/140001/chrome/browser/autofil...
chrome/browser/autofill/android/personal_data_manager_android.h:72: // include
in the label.
On 2016/06/28 17:30:29, dfalcantara wrote:
> included

Next you're going to tell me "All your base are belonged to us."

[gone, 2016-06-29 17:12:45]

lgtm

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

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:215:
private final Callback<PaymentInformation> mUpdateSectionsCallback;
On 2016/06/29 00:28:45, Rouslan (ツ) wrote:
> On 2016/06/28 17:30:29, dfalcantara wrote:
> > mUpdateSectionCallback?
> 
> It updates all sections, so plural sounds right to my ear.
> 
> Are you sure you want to change "mUpdateSectionsCallback" to
> "mUpdateSectionCallback"?
> 
> [yes] [no] [abort]

Ah, misread what it did.  [eject]

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/browser/...
File chrome/browser/autofill/android/personal_data_manager_android.h (right):

https://chromiumcodereview.appspot.com/2093363002/diff/140001/chrome/browser/...
chrome/browser/autofill/android/personal_data_manager_android.h:72: // include
in the label.
On 2016/06/29 00:28:45, Rouslan (ツ) wrote:
> On 2016/06/28 17:30:29, dfalcantara wrote:
> > included
> 
> Next you're going to tell me "All your base are belonged to us."

Launch all zig.

[please use gerrit instead, 2016-06-29 19:27:48]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-06-29 19:27:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2093363002/#ps200001
(title: "Fix try-bot")

[commit-bot: I haz the power, 2016-06-29 19:28:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 21:05:27]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2016-06-29 21:05:48]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 21:07:29]

Description was changed from

==========
Autofill address editor in PaymentRequest UI.

BUG=603635
==========

to

==========
Autofill address editor in PaymentRequest UI.

BUG=603635

Committed: https://crrev.com/deafd574419e38a08a9f566242ebe20ad986e06e
Cr-Commit-Position: refs/heads/master@{#402930}
==========

[commit-bot: I haz the power, 2016-06-29 21:07:31]

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