[Payments] Normalize Shipping Address Change on Android.

[Payments] Normalize Shipping Address Change on Android.

BUG=713735
Review-Url: https://codereview.chromium.org/2828913002
Cr-Commit-Position: refs/heads/master@{#466182}
Committed: https://chromium.googlesource.com/chromium/src/+/f0b43ac1829b9e7bbb7a49e60db386072f5dd850

[sebsg, 2017-04-19 21:34:43]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-19 21:35:11]

Description was changed from

==========
[Payments] Normalize Shipping Address Change on Desktop.

BUG=
==========

to

==========
[Payments] Normalize Shipping Address Change on Android.

BUG=
==========

[commit-bot: I haz the power, 2017-04-19 21:35:28]

Dry run: 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, 2017-04-19 23:40:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 23:40:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sebsg, 2017-04-20 14:41:46]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 14:42:15]

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

[sebsg, 2017-04-20 14:58:58]

Patchset #2 (id:20001) has been deleted

[sebsg, 2017-04-20 14:59:07]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-04-20 15:19:27]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 15:19:45]

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

[sebsg, 2017-04-20 15:19:54]

Patchset #1 (id:40001) has been deleted

[sebsg, 2017-04-20 15:20:35]

Description was changed from

==========
[Payments] Normalize Shipping Address Change on Android.

BUG=
==========

to

==========
[Payments] Normalize Shipping Address Change on Android.

BUG=713735
==========

[sebsg, 2017-04-20 15:21:30]

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

[sebsg, 2017-04-20 15:21:31]

Hi Rouslan, PTAL?

The current timeout in prod is 5 seconds, but I think we can and should lower
that to maybe 2-3 seconds? WDYT?

[please use gerrit instead, 2017-04-20 15:41:44]

Excellent testing habits!

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:88:
public class PaymentRequestImpl implements PaymentRequest,
PaymentRequestUI.Client,
Question: Which formatter does this?

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1600:
if (mIsWaitingForShippingAddressChangeNormalization) {
Question: Why would onAddressNormalized() be called more than once? I assume
that's why the "isWaiting" boolean is required.

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1605:
new AutofillAddress(ChromeActivity.fromWebContents(mWebContents), profile);
Please check the return value of ChromeActivity.fromWebContents() for null.

if (chromeActivity == null) {
    recordAbortReasonHistogram(PaymentRequestMetrics.ABORT_REASON_OTHER);
    disconnectFromClientWithDebugMessage("Unable to find Chrome activity");
    if (sObserverForTest != null)
sObserverForTest.onPaymentRequestServiceShowFailed();
    return;
}

This can happen if the tab is closed during normalization process, probably.

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java
(right):

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java:36:
true, "Jon Doe", "Google", "340 Main St", "CA", "Los Angeles", "", "90291", "",
Let's change "CA" to "California" to verify that it gets normalized.

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java:57:
clickCardUnmaskButtonAndWait(DialogInterface.BUTTON_POSITIVE, mDismissed);
No need to type in the CVC. 'shippingoptionchange' event should print out the
address. So it's OK to cancel the UI at this point. (Replace lines 55-57 with
"clickAndWait(R.id.close_button, mDismissed);".)

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
File chrome/test/data/payments/shipping_address_change.js (right):

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:7: /* global
PaymentRequest:false */
This line is no longer necessary in new tests. The linters are now aware of
PaymentRequest API.

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:37: shippingAddressChange =
request.shippingAddress;
Print the address from here instead:

print(JSON.stringify(request.shippingAddress, undefined, 2));

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:38:
resolve(updateDetails(details, request.shippingAddress));
Technically this test does not require updating price based on the address. You
can simplify this line to "resolve(details);" and remove the updateDetails()
function below.

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:42: request.show()
"request.show()" is sufficient. Important print() statement is in the
'shippingaddresschange' event listener. Other print() statements are not
important for this test. (Remove lines 43-54.)

[commit-bot: I haz the power, 2017-04-20 16:19:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 16:19:59]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-04-20 21:16:40]

Thanks! Another look?

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:88:
public class PaymentRequestImpl implements PaymentRequest,
PaymentRequestUI.Client,
On 2017/04/20 15:41:43, ಠ_ಠ wrote:
> Question: Which formatter does this?

