Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2413533003:
[Payments] Normalize billing address before sending to the merchant. (Closed)
|
Created:
4 years, 2 months ago by sebsg Modified:
4 years, 1 month ago CC:
chromium-reviews, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Normalize billing address before sending to the merchant.
BUG=654773
Committed: https://crrev.com/8081c02908534b12968e52de312c2b7ca7b19456
Cr-Commit-Position: refs/heads/master@{#427242}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressed comments #
Total comments: 60
Patch Set 3 : Addressed comments #
Total comments: 12
Patch Set 4 : Addressed comments #Patch Set 5 : Rebase #Messages
Total messages: 61 (45 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Description was changed from ========== WIP [Payments] Normalize billing address before sending to the merchant. BUG= ========== to ========== [Payments] Normalize billing address before sending to the merchant. BUG=654773 ==========
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, could you please take a look? Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Some comments to start. Good work so far! https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:36: private String mTempsCvc; 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. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:65: List<PaymentItem> unusedCart, JSONObject unusedDetails, DetailsCallback callback) { getDetails() shows the CVC prompt. This is the perfect time to normalize the address in the background, while the user is typing in the CVC. getDetails()-->getFullCard()------->address norm?-->sendInstrumentDetails() \ / \---->normalizeAddress()---->card full?---->/ https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:73: public void onFullCardDetails(CreditCard unusedCard, String cvc) { 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 :-). https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:78: mCallback.loadingInstrumentDetails(); 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(). https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:80: if (mBillingAddress != null) { mBillingAddress should never be null when getDetails() or onFullCardDetails() is called. You can even "assert" that at the top of the function. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:84: PersonalDataManager.getInstance().normalizeAddress(mBillingAddress.getGUID(), 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 :-) https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:109: if (!mIsWaitingForBillingNormalization) return; 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(). https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:166: mTempsCvc = ""; 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. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:31: * Called when loading the instrument details. Please elaborate a bit more on this comment. This callback is important because the instrument is not showing any UI, but the onInstrumentDetailsReady/Error() has not been called yet. So the PaymentRequestUI should show it's own "Loading..." UI at this time. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:33: void loadingInstrumentDetails(); Let's move this callback to be the first one in the interface. When thinking about the time progression, this is called before onInstrumentDetailsReady/Error(). So it would make sense to also place "loading..." callback above these "ready" and "error" callbacks. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:73: PaymentResponseHelper.PaymentResponseRequesterDelegate { Eeek. Don't let Dan see this. Android engineers don't like this style of indent... :-( That is, please change back this indentation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Rouslan, thanks for the comments. Another look? https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:36: private String mTempsCvc; On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > 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. Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:65: List<PaymentItem> unusedCart, JSONObject unusedDetails, DetailsCallback callback) { On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > getDetails() shows the CVC prompt. This is the perfect time to normalize the > address in the background, while the user is typing in the CVC. > > getDetails()-->getFullCard()------->address norm?-->sendInstrumentDetails() > \ / > \---->normalizeAddress()---->card full?---->/ Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:73: public void onFullCardDetails(CreditCard unusedCard, String cvc) { On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > 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 :-). 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. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:78: mCallback.loadingInstrumentDetails(); On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > 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(). Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:80: if (mBillingAddress != null) { On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > mBillingAddress should never be null when getDetails() or onFullCardDetails() is > called. You can even "assert" that at the top of the function. Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:84: PersonalDataManager.getInstance().normalizeAddress(mBillingAddress.getGUID(), On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > 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 :-) 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! https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:109: if (!mIsWaitingForBillingNormalization) return; On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > 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(). 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()----/ https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:166: mTempsCvc = ""; On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > 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. Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:31: * Called when loading the instrument details. On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > Please elaborate a bit more on this comment. This callback is important because > the instrument is not showing any UI, but the onInstrumentDetailsReady/Error() > has not been called yet. So the PaymentRequestUI should show it's own > "Loading..." UI at this time. Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:33: void loadingInstrumentDetails(); On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > Let's move this callback to be the first one in the interface. When thinking > about the time progression, this is called before > onInstrumentDetailsReady/Error(). So it would make sense to also place > "loading..." callback above these "ready" and "error" callbacks. Done. https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:73: PaymentResponseHelper.PaymentResponseRequesterDelegate { On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > Eeek. Don't let Dan see this. Android engineers don't like this style of > indent... :-( > > That is, please change back this indentation. Done.
Hi Rouslan, thanks for the comments. Another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looking better! (Sorry for so many comments...) https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:73: public void onFullCardDetails(CreditCard unusedCard, String cvc) { On 2016/10/18 14:38:19, sebsg wrote: > On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > > 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 :-). > > 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. 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. https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:67: // The billing address should never be null for a credit card. ... at this point. (A credit card can have a null billing address initially, but PaymentRequest UI will force user to set a non-null billing address before the user can use that card. This method is called when the user clicks Pay with this card selected as the payment instrument.) https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:95: // TODO(crbug.com/656981): Update the credit card expiration on file. crbug.com/656981 should be fixed in full_card_request.cc instead. https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:114: sendIntrumentDetails(); Instead of early-returning immediately before the end of a function, a better style could be if-else statement. Consider: void stuff() { if (x) { doY(); return; } doZ(); } vs: void stuff() { if (x) doY(); else doZ(); } I think the latter is more clear to understand. Early returns are useful if there are multiple points in the function that can prevent it from full completion. What do you think? https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:128: sendIntrumentDetails(); Shorter: if (!mIsWatingForFullCardDetails) sendInstrumentDetails(); https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:177: mSecurityCode = ""; Remove https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:179: } } finally { mSecurityCode = ""; } https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:184: mSecurityCode = ""; Remove https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:23: * Called when loading the instrument details to tell the Payment Request UI to show a s/when loading the instrument details/by the credit card payment instrument when CVC has been unmasked, but billing address has not been normalized yet/ https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java (right): https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:41: public PaymentResponseHelper(PaymentOption selectedShippingAddress, Everything public should have javadoc comments. https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:50: // Contacts are created in show(). These should all be instances of AutofillContact. s/show()/PaymentRequestImpl.init()/ https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:63: // Shipping addresses are created in show(). These should all be instances of Ditto https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:80: boolean willNormalizeAsync = PersonalDataManager.getInstance().normalizeAddress( Can you do the same thing here? mIsWaitingForShippingNormalization = PersonalDataManager().getInstance().normalizeAddress(selectedAddress.getProfile().getGUID(), AutofillAddress.getCountryCode(selectedAutofillAddress.getProfile()), this); https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:97: public void onInstrumentDetailsReceived(String methodName, String stringifiedDetails) { javadoc https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:104: if (mIsWaitingForShippingNormalization) return; if (!waiting) delegate.ready(response); https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:115: if (profile != null && !TextUtils.isEmpty(profile.getGUID())) { You check for empty GUID here, but you don't do this in AutofillPaymentInstrument. The code should either be identical or there should be a comment explaining why empty GUID check is necessary. https://codereview.chromium.org/2413533003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:122: if (mIsWaitingForPaymentsDetails) return; if (!waiting) delegate.ready(response); https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:270: class AddressNormalizationRequester personal_data_manager_android controls the life-cycle of this object in entirety, because only personal_data_manager_android is the one that calls AddressNormalizationRequester::OnRulesSuccessfullyLoaded(). Therefore, let's not make this object self-deleting. (Sorry for the "drive-by".) Instead, make personal_data_manager_android own this object. The easiest way to accomplish that is through two steps: (1) Remove "delete this" on line 298. (2) Change container data structure for these objects to use smart pointers std::unique_ptr<Delegate> instead of raw pointers Delegate*. Smart pointers self-delete when they go out of scope. They also self-delete when you remove them from their container. https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:584: personal_data_manager_->UpdateProfileForTest(*profile); No tests here, so let's not add this call until we know for sure that we need it. https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:629: personal_data_manager_->UpdateCreditCardForTest(*card); Ditto https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:669: std::map<std::string, std::vector<Delegate*>>::iterator it = s/std::map<std::string, std::vector<Delegate*>>::iterator/auto/ https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:672: // Each Delegate will self delete after normalizing. Remove this comment, please. https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:673: std::vector<Delegate*> delegates = it->second; std::vector<std::unique_ptr<Delegate>> https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:674: for (size_t i = 0; i < it->second.size(); ++i) s/it->second/delegates/ https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:675: it->second[i]->OnRulesSuccessfullyLoaded(); s/it->second/delegates/ https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:676: pending_normalization_.erase(it); This is the line that deletes the Delegate objects. Or, at least, it should. Put a LOG(ERROR)<<"DELETE"; in the destructor of AddressNormalizationRequester temporarily to verify. https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:689: Delegate* requester = new AddressNormalizationRequester( std::unique_ptr<Delegate> requester = ... https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:702: std::pair<std::string, std::vector<Delegate*>>( pending_normalization_.insert(std::make_pair(region_code, {std::move(requester)})); {requester} is "uniform initialization syntax". It's a C++11 feature that's allowed in Chromium: https://chromium-cpp.appspot.com/. If it does not work for some reason, then do this instead: pending_normalization_.insert(std::make_pair(region_code, std::vector<Delegate*>(1, std::move(requester)))); In either case, you don't need the extra .find() and .push_back() calls on lines 706-708. https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:745: for (auto it = pending_normalization_.begin(); Since PersonalDataManagerAndroid now owns all of the Delegate objects, simply clearning the pending_normalization_ container will delete all of the Delegate objects inside of it. https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2413533003/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:345: std::map<std::string, std::vector<Delegate*>> pending_normalization_; Raw pointers are scary because of possible memory leaks. It looks like PersonalDataManagerAndroid actually controls the life-cycle of Delegate objects entirely. Therefore, let's change this data structure to own its own elements instead. See more explanation about this in .cc file. std::map<std::string, std::vector<std::unique_ptr<Delegate>>> pending_normalization_; https://codereview.chromium.org/2413533003/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2413533003/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:174: void UpdateProfileForTest(const AutofillProfile& profile); No tests in this patch, so let's not add test scaffolding prematurely.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #3 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Another look? https://chromiumcodereview.appspot.com/2413533003/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:73: public void onFullCardDetails(CreditCard unusedCard, String cvc) { On 2016/10/18 17:30:30, rouslan (slow 10.10-10.17) wrote: > On 2016/10/18 14:38:19, sebsg wrote: > > On 2016/10/17 20:44:18, rouslan (slow 10.10-10.17) wrote: > > > 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 :-). > > > > 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. > > 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. 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). https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:67: // The billing address should never be null for a credit card. On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > ... at this point. > > > (A credit card can have a null billing address initially, but PaymentRequest UI > will force user to set a non-null billing address before the user can use that > card. This method is called when the user clicks Pay with this card selected as > the payment instrument.) Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:95: // TODO(crbug.com/656981): Update the credit card expiration on file. On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > crbug.com/656981 should be fixed in full_card_request.cc instead. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:114: sendIntrumentDetails(); On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > Instead of early-returning immediately before the end of a function, a better > style could be if-else statement. Consider: > > void stuff() { > if (x) { > doY(); > return; > } > > doZ(); > } > > > vs: > > void stuff() { > if (x) > doY(); > else > doZ(); > } > > I think the latter is more clear to understand. Early returns are useful if > there are multiple points in the function that can prevent it from full > completion. What do you think? Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:128: sendIntrumentDetails(); On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > Shorter: > > if (!mIsWatingForFullCardDetails) sendInstrumentDetails(); Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:177: mSecurityCode = ""; On 2016/10/18 17:30:30, rouslan (slow 10.10-10.17) wrote: > Remove Wow, I still thought I had to keep that info before calling the callback (like when I still used a mTempCard)... That's why I didn't do the finally... Sorry! https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:179: } On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > } finally { > mSecurityCode = ""; > } Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:184: mSecurityCode = ""; On 2016/10/18 17:30:30, rouslan (slow 10.10-10.17) wrote: > Remove Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentInstrument.java:23: * Called when loading the instrument details to tell the Payment Request UI to show a On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > s/when loading the instrument details/by the credit card payment instrument when > CVC has been unmasked, but billing address has not been normalized yet/ This is very specific. I left it more general so it could be used by other PaymentInstruments while they do some background tasks. You know more about all of that though so I'll defer to your take on this. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:41: public PaymentResponseHelper(PaymentOption selectedShippingAddress, On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > Everything public should have javadoc comments. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:50: // Contacts are created in show(). These should all be instances of AutofillContact. On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > s/show()/PaymentRequestImpl.init()/ Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:63: // Shipping addresses are created in show(). These should all be instances of On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > Ditto Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:80: boolean willNormalizeAsync = PersonalDataManager.getInstance().normalizeAddress( On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > Can you do the same thing here? > > mIsWaitingForShippingNormalization = > PersonalDataManager().getInstance().normalizeAddress(selectedAddress.getProfile().getGUID(), > AutofillAddress.getCountryCode(selectedAutofillAddress.getProfile()), this); Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:97: public void onInstrumentDetailsReceived(String methodName, String stringifiedDetails) { On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > javadoc Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:104: if (mIsWaitingForShippingNormalization) return; On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > if (!waiting) delegate.ready(response); Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:115: if (profile != null && !TextUtils.isEmpty(profile.getGUID())) { On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > You check for empty GUID here, but you don't do this in > AutofillPaymentInstrument. The code should either be identical or there should > be a comment explaining why empty GUID check is necessary. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:122: if (mIsWaitingForPaymentsDetails) return; On 2016/10/18 17:30:31, rouslan (slow 10.10-10.17) wrote: > if (!waiting) delegate.ready(response); Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:270: class AddressNormalizationRequester On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > personal_data_manager_android controls the life-cycle of this object in > entirety, because only personal_data_manager_android is the one that calls > AddressNormalizationRequester::OnRulesSuccessfullyLoaded(). Therefore, let's not > make this object self-deleting. (Sorry for the "drive-by".) Instead, make > personal_data_manager_android own this object. The easiest way to accomplish > that is through two steps: > > (1) Remove "delete this" on line 298. > > (2) Change container data structure for these objects to use smart pointers > std::unique_ptr<Delegate> instead of raw pointers Delegate*. Smart pointers > self-delete when they go out of scope. They also self-delete when you remove > them from their container. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:584: personal_data_manager_->UpdateProfileForTest(*profile); On 2016/10/18 17:30:32, rouslan wrote: > No tests here, so let's not add this call until we know for sure that we need > it. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:629: personal_data_manager_->UpdateCreditCardForTest(*card); On 2016/10/18 17:30:32, rouslan wrote: > Ditto Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:669: std::map<std::string, std::vector<Delegate*>>::iterator it = On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > s/std::map<std::string, std::vector<Delegate*>>::iterator/auto/ Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:672: // Each Delegate will self delete after normalizing. On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > Remove this comment, please. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:673: std::vector<Delegate*> delegates = it->second; On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > std::vector<std::unique_ptr<Delegate>> Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:674: for (size_t i = 0; i < it->second.size(); ++i) On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > s/it->second/delegates/ Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:675: it->second[i]->OnRulesSuccessfullyLoaded(); On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > s/it->second/delegates/ Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:676: pending_normalization_.erase(it); On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > This is the line that deletes the Delegate objects. Or, at least, it should. Put > a LOG(ERROR)<<"DELETE"; in the destructor of AddressNormalizationRequester > temporarily to verify. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:689: Delegate* requester = new AddressNormalizationRequester( On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > std::unique_ptr<Delegate> requester = ... Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:702: std::pair<std::string, std::vector<Delegate*>>( On 2016/10/18 17:30:32, rouslan wrote: > pending_normalization_.insert(std::make_pair(region_code, > {std::move(requester)})); > > {requester} is "uniform initialization syntax". It's a C++11 feature that's > allowed in Chromium: https://chromium-cpp.appspot.com/. If it does not work for > some reason, then do this instead: > > pending_normalization_.insert(std::make_pair(region_code, > std::vector<Delegate*>(1, std::move(requester)))); > > In either case, you don't need the extra .find() and .push_back() calls on lines > 706-708. It seems you cannot use initializer lists with move semantics (it takes everything as a const ref) But I got rid of the second find. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:745: for (auto it = pending_normalization_.begin(); On 2016/10/18 17:30:32, rouslan wrote: > Since PersonalDataManagerAndroid now owns all of the Delegate objects, simply > clearning the pending_normalization_ container will delete all of the Delegate > objects inside of it. Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://chromiumcodereview.appspot.com/2413533003/diff/160001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.h:345: std::map<std::string, std::vector<Delegate*>> pending_normalization_; On 2016/10/18 17:30:32, rouslan (slow 10.10-10.17) wrote: > Raw pointers are scary because of possible memory leaks. It looks like > PersonalDataManagerAndroid actually controls the life-cycle of Delegate objects > entirely. Therefore, let's change this data structure to own its own elements > instead. See more explanation about this in .cc file. > > std::map<std::string, std::vector<std::unique_ptr<Delegate>>> > pending_normalization_; Done. https://chromiumcodereview.appspot.com/2413533003/diff/160001/components/auto... File components/autofill/core/browser/personal_data_manager.h (right): https://chromiumcodereview.appspot.com/2413533003/diff/160001/components/auto... components/autofill/core/browser/personal_data_manager.h:174: void UpdateProfileForTest(const AutofillProfile& profile); On 2016/10/18 17:30:32, rouslan wrote: > No tests in this patch, so let's not add test scaffolding prematurely. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comments. Most of the comments are optional nits. Feel free to ignore me. I will implement them myself, if I find the time. Or you can implement them in follow up, if you like. One exception is the last comment about Update*ForTest. That one is a must :-) https://codereview.chromium.org/2413533003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2413533003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:33: private final Handler mHandler = new Handler(); Please remove this for reasons outlined below. https://codereview.chromium.org/2413533003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:82: mHandler.postDelayed(new Runnable() { This Handler is going to be accessed only once and only for one of the AutofillPaymentInstrument objects. Because it's expensive to create new objects on Android, let's instantiate the Handler right here. For a user with 4 credit cards, for example, this will reduce the number Handler objects by 3. new Handler().postDelaued(...) https://codereview.chromium.org/2413533003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:87: }, PersonalDataManager.getInstance().getNormalizationTimeoutMS()); We timebox normalization to 5 seconds, but unmasking the card might take 10 seconds. We should let normalization run the full span of unmasking period and only start the timer after the card has been unmasked. I.e., move the "if(mIsWaitingForBillingNormalization){startTimer()}" block inside of onFullCardDetails(). https://codereview.chromium.org/2413533003/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java (right): https://codereview.chromium.org/2413533003/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:89: if (mIsWaitingForShippingNormalization) { Same comment as in AutofillPaymentInstrument: getting instrument details can take 10 seconds. We should let the normalization run the full 10 seconds, in case if that happens. Therefore, the "if(waiting){startTimer();}" block should be moved to onInstrumentDetailsReceived(). https://codereview.chromium.org/2413533003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestUseStatsTest.java (right): https://codereview.chromium.org/2413533003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestUseStatsTest.java:71: assertEquals(2, mHelper.getCreditCardUseCountForTesting(mCreditCardId)); What's the difference between going from 10 to 11 and 1 to 2 for use counts? https://codereview.chromium.org/2413533003/diff/200001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2413533003/diff/200001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:174: void UpdateProfileForTest(const AutofillProfile& profile); Please remove these two declarations. They don't have definitions and are not used.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:220001) has been deleted
Hi Rouslan, thanks for the suggestions, see the responses inline. https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:33: private final Handler mHandler = new Handler(); On 2016/10/19 19:10:59, rouslan wrote: > Please remove this for reasons outlined below. Done. https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:82: mHandler.postDelayed(new Runnable() { On 2016/10/19 19:10:59, rouslan wrote: > This Handler is going to be accessed only once and only for one of the > AutofillPaymentInstrument objects. Because it's expensive to create new objects > on Android, let's instantiate the Handler right here. For a user with 4 credit > cards, for example, this will reduce the number Handler objects by 3. > > new Handler().postDelaued(...) Done. https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:87: }, PersonalDataManager.getInstance().getNormalizationTimeoutMS()); On 2016/10/19 19:10:59, rouslan wrote: > We timebox normalization to 5 seconds, but unmasking the card might take 10 > seconds. We should let normalization run the full span of unmasking period and > only start the timer after the card has been unmasked. I.e., move the > "if(mIsWaitingForBillingNormalization){startTimer()}" block inside of > onFullCardDetails(). Done. https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentResponseHelper.java:89: if (mIsWaitingForShippingNormalization) { On 2016/10/19 19:10:59, rouslan wrote: > Same comment as in AutofillPaymentInstrument: getting instrument details can > take 10 seconds. We should let the normalization run the full 10 seconds, in > case if that happens. Therefore, the "if(waiting){startTimer();}" block should > be moved to onInstrumentDetailsReceived(). The only problem with this one is that the normalization timeouts are now sequential instead of parralel. if the rules don't load, you'll wait after the unmask. Then after you finished waiting for that you start the timer for the Shipping. Instead maybe the Payment response could get a call from the PaymentRequestImpl when it gets loadingInstrumentDetails() called? This is called after the unmask so I think it would be the best moment. What do you think? https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestUseStatsTest.java (right): https://chromiumcodereview.appspot.com/2413533003/diff/200001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestUseStatsTest.java:71: assertEquals(2, mHelper.getCreditCardUseCountForTesting(mCreditCardId)); On 2016/10/19 19:10:59, rouslan wrote: > What's the difference between going from 10 to 11 and 1 to 2 for use counts? That's what I was talking about offline. The difference is that setting 10 will be overwritten by the refresh when normalizing the billing address of the card. So it will be reset to 1. This is only a test problem though because setting a use count like this never happens in prod. The goal of this test is to test the RecordAndLogUse which is used in prod (only way to change the use count). For this purpose, the test still works. The alternative would be saving the data to disk in the tests, which we thought was not a good idea until we figure out what happens with server cards and etc. https://chromiumcodereview.appspot.com/2413533003/diff/200001/components/auto... File components/autofill/core/browser/personal_data_manager.h (right): https://chromiumcodereview.appspot.com/2413533003/diff/200001/components/auto... components/autofill/core/browser/personal_data_manager.h:174: void UpdateProfileForTest(const AutofillProfile& profile); On 2016/10/19 19:10:59, rouslan wrote: > Please remove these two declarations. They don't have definitions and are not > used. Done.
sebsg@chromium.org changed reviewers: + dfalcantara@chromium.org, mathp@chromium.org
mathp@chromium.org: Please review changes in chrome/browser/* components/autofill/* dfalcantara@chromium.org: Please review changes in PersonalDataManager.java and PaymentRequestUseStatsTest.java Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/20 17:08:37, sebsg wrote: > mailto:mathp@chromium.org: Please review changes in > chrome/browser/* > components/autofill/* > > mailto:dfalcantara@chromium.org: Please review changes in > PersonalDataManager.java and PaymentRequestUseStatsTest.java > > Thanks! autofill bits lgtm
lgtm for files you asked me to look at
The CQ bit was checked by sebsg@chromium.org
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/2413533003/#ps240001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2413533003/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Payments] Normalize billing address before sending to the merchant. BUG=654773 ========== to ========== [Payments] Normalize billing address before sending to the merchant. BUG=654773 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [Payments] Normalize billing address before sending to the merchant. BUG=654773 ========== to ========== [Payments] Normalize billing address before sending to the merchant. BUG=654773 Committed: https://crrev.com/8081c02908534b12968e52de312c2b7ca7b19456 Cr-Commit-Position: refs/heads/master@{#427242} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8081c02908534b12968e52de312c2b7ca7b19456 Cr-Commit-Position: refs/heads/master@{#427242} |
