[Payments] Normalize billing address before sending to the merchant.

chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java

Index: chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
index 62ee4cffa1fa44ede450a96efa8e00bfed5e7d77..d975bd7fc0b0706838f5ce47fde130d408780fc7 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java
@@ -4,18 +4,20 @@
 
 package org.chromium.chrome.browser.payments;
 
+import android.os.Handler;
 import android.text.TextUtils;
 import android.util.JsonWriter;
 
+import org.json.JSONObject;
+
 import org.chromium.chrome.browser.autofill.PersonalDataManager;
 import org.chromium.chrome.browser.autofill.PersonalDataManager.AutofillProfile;
 import org.chromium.chrome.browser.autofill.PersonalDataManager.CreditCard;
 import org.chromium.chrome.browser.autofill.PersonalDataManager.FullCardRequestDelegate;
+import org.chromium.chrome.browser.autofill.PersonalDataManager.NormalizedAddressRequestDelegate;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.payments.mojom.PaymentItem;
 
-import org.json.JSONObject;
-
 import java.io.IOException;
 import java.io.StringWriter;
 import java.util.List;
@@ -25,13 +27,16 @@ import javax.annotation.Nullable;
 /**
  * The locally stored credit card payment instrument.
  */
-public class AutofillPaymentInstrument
-        extends PaymentInstrument implements FullCardRequestDelegate {
+public class AutofillPaymentInstrument extends PaymentInstrument
+        implements FullCardRequestDelegate, NormalizedAddressRequestDelegate {
     private final WebContents mWebContents;
+    private final Handler mHandler = new Handler();
     private CreditCard mCard;
     private boolean mIsComplete;
+    private String mTempsCvc;

[please use gerrit instead, 2016/10/17 20:44:18]

Let's not put the word "temp" in the variable name. All non-constant non-static
variables are "temporary". That being said, "mCvc" is a bit short for a name.
Java writes will give you weird looks. Let's call it "mSecurityCode" :-).

Keep clearing the variable after use, of course.

[sebsg, 2016/10/18 14:38:19]

Done.

     @Nullable private AutofillProfile mBillingAddress;
     @Nullable private DetailsCallback mCallback;
+    private boolean mIsWaitingForBillingNormalization;
 
     /**
      * Builds a payment instrument for the given credit card.
@@ -65,17 +70,62 @@ public class AutofillPaymentInstrument
     }
 
     @Override
-    public void onFullCardDetails(CreditCard card, String cvc) {
+    public void onFullCardDetails(CreditCard unusedCard, String cvc) {

[please use gerrit instead, 2016/10/17 20:44:18]

You can't throw away the card, because it might have updated expiration date.
You can trigger this by saving an expired credit card in autofill.
PaymentRequest will let you use it just fine, but CVC unlock UI will ask for the
updated expiration date. This new expiration date comes back here inside of the
"card". So we kind of have to use it :-).

[sebsg, 2016/10/18 14:38:19]

This will require some rework then, because as it is now, we don't actually
update the credit card. So we will ask for a new exp all the time. Added a todo
and a bug.

[please use gerrit instead, 2016/10/18 17:30:30]

The merchant website currently gets the updated expiration date. The bug in
current implementation is that updated expiration date is not saved on disk. The
expiration date should be updated in FullCardRequest::OnUnmaskResponse() via
personal_data_manager_->UpdateCreditCard(). 
 
https://cs.chromium.org/chromium/src/components/autofill/core/browser/payment...

If you ignore the "card" here in onFullCardDetails(), however, then the merchant
website will also not get the updated expiration date. That would be a new,
different bug.

[sebsg, 2016/10/18 22:44:31]

Yup! that's why I assigned the new exp month and year from the card from
onFullCardDetails :) 

I was just trying to say that it will require some rework to save it because
currently the card that is sent to unmask is a copy of the original card, not
the original itself (same except guid and use stats). 

+        // Keep the cvc for after the normalization.
+        mTempsCvc = cvc;
+
+        // Show the processing message while the billing address gets normalized.
+        mCallback.loadingInstrumentDetails();

[please use gerrit instead, 2016/10/17 20:44:18]

This is a good idea. We should keep this, but move it to "address normalized?"
check. So, if address is not normalized yet, call loadingInstrumentDetails().
Otherwise call sendInstrumentDetails().

[sebsg, 2016/10/18 14:38:19]

Done.

+
+        if (mBillingAddress != null) {

[please use gerrit instead, 2016/10/17 20:44:18]

mBillingAddress should never be null when getDetails() or onFullCardDetails() is
called. You can even "assert" that at the top of the function.

[sebsg, 2016/10/18 14:38:19]

Done.

+            // Start the normalization task.
+            mIsWaitingForBillingNormalization = true;
+            boolean willNormalizeAsync =
+                    PersonalDataManager.getInstance().normalizeAddress(mBillingAddress.getGUID(),

[please use gerrit instead, 2016/10/17 20:44:18]

if willNormalizeAsync is false, then the address is normalized after this line.
Technically this means that you're not longer waiting for normalization, right?
Thus, can you can do something like this instead?

mIsWaitingForBillingNormalization =
PersonalDataManager.getInstance().normalizeAddress(mBillingAddress.getGUID(), 
AutofillAddress.getCountryCode(mBillingAddress), this);

If not, let me know why. I am curious :-)

[sebsg, 2016/10/18 14:38:19]

Right, with the way I did it initially I needed this other name. But I don't
need it anymore, thanks for pointing it out!

+                            AutofillAddress.getCountryCode(mBillingAddress), this);
+
+            if (willNormalizeAsync) {
+                // If the normalization was not done synchronously, start a timer to cancel the
+                // asynchronous normalization if it takes too long.
+                mHandler.postDelayed(new Runnable() {
+                    @Override
+                    public void run() {
+                        onAddressNormalized(null);
+                    }
+                }, PersonalDataManager.getInstance().getNormalizationTimeoutMS());
+            }
+
+            // SendInstrumentDetails will be called when the billing address has been normalized or
+            // when the timer has expired.
+            return;
+        }
+
+        // No billing normalization required, return the details now.
+        sendIntrumentDetails();
+    }
+
+    @Override
+    public void onAddressNormalized(AutofillProfile profile) {
+        if (!mIsWaitingForBillingNormalization) return;

[please use gerrit instead, 2016/10/17 20:44:18]

This check is not necessary here, I think. If you follow this diagram:

getDetails()-->getFullCard()------->address norm?-->sendInstrumentDetails()
        \                                             /
         \---->normalizeAddress()---->card full?---->/

... then this check should be performed in "address normalized?" check when
getFullCard() returns. getFullCard() returns in onFullCardDetails().

[sebsg, 2016/10/18 14:38:19]

I see what you mean and I have done the flow suggested above to normalize the
billing while the card gets unmasked. However, this check here is necessary
because onAddressNormalized can be called by both the actual normalization and
the normalization timeout

normalizeAddress()------->actualNormalization()----->onAddressNormalized()--->...
     \                                               /
      \------------------>normalizationTimeout()----/

+        mIsWaitingForBillingNormalization = false;
+
+        // If the normalization finished first, use the normalized address.
+        if (profile != null) mBillingAddress = profile;
+
+        sendIntrumentDetails();
+    }
+
+    private void sendIntrumentDetails() {
         StringWriter stringWriter = new StringWriter();
         JsonWriter json = new JsonWriter(stringWriter);
         try {
             json.beginObject();
 
-            json.name("cardholderName").value(card.getName());
-            json.name("cardNumber").value(card.getNumber());
-            json.name("expiryMonth").value(card.getMonth());
-            json.name("expiryYear").value(card.getYear());
-            json.name("cardSecurityCode").value(cvc);
+            json.name("cardholderName").value(mCard.getName());
+            json.name("cardNumber").value(mCard.getNumber());
+            json.name("expiryMonth").value(mCard.getMonth());
+            json.name("expiryYear").value(mCard.getYear());
+            json.name("cardSecurityCode").value(mTempsCvc);
 
             json.name("billingAddress").beginObject();
 
@@ -110,7 +160,10 @@ public class AutofillPaymentInstrument
             return;
         }
 
-        mCallback.onInstrumentDetailsReady(card.getBasicCardPaymentType(), stringWriter.toString());
+        mCallback.onInstrumentDetailsReady(
+                mCard.getBasicCardPaymentType(), stringWriter.toString());
+
+        mTempsCvc = "";

[please use gerrit instead, 2016/10/17 20:44:18]

This should be inside of the finally{} block for the try-catch statement above.
That way the CVC is erased even in case of IOException.

[sebsg, 2016/10/18 14:38:19]

Done.

     }
 
     private static String ensureNotNull(@Nullable String value) {
@@ -161,4 +214,9 @@ public class AutofillPaymentInstrument
     public CreditCard getCard() {
         return mCard;
     }
-}
+
+    /** @return The billing address associated with this credit card. */
+    public AutofillProfile getBillingAddress() {
+        return mBillingAddress;
+    }
+}