git cl format. but it seems to change what it does to this every week lol. Is
this formatting fine?

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1600:
if (mIsWaitingForShippingAddressChangeNormalization) {
On 2017/04/20 15:41:43, ಠ_ಠ wrote:
> Question: Why would onAddressNormalized() be called more than once? I assume
> that's why the "isWaiting" boolean is required.

Right! I made this be handled in the normalizer itself, no need for that
anymore. Thanks!

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1605:
new AutofillAddress(ChromeActivity.fromWebContents(mWebContents), profile);
On 2017/04/20 15:41:43, ಠ_ಠ wrote:
> Please check the return value of ChromeActivity.fromWebContents() for null.
> 
> if (chromeActivity == null) {
>     recordAbortReasonHistogram(PaymentRequestMetrics.ABORT_REASON_OTHER);
>     disconnectFromClientWithDebugMessage("Unable to find Chrome activity");
>     if (sObserverForTest != null)
> sObserverForTest.onPaymentRequestServiceShowFailed();
>     return;
> }
> 
> This can happen if the tab is closed during normalization process, probably.

Done.

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java
(right):

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java:36:
true, "Jon Doe", "Google", "340 Main St", "CA", "Los Angeles", "", "90291", "",
On 2017/04/20 15:41:44, ಠ_ಠ wrote:
> Let's change "CA" to "California" to verify that it gets normalized.

Done.

https://codereview.chromium.org/2828913002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java:57:
clickCardUnmaskButtonAndWait(DialogInterface.BUTTON_POSITIVE, mDismissed);
On 2017/04/20 15:41:44, ಠ_ಠ wrote:
> No need to type in the CVC. 'shippingoptionchange' event should print out the
> address. So it's OK to cancel the UI at this point. (Replace lines 55-57 with
> "clickAndWait(R.id.close_button, mDismissed);".)

Done.

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
File chrome/test/data/payments/shipping_address_change.js (right):

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:7: /* global
PaymentRequest:false */
On 2017/04/20 15:41:44, ಠ_ಠ wrote:
> This line is no longer necessary in new tests. The linters are now aware of
> PaymentRequest API.

Done.

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:37: shippingAddressChange =
request.shippingAddress;
On 2017/04/20 15:41:44, ಠ_ಠ wrote:
> Print the address from here instead:
> 
> print(JSON.stringify(request.shippingAddress, undefined, 2));

Done.

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:38:
resolve(updateDetails(details, request.shippingAddress));
On 2017/04/20 15:41:44, ಠ_ಠ wrote:
> Technically this test does not require updating price based on the address.
You
> can simplify this line to "resolve(details);" and remove the updateDetails()
> function below.

Done.

https://codereview.chromium.org/2828913002/diff/60001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:42: request.show()
On 2017/04/20 15:41:44, ಠ_ಠ wrote:
> "request.show()" is sufficient. Important print() statement is in the
> 'shippingaddresschange' event listener. Other print() statements are not
> important for this test. (Remove lines 43-54.)

Done.

[please use gerrit instead, 2017-04-20 21:23:13]

LGTM % comment

https://codereview.chromium.org/2828913002/diff/80001/chrome/test/data/paymen...
File chrome/test/data/payments/shipping_address_change.js (right):

https://codereview.chromium.org/2828913002/diff/80001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:31: var
shippingAddressChange;
No need.

https://codereview.chromium.org/2828913002/diff/80001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:36:
print(JSON.stringify(shippingAddressChange, undefined, 2));
Use request.shippingAddress directly.

[sebsg, 2017-04-20 21:27:46]

Thanks, sending to CQ.

https://codereview.chromium.org/2828913002/diff/80001/chrome/test/data/paymen...
File chrome/test/data/payments/shipping_address_change.js (right):

https://codereview.chromium.org/2828913002/diff/80001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:31: var
shippingAddressChange;
On 2017/04/20 21:23:13, ಠ_ಠ wrote:
> No need.

Done.

https://codereview.chromium.org/2828913002/diff/80001/chrome/test/data/paymen...
chrome/test/data/payments/shipping_address_change.js:36:
print(JSON.stringify(shippingAddressChange, undefined, 2));
On 2017/04/20 21:23:13, ಠ_ಠ wrote:
> Use request.shippingAddress directly.

Done.

[sebsg, 2017-04-20 21:27:51]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-04-20 21:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2828913002/#ps100001
(title: "Addressed comment")

[commit-bot: I haz the power, 2017-04-20 21:28:41]

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, 2017-04-20 23:15:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492723671341560,
"parent_rev": "8487c6bd015949404b45ad174f6b8fc806f95f2c", "commit_rev":
"f0b43ac1829b9e7bbb7a49e60db386072f5dd850"}

[commit-bot: I haz the power, 2017-04-20 23:15:43]

Description was changed from

==========
[Payments] Normalize Shipping Address Change on Android.

BUG=713735
==========

to

==========
[Payments] Normalize Shipping Address Change on Android.

BUG=713735

Review-Url: https://codereview.chromium.org/2828913002
Cr-Commit-Position: refs/heads/master@{#466182}
Committed:
https://chromium.googlesource.com/chromium/src/+/f0b43ac1829b9e7bbb7a49e60db3...
==========

[commit-bot: I haz the power, 2017-04-20 23:15:44]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f0b43ac1829b9e7bbb7a49e60db3...