Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(241)

Issue 2879853003: [Payments] Code Refactoring for the Subkey Request (Closed)

Created:
3 years, 7 months ago by Parastoo
Modified:
3 years, 7 months ago
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+459 lines, -153 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java View 7 chunks +20 lines, -14 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java View 1 1 chunk +9 lines, -29 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java View 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/autofill/android/personal_data_manager_android.h View 6 chunks +8 lines, -32 lines 0 comments Download
M chrome/browser/autofill/android/personal_data_manager_android.cc View 1 2 3 4 5 6 5 chunks +20 lines, -75 lines 0 comments Download
M components/payments/core/BUILD.gn View 2 chunks +3 lines, -0 lines 0 comments Download
A components/payments/core/subkey_requester.h View 1 2 3 4 5 1 chunk +73 lines, -0 lines 0 comments Download
A components/payments/core/subkey_requester.cc View 1 2 3 4 5 1 chunk +129 lines, -0 lines 0 comments Download
A components/payments/core/subkey_requester_unittest.cc View 1 2 3 4 5 1 chunk +194 lines, -0 lines 0 comments Download

Messages

Total messages: 134 (109 generated)
Parastoo
3 years, 7 months ago (2017-05-17 19:13:17 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2879853003/60001
3 years, 7 months ago (2017-05-17 19:57:13 UTC) #36
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
3 years, 7 months ago (2017-05-17 19:57:15 UTC) #38
sebsg
Very nice! Some comments. Nice tests btw :) https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java#newcode341 chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:341: // ...
3 years, 7 months ago (2017-05-18 17:50:52 UTC) #66
Parastoo
https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java File chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java (right): https://codereview.chromium.org/2879853003/diff/140001/chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java#newcode341 chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:341: // For the testing case, the time-out is set ...
3 years, 7 months ago (2017-05-18 20:59:46 UTC) #69
Parastoo
https://codereview.chromium.org/2879853003/diff/140001/components/payments/core/subkey_requester.cc File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/140001/components/payments/core/subkey_requester.cc#newcode62 components/payments/core/subkey_requester.cc:62: SubKeyRequester::Delegate* delegate_; On 2017/05/18 17:50:51, sebsg wrote: > Please ...
3 years, 7 months ago (2017-05-18 21:05:21 UTC) #70
sebsg
lgtm for autofill/*. Very nice cleanup, thanks! https://codereview.chromium.org/2879853003/diff/160001/components/payments/core/subkey_requester.cc File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/160001/components/payments/core/subkey_requester.cc#newcode83 components/payments/core/subkey_requester.cc:83: Can we ...
3 years, 7 months ago (2017-05-19 13:39:09 UTC) #76
Parastoo
https://codereview.chromium.org/2879853003/diff/160001/components/payments/core/subkey_requester.cc File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/160001/components/payments/core/subkey_requester.cc#newcode83 components/payments/core/subkey_requester.cc:83: On 2017/05/19 13:39:09, sebsg wrote: > Can we DCHECK(requester) ...
3 years, 7 months ago (2017-05-19 15:18:44 UTC) #80
Mathieu
Thanks, this is clean! lgtm https://codereview.chromium.org/2879853003/diff/180001/components/payments/core/subkey_requester.cc File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/180001/components/payments/core/subkey_requester.cc#newcode114 components/payments/core/subkey_requester.cc:114: // empty list of ...
3 years, 7 months ago (2017-05-19 15:36:21 UTC) #81
Parastoo
dtrainor@chromium.org: Please review changes in
3 years, 7 months ago (2017-05-19 15:52:16 UTC) #85
Parastoo
https://codereview.chromium.org/2879853003/diff/180001/components/payments/core/subkey_requester.cc File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/180001/components/payments/core/subkey_requester.cc#newcode114 components/payments/core/subkey_requester.cc:114: // empty list of subkeys. => No need to ...
3 years, 7 months ago (2017-05-19 15:56:30 UTC) #90
Parastoo
3 years, 7 months ago (2017-05-19 15:56:45 UTC) #92
David Trainor- moved to gerrit
https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode791 chrome/browser/autofill/android/personal_data_manager_android.cc:791: new AndroidSubKeyRequesterDelegate(env, jdelegate); This feels scary (with the "delete ...
3 years, 7 months ago (2017-05-19 17:44:35 UTC) #93
Parastoo
https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/220001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode791 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 ...
3 years, 7 months ago (2017-05-23 18:09:52 UTC) #104
Parastoo
https://codereview.chromium.org/2879853003/diff/220001/components/payments/core/subkey_requester.cc File components/payments/core/subkey_requester.cc (right): https://codereview.chromium.org/2879853003/diff/220001/components/payments/core/subkey_requester.cc#newcode52 components/payments/core/subkey_requester.cc:52: return; On 2017/05/19 17:44:35, David Trainor-ping if over 24h ...
3 years, 7 months ago (2017-05-23 18:10:39 UTC) #106
Parastoo
3 years, 7 months ago (2017-05-23 19:11:17 UTC) #108
David Trainor- moved to gerrit
Final nits around removing the class. Thanks! https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode310 chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver ...
3 years, 7 months ago (2017-05-23 21:03:59 UTC) #111
Parastoo
Thank you for your comments. https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode310 chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { Then ...
3 years, 7 months ago (2017-05-24 14:57:20 UTC) #114
Parastoo
https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode310 chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { Correcting a typo: we should NOT ...
3 years, 7 months ago (2017-05-24 14:59:15 UTC) #115
David Trainor- moved to gerrit
https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode310 chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { On 2017/05/24 14:59:15, Parastoo wrote: > ...
3 years, 7 months ago (2017-05-24 15:29:37 UTC) #116
Parastoo
Thank you for your quick response. https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/260001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode310 chrome/browser/autofill/android/personal_data_manager_android.cc:310: class AndroidSubKeyReceiver { ...
3 years, 7 months ago (2017-05-24 18:53:57 UTC) #121
David Trainor- moved to gerrit
lgtm % nit! https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode757 chrome/browser/autofill/android/personal_data_manager_android.cc:757: void OnSubKeysReceived(ScopedJavaGlobalRef<jobject> jdelegate, Final nit! keep ...
3 years, 7 months ago (2017-05-24 18:55:21 UTC) #122
Parastoo
Sure! https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofill/android/personal_data_manager_android.cc File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2879853003/diff/300001/chrome/browser/autofill/android/personal_data_manager_android.cc#newcode757 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 ...
3 years, 7 months ago (2017-05-24 19:02:01 UTC) #125
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2879853003/320001
3 years, 7 months ago (2017-05-24 23:09:20 UTC) #131
commit-bot: I haz the power
3 years, 7 months ago (2017-05-24 23:14:29 UTC) #134
Message was sent while issue was closed.
Committed patchset #7 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/fbe49a4a05a04c51bc6ac3be5765...

Powered by Google App Engine
This is Rietveld 408576698