Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14904002:
Load and send Wallet Risk params after user has agreed (Closed)
|
Created:
7 years, 7 months ago by Dan Beam Modified:
7 years, 7 months ago CC:
chromium-reviews, Raman Kakilate, benquan, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLoad and send Wallet Risk params after user has agreed
R=isherman@chromium.org
BUG=173505
TEST=unit_tests, no sued
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201981
Patch Set 1 #
Total comments: 27
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 1
Patch Set 5 : . #
Total comments: 3
Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : USE -> ENABLE #Patch Set 11 : . #Patch Set 12 : w00t #Patch Set 13 : fixes #
Total comments: 26
Patch Set 14 : . #Patch Set 15 : magic #
Total comments: 16
Patch Set 16 : remove "risky business" #Patch Set 17 : shave #Patch Set 18 : nits #Patch Set 19 : shave #Patch Set 20 : . #
Messages
Total messages: 22 (0 generated)
as you can tell, I just chose base64 for right now... https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2072: if (!base::Base64Encode(proto_data, &risk_data_)) btw, I have no idea how this could fail, but it doesn't hurt... I guess....
https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/autofil... File chrome/browser/autofill/risk/fingerprint_browsertest.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/autofil... chrome/browser/autofill/risk/fingerprint_browsertest.cc:21: const uint64 kGaiaId = GG_UINT64_C(99194853094755497); Could you update this constant so that it's outside the int64 range? https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1423: return risk_data_.empty() ? "risky business" : risk_data_; Why can risk_data_ be empty? We shouldn't issue requests that require risk data until it's loaded. https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1713: return; Can this be a DCHECK instead? https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2072: if (!base::Base64Encode(proto_data, &risk_data_)) On 2013/05/03 07:29:09, Dan Beam wrote: > btw, I have no idea how this could fail, but it doesn't hurt... I guess.... Yeah, I'm also uncertain. I think a DCHECK might be reasonable. https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:262: // rather than const-ref (as it can be NULL this way). This comment doesn't actually describe what the method does. Also, why is it important for the |fingerprint| to possibly be NULL in tests? https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:494: // discourage) risk challenges; sent if the user's up to date on legal docs. nit: "user's" -> "user is" IMO https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:553: bool legal_docs_current_; nit: "legal_docs_are_current_" or "are_legal_docs_current_" https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:186: void set_fingerprint_data(std::string fingerprint_data) { nit: Pass by const-reference. https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:205: // Ignore |fingerprint|, it's NULL. Why? It seems much better not to override this method at all, and just test for an actual serialized protocol buffer in the test. https://chromiumcodereview.appspot.com/14904002/diff/1/components/autofill/br... File components/autofill/browser/risk/proto/fingerprint.proto (right): https://chromiumcodereview.appspot.com/14904002/diff/1/components/autofill/br... components/autofill/browser/risk/proto/fingerprint.proto:205: // Gaia id associated with transaction. nit: Please update this comment to indicate that this is an obfuscated Gaia id.
https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1423: return risk_data_.empty() ? "risky business" : risk_data_; On 2013/05/03 07:38:31, Ilya Sherman wrote: > Why can risk_data_ be empty? We shouldn't issue requests that require risk data > until it's loaded. Failed encoding, not loaded yet (there's nothing to say that risk loads first). https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1713: return; On 2013/05/03 07:38:31, Ilya Sherman wrote: > Can this be a DCHECK instead? A DCHECK() is not a strong assurance in release builds or when half the devs don't build with them on ;). https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2072: if (!base::Base64Encode(proto_data, &risk_data_)) On 2013/05/03 07:38:31, Ilya Sherman wrote: > On 2013/05/03 07:29:09, Dan Beam wrote: > > btw, I have no idea how this could fail, but it doesn't hurt... I guess.... > > Yeah, I'm also uncertain. I think a DCHECK might be reasonable. Maybe for this... I just don't understand why this returns a bool... https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:262: // rather than const-ref (as it can be NULL this way). On 2013/05/03 07:38:31, Ilya Sherman wrote: > This comment doesn't actually describe what the method does. Also, why is it > important for the |fingerprint| to possibly be NULL in tests? It has no good use outside of testing and I want to be able to easily control the serialized value. https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:205: // Ignore |fingerprint|, it's NULL. On 2013/05/03 07:38:31, Ilya Sherman wrote: > Why? It seems much better not to override this method at all, and just test for > an actual serialized protocol buffer in the test. If it's obvious what the base64 encoded expected text would be, that's fine by me. https://chromiumcodereview.appspot.com/14904002/diff/1/components/autofill/br... File components/autofill/browser/risk/proto/fingerprint.proto (right): https://chromiumcodereview.appspot.com/14904002/diff/1/components/autofill/br... components/autofill/browser/risk/proto/fingerprint.proto:205: // Gaia id associated with transaction. On 2013/05/03 07:38:31, Ilya Sherman wrote: > nit: Please update this comment to indicate that this is an obfuscated Gaia id. Yeah, I'll rename this everywhere tomorrow.
https://codereview.chromium.org/14904002/diff/1/chrome/browser/autofill/risk/... File chrome/browser/autofill/risk/fingerprint_browsertest.cc (right): https://codereview.chromium.org/14904002/diff/1/chrome/browser/autofill/risk/... chrome/browser/autofill/risk/fingerprint_browsertest.cc:21: const uint64 kGaiaId = GG_UINT64_C(99194853094755497); On 2013/05/03 07:38:31, Ilya Sherman wrote: > Could you update this constant so that it's outside the int64 range? Done. https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:494: // discourage) risk challenges; sent if the user's up to date on legal docs. On 2013/05/03 07:38:31, Ilya Sherman wrote: > nit: "user's" -> "user is" IMO Done. https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:553: bool legal_docs_current_; On 2013/05/03 07:38:31, Ilya Sherman wrote: > nit: "legal_docs_are_current_" or "are_legal_docs_current_" Done. https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:186: void set_fingerprint_data(std::string fingerprint_data) { On 2013/05/03 07:38:31, Ilya Sherman wrote: > nit: Pass by const-reference. Done. https://codereview.chromium.org/14904002/diff/1/components/autofill/browser/r... File components/autofill/browser/risk/proto/fingerprint.proto (right): https://codereview.chromium.org/14904002/diff/1/components/autofill/browser/r... components/autofill/browser/risk/proto/fingerprint.proto:205: // Gaia id associated with transaction. On 2013/05/03 09:24:47, Dan Beam wrote: > On 2013/05/03 07:38:31, Ilya Sherman wrote: > > nit: Please update this comment to indicate that this is an obfuscated Gaia > id. > > Yeah, I'll rename this everywhere tomorrow. Done.
https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1423: return risk_data_.empty() ? "risky business" : risk_data_; On 2013/05/03 09:24:47, Dan Beam wrote: > On 2013/05/03 07:38:31, Ilya Sherman wrote: > > Why can risk_data_ be empty? We shouldn't issue requests that require risk > data > > until it's loaded. > > Failed encoding, not loaded yet (there's nothing to say that risk loads first). We should structure the code so that it's guaranteed that risk data is loaded by the time anyone needs to call GetRiskData(). Likewise, we should structure the code so that it's not possible for the encoding to fail. If either of the above goes wrong, then we don't have risk data to offer the server, which essentially means we're a misbehaving client. It seems pretty appropriate for the server to reject requests from misbehaving clients. https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1713: return; On 2013/05/03 09:24:47, Dan Beam wrote: > On 2013/05/03 07:38:31, Ilya Sherman wrote: > > Can this be a DCHECK instead? > > A DCHECK() is not a strong assurance in release builds or when half the devs > don't build with them on ;). DCHECKs are used to document expected state. Unless there are expected code paths that could reach here with |legal_docs_current_| being false, please make this a DCHECK. I don't really understand why Evan builds with DCHECKs off, but that doesn't mean that our code should be different from how most other Chrome code is structured. https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:205: // Ignore |fingerprint|, it's NULL. On 2013/05/03 09:24:47, Dan Beam wrote: > On 2013/05/03 07:38:31, Ilya Sherman wrote: > > Why? It seems much better not to override this method at all, and just test > for > > an actual serialized protocol buffer in the test. > > If it's obvious what the base64 encoded expected text would be, that's fine by > me. It's pretty easy to encode the data and DLOG what the encoding is, then set that as the test's expectation. https://codereview.chromium.org/14904002/diff/19001/components/autofill/brows... File components/autofill/browser/risk/proto/fingerprint.proto (right): https://codereview.chromium.org/14904002/diff/19001/components/autofill/brows... components/autofill/browser/risk/proto/fingerprint.proto:205: // Gaia id associated with transaction. nit: Please update the comment as well, to indicate that the id is obfuscated.
https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/1/chrome/browser/ui/autofill/au... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1713: return; On 2013/05/07 00:59:30, Ilya Sherman wrote: > On 2013/05/03 09:24:47, Dan Beam wrote: > > On 2013/05/03 07:38:31, Ilya Sherman wrote: > > > Can this be a DCHECK instead? > > > > A DCHECK() is not a strong assurance in release builds or when half the devs > > don't build with them on ;). > > DCHECKs are used to document expected state. Unless there are expected code > paths that could reach here with |legal_docs_current_| being false, please make > this a DCHECK. > > I don't really understand why Evan builds with DCHECKs off, but that doesn't > mean that our code should be different from how most other Chrome code is > structured. Done. https://codereview.chromium.org/14904002/diff/30001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/14904002/diff/30001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2028: // been accepted prior to calling into this method. Also, ensure that the UI why do we care if risk data is *loaded*? we care more about whether it's sent or not, correct?
https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/14904002/diff/1/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:205: // Ignore |fingerprint|, it's NULL. On 2013/05/07 00:59:30, Ilya Sherman wrote: > On 2013/05/03 09:24:47, Dan Beam wrote: > > On 2013/05/03 07:38:31, Ilya Sherman wrote: > > > Why? It seems much better not to override this method at all, and just test > > for > > > an actual serialized protocol buffer in the test. > > > > If it's obvious what the base64 encoded expected text would be, that's fine by > > me. > > It's pretty easy to encode the data and DLOG what the encoding is, then set that > as the test's expectation. Done.
1) saveToWallet still requires a non-empty risk_params field or we get a wallet error until dgwallinga@ (or somebody does) fixes this on the server. 2) why do you want to block until we have risk params? blocking on 2 unknown asynchronous tasks in various cases will complicate the code. if we do want to block on this, I think we should preload the risk params on dialog start so accepting legal docs doesn't slow down this flow (especially for first time users).
On 2013/05/07 05:09:18, Dan Beam wrote: > 1) saveToWallet still requires a non-empty risk_params field or we get a wallet > error until dgwallinga@ (or somebody does) fixes this on the server. Right, because Risk data is required for using Wallet. > 2) why do you want to block until we have risk params? blocking on 2 unknown > asynchronous tasks in various cases will complicate the code. if we do want to > block on this, I think we should preload the risk params on dialog start so > accepting legal docs doesn't slow down this flow (especially for first time > users). Because the Risk params are required to make a Wallet API call. Do you have reason to believe that blocking on getting Risk params adds a lot of latency to the calls? Also, is it even valid ot re-use Risk params across multiple requests? I kind of thought we were supposed to generate them afresh for each request.
On 2013/05/07 05:13:35, Ilya Sherman wrote: > On 2013/05/07 05:09:18, Dan Beam wrote: > > 1) saveToWallet still requires a non-empty risk_params field or we get a > wallet > > error until dgwallinga@ (or somebody does) fixes this on the server. > > Right, because Risk data is required for using Wallet. No, it's not required for any of the other calls. > > > 2) why do you want to block until we have risk params? blocking on 2 unknown > > asynchronous tasks in various cases will complicate the code. if we do want > to > > block on this, I think we should preload the risk params on dialog start so > > accepting legal docs doesn't slow down this flow (especially for first time > > users). > > Because the Risk params are required to make a Wallet API call. See above. > Do you have > reason to believe that blocking on getting Risk params adds a lot of latency to > the calls? This will vary wildly on different computers. For instance, it hangs for over 30s on the bots sometimes... https://code.google.com/p/chromium/issues/detail?id=174296 Anyways, my argument was that it adds complexity to the code. Deferring loading until we have |wallet_items_| already adds complexity to the code... I'd like to remove that as well. For users that are accepting legal documents, we send off an acceptLegalDocuments call and queue pending things (if the acceptLegalDocuments call fails, it cancels the rest). So we'd need to rework this code - https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > Also, is it even valid ot re-use Risk params across multiple > requests? I kind of thought we were supposed to generate them afresh for each > request. You should ask the person that implemented risk::Fingerprint ;), because if they don't know, I sure wont :P.
if we choose to block on risk params, one way to accomplish what we want would be to just queue walletclient's requests until loading the risk params (and resume doing them from there). this would keep the code simple as accept/save/update requests could just be queued without change to what's currently there.
On 2013/05/07 05:23:46, Dan Beam wrote: > > Do you have > > reason to believe that blocking on getting Risk params adds a lot of latency > to > > the calls? > > This will vary wildly on different computers. For instance, it hangs for over > 30s on the bots sometimes... > https://code.google.com/p/chromium/issues/detail?id=174296 It hangs for over 30s because it will hang forever when GL dies. That's a bug in the GPU info code, which we should either fix or work around. > Anyways, my argument was that it adds complexity to the code. Deferring loading > until we have |wallet_items_| already adds complexity to the code... I'd like to > remove that as well. For users that are accepting legal documents, we send off > an acceptLegalDocuments call and queue pending things (if the > acceptLegalDocuments call fails, it cancels the rest). So we'd need to rework > this code - > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > > > Also, is it even valid ot re-use Risk params across multiple > > requests? I kind of thought we were supposed to generate them afresh for each > > request. > > You should ask the person that implemented risk::Fingerprint ;), because if they > don't know, I sure wont :P. Well, that person strongly suspects that we're expected to generate a new fingerprint for each request, but mostly just wired up code to fill out a protocol buffer ;) On 2013/05/07 05:35:31, Dan Beam wrote: > if we choose to block on risk params, one way to accomplish what we want would > be to just queue walletclient's requests until loading the risk params (and > resume doing them from there). this would keep the code simple as > accept/save/update requests could just be queued without change to what's > currently there. That seems like a potentially reasonable approach. We could change the GetRiskData() interface to be asynchronous, and take a callback rather than returning a string directly. https://chromiumcodereview.appspot.com/14904002/diff/30001/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://chromiumcodereview.appspot.com/14904002/diff/30001/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2028: // been accepted prior to calling into this method. Also, ensure that the UI On 2013/05/07 04:02:50, Dan Beam wrote: > why do we care if risk data is *loaded*? we care more about whether it's sent > or not, correct? Yes, what we really care about is whether it's sent.
https://chromiumcodereview.appspot.com/14904002/diff/30001/chrome/browser/ui/... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://chromiumcodereview.appspot.com/14904002/diff/30001/chrome/browser/ui/... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2028: // been accepted prior to calling into this method. Also, ensure that the UI On 2013/05/07 09:11:41, Ilya Sherman wrote: > On 2013/05/07 04:02:50, Dan Beam wrote: > > why do we care if risk data is *loaded*? we care more about whether it's sent > > or not, correct? > > Yes, what we really care about is whether it's sent. Actually, belay that. It looks like determining the user's geolocation involves communicating with Google servers, which we shouldn't do until the user grants approval.
this new CL _should_ block on loading risk params for only getFullWallet (what I discussed with charliewon@), isherman@ ptal
Could you split off the Android compile fixes and the obfuscated GAIA id changes into two separate CLs? Those parts look pretty good right now, so we might as well land them now and make this CL more focused on the trickier bits :) https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2397: DCHECK(!active_address_id_.empty()); Why did you remove these DCHECKs? https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1504: // TODO(dbeam): remove the server restriction that this must not be empty. Could you file a tracking bug for the specific changes that need to be made? I'm assuming it's correct for the requirement to remain for GetFullWallet() calls, right? https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1510: LoadRiskFingerprintData(); Is this method guaranteed to only be called after 'Submit' is pressed? If so, could you add a DCHECK to verify that invariant? https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1624: OnDidSaveAddress(address_id, required_actions); Why is the address treated differently than the payment instrument? https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2306: if (!active_instrument_id_.empty() && !active_address_id_.empty()) This block could use a comment now. Seems like it might be cleaner to decompose methods for saving the instrument and address if needed, though. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2414: DCHECK(is_submitting_ && IsPayingWithWallet()); nit: Better to have these as separate DCHECKs, so that a failure tells you which check failed. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:288: RiskLoadsAfterAcceptingLegalDocuments); Rather than adding lots of friend tests, please up the visibility of OnDidLoadRiskFingerprintData() to be protected. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:455: // Gets a full wallet from Online Wallet so the user can purchase something if nit: Spurious diff. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:605: bool accepted_legal_documents_; nit: "has_accepted_..." https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:272: RiskLoadsAfterAcceptingLegalDocuments); Why not just make the relevant method public? There's not much point to having tests be friends of other code in the same unittest file. https://codereview.chromium.org/14904002/diff/57001/components/autofill/brows... File components/autofill/browser/risk/fingerprint.cc (right): https://codereview.chromium.org/14904002/diff/57001/components/autofill/brows... components/autofill/browser/risk/fingerprint.cc:367: has_loaded_plugins_ && Did you mean to remove this line? It seems like it might be cleaner to just add an #else block after line 288 and set has_loaded_plugins_ to true if plugins aren't enabled.
https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2397: DCHECK(!active_address_id_.empty()); On 2013/05/22 22:34:24, Ilya Sherman wrote: > Why did you remove these DCHECKs? I made this function GetFullWalletIfReady(), but decided to split later. Added back. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1510: LoadRiskFingerprintData(); On 2013/05/22 22:34:24, Ilya Sherman wrote: > Is this method guaranteed to only be called after 'Submit' is pressed? If so, > could you add a DCHECK to verify that invariant? Done. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1595: DCHECK(is_submitting_ && IsPayingWithWallet()); ^ this DCHECK() https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1624: OnDidSaveAddress(address_id, required_actions); On 2013/05/22 22:34:24, Ilya Sherman wrote: > Why is the address treated differently than the payment instrument? OnDidSaveInstrument() can set is_submitting_ to false, which makes the DCHECK() at the top of OnDidSaveAddress() fail. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2306: if (!active_instrument_id_.empty() && !active_address_id_.empty()) On 2013/05/22 22:34:24, Ilya Sherman wrote: > This block could use a comment now. Seems like it might be cleaner to decompose > methods for saving the instrument and address if needed, though. Done. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2414: DCHECK(is_submitting_ && IsPayingWithWallet()); On 2013/05/22 22:34:24, Ilya Sherman wrote: > nit: Better to have these as separate DCHECKs, so that a failure tells you which > check failed. Done. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:288: RiskLoadsAfterAcceptingLegalDocuments); On 2013/05/22 22:34:24, Ilya Sherman wrote: > Rather than adding lots of friend tests, please up the visibility of > OnDidLoadRiskFingerprintData() to be protected. Done. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:455: // Gets a full wallet from Online Wallet so the user can purchase something if On 2013/05/22 22:34:24, Ilya Sherman wrote: > nit: Spurious diff. Done. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:605: bool accepted_legal_documents_; On 2013/05/22 22:34:24, Ilya Sherman wrote: > nit: "has_accepted_..." Done. https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:272: RiskLoadsAfterAcceptingLegalDocuments); On 2013/05/22 22:34:24, Ilya Sherman wrote: > Why not just make the relevant method public? There's not much point to having > tests be friends of other code in the same unittest file. Done. https://codereview.chromium.org/14904002/diff/57001/components/autofill/brows... File components/autofill/browser/risk/fingerprint.cc (right): https://codereview.chromium.org/14904002/diff/57001/components/autofill/brows... components/autofill/browser/risk/fingerprint.cc:367: has_loaded_plugins_ && On 2013/05/22 22:34:24, Ilya Sherman wrote: > Did you mean to remove this line? It seems like it might be cleaner to just add > an #else block after line 288 and set has_loaded_plugins_ to true if plugins > aren't enabled. this was messed up in a merge conflict, they haven't loaded if there's no plugins, fixed
LGTM % nits. Thanks! https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1504: // TODO(dbeam): remove the server restriction that this must not be empty. On 2013/05/22 22:34:24, Ilya Sherman wrote: > Could you file a tracking bug for the specific changes that need to be made? > I'm assuming it's correct for the requirement to remain for GetFullWallet() > calls, right? ^^^ https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1624: OnDidSaveAddress(address_id, required_actions); On 2013/05/22 23:25:20, Dan Beam wrote: > On 2013/05/22 22:34:24, Ilya Sherman wrote: > > Why is the address treated differently than the payment instrument? > > OnDidSaveInstrument() can set is_submitting_ to false, which makes the DCHECK() > at the top of OnDidSaveAddress() fail. Ah, ok. Please add a comment to this effect so that this looks less confusing / surprising. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1785: #endif nit: Would be nice to merge this change into the split-off Android CL as well; but fine to leave here if that introduces too much merge headachiness. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2437: DCHECK(is_submitting_ && IsPayingWithWallet()); nit: Please write this as two separate DCHECKs https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:272: // Called when loading of risk fingerprint data is done. nit: This comment seems redundant with the one on line 269. I'd keep these two methods grouped as they were before. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:424: bool LegalDocumentsAreCurrent() const; nit: "AreLegalDocumentsCurrent()" https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (left): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:37: namespace { Why did you remove this? It should wrap everything other than AutofillDialogControllerTest (which looks like it accidentally got tucked into there). https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:231: } Optional nit: I think you can just write "using AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData;" instead of lines 227-231. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:328: } nit: Please define this as a free function rather than a static class method. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1344: EXPECT_EQ("risky business", controller()->GetRiskData()); Should this test call controller()->OnAccept(); before this, to give a bit more coverage? https://codereview.chromium.org/14904002/diff/68008/components/autofill/brows... File components/autofill/browser/risk/proto/fingerprint.proto (right): https://codereview.chromium.org/14904002/diff/68008/components/autofill/brows... components/autofill/browser/risk/proto/fingerprint.proto:224: } nit: Would be nice to split off the fixes in this file to a separate CL as well, but it's ok to leave them here if that's too much of a PITA.
https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1504: // TODO(dbeam): remove the server restriction that this must not be empty. On 2013/05/22 23:53:59, Ilya Sherman wrote: > On 2013/05/22 22:34:24, Ilya Sherman wrote: > > Could you file a tracking bug for the specific changes that need to be made? > > I'm assuming it's correct for the requirement to remain for GetFullWallet() > > calls, right? > > ^^^ omitting risk params works now, I've updated the code in later patchsets https://codereview.chromium.org/14904002/diff/57001/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1624: OnDidSaveAddress(address_id, required_actions); On 2013/05/22 23:53:59, Ilya Sherman wrote: > On 2013/05/22 23:25:20, Dan Beam wrote: > > On 2013/05/22 22:34:24, Ilya Sherman wrote: > > > Why is the address treated differently than the payment instrument? > > > > OnDidSaveInstrument() can set is_submitting_ to false, which makes the > DCHECK() > > at the top of OnDidSaveAddress() fail. > > Ah, ok. Please add a comment to this effect so that this looks less confusing / > surprising. Done. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2437: DCHECK(is_submitting_ && IsPayingWithWallet()); On 2013/05/22 23:53:59, Ilya Sherman wrote: > nit: Please write this as two separate DCHECKs Done. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:272: // Called when loading of risk fingerprint data is done. On 2013/05/22 23:53:59, Ilya Sherman wrote: > nit: This comment seems redundant with the one on line 269. I'd keep these two > methods grouped as they were before. Done. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:424: bool LegalDocumentsAreCurrent() const; On 2013/05/22 23:53:59, Ilya Sherman wrote: > nit: "AreLegalDocumentsCurrent()" Done. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (left): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:37: namespace { On 2013/05/22 23:53:59, Ilya Sherman wrote: > Why did you remove this? It should wrap everything other than > AutofillDialogControllerTest (which looks like it accidentally got tucked into > there). look at newer patchsets https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right): https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:231: } On 2013/05/22 23:53:59, Ilya Sherman wrote: > Optional nit: I think you can just write "using > AutofillDialogControllerImpl::OnDidLoadRiskFingerprintData;" instead of lines > 227-231. Done. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:328: } On 2013/05/22 23:53:59, Ilya Sherman wrote: > nit: Please define this as a free function rather than a static class method. Done. https://codereview.chromium.org/14904002/diff/68008/chrome/browser/ui/autofil... chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:1344: EXPECT_EQ("risky business", controller()->GetRiskData()); On 2013/05/22 23:53:59, Ilya Sherman wrote: > Should this test call controller()->OnAccept(); before this, to give a bit more > coverage? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/14904002/103001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbeam@chromium.org/14904002/103001
Message was sent while issue was closed.
Change committed as 201981 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
