Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708933003:
[Payments] Add timeout to the address_normalizer. (Closed)
|
Created:
3 years, 10 months ago by sebsg Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, gogerald+paymentswatch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Add timeout to the address_normalizer.
Adds a timeout parameter to the AddressNormalizer so that the requester will always receive a response before that time limit (be it a successful normalization or not).
BUG=692643
Review-Url: https://codereview.chromium.org/2708933003
Cr-Commit-Position: refs/heads/master@{#452138}
Committed: https://chromium.googlesource.com/chromium/src/+/5a2f0a362c5c1257d5893f973eb6e2e6696faa04
Patch Set 1 #
Total comments: 40
Patch Set 2 : Addressed comments #Patch Set 3 : Updated BUILD file #
Total comments: 6
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Nits #Messages
Total messages: 40 (28 generated)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Payments] Add timeout to the address_normalizer. BUG=692643 ========== to ========== [Payments] Add timeout to the address_normalizer. Adds a timeout parameter to the AddressNormalizer so that the requester will always receive a response before that time limit (be it a successful normalization or not). BUG=692643 ==========
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...)
https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:129: if (mIsWaitingForBillingNormalization) return; Collapse 3 lines into 1: if (!mIsWaitingForBillingNormalization) sendIntrumentDetails(); https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:57: has_responded_ = true; This pattern is used to guard against OnRulesLoaded() being called multiple times. There's no valid use case for that happening. Therefore, it's a bug. Let's not add bug-handling logic to the code. Instead, let's fix the bug and remove |has_responded_|. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:143: // Start loading the rules for that region. If the rule were already in the s/ rule / rules / https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.h (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.h:53: // the requested will be informed and the request cancelled. This value should s/requested/requester/ https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.h:54: // be greated or equal to 0, for which it means that the normalization should s/greated or equal to/at least/ https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.h:55: // happen synchronously, or not at all. Will start loading the rules for the Please explain why 0 could be no normalization at all. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer_unittest.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:19: nit: no newline. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:25: using ::i18n::addressinput::TestdataSource; Somone's been reading up on C++, I see. Good job! :-D https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:32: Need to override the destructor. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:42: bool NormalizedCalled() { return normalized_called_; } nit: hacker_case for simple accessor. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:44: bool NotNormalizedCalled() { return not_normalized_called_; } ditto. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:52: class ChromiumTestdataSource : public TestdataSource { Why not use TestdataSource as-is? One caveat is that you need to run the components_unittests binary from chrome/ instead of chrome/src/ for TestdataSource to find the test data file. That's what trybots do, by the way. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:73: should_load_rules_(true) {} Need to override the destructor. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:104: DISALLOW_COPY_AND_ASSIGN(AddressNormalizerTest); This works only in a "private:" section. https://codereview.chromium.org/2708933003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/BUILD.gn (right): https://codereview.chromium.org/2708933003/diff/40001/third_party/libaddressi... third_party/libaddressinput/BUILD.gn:128: source_set("libaddressinput_unittests_utils") { Let's name this target "test_support", which is a common pattern in Chrome code.
mathp@chromium.org changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:131: sendIntrumentDetails(); Instrument? https://codereview.chromium.org/2708933003/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2708933003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:296: // case the the |jdelagate| will be notified. If the rules are loaded before jdelegate https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:123: new AddressNormalizationRequest(profile, region_code, timeout_seconds, let's use base::MakeUnique instead of new https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer_unittest.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:52: class ChromiumTestdataSource : public TestdataSource { On 2017/02/21 17:15:18, rouslan wrote: > Why not use TestdataSource as-is? One caveat is that you need to run the > components_unittests binary from chrome/ instead of chrome/src/ for > TestdataSource to find the test data file. That's what trybots do, by the way. We could try, but I'm pretty sure this won't work on all bots. Isolate/non-isolate both run things from separate dirs, and the hardcoded-ness of TestDataSource's path does not play well with this. This is why I suggested working around this issue.
On 2017/02/21 17:40:44, Mathieu Perreault wrote: > > Why not use TestdataSource as-is? One caveat is that you need to run the > > components_unittests binary from chrome/ instead of chrome/src/ for > > TestdataSource to find the test data file. That's what trybots do, by the way. > > We could try, but I'm pretty sure this won't work on all bots. > Isolate/non-isolate both run things from separate dirs, and the hardcoded-ness > of TestDataSource's path does not play well with this. This is why I suggested > working around this issue. Makes sense. Is there a good way to avoid the hard-codedness? Future users of libaddressinput will thank you ;-).
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...
Thanks guys, another look? https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:129: if (mIsWaitingForBillingNormalization) return; On 2017/02/21 17:15:12, rouslan wrote: > Collapse 3 lines into 1: > > if (!mIsWaitingForBillingNormalization) sendIntrumentDetails(); Done. https://codereview.chromium.org/2708933003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:131: sendIntrumentDetails(); On 2017/02/21 17:40:43, Mathieu Perreault wrote: > Instrument? Done. https://codereview.chromium.org/2708933003/diff/40001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2708933003/diff/40001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:296: // case the the |jdelagate| will be notified. If the rules are loaded before On 2017/02/21 17:40:43, Mathieu Perreault wrote: > jdelegate Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:57: has_responded_ = true; On 2017/02/21 17:15:12, rouslan wrote: > This pattern is used to guard against OnRulesLoaded() being called multiple > times. There's no valid use case for that happening. Therefore, it's a bug. > Let's not add bug-handling logic to the code. Instead, let's fix the bug and > remove |has_responded_|. With the current code, if the timeout happens first, when the rules are finally loaded or aborted this will be called a second time. This is why I added this so that the delegate only gets called once. Example, the server is not available and it takes 3 seconds for the rule loader to give up. Someone asks to normalize with a timeout of 2 seconds. In that case, the timeout will trigger then the rules loaded will return with success=false. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:123: new AddressNormalizationRequest(profile, region_code, timeout_seconds, On 2017/02/21 17:40:43, Mathieu Perreault wrote: > let's use base::MakeUnique instead of new Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:143: // Start loading the rules for that region. If the rule were already in the On 2017/02/21 17:15:12, rouslan wrote: > s/ rule / rules / Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.h (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.h:53: // the requested will be informed and the request cancelled. This value should On 2017/02/21 17:15:13, rouslan wrote: > s/requested/requester/ Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.h:54: // be greated or equal to 0, for which it means that the normalization should On 2017/02/21 17:15:12, rouslan wrote: > s/greated or equal to/at least/ Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.h:55: // happen synchronously, or not at all. Will start loading the rules for the On 2017/02/21 17:15:13, rouslan wrote: > Please explain why 0 could be no normalization at all. Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer_unittest.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:19: On 2017/02/21 17:15:16, rouslan wrote: > nit: no newline. Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:25: using ::i18n::addressinput::TestdataSource; On 2017/02/21 17:15:14, rouslan wrote: > Somone's been reading up on C++, I see. Good job! :-D Thanks :) https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:32: On 2017/02/21 17:15:16, rouslan wrote: > Need to override the destructor. Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:42: bool NormalizedCalled() { return normalized_called_; } On 2017/02/21 17:15:16, rouslan wrote: > nit: hacker_case for simple accessor. Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:44: bool NotNormalizedCalled() { return not_normalized_called_; } On 2017/02/21 17:15:15, rouslan wrote: > ditto. Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:52: class ChromiumTestdataSource : public TestdataSource { On 2017/02/21 17:15:18, rouslan wrote: > Why not use TestdataSource as-is? One caveat is that you need to run the > components_unittests binary from chrome/ instead of chrome/src/ for > TestdataSource to find the test data file. That's what trybots do, by the way. Acknowledged. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:52: class ChromiumTestdataSource : public TestdataSource { On 2017/02/21 17:40:43, Mathieu Perreault wrote: > On 2017/02/21 17:15:18, rouslan wrote: > > Why not use TestdataSource as-is? One caveat is that you need to run the > > components_unittests binary from chrome/ instead of chrome/src/ for > > TestdataSource to find the test data file. That's what trybots do, by the way. > > We could try, but I'm pretty sure this won't work on all bots. > Isolate/non-isolate both run things from separate dirs, and the hardcoded-ness > of TestDataSource's path does not play well with this. This is why I suggested > working around this issue. Acknowledged. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:73: should_load_rules_(true) {} On 2017/02/21 17:15:13, rouslan wrote: > Need to override the destructor. Done. https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer_unittest.cc:104: DISALLOW_COPY_AND_ASSIGN(AddressNormalizerTest); On 2017/02/21 17:15:19, rouslan wrote: > This works only in a "private:" section. Done. https://codereview.chromium.org/2708933003/diff/40001/third_party/libaddressi... File third_party/libaddressinput/BUILD.gn (right): https://codereview.chromium.org/2708933003/diff/40001/third_party/libaddressi... third_party/libaddressinput/BUILD.gn:128: source_set("libaddressinput_unittests_utils") { On 2017/02/21 17:15:20, rouslan wrote: > Let's name this target "test_support", which is a common pattern in Chrome code. Done.
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...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:57: has_responded_ = true; On 2017/02/21 18:30:59, sebsg wrote: > On 2017/02/21 17:15:12, rouslan wrote: > > This pattern is used to guard against OnRulesLoaded() being called multiple > > times. There's no valid use case for that happening. Therefore, it's a bug. > > Let's not add bug-handling logic to the code. Instead, let's fix the bug and > > remove |has_responded_|. > > With the current code, if the timeout happens first, when the rules are finally > loaded or aborted this will be called a second time. This is why I added this so > that the delegate only gets called once. > > Example, the server is not available and it takes 3 seconds for the rule loader > to give up. Someone asks to normalize with a timeout of 2 seconds. In that case, > the timeout will trigger then the rules loaded will return with success=false. Is it possible for rules to never load? If the rules never load, this object will continue occupying memory in pending_normalization_ map. I wonder if it's possible to delete this object upon timeout. https://codereview.chromium.org/2708933003/diff/80001/components/payments/BUI... File components/payments/BUILD.gn (right): https://codereview.chromium.org/2708933003/diff/80001/components/payments/BUI... components/payments/BUILD.gn:92: "//testing/gtest", nit: no need for ":browser", because "//components/autofill/core/browser:browser" is equivalent to "//components/autofill/core/browser". https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... File components/payments/address_normalizer.h (right): https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... components/payments/address_normalizer.h:55: // happen synchronously, or not at all if there rules are not already loaded. s/there/the/ https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... File components/payments/address_normalizer_unittest.cc (right): https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... components/payments/address_normalizer_unittest.cc:56: Override the destructor.
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 checked by sebsg@chromium.org to run a CQ dry run
Patchset #4 (id:100001) 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://codereview.chromium.org/2708933003/diff/40001/components/payments/add... File components/payments/address_normalizer.cc (right): https://codereview.chromium.org/2708933003/diff/40001/components/payments/add... components/payments/address_normalizer.cc:57: has_responded_ = true; On 2017/02/21 19:56:16, rouslan wrote: > On 2017/02/21 18:30:59, sebsg wrote: > > On 2017/02/21 17:15:12, rouslan wrote: > > > This pattern is used to guard against OnRulesLoaded() being called multiple > > > times. There's no valid use case for that happening. Therefore, it's a bug. > > > Let's not add bug-handling logic to the code. Instead, let's fix the bug and > > > remove |has_responded_|. > > > > With the current code, if the timeout happens first, when the rules are > finally > > loaded or aborted this will be called a second time. This is why I added this > so > > that the delegate only gets called once. > > > > Example, the server is not available and it takes 3 seconds for the rule > loader > > to give up. Someone asks to normalize with a timeout of 2 seconds. In that > case, > > the timeout will trigger then the rules loaded will return with success=false. > > Is it possible for rules to never load? If the rules never load, this object > will continue occupying memory in pending_normalization_ map. I wonder if it's > possible to delete this object upon timeout. As discussed offline, The rule loader will always return, but in the case where it returns after a timeout has occured, we don't want to call the delegate twice. Added comment. Thanks! https://codereview.chromium.org/2708933003/diff/80001/components/payments/BUI... File components/payments/BUILD.gn (right): https://codereview.chromium.org/2708933003/diff/80001/components/payments/BUI... components/payments/BUILD.gn:92: "//testing/gtest", On 2017/02/21 19:56:16, rouslan wrote: > nit: no need for ":browser", because > "//components/autofill/core/browser:browser" is equivalent to > "//components/autofill/core/browser". Done. https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... File components/payments/address_normalizer.h (right): https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... components/payments/address_normalizer.h:55: // happen synchronously, or not at all if there rules are not already loaded. On 2017/02/21 19:56:16, rouslan wrote: > s/there/the/ Done. https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... File components/payments/address_normalizer_unittest.cc (right): https://codereview.chromium.org/2708933003/diff/80001/components/payments/add... components/payments/address_normalizer_unittest.cc:56: On 2017/02/21 19:56:16, rouslan wrote: > Override the destructor. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm % nits https://codereview.chromium.org/2708933003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2708933003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:104: PersonalDataManager.getInstance().getNormalizationTimeoutSeconds(), this); I don't understand well why this can't be internal to PersonalDataManager (i.e. call getNormalizationTimeoutSeconds from within PersonalDataManager.normalizeAddress) ? https://codereview.chromium.org/2708933003/diff/120001/third_party/libaddress... File third_party/libaddressinput/BUILD.gn (right): https://codereview.chromium.org/2708933003/diff/120001/third_party/libaddress... third_party/libaddressinput/BUILD.gn:128: source_set("libaddressinput_test_support") { "test_support" is enough, because the name of the library is already in the path when you reference it
Thanks! Sending to CQ https://codereview.chromium.org/2708933003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2708933003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:104: PersonalDataManager.getInstance().getNormalizationTimeoutSeconds(), this); On 2017/02/22 02:01:31, Mathieu Perreault wrote: > I don't understand well why this can't be internal to PersonalDataManager (i.e. > call getNormalizationTimeoutSeconds from within > PersonalDataManager.normalizeAddress) ? Done. https://codereview.chromium.org/2708933003/diff/120001/third_party/libaddress... File third_party/libaddressinput/BUILD.gn (right): https://codereview.chromium.org/2708933003/diff/120001/third_party/libaddress... third_party/libaddressinput/BUILD.gn:128: source_set("libaddressinput_test_support") { On 2017/02/22 02:01:31, Mathieu Perreault wrote: > "test_support" is enough, because the name of the library is already in the path > when you reference it Done.
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 Link to the patchset: https://codereview.chromium.org/2708933003/#ps130001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1487783063368040,
"parent_rev": "5aa2bdd883754f0d62a743c528e2c69107c3550d", "commit_rev":
"5a2f0a362c5c1257d5893f973eb6e2e6696faa04"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Add timeout to the address_normalizer. Adds a timeout parameter to the AddressNormalizer so that the requester will always receive a response before that time limit (be it a successful normalization or not). BUG=692643 ========== to ========== [Payments] Add timeout to the address_normalizer. Adds a timeout parameter to the AddressNormalizer so that the requester will always receive a response before that time limit (be it a successful normalization or not). BUG=692643 Review-Url: https://codereview.chromium.org/2708933003 Cr-Commit-Position: refs/heads/master@{#452138} Committed: https://chromium.googlesource.com/chromium/src/+/5a2f0a362c5c1257d5893f973eb6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:130001) as https://chromium.googlesource.com/chromium/src/+/5a2f0a362c5c1257d5893f973eb6... |
