|
|
DescriptionCode refactoring:
For the sake of modularity, removes the subkey request procedure
from PersonalDataManagerAndroid to the new class SubKeyRequester, only
leaves the interface in the PersonalDataManagerAndroid.
Adds a timeout parameter to the SubKeyRequester so that the
requester will always receive a response before the time limit.
Therefore, timeout checking is removed from the AddressEditor
(except for the tests).
Adds unit tests.
BUG=723844
Review-Url: https://codereview.chromium.org/2879853003
Cr-Commit-Position: refs/heads/master@{#474467}
Committed: https://chromium.googlesource.com/chromium/src/+/fbe49a4a05a04c51bc6ac3be5765c59740029192
Patch Set 1 : Refactoring #
Total comments: 26
Patch Set 2 : Comments Addressed. #
Total comments: 2
Patch Set 3 : requester not null #
Total comments: 2
Patch Set 4 : Changing a comment. #
Total comments: 8
Patch Set 5 : Remove Delegate #
Total comments: 7
Patch Set 6 : Removing a redundant class. #
Total comments: 2
Patch Set 7 : Final nit #Messages
Total messages: 134 (109 generated)
The CQ bit was checked by parastoog@google.com to run a CQ dry run
The CQ bit was unchecked by parastoog@google.com
Description was changed from ========== modifying subkey_requester adding subkey_requester BUG= ========== to ========== modifying subkey_requester adding subkey_requester BUG= ==========
The CQ bit was checked by parastoog@google.com
The CQ bit was unchecked by parastoog@google.com
The CQ bit was unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com 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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_asan_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 parastoog@google.com 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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 parastoog@google.com 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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 parastoog@google.com to run a CQ dry run
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by parastoog@google.com
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 parastoog@google.com 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 parastoog@google.com 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 #1 (id:20001) has been deleted
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 parastoog@google.com to run a CQ dry run
The CQ bit was unchecked by parastoog@google.com
Description was changed from ========== modifying subkey_requester adding subkey_requester BUG= ========== to ========== modifying subkey_requester adding subkey_requester tests BUG= ==========
parastoog@google.com changed reviewers: + sebsg@chromium.org
The CQ bit was checked by parastoog@google.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by parastoog@google.com 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 parastoog@google.com 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 parastoog@google.com 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 parastoog@google.com 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 parastoog@google.com 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 #3 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by parastoog@google.com to run a CQ dry run
Patchset #1 (id:120001) 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 unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com 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...
Description was changed from ========== modifying subkey_requester adding subkey_requester tests BUG= ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG= ==========
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG= ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor ( except for the testing case). Adds unit tests. BUG= ==========
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor ( except for the testing case). Adds unit tests. BUG= ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG= ==========
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG= ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG=723844 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Very nice! Some comments. Nice tests btw :) https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:341: // For the testing case, the time-out is set to 0. In this case, we should not Nit: "For the testing case" -> "For tests" https://codereview.chromium.org/2879853003/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:1: nit: Please remove this empty line https://codereview.chromium.org/2879853003/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:349: std::unique_ptr<::i18n::addressinput::Source>( Can you use MakeUnique? It's generally the preferred way. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:25: using ::autofill::AutofillProfile; needed? https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:62: SubKeyRequester::Delegate* delegate_; Please mention the lifetime of these pointer. For example see https://cs.chromium.org/chromium/src/components/payments/content/payment_requ... https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:97: LoadRulesForRegion(region_code); Move this in the "else"? https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:116: pending_subkey_region_code_.clear(); I'm curious about this. Do you know if my address_validator can impact this? Let's say you start to load "US" and I start to load "CA". When "CA" gets loaded will this function be called? If yes, we should not reset your request and etc. If no, we should DCHECK !pending_subkey_region_code_.compare(region_code) https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:9: #include "components/autofill/core/browser/autofill_profile.h" needed? https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:12: using autofill::AutofillProfile; needed? https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:16: // SubKeyRequester Load Rules from the server and extracts the subkeys. Can you give a short description of what a subkey is and what it does? https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:37: void StartRegionSubKeysRequest(const std::string& region_code, Could you please add a short comment? For example, mention that the operation can be synchroneous, or asynchroneous, and in which conditions. Thanks! https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:12: #include "components/autofill/core/browser/autofill_profile.h" needed? https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:22: using ::autofill::AutofillProfile; needed?
The CQ bit was checked by parastoog@google.com 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...
https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:341: // For the testing case, the time-out is set to 0. In this case, we should not On 2017/05/18 17:50:51, sebsg wrote: > Nit: "For the testing case" -> "For tests" Done. https://codereview.chromium.org/2879853003/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:1: On 2017/05/18 17:50:51, sebsg wrote: > nit: Please remove this empty line Done. https://codereview.chromium.org/2879853003/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:349: std::unique_ptr<::i18n::addressinput::Source>( On 2017/05/18 17:50:51, sebsg wrote: > Can you use MakeUnique? It's generally the preferred way. Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:25: using ::autofill::AutofillProfile; On 2017/05/18 17:50:51, sebsg wrote: > needed? Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:97: LoadRulesForRegion(region_code); On 2017/05/18 17:50:51, sebsg wrote: > Move this in the "else"? Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:116: pending_subkey_region_code_.clear(); No. For further reference: You (AddressNormalizer) and I (SubKeyRequester) are both the children of the abstract class autofill::LoadRuleListener. When calling the constructor of the autofill::AddressValidator, we each pass different type of objects as load_rule_listener (I pass a SubKeyRequester object, you pass an AddressNormalizer one.) So the AddressValidator will know which one is which, here: (https://codesearch.chromium.org/chromium/src/third_party/libaddressinput/chro...). I think we should not use DCHECK, because there is a case when the rules are loaded after the timeout, or the request is cancelled when the loading has already started. In these cases, the rules will be loaded, and this function is called, and we should simply discard the rules. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:9: #include "components/autofill/core/browser/autofill_profile.h" On 2017/05/18 17:50:51, sebsg wrote: > needed? Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:12: using autofill::AutofillProfile; On 2017/05/18 17:50:51, sebsg wrote: > needed? Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:16: // SubKeyRequester Load Rules from the server and extracts the subkeys. On 2017/05/18 17:50:51, sebsg wrote: > Can you give a short description of what a subkey is and what it does? Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.h:37: void StartRegionSubKeysRequest(const std::string& region_code, On 2017/05/18 17:50:51, sebsg wrote: > Could you please add a short comment? > > For example, mention that the operation can be synchroneous, or asynchroneous, > and in which conditions. > > Thanks! Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester_unittest.cc (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:12: #include "components/autofill/core/browser/autofill_profile.h" On 2017/05/18 17:50:52, sebsg wrote: > needed? Done. https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester_unittest.cc:22: using ::autofill::AutofillProfile; On 2017/05/18 17:50:52, sebsg wrote: > needed? Done.
https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/co... components/payments/core/subkey_requester.cc:62: SubKeyRequester::Delegate* delegate_; On 2017/05/18 17:50:51, sebsg wrote: > Please mention the lifetime of these pointer. For example see > https://cs.chromium.org/chromium/src/components/payments/content/payment_requ... Done.
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to a new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG=723844 ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG=723844 ==========
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before that time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG=723844 ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before the time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG=723844 ==========
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before the time limit. Therefore, timeout checking is removed from the AddressEditor (except for the testing case). Adds unit tests. BUG=723844 ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before the time limit. Therefore, timeout checking is removed from the AddressEditor (except for the tests). Adds unit tests. BUG=723844 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for autofill/*. Very nice cleanup, thanks! https://codereview.chromium.org/2879853003/diff/160001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/160001/components/payments/co... components/payments/core/subkey_requester.cc:83: Can we DCHECK(requester) too? Thanks!
The CQ bit was checked by parastoog@google.com 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...
parastoog@google.com changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2879853003/diff/160001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/160001/components/payments/co... components/payments/core/subkey_requester.cc:83: On 2017/05/19 13:39:09, sebsg wrote: > Can we DCHECK(requester) too? Thanks! Done.
Thanks, this is clean! lgtm https://codereview.chromium.org/2879853003/diff/180001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/180001/components/payments/co... components/payments/core/subkey_requester.cc:114: // empty list of subkeys. => No need to check for |success|. can you rephrase?
The CQ bit was checked by parastoog@google.com 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...
parastoog@google.com changed reviewers: + dtrainor@chromium.org
dtrainor@chromium.org: Please review changes in
The CQ bit was checked by parastoog@google.com to run a CQ dry run
Patchset #4 (id:200001) 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...
parastoog@google.com changed reviewers: - dtrainor@chromium.org, mathp@chromium.org, sebsg@chromium.org
https://codereview.chromium.org/2879853003/diff/180001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/180001/components/payments/co... components/payments/core/subkey_requester.cc:114: // empty list of subkeys. => No need to check for |success|. On 2017/05/19 15:36:21, Mathieu wrote: > can you rephrase? Done.
parastoog@google.com changed reviewers: + dtrainor@chromium.org
https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:791: new AndroidSubKeyRequesterDelegate(env, jdelegate); This feels scary (with the "delete this" in the callback method). If you need a way to call a single function with some parameters just use a base::Callback or base::OnceCallback into these classes instead. Then you can bind the jobject as a ScopedJavaGlobalRef to the bind. Then you can remove the delegate concept https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... components/payments/core/subkey_requester.cc:52: return; Will this leak the delegate? This is why we should probably just use a base::Bind callback IMO. https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... components/payments/core/subkey_requester.cc:85: std::unique_ptr<SubKeyRequest> request(base::MakeUnique<SubKeyRequest>( Could just store a base::Callback or base::CallbackOnce here as well since the interface is basically just a single method. I don't feel as strongly about this one though! https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... components/payments/core/subkey_requester.cc:119: pending_subkey_request_->OnRulesLoaded(); Are we guaranteed to get a callback for this? Just want to make sure we won't have a case where we never notify the callback.
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 parastoog@google.com 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 parastoog@google.com to run a CQ dry run
Patchset #5 (id:240001) 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...
parastoog@google.com changed reviewers: + mathp@chromium.org, sebsg@chromium.org
https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:791: new AndroidSubKeyRequesterDelegate(env, jdelegate); On 2017/05/19 17:44:35, David Trainor-ping if over 24h wrote: > This feels scary (with the "delete this" in the callback method). If you need a > way to call a single function with some parameters just use a base::Callback or > base::OnceCallback into these classes instead. Then you can bind the jobject as > a ScopedJavaGlobalRef to the bind. > > Then you can remove the delegate concept Done. https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... components/payments/core/subkey_requester.cc:85: std::unique_ptr<SubKeyRequest> request(base::MakeUnique<SubKeyRequest>( On 2017/05/19 17:44:35, David Trainor-ping if over 24h wrote: > Could just store a base::Callback or base::CallbackOnce here as well since the > interface is basically just a single method. I don't feel as strongly about > this one though! Done. https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... components/payments/core/subkey_requester.cc:119: pending_subkey_request_->OnRulesLoaded(); On 2017/05/19 17:44:35, David Trainor-ping if over 24h wrote: > Are we guaranteed to get a callback for this? Just want to make sure we won't > have a case where we never notify the callback. Yes.
parastoog@google.com changed reviewers: - dtrainor@chromium.org, mathp@chromium.org, sebsg@chromium.org
https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/220001/components/payments/co... components/payments/core/subkey_requester.cc:52: return; On 2017/05/19 17:44:35, David Trainor-ping if over 24h wrote: > Will this leak the delegate? This is why we should probably just use a > base::Bind callback IMO. Acknowledged.
parastoog@google.com changed reviewers: + dtrainor@chromium.org, mathp@chromium.org, sebsg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Final nits around removing the class. Thanks! https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { Don't even need this now :). Remove the class and just use something like: void HandleSubKeysReceived(ScopedJavaGlobalRef<jobject> delegate, const std::vector<std::string>& sub_keys) { JNIEnv* env = base::android::AttachCurrentThread(); Java_GetSubKeysRequestDelegate_onSubKeysReceived(env, delegate, base::android::ToJavaArrayOfStrings(env, sub_keys)); } ... base::OnceCallback<void(const std::vector<std::string>&)> cb = base::BindOnce(&HandleSubKeysReceived, ScopedJavaGlobalRef<jobject>(jdelegate)); ... https://codereview.chromium.org/2879853003/diff/260001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2879853003/diff/260001/components/payments/co... components/payments/core/subkey_requester.h:41: base::OnceCallback<void(std::vector<std::string>)> cb); Why not const & on the vector? If it doesn't work don't worry about it. You can add a "using" to make this cleaner here and personal_data_manager_android if you want. Something like: using SubKeysCallback = base::OnceCallback<void(const std::vector<std::string>&); then can just use SubKeysCallback everywhere.
The CQ bit was checked by parastoog@google.com 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...
Thank you for your comments. https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { Then we would need to have the definition of the new function in SubKeyRequester, which would require the subkey_requester.h to understand ScopedJavaGlobalRef, which is not possible because there is no jni file for that file. (also not very modular.) + The SubKeyRequester may be later used in non-android applications, which means that we should deal with the ScopedJavaGlobalRef, .. in this file. https://codereview.chromium.org/2879853003/diff/260001/components/payments/co... File components/payments/core/subkey_requester.h (right): https://codereview.chromium.org/2879853003/diff/260001/components/payments/co... components/payments/core/subkey_requester.h:41: base::OnceCallback<void(std::vector<std::string>)> cb); On 2017/05/23 21:03:59, David Trainor-ping if over 24h wrote: > Why not const & on the vector? If it doesn't work don't worry about it. > > > You can add a "using" to make this cleaner here and > personal_data_manager_android if you want. Something like: > > using SubKeysCallback = base::OnceCallback<void(const > std::vector<std::string>&); > > then can just use SubKeysCallback everywhere. Done.
https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { Correcting a typo: we should NOT deal with the ScopedJavaGlobalRef, .. in this file.*
https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { On 2017/05/24 14:59:15, Parastoo wrote: > Correcting a typo: we should NOT deal with the ScopedJavaGlobalRef, .. in this > file.* Look at "Partial Binding Of Parameters" at https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md. base::Bind solves this for you by allowing you to partially bind the parameters (yay!). That means you can pass in the ScopedJavaGlobalRef<> in this file and SubKeyRequester will only ever see or know about a callback that takes an std::vector<std::string>. That's exactly the behavior you want. Notice that in your current code, SubKeyRequester doesn't need to know about the AndroidSubKeyReceiver you're passing in to the callback? Same kind of concept. From the code above, this will partially bind the jobject so the callback you're left with only takes an std::vector<std::string> (see the callback parameters and how they match your signature below): base::OnceCallback<void(const std::vector<std::string>&)> cb = base::BindOnce(&HandleSubKeysReceived, ScopedJavaGlobalRef<jobject>(jdelegate));
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 parastoog@google.com 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...
Thank you for your quick response. https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { Cool! Thank you. Done.
lgtm % nit! https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:757: void OnSubKeysReceived(ScopedJavaGlobalRef<jobject> jdelegate, Final nit! keep this in the anonymous namespace? :D
The CQ bit was checked by parastoog@google.com 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...
Sure! https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:757: void OnSubKeysReceived(ScopedJavaGlobalRef<jobject> jdelegate, On 2017/05/24 18:55:20, David Trainor-ping if over 24h wrote: > Final nit! keep this in the anonymous namespace? :D Done.
Patchset #6 (id:280001) has been deleted
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 parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, mathp@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2879853003/#ps320001 (title: "Final nit")
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": 320001, "attempt_start_ts": 1495667294364490, "parent_rev": "d39ace85b272f486a259b2678c5dcc1ca22b9d5f", "commit_rev": "fbe49a4a05a04c51bc6ac3be5765c59740029192"}
Message was sent while issue was closed.
Description was changed from ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before the time limit. Therefore, timeout checking is removed from the AddressEditor (except for the tests). Adds unit tests. BUG=723844 ========== to ========== Code refactoring: For the sake of modularity, removes the subkey request procedure from PersonalDataManagerAndroid to the new class SubKeyRequester, only leaves the interface in the PersonalDataManagerAndroid. Adds a timeout parameter to the SubKeyRequester so that the requester will always receive a response before the time limit. Therefore, timeout checking is removed from the AddressEditor (except for the tests). Adds unit tests. BUG=723844 Review-Url: https://codereview.chromium.org/2879853003 Cr-Commit-Position: refs/heads/master@{#474467} Committed: https://chromium.googlesource.com/chromium/src/+/fbe49a4a05a04c51bc6ac3be5765... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001) as https://chromium.googlesource.com/chromium/src/+/fbe49a4a05a04c51bc6ac3be5765... |