Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by Parastoo
Modified:
2 days, 21 hours 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

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: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+453 lines, -139 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 5 chunks +20 lines, -61 lines 1 comment 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 1 chunk +77 lines, -0 lines 0 comments Download
A components/payments/core/subkey_requester.cc View 1 2 3 1 chunk +129 lines, -0 lines 3 comments Download
A components/payments/core/subkey_requester_unittest.cc View 1 1 chunk +184 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 95 (82 generated)
Parastoo
4 days, 20 hours 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
4 days, 19 hours 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 ...
4 days, 19 hours 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 days, 21 hours 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 days, 18 hours 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 days, 18 hours 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 days, 1 hour 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 days 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 days ago (2017-05-19 15:36:21 UTC) #81
Parastoo
dtrainor@chromium.org: Please review changes in
2 days, 23 hours 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 ...
2 days, 23 hours ago (2017-05-19 15:56:30 UTC) #90
Parastoo
2 days, 23 hours ago (2017-05-19 15:56:45 UTC) #92
David Trainor-ping if over 24h
2 days, 21 hours ago (2017-05-19 17:44:35 UTC) #93
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06