Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2338283003:
[Payments] Normalize addresses before passing them to merchants. (Closed)
|
Created:
4 years, 3 months ago by sebsg Modified:
4 years, 2 months ago Reviewers:
Lei Zhang, please use gerrit instead, Evan Stade, vabr (Chromium), Nicolas Zea, gone, Justin Donnelly CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNormalizes the state and city of the selected address before sending the information to the merchant.
BUG=641051
Committed: https://crrev.com/be9290202c973b5be6da49150c8ce8eb0334ed93
Cr-Commit-Position: refs/heads/master@{#423162}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Async #
Total comments: 64
Patch Set 3 : Addressed comments #
Total comments: 10
Patch Set 4 : Addressed vabr@'s comments #
Total comments: 13
Patch Set 5 : Addressed dfalcantara's comments #Patch Set 6 : Addressed comment #
Total comments: 4
Patch Set 7 : Addressed thestig's comments #Patch Set 8 : Rebase #Patch Set 9 : Fixed tests #Patch Set 10 : iOS fix #Patch Set 11 : Test timeout #Patch Set 12 : Changed where URLRequestContextGetter is set #
Total comments: 1
Patch Set 13 : import order #Messages
Total messages: 91 (64 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Payments] Normalize addresses before passing them to merchants. BUG= ========== to ========== Normalizes the state and city of the selected address before sending the information to the merchant. BUG=641051 ==========
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, could you please take a look? Thanks!
Some initial comments. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:301: // Start loading the validation rules for the regions associated with the profiles. Put this block after line 291, which is the last line where "addresses" is modified. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:302: for (AutofillAddress address : addresses) { Use index loop. It's faster on Android. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1042: if (!normalizedProfile.getGUID().equals("")) { If GUID cannot be null, use .isEmpty() instead of .equals(""). It's clearer. If GUID can be null, use TextUtils.isEmpty(guid), because that checks for null as well. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:562: assertTrue(duration < 6000); Yikes! Nuke this test, quick! These tests will flake all the time. It's better to write a doc for humans to test this. https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:22: #include "chrome/browser/autofill/android/mock_address_validator.h" Yah, we can't include test code from production code. Sorry! https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:625: const base::android::JavaParamRef<jstring>& jregion_code) { This code might go away after you make normalization async, but nevertheless here's a shorter way to write this function: return loaded_rules_.find(ConvertJavaStringToUTF8(env, jregion_code)) != loaded_rules_.end(); https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:654: for (int i = 0; i < 5; ++i) { /me closes his eyes. I'm going to pretend I never saw this :-P https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:671: // Set the normalized state and city. Normalize the dependent locality as well, please. https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:285: base::android::ScopedJavaLocalRef<jobject> GetNormalizedProfile( Make async plz. https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/BUILD.gn (right): https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... third_party/libaddressinput/BUILD.gn:187: defines = [ "TEST_DATA_DIR=\"third_party/libaddressinput/src/testdata\"" ] Tests on bots run in such a way that you need to make your TEST_DATA_DIR be src/third_party/libaddressinput/src/testdata. https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:123: address->locality = tmp.locality; dedepdent_locality as well https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.h:138: // Normalizwes the administrative area and the locality in |address_data|. For s/the administrative area and the locality in/the/
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Another look? Thanks! https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:301: // Start loading the validation rules for the regions associated with the profiles. On 2016/09/16 22:30:41, rouslan wrote: > Put this block after line 291, which is the last line where "addresses" is > modified. > Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:302: for (AutofillAddress address : addresses) { On 2016/09/16 22:30:41, rouslan wrote: > Use index loop. It's faster on Android. Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1042: if (!normalizedProfile.getGUID().equals("")) { On 2016/09/16 22:30:41, rouslan wrote: > If GUID cannot be null, use .isEmpty() instead of .equals(""). It's clearer. If > GUID can be null, use TextUtils.isEmpty(guid), because that checks for null as > well. Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:562: assertTrue(duration < 6000); On 2016/09/16 22:30:41, rouslan wrote: > Yikes! Nuke this test, quick! These tests will flake all the time. It's better > to write a doc for humans to test this. Haha, yeah this was not meant to get into production. It was the way I used to test and I kept it to show you. I'll write that doc! https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:22: #include "chrome/browser/autofill/android/mock_address_validator.h" On 2016/09/16 22:30:41, rouslan wrote: > Yah, we can't include test code from production code. Sorry! Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:625: const base::android::JavaParamRef<jstring>& jregion_code) { On 2016/09/16 22:30:41, rouslan (slow) wrote: > This code might go away after you make normalization async, but nevertheless > here's a shorter way to write this function: > > return loaded_rules_.find(ConvertJavaStringToUTF8(env, jregion_code)) != > loaded_rules_.end(); Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:654: for (int i = 0; i < 5; ++i) { On 2016/09/16 22:30:42, rouslan (slow) wrote: > /me closes his eyes. > > I'm going to pretend I never saw this :-P Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:671: // Set the normalized state and city. On 2016/09/16 22:30:41, rouslan wrote: > Normalize the dependent locality as well, please. Done. https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:285: base::android::ScopedJavaLocalRef<jobject> GetNormalizedProfile( On 2016/09/16 22:30:42, rouslan (slow) wrote: > Make async plz. Done. https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/BUILD.gn (right): https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... third_party/libaddressinput/BUILD.gn:187: defines = [ "TEST_DATA_DIR=\"third_party/libaddressinput/src/testdata\"" ] On 2016/09/16 22:30:42, rouslan (slow) wrote: > Tests on bots run in such a way that you need to make your TEST_DATA_DIR be > src/third_party/libaddressinput/src/testdata. Done. https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:123: address->locality = tmp.locality; On 2016/09/16 22:30:42, rouslan wrote: > dedepdent_locality as well Done. https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.h (right): https://codereview.chromium.org/2338283003/diff/40001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.h:138: // Normalizwes the administrative area and the locality in |address_data|. For On 2016/09/16 22:30:42, rouslan wrote: > s/the administrative area and the locality in/the/ Done.
Great improvements! Please also normalize mBillingAddress in https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:63: Put NormalizedAddressRequester here. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:482: private NormalizedAddressRequester mRequester; Don't save the pointer here. Follow the pattern of FullCardRequestDelegate, which is passed to native code. See getFullCard() method for example. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:742: public void startAddressNormalizationTask( "normalizeAddress()" is a simpler name. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:752: public void cancelAddressNormalizationTask() { "cancelPendingAddressNormalization()" https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:70: public class PaymentRequestImpl Please don't put a newline before "implements". https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:197: private Handler mNormalizationTimer; Reuse mHandler instead. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:198: private static final int NORMALIZATION_TIMEOUT_MS = 5000; Put all "private static final" variables together. (Line 112 for example.) https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:305: } If you have 4 address from "CA", then you will call loadRulesForRegion("CA") 4 times, whereas it should really be called only once. Please put the country codes into a Set and iterate over that. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:936: mPendingInstruments.addAll(mPendingAutofillInstruments); mPendingAutofillInstruments is a list of credit cards, whose billing addresses should also be normalized. Let's load the rules for the country codes of these billing addresses as well. You can do it here. Use Set to avoid duplicate calls to loadRulesForRegion(). https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1054: Write "mUI.showProcessingMessage();" here and remove the two instances of it below. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1074: mIsPendingResponseProfileComplete = selectedAutofillAddress.isComplete(); This should always be true. In fact you can write "assert selectedAutofillAddress.isComplete();" if you want to feel safer. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1080: if (!mIsWaitingForNormalization) { Seems like incorrect logic here. if (!mIsWaitingForNormalization) return; Right? https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1082: mNormalizationTimer = new Handler(); Reuse mHandler. No need to create a new object. mHandler.postDelayed(new Runnable() { @Override public void run() { onAddressNormalized(null); } }, NORMALIZATION_TIMEOUT_MS); https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1122: new AutofillAddress(profile, mIsPendingResponseProfileComplete) s/mIsPendingResponseProfileComplete/true/ ...because this address is always complete. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1185: mIsWaitingForNormalization = false; Did you find this necessary? If this does not solve a problem that you've encountered, let's not add it. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:199: class FullCardRequester : public payments::FullCardRequest::Delegate, Is it possible to use this pattern of self-deleting object? https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:641: pending_normalization_profile_guid_ = ""; pending_normalization_profile_guid_.clear(); https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:642: pending_normalization_region_code_ = ""; pending_normalization_region_code_.clear(); https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:683: if (pending_normalization_profile_guid_ != "" && !pending_normalization_profile_guid_.empty() https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:684: pending_normalization_region_code_ != "") { !pending_normalization_region_code_.empty() https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:687: pending_normalization_profile_guid_ = ""; .clear() https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:688: pending_normalization_region_code_ = ""; .clear() https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:16: #include "third_party/libaddressinput/chromium/chrome_storage_impl.h" Includes for lines 14-16 should go into the *.cc file. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:257: // These functions help address normalization. // -------------------- https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:299: bool AreRulesDoneLoadingForRegion(std::string region_code); const std::string& ... to avoid string copies, which are expensive in C++. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:304: bool AreRulesSuccessfullyLoadedForRegion(std::string region_code); const std::string& https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:309: void NormalizeAddress(std::string guid, std::string region_code); const std::string& https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... File chrome/browser/autofill/personal_data_manager_factory.cc (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager_factory.cc:53: ->GetURLRequestContext()); Getting the URL request context from the storage partition seems illogical. Can you find a better way to retrieve the URL request context? https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:46: #include "net/url_request/url_request_context_getter.h" Is this include necessary? https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:27: #include "net/url_request/url_fetcher_delegate.h" Is this include necessary? https://codereview.chromium.org/2338283003/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2338283003/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:116: if (!supplier_->IsLoaded(address->region_code)) If you expose supplier_->IsLoaded(region_code) through a method like AddressValidator::AreRulesLoadedForRegion(region_codes), then you can remove the "loaded_rules_" mapping in personal_data_manager_android.cc. https://codereview.chromium.org/2338283003/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:124: address->dependent_locality = tmp.dependent_locality; You can simplify lines 119-124 now: normalizer_->Normalize(address);
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
sebsg@chromium.org changed reviewers: + dfalcantara@chromium.org, vabr@chromium.org
dfalcantara@chromium.org: Could you please review the changes in chrome/android ? vabr@chromium.org: Could you please review the changes in chrome/browser/autofill and components/autofill ? Thanks! https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:63: On 2016/09/23 09:04:39, rouslan (OOO 9.23-10.10) wrote: > Put NormalizedAddressRequester here. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:482: private NormalizedAddressRequester mRequester; On 2016/09/23 09:04:39, rouslan (OOO 9.23-10.10) wrote: > Don't save the pointer here. Follow the pattern of FullCardRequestDelegate, > which is passed to native code. See getFullCard() method for example. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:742: public void startAddressNormalizationTask( On 2016/09/23 09:04:39, rouslan (OOO 9.23-10.10) wrote: > "normalizeAddress()" is a simpler name. I wanted the name to reflect the fact that it can be async. Would normalizeAddressAsync() be good? https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:752: public void cancelAddressNormalizationTask() { On 2016/09/23 09:04:39, rouslan (OOO 9.23-10.10) wrote: > "cancelPendingAddressNormalization()" Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:70: public class PaymentRequestImpl On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Please don't put a newline before "implements". Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:197: private Handler mNormalizationTimer; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Reuse mHandler instead. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:198: private static final int NORMALIZATION_TIMEOUT_MS = 5000; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Put all "private static final" variables together. (Line 112 for example.) Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:305: } On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > If you have 4 address from "CA", then you will call loadRulesForRegion("CA") 4 > times, whereas it should really be called only once. Please put the country > codes into a Set and iterate over that. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:936: mPendingInstruments.addAll(mPendingAutofillInstruments); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > mPendingAutofillInstruments is a list of credit cards, whose billing addresses > should also be normalized. Let's load the rules for the country codes of these > billing addresses as well. You can do it here. Use Set to avoid duplicate calls > to loadRulesForRegion(). To normalize these addresses too I'll need to change a few things. Since this CL is already pretty big, I would do it as a follow up CL. What do you think? https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1054: On 2016/09/23 09:04:39, rouslan (OOO 9.23-10.10) wrote: > Write "mUI.showProcessingMessage();" here and remove the two instances of it > below. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1074: mIsPendingResponseProfileComplete = selectedAutofillAddress.isComplete(); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > This should always be true. In fact you can write "assert > selectedAutofillAddress.isComplete();" if you want to feel safer. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1080: if (!mIsWaitingForNormalization) { On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Seems like incorrect logic here. > > if (!mIsWaitingForNormalization) return; > > Right? Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1082: mNormalizationTimer = new Handler(); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Reuse mHandler. No need to create a new object. > > mHandler.postDelayed(new Runnable() { > @Override > public void run() { > onAddressNormalized(null); > } > }, NORMALIZATION_TIMEOUT_MS); Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1122: new AutofillAddress(profile, mIsPendingResponseProfileComplete) On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > s/mIsPendingResponseProfileComplete/true/ > > ...because this address is always complete. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1185: mIsWaitingForNormalization = false; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Did you find this necessary? If this does not solve a problem that you've > encountered, let's not add it. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:199: class FullCardRequester : public payments::FullCardRequest::Delegate, On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Is it possible to use this pattern of self-deleting object? Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:641: pending_normalization_profile_guid_ = ""; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > pending_normalization_profile_guid_.clear(); Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:642: pending_normalization_region_code_ = ""; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > pending_normalization_region_code_.clear(); Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:683: if (pending_normalization_profile_guid_ != "" && On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > !pending_normalization_profile_guid_.empty() Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:684: pending_normalization_region_code_ != "") { On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > !pending_normalization_region_code_.empty() Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:687: pending_normalization_profile_guid_ = ""; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > .clear() Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:688: pending_normalization_region_code_ = ""; On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > .clear() Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:16: #include "third_party/libaddressinput/chromium/chrome_storage_impl.h" On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Includes for lines 14-16 should go into the *.cc file. The chrome_address_validator has to be there to implement LoadRulesListener but I moved the others to the .cc file. Thanks. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:257: On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > // These functions help address normalization. > // -------------------- Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:299: bool AreRulesDoneLoadingForRegion(std::string region_code); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > const std::string& > > > ... to avoid string copies, which are expensive in C++. Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:304: bool AreRulesSuccessfullyLoadedForRegion(std::string region_code); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > const std::string& Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:309: void NormalizeAddress(std::string guid, std::string region_code); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > const std::string& Done. https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... File chrome/browser/autofill/personal_data_manager_factory.cc (right): https://codereview.chromium.org/2338283003/diff/120001/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager_factory.cc:53: ->GetURLRequestContext()); On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Getting the URL request context from the storage partition seems illogical. Can > you find a better way to retrieve the URL request context? Done. https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:46: #include "net/url_request/url_request_context_getter.h" On 2016/09/23 09:04:40, rouslan (OOO 9.23-10.10) wrote: > Is this include necessary? I use a URLRequestContextGetter lower in the code. https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2338283003/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:27: #include "net/url_request/url_fetcher_delegate.h" On 2016/09/23 09:04:41, rouslan (OOO 9.23-10.10) wrote: > Is this include necessary? Done. https://codereview.chromium.org/2338283003/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2338283003/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:116: if (!supplier_->IsLoaded(address->region_code)) On 2016/09/23 09:04:41, rouslan (OOO 9.23-10.10) wrote: > If you expose supplier_->IsLoaded(region_code) through a method like > AddressValidator::AreRulesLoadedForRegion(region_codes), then you can remove the > "loaded_rules_" mapping in personal_data_manager_android.cc. Done. https://codereview.chromium.org/2338283003/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:124: address->dependent_locality = tmp.dependent_locality; On 2016/09/23 09:04:41, rouslan (OOO 9.23-10.10) wrote: > You can simplify lines 119-124 now: > > normalizer_->Normalize(address); Done.
chrome/browser/autofill and components/autofill LGTM. Thanks, Vaclav https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:320: address_validator_.reset(new AddressValidator( Unless |address_validator_| needs to be constructed after the call on line 318 (does not look so to me), then please consider aggregating it directly into PersonalDataManagerAndroid instead of indirectly via unique_ptr. Avoiding the extra heap allocation is a good practice for performance and memory efficiency, and it also clarifies the life-span of the object. If you do need unique_ptr, please use assignment and base::MakeUnique instead of reset and new, for both code readability and safety (see recent chromium-dev discussion on this topic). https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:670: // The AddressNormalizationRequester will self delete after normalizing. optional nit: Perhaps call it Delegate instead of AddressNormalizationRequester. While the latter is an implementation of the former, it might not be clear here without looking at line 685. https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:682: std::string region_code = ConvertJavaStringToUTF8(env, jregion_code); optional: Mark both strings as const? https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:311: // Returns whether the rules are loading for the specified |region_code|. nit: loading -> loaded ? https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/personal_data_manager_factory.cc (right): https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager_factory.cc:19: #include "content/public/browser/storage_partition.h" Why is this #include needed?
Thanks for the comments vabr@! https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:320: address_validator_.reset(new AddressValidator( On 2016/09/28 08:41:36, vabr (Chromium) wrote: > Unless |address_validator_| needs to be constructed after the call on line 318 > (does not look so to me), then please consider aggregating it directly into > PersonalDataManagerAndroid instead of indirectly via unique_ptr. Avoiding the > extra heap allocation is a good practice for performance and memory efficiency, > and it also clarifies the life-span of the object. > > If you do need unique_ptr, please use assignment and base::MakeUnique instead of > reset and new, for both code readability and safety (see recent chromium-dev > discussion on this topic). Thanks, coming from C# this was an interesting read! https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:670: // The AddressNormalizationRequester will self delete after normalizing. On 2016/09/28 08:41:36, vabr (Chromium) wrote: > optional nit: Perhaps call it Delegate instead of AddressNormalizationRequester. > While the latter is an implementation of the former, it might not be clear here > without looking at line 685. Done. https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:682: std::string region_code = ConvertJavaStringToUTF8(env, jregion_code); On 2016/09/28 08:41:36, vabr (Chromium) wrote: > optional: Mark both strings as const? Done. https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:311: // Returns whether the rules are loading for the specified |region_code|. On 2016/09/28 08:41:36, vabr (Chromium) wrote: > nit: loading -> loaded ? Done. https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/personal_data_manager_factory.cc (right): https://codereview.chromium.org/2338283003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/personal_data_manager_factory.cc:19: #include "content/public/browser/storage_partition.h" On 2016/09/28 08:41:36, vabr (Chromium) wrote: > Why is this #include needed? Done.
https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:746: * completion, the normalized profile will be sent to the {@code requester}. I don't think you have a |requester| anymore. https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1083: if (mIsWaitingForNormalization) { Given that this will set mIsWaitingForNormalization to true immediately above, when would this not trigger? If normalizeAddressAsync can actually return synchronously and set mIsWaitingForNormalization to false before this point, then you need to rename that function to reflect that. https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1114: if (!mIsWaitingForNormalization || mClient == null || mPendingPaymentResponse == null) { Should you play it safe and set mIsWaitingForNormalization = false here? Something like this: if (!mIsWaitingForNormalization) return; mIsWaitingForNormalization = false; if (mClient == null || mPendingPaymentResponse == null) return; https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/browser/... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:292: if (personal_data_manager_android_) { The only place the personal_data_manager_android_ seems to be set is in the constructor. If it's null before this thing is created, does this object need to be created at all? Did you mean to hold a weak pointer here, instead? https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.cc:714: // Set the normalized state, city and dependent locality. Don't know if this comment's too helpful; seems pretty obvious from the lines below. https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/browser/... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://chromiumcodereview.appspot.com/2338283003/diff/240001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.h:295: void CancelPendingAddressNormalization( Add a function comment here?
Patchset #5 (id:260001) has been deleted
Thanks for the comments, could you take another look? https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:746: * completion, the normalized profile will be sent to the {@code requester}. On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > I don't think you have a |requester| anymore. Done. https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1083: if (mIsWaitingForNormalization) { On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > Given that this will set mIsWaitingForNormalization to true immediately above, > when would this not trigger? If normalizeAddressAsync can actually return > synchronously and set mIsWaitingForNormalization to false before this point, > then you need to rename that function to reflect that. Ah I see, it's not clear enough. If the rules have finished loading when this is called, it will normalize synchronously, otherwise it will normalize when they have finished loading. So it can be either. Would something like normalizeAddressOrSetupTask work? https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1114: if (!mIsWaitingForNormalization || mClient == null || mPendingPaymentResponse == null) { On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > Should you play it safe and set mIsWaitingForNormalization = false here? > > Something like this: > > if (!mIsWaitingForNormalization) return; > mIsWaitingForNormalization = false; > > if (mClient == null || mPendingPaymentResponse == null) return; Done. https://codereview.chromium.org/2338283003/diff/240001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2338283003/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:292: if (personal_data_manager_android_) { On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > The only place the personal_data_manager_android_ seems to be set is in the > constructor. If it's null before this thing is created, does this object need > to be created at all? Did you mean to hold a weak pointer here, instead? Done. https://codereview.chromium.org/2338283003/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:714: // Set the normalized state, city and dependent locality. On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > Don't know if this comment's too helpful; seems pretty obvious from the lines > below. Done. https://codereview.chromium.org/2338283003/diff/240001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2338283003/diff/240001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:295: void CancelPendingAddressNormalization( On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > Add a function comment here? Done.
https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1083: if (mIsWaitingForNormalization) { On 2016/09/28 18:50:39, sebsg wrote: > On 2016/09/28 18:04:31, dfalcantara (slow) wrote: > > Given that this will set mIsWaitingForNormalization to true immediately above, > > when would this not trigger? If normalizeAddressAsync can actually return > > synchronously and set mIsWaitingForNormalization to false before this point, > > then you need to rename that function to reflect that. > > Ah I see, it's not clear enough. If the rules have finished loading when this is > called, it will normalize synchronously, otherwise it will normalize when they > have finished loading. So it can be either. > > Would something like normalizeAddressOrSetupTask work? I know you argued against it earlier, but I think that normalizeAddress() would still sense, given that it can be either async or sync. Would be better if normalizeAddress returned whether or not it was being handled asynchronously, which would let you set that boolean properly.
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
Thanks, The mIsWaitingForNormalization stills need to be set to true though, and the synchronous normalization will set it to false. Added the return value when starting the normalization for better readability.
Cool, lgtm.
sebsg@chromium.org changed reviewers: + estade@chromium.org, thestig@chromium.org
thestig@chromium.org: Could you please review chrome/browser/BUILD.gn ? estade@chromium.org: Could you please review third_party/libaddressinput/* ? Since Rouslan is OOO. Thanks!
https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (left): https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2517: "//third_party/libaddressinput", So Android is going to start using all of libaddressinput now? https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/DEPS#ne... chrome/browser/DEPS:78: "+third_party/libaddressinput", I'm not familiar enough to know if we can just do this, or if we should narrow the scope to some sub-directories of libaddressinput. Maybe the change should be in chrome/browser/autofill/DEPS instead? As is, this makes several DEPS rules inside of c/b/ redundent. e.g. those in chrome/browser/ui/autofill/DEPS.
Thanks for the comments Lei, another look? https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (left): https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2517: "//third_party/libaddressinput", On 2016/09/28 21:40:39, Lei Zhang wrote: > So Android is going to start using all of libaddressinput now? Pretty much. Payment Request will use it to normalize addresses. So it needs the validator and the rules loader for example. https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2338283003/diff/340001/chrome/browser/DEPS#ne... chrome/browser/DEPS:78: "+third_party/libaddressinput", On 2016/09/28 21:40:39, Lei Zhang wrote: > I'm not familiar enough to know if we can just do this, or if we should narrow > the scope to some sub-directories of libaddressinput. > > Maybe the change should be in chrome/browser/autofill/DEPS instead? > > As is, this makes several DEPS rules inside of c/b/ redundent. e.g. those in > chrome/browser/ui/autofill/DEPS. Thanks, I put it in chrome/browser/autofill/android since that's the only place that needs it.
lgtm
lgtm
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
sebsg@chromium.org changed reviewers: + zea@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
browser_sync lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sebsg@chromium.org changed reviewers: + jdonnelly@chromium.org
Hi Justin, could you please review ios/chrome/browser/autofill/personal_data_manager_factory.cc ? Thanks!
lgtm
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, dfalcantara@chromium.org, thestig@chromium.org, estade@chromium.org, zea@chromium.org, jdonnelly@chromium.org Link to the patchset: https://codereview.chromium.org/2338283003/#ps440001 (title: "Test timeout")
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
Exceeded global retry quota
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #13 (id:480001) has been deleted
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 #12 (id:460001) has been deleted
chrome/browser/autofill and components/autofill still LGTM in the patch set https://codereview.chromium.org/2338283003/#ps500001. Cheers, Vaclav
To fix the presubmit error: https://codereview.chromium.org/2338283003/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2338283003/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:51: import org.json.JSONException; According to tools/android/eclipse/android.importorder, lines 51 and 52 now need to precede the org.chromium section.
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 #13 (id:520001) has been deleted
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, estade@chromium.org, zea@chromium.org, dfalcantara@chromium.org, jdonnelly@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2338283003/#ps540001 (title: "import order")
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 ========== Normalizes the state and city of the selected address before sending the information to the merchant. BUG=641051 ========== to ========== Normalizes the state and city of the selected address before sending the information to the merchant. BUG=641051 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Normalizes the state and city of the selected address before sending the information to the merchant. BUG=641051 ========== to ========== Normalizes the state and city of the selected address before sending the information to the merchant. BUG=641051 Committed: https://crrev.com/be9290202c973b5be6da49150c8ce8eb0334ed93 Cr-Commit-Position: refs/heads/master@{#423162} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/be9290202c973b5be6da49150c8ce8eb0334ed93 Cr-Commit-Position: refs/heads/master@{#423162} |
