Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11293078:
Integrating Online Wallet into Chrome. (Closed)
|
Created:
8 years, 1 month ago by ahutter Modified:
8 years ago Reviewers:
willchan no longer on Chromium, Dan Beam, benquan, Evan Stade, Dane Wallinga, Albert Bodenhamer, Raman Kakilate, darin (slow to review), brettw, Ilya Sherman, sky CC:
sky Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionIntegrating Online Wallet into Chrome. This CL is modeled after the Gaia OAuth client.
BUG=163609
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174078
Patch Set 1 #Patch Set 2 : Unit tests and more functionality #Patch Set 3 : Adding more functionality and unit testse #Patch Set 4 : More functionality for full wallet #Patch Set 5 : More unit tests and functionality #
Total comments: 8
Patch Set 6 : More tests. Real encryption/decryption. #
Total comments: 3
Patch Set 7 : Changes from Dane's review #
Total comments: 46
Patch Set 8 : Fixes from Raman's code review #
Total comments: 20
Patch Set 9 : Rebase and most fixes from Albert's initial review. #
Total comments: 180
Patch Set 10 : Fixes from code review. #Patch Set 11 : More fixes from code review #
Total comments: 57
Patch Set 12 : Fixes from latest code review #
Total comments: 7
Patch Set 13 : Fixed linter issue #
Total comments: 99
Patch Set 14 : Fixes from code review #
Total comments: 4
Patch Set 15 : Removing Core from WalletClient #Patch Set 16 : Adding comments for flags #Patch Set 17 : Fixing some errors in comments #Patch Set 18 : Adding comment to string_number_conversions_unittest.cc #Patch Set 19 : Fixing [chromium-style] error #
Total comments: 3
Patch Set 20 : Fixing HexStringToInt #Messages
Total messages: 51 (0 generated)
https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:174: // TODO(ahutter): Probably want to do some checks on reason Ask team. https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:194: request_->SetMaxRetries(0); Do we want retry logic? Who to ask? https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:274: delegate_->OnWalletError(); Does this need error codes? Also needs tests. https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:289: delegate_->OnWalletError(); Need test https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:299: delegate_->OnWalletError(); Need test. https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:308: } If there are addresses but no default address should that be treated as an error?
https://codereview.chromium.org/11293078/diff/17001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/17001/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:41: scoped_ptr<FullWallet> FullWallet::FromDictionary(DictionaryValue* dictionary) { return raw pointer, change name or add comment to make more clear that the caller owns the result. https://codereview.chromium.org/11293078/diff/17001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/17001/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:46: void SetCountryNameCode(const std::string& country_name_code) { don't know what chromium/cpp policy is on builders. maybe consider?
https://codereview.chromium.org/11293078/diff/17001/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/17001/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:41: scoped_ptr<FullWallet> FullWallet::FromDictionary(DictionaryValue* dictionary) { On 2012/11/16 01:27:25, dgwallinga wrote: > return raw pointer, change name or add comment to make more clear that the > caller owns the result. Done.
will have to look some more files. https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:55: std::string GetRiskParams() { return ""; } Add a TODO ? https://codereview.chromium.org/11293078/diff/14002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:86: DCHECK_EQ(request_type_, NO_PENDING_REQUEST); What happens if we attempt multiple call on same object (optimized binary case) ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:25: const std::string& rest, should we call out that this value is otp-crypted ? encrypted_rest ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:147: } Do we need this in final code ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:155: CHECK_EQ(length, operating_data.size()); Do we have to crash chrome ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:157: char* otp_thing = (char*) otp; What not just char* in param ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:166: } Extract this block of code to a private method ?.. which will also allow you to %s/otp_thing/otp https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:201: size_t split = kPanSize - kBinSize; Why do we do this in a getter ? GetPAN and GetCVN share 95% of the code. Why not do it once either while constructing the object or on the first getter ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:35: static FullWallet* CreateFromDictionary(base::DictionaryValue* dictionary); We return empty wallet with required actions or null or valid wallet. We should document it here. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:50: &country_name_code)) { Indentation not correct. here and below. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:100: Address* Address::CreateFromDisplayDictionary( I couldn't understand difference between DisplayDictionary and IdedDictonary, Adding some context in the .h file might be helpful for future readers of the code. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:22: // is_minimal_address, is_valid, or is_default we need is_minimal_address for upgrade flow ? and is this data part of wallet response ? or client needs to do more calls ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:29: const std::string& postal_code_number, extra space before "postal_.." https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:71: void set_object_id(const std::string& object_id) { optional: %s/object_id/address_id ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:75: static Address* CreateFromIdedDictionary( Create and Dictionary doesn't seem to go well together. How about BuildIdedAddress Build<something>Address ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:63: std::string GetRiskParams() { return ""; } TODO ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:94: DCHECK_EQ(request_type_, NO_PENDING_REQUEST); What happens if this occurs in production, Slow networks etc. ? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:120: CHECK_LT(num_bits, kMaxBits); DCHECK ?
https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:71: void set_object_id(const std::string& object_id) { On 2012/11/17 02:28:31, Raman Kakilate wrote: > optional: %s/object_id/address_id ? Ah .. ok. object_id is name used within API itself. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:50: return GURL("http://nogravitas.mtv.corp.google.com:8888/online-secure/" get rid of this. and set is a flag similar to service URL ?
https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:25: const std::string& rest, On 2012/11/17 02:28:31, Raman Kakilate wrote: > should we call out that this value is otp-crypted ? encrypted_rest ? Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:147: } On 2012/11/17 02:28:31, Raman Kakilate wrote: > Do we need this in final code ? Removing. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:155: CHECK_EQ(length, operating_data.size()); On 2012/11/17 02:28:31, Raman Kakilate wrote: > Do we have to crash chrome ? Probably not. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:157: char* otp_thing = (char*) otp; On 2012/11/17 02:28:31, Raman Kakilate wrote: > What not just char* in param ? Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:166: } On 2012/11/17 02:28:31, Raman Kakilate wrote: > Extract this block of code to a private method ?.. which will also allow you to > %s/otp_thing/otp Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:201: size_t split = kPanSize - kBinSize; On 2012/11/17 02:28:31, Raman Kakilate wrote: > Why do we do this in a getter ? GetPAN and GetCVN share 95% of the code. Why not > do it once either while constructing the object or on the first getter ? I was being lazy. Fixed. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:35: static FullWallet* CreateFromDictionary(base::DictionaryValue* dictionary); On 2012/11/17 02:28:31, Raman Kakilate wrote: > We return empty wallet with required actions or null or valid wallet. > We should document it here. Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:50: &country_name_code)) { On 2012/11/17 02:28:31, Raman Kakilate wrote: > Indentation not correct. here and below. Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:100: Address* Address::CreateFromDisplayDictionary( On 2012/11/17 02:28:31, Raman Kakilate wrote: > I couldn't understand difference between DisplayDictionary and IdedDictonary, > Adding some context in the .h file might be helpful for future readers of the > code. Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:22: // is_minimal_address, is_valid, or is_default On 2012/11/17 02:28:31, Raman Kakilate wrote: > we need is_minimal_address for upgrade flow ? > > and is this data part of wallet response ? or client needs to do more calls ? It's part of the response so we're ok there. I'm trying to decide if I should add this now or when I implement save to wallet. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:29: const std::string& postal_code_number, On 2012/11/17 02:28:31, Raman Kakilate wrote: > extra space before "postal_.." Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:71: void set_object_id(const std::string& object_id) { On 2012/11/17 04:10:51, Raman Kakilate wrote: > On 2012/11/17 02:28:31, Raman Kakilate wrote: > > optional: %s/object_id/address_id ? > > Ah .. ok. object_id is name used within API itself. ack. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:71: void set_object_id(const std::string& object_id) { On 2012/11/17 02:28:31, Raman Kakilate wrote: > optional: %s/object_id/address_id ? ack. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:75: static Address* CreateFromIdedDictionary( On 2012/11/17 02:28:31, Raman Kakilate wrote: > Create and Dictionary doesn't seem to go well together. > > How about > BuildIdedAddress > Build<something>Address ? Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:63: std::string GetRiskParams() { return ""; } On 2012/11/17 02:28:31, Raman Kakilate wrote: > TODO ? Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:63: std::string GetRiskParams() { return ""; } On 2012/11/17 02:28:31, Raman Kakilate wrote: > TODO ? Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:120: CHECK_LT(num_bits, kMaxBits); On 2012/11/17 02:28:31, Raman Kakilate wrote: > DCHECK ? Done. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:50: return GURL("http://nogravitas.mtv.corp.google.com:8888/online-secure/" On 2012/11/17 04:10:51, Raman Kakilate wrote: > get rid of this. and set is a flag similar to service URL ? Done.
A few initial comments. https://codereview.chromium.org/11293078/diff/28005/base/string_number_conver... File base/string_number_conversions.h (right): https://codereview.chromium.org/11293078/diff/28005/base/string_number_conver... base/string_number_conversions.h:98: BASE_EXPORT bool HexStringToInt64(const StringPiece& input, int64* output); You should probably add a new test in base/string_number_conversions_unittest.cc https://codereview.chromium.org/11293078/diff/28005/base/string_number_conver... base/string_number_conversions.h:109: #endif // BASE_STRING_NUMBER_CONVERSIONS_H_ Looks like a whitespace change. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:19: class Cart { Some comments here would be good. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:20: public: I'm not 100% sure cart is the right name for this. I usually expect a cart to track items and this seems to be focused on just price. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:59: LOG(ERROR) << "Response from Google wallet missing expiration month"; We generally prefer DLOG over LOG (and friends) in an effort to reduce binary size. If these will be useful to end users in some way (eg when contacting support) leave them in. If not, make them debug only. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:34: // Decrypts and returns PAN using generated OTP What does PAN stand for? https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:37: const std::string GetCVN(void* bytes, size_t length); Ditto CVN. Keep in mind not all Chrome contributors will understand your acronyms.
https://codereview.chromium.org/11293078/diff/28005/base/string_number_conver... File base/string_number_conversions.h (right): https://codereview.chromium.org/11293078/diff/28005/base/string_number_conver... base/string_number_conversions.h:109: #endif // BASE_STRING_NUMBER_CONVERSIONS_H_ On 2012/11/28 23:50:56, Albert Bodenhamer wrote: > Looks like a whitespace change. Yeah, the .vimrc file Google provides is doing something weird with newline at the end of files. After everything else is good I'll fix all of them. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:19: class Cart { On 2012/11/28 23:50:56, Albert Bodenhamer wrote: > Some comments here would be good. Done. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:20: public: On 2012/11/28 23:50:56, Albert Bodenhamer wrote: > I'm not 100% sure cart is the right name for this. I usually expect a cart to > track items and this seems to be focused on just price. So in the current api we're allowing support for a cart (with items and their individual prices) so I was trying to be consistent with that. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:59: LOG(ERROR) << "Response from Google wallet missing expiration month"; On 2012/11/28 23:50:56, Albert Bodenhamer wrote: > We generally prefer DLOG over LOG (and friends) in an effort to reduce binary > size. If these will be useful to end users in some way (eg when contacting > support) leave them in. If not, make them debug only. Done. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:34: // Decrypts and returns PAN using generated OTP On 2012/11/28 23:50:56, Albert Bodenhamer wrote: > What does PAN stand for? Done. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:37: const std::string GetCVN(void* bytes, size_t length); On 2012/11/28 23:50:56, Albert Bodenhamer wrote: > Ditto CVN. Keep in mind not all Chrome contributors will understand your > acronyms. Done.
A few more. Sorry for delays. It seems to be the day of interruptions. Also, please update the description to reference a bug # in crbug. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:18: class Address { Autofill has a class very similar to this in chrome/browser/autofill/address.h. Can we reduce the redundancy at all? https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:45: void EncryptOtp(const void* otp, size_t length, Delegate* delegate); Does this really need to be a void*? https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:63: // TODO(ahutter): Implement this. Ilya is currently working on the risk encoder. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:26: class Delegate { Use a more descriptive name than "Delegate". It should be obvious what you are delegating and why.
This is big enough that I'd like to get a second set of eyes on it. Dan, can you take a look as well? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:228: void WalletDataRetriever::Core::HandleResponse( nit: This function is a bit long. It might be a bit more readable if you break it up. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:51: void AcceptLegalDocuments(const std::vector<std::string> document_ids, Since other code will call into this it would be good to add comments on expected usage to these prototypes.
my brain was full at wallet_data_retriever.h, I'll come back later https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart_unittest.cc:9: namespace wallet { nit: do we usually namespace tests? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:10: nit: remove double \n https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:35: required_actions_(required_actions) {} do you need to DCHECK() any of these pointers? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:40: FullWallet* FullWallet::CreateFullWallet(DictionaryValue* dictionary) { nit: why isn't this also a scoped_ptr<FullWallet> or taking an out param (i.e. bool FullWallet::CreateFullWallet(Dictionary*, FullWallet*))? put in another way - who owns or deletes this memory? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:54: VLOG(1) << "Response from Google wallet missing required actions"; nit: DVLOG unless you want this to compile into release builds https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:61: } nit: \n after each if() IMO https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:137: const std::string FullWallet::GetPAN(void* otp, size_t length) { nit: generally abbreviations that are 3 or more letters are Camelcased rather than ALLUPPER (i.e. prefer HttpUrl instead of HTTPURL) https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:149: void FullWallet::DecryptCardInfo(uint8* otp, size_t length) { if you'd like to add comments to any of these steps, I think the reader'd benefit. for instance, I have little to no way to verify your code works as we'd anticipate just by looking at it as I have no idea what it should logically be doing. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:156: uint8* result = new uint8[length]; I'm a bit of a C++ n00b, but why can't you simply do uint8 result[length]; and then later: std::string hex_decrypted = base::HexEncode(&result, length); https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:163: DLOG(ERROR) << "Failed to parse decrypted"; decrypted? maybe decrypted needs a better? sentences that end with descriptive https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:166: std::string card_info = base::Int64ToString(decrypted); nit: are we in a \n famine? :P https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:168: size_t padded_length = (kPanSize - kBinSize + kCvnSize); nit: these () aren't necessary, and the C++ style guide recommends unnecessary () for at least return values (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Return_Values) so I'd say that's probably applicable... https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:30: ~FullWallet(); nit: \n before each method with a description https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:35: // pad (OTP) nit: finish comments (even if sentence fragments, IMO) with a . https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:55: int expiration_month_; I assume the compiler will optimize this for us (as we only essentially need the equivalent of an unsigned short or smaller). https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:60: std::string encrypted_rest_; if I were just looking at this file, I'd have no good idea what iin_, pan_, cvn, encrypted_rest_ meant... https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet_unittest.cc:15: " \"expiration_month\":12," nit: 3 \s -> 2 \s https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet_unittest.cc:411: FullWallet fullWallet(12, nit: s/fullWallet/full_wallet/ https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:15: const std::string& recipient_name, nit: line up with (, i.e. Address::Address(const std::string& country_name_code, const std::string& recipient_name, https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:33: Address::~Address() {} nit: make wallet::Cart::~Cart() into Cart::~Cart() {} or make this Address::~Address() { } IMO https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:35: Address* Address::CreateIdedAddress( can this be CreateAddressWithID to avoid strange "Ided"? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:46: VLOG(1) << "Response from Google Wallet missing phone number"; nit: why is this VLOG(1) vs DLOG()? also, why not DVLOG()? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:86: } nit: needs moar \n's, IMO https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:101: base::DictionaryValue* dictionary) { nit: is there any reason these params can't be const ref and these methods can't be const? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:147: if (country_name_code_.compare(other.country_name_code_) != 0) nit: why are you using compare instead of ==? this seems to be shorter and easier to understand (and not mess up), IMO return country_name_code_ == other.country_name_code_ && recipient_name_ == other.recipient_name_ && address_line_1_ == other.address_line_1_ && address_line_2_ == other.address_line_2_ && administrative_area_name_ == other.administrative_area_name_ && postal_code_number_ == other.postal_code_number_ && phone_number_ == other.phone_number_ && object_id_ == other.object_id_; https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:22: // is_minimal_address, is_valid, or is_default nit: end with . https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:76: // Google Wallet use. Caller owns returned pointer. nit: 1 \s between sentences in comments (if the rest of your code does). https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:161: if (value.get() && value->IsType(Value::TYPE_DICTIONARY)) { are these checks expected to ever fail? if not, let's make them DCHECK()s rather than if() https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:28: // Invoked on response to AcceptLegalDocuments request. nit: all these comments read a bit strangely to me... // Called when an AcceptLegalDocuments request finishes. or something like this might be easier to read. If this is the norm, ignore me. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:29: virtual void OnAcceptLegalDocuments() = 0; nit: \n after each method with a description https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:30: // Invoked on reponse to encrypt otp request. nit: s/abbr/ABBR/ in comments, IMO (i.e. OTP instead of otp) https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:41: // Invoked when there is a network erro or upon receiving an invalid s/erro/error/ https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:54: void EncryptOtp(const void* otp, size_t length, Delegate* delegate); add doc comments https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1350: const char kWalletSecureServiceUrl[] = "wallet-secure-service-url"; nit: line up = with = above (make some ASCII art)
https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:26: namespace { nit: \n https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:41: request_type_(NO_PENDING_REQUEST) {} nit: : context_getter_(context_getter), delegate_(NULL), request_type_(NO_PENDING_REQUEST) {} https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:42: void AcceptLegalDocuments(const std::vector<std::string> document_ids, nit: is this supposed to be void AcceptLegalDocuments(const std::vector<std::string>& document_ids, ^ ? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:53: Delegate* delegate); if this class already has |delegate_| why does it need |delegate| as a param? what's the difference? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:80: const std::string& content_type = "application/json"); according to the style guide, optional params aren't allowed in this case - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:88: RequestType request_type_; does this need a DISALLOW_COPY_AND_ASSIGN()? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:104: ++it) nit: curlies (i.e. {}) around the body of this for https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:151: request_dict.Set("cart", cart_dict.release()); nit: why not card.ToDictionary().release() ? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:185: if (!success) nit: curlies https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:200: delegate_ = delegate; you should DCHECK(delegate) or add a lot of if (delegate_) delegate_->DoSomething(); if it's valid to send a NULL delegate https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:213: bool should_retry = false; I think a caller should not have to set an out param before passing it to the function, i.e. bool should_retry; HandleResponse(source, &should_retry); and in HandleResponse() *should_retry_request = false; as currently if somebody calls HandleResponse() with a boolean that was previously true and 404, there will be an endless loop of retry + 404s, right? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:221: // We must set our context_getter_ again because nit: some don't like pronouns in comments (https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de...) and this is generally discouraged https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:222: // URLFetcher::Core::RetryOrCompleteUrlFetch resets it to NULL... s/..././ https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:239: delegate_->OnWalletError(); nit: if you don't DCHECK() delegate_, if (delegate_) delegate_->OnWalletError(); else // Some fallback if there is one. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:256: // TODO(ahutter): this condition sucks. needs to be simplified. nit: capitalize the start of sentences https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:262: if ((source->GetMaxRetries() != -1)) { nit: if (()) -> if () https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:277: case ACCEPT_LEGAL_DOCUMENTS: nit: I'd just use { } around here even though you're not introducing any new variables for style consistency and for future authors, IMO https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:286: delegate_->OnWalletError(); nit: no curlies (here + below x 2) https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:288: delegate_->OnEncryptOtp(splits[1], splits[0]); nit: I'd make this: if (splits.size() == 2) delegate_->OnEncryptOtp(splits[1], splits[0]); else delegate_->OnWalletError(); to match the: if success good response, have a cookie else bad response, go to your room pattern https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:321: net::URLRequestContextGetter* context_getter) { nit: one assumes |content_getter| can never be NULL? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:23: darth dan finds your lack of comments disturbing https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:176: MessageLoop message_loop_; nit: unused? intentionally overriding a different |message_loop_|? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:181: : public wallet::WalletDataRetriever::Delegate { nit: not totally sure, but I think this should be 4 \s, not 2 \s https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:208: fetcher->set_response_code(response_code); nit: why not just inline |response_code| throughout these tests? i.e. fetcher->set_response_code(nit::HTTP_INTERNAL_SERVER_ERROR); https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:246: // TODO(ahutter): make this more specific nit: . https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:293: // TODO(ahutter): make this more specific nit: . https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:37: WalletItems::MaskedInstrument::CreateMaskedInstrument( nit: I think this should be 4 \s indented ? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:56: if (supported_currency_list->GetString(i, ¤cy)) how would this fail? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:98: } 60 lines without a \n makes baby seals cry :_( https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:153: if (type_string.compare("VISA") == 0) I think my == comment applies to here, as well (== > .compare() == 0) https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:166: return SWITCH; nit: should there be a NOTREACHED() here? do we expect to get here often? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:184: return BILLING_INCOMPLETE; nit: same question about NOTREACHED() https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:190: const std::string& document_body) nit: indent is off https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:235: WalletItems::WalletItems(const std::vector<std::string> required_actions, nit: same question about passing this vector by value vs. const ref https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:258: if (required_action.size() > 0) { nit: no curlies https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:290: if (instrument) { nit: no curlies https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:306: if (address) { nit: no curlies https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:322: if (legal_doc) { same nit https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:21: doc comement https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:64: const std::string get_descriptive_name() const { return descriptive_name_; } nit: s/get_// for all simple dumb unix_hacker_getters() https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:105: nit: same nit re: removing get_ https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:122: // Returns null if input is invalid, an empty wallet items with required nit: this looks like a run-on sentence, use ; or . or change the wording, IMO https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:339: Address* address = new Address("country_code", does ~MaskedInstrument delete |address|? else the tests are leaking. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:391: WalletItems expected(required_actions, nit: WalletItems expected(required_actions, "", "", ""); https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:433: // expected.AddAddress(address); nit: don't keep commented out source code, IMO https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:21: std::string GetBaseWalletUrl() { nit: return a GURL https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:25: if (baseWalletServiceUrl.empty()) nit: ternary, IMO, i.e. return !baseWalletServiceUrl.empty() ? baseWalletServiceUrl : kDefaultWalletServiceUrl; or reverse the logic: if (!baseWalletServiceUrl.empty()) return baseWalletServiceUrl; return kDefaultWalletServiceUrl; https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:33: // TODO(ahutter): Should this live here? no, this will probably need to go into a different file that internal google developers can use locally and chromium builders will have some default https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:38: return GURL(baseWalletServiceUrl + "getWalletItemsJwtless"); once GetBaseWalletUrl() returns a GURL return GetBaseWalletUrl().resolve("getWalletItemsJwtless"); https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:64: if (walletSecureUrl.empty()) same nit re: ternary or order https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url_unittest.cc:11: TEST(WalletServiceUrl, CheckDefaultUrls) { I think this test isn't that useful... https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1349: // Secure service url for Online Wallet Service nit: . at end of comments, s/url/URL/, also I think "online" is implied by it being a service URL ;) https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1353: const char kWalletServiceUrl[] = "wallet-service-url"; nit: are we ever talking to wallet *insecurely*? if so - why?!
Thanks so much for looking at this Dan. I made all the changes you suggested. I was wondering if you or Albert know how to fix this error: [19483:19483:1130/200245:4623366393641:WARNING:url_request_context_getter.cc(29)] URLRequestContextGetter leaking due to no owning thread. It started when I added a RequestContext to the TestingProfile in wallet_client_unittest.cc. I have a few more TODOs to finish and a couple tests to add but otherwise this CL should be mostly done. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:18: class Address { On 2012/11/30 01:06:02, Albert Bodenhamer wrote: > Autofill has a class very similar to this in chrome/browser/autofill/address.h. > Can we reduce the redundancy at all? Added a TODO https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:45: void EncryptOtp(const void* otp, size_t length, Delegate* delegate); On 2012/11/30 01:06:02, Albert Bodenhamer wrote: > Does this really need to be a void*? It's a void* because that's what crypto/random.h spits out. If you prefer, I could switch it to a uint8*. https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.h (right): https://codereview.chromium.org/11293078/diff/28005/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:26: class Delegate { On 2012/11/30 01:06:02, Albert Bodenhamer wrote: > Use a more descriptive name than "Delegate". It should be obvious what you are > delegating and why. Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart_unittest.cc:9: namespace wallet { On 2012/11/30 19:55:52, Dan Beam wrote: > nit: do we usually namespace tests? +abodenha, do you know? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:10: On 2012/11/30 19:55:52, Dan Beam wrote: > nit: remove double \n Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:35: required_actions_(required_actions) {} On 2012/11/30 19:55:52, Dan Beam wrote: > do you need to DCHECK() any of these pointers? Except for testing, this constructor should only be used in CreateFullWallet. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:40: FullWallet* FullWallet::CreateFullWallet(DictionaryValue* dictionary) { On 2012/11/30 19:55:52, Dan Beam wrote: > nit: why isn't this also a scoped_ptr<FullWallet> or taking an out param (i.e. > bool FullWallet::CreateFullWallet(Dictionary*, FullWallet*))? put in another way > - who owns or deletes this memory? Header file comment specifies that the caller owns the memory. Is there a different style you would prefer to make that more clear. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:54: VLOG(1) << "Response from Google wallet missing required actions"; On 2012/11/30 19:55:52, Dan Beam wrote: > nit: DVLOG unless you want this to compile into release builds Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:61: } On 2012/11/30 19:55:52, Dan Beam wrote: > nit: \n after each if() IMO Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:137: const std::string FullWallet::GetPAN(void* otp, size_t length) { On 2012/11/30 19:55:52, Dan Beam wrote: > nit: generally abbreviations that are 3 or more letters are Camelcased rather > than ALLUPPER (i.e. prefer HttpUrl instead of HTTPURL) Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:149: void FullWallet::DecryptCardInfo(uint8* otp, size_t length) { On 2012/11/30 19:55:52, Dan Beam wrote: > if you'd like to add comments to any of these steps, I think the reader'd > benefit. for instance, I have little to no way to verify your code works as we'd > anticipate just by looking at it as I have no idea what it should logically be > doing. Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:156: uint8* result = new uint8[length]; On 2012/11/30 19:55:52, Dan Beam wrote: > I'm a bit of a C++ n00b, but why can't you simply do > > uint8 result[length]; > > and then later: > > std::string hex_decrypted = base::HexEncode(&result, length); The linter doesn't like arrays the aren't sized by a constant. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:163: DLOG(ERROR) << "Failed to parse decrypted"; On 2012/11/30 19:55:52, Dan Beam wrote: > decrypted? maybe decrypted needs a better? sentences that end with descriptive Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:166: std::string card_info = base::Int64ToString(decrypted); On 2012/11/30 19:55:52, Dan Beam wrote: > nit: are we in a \n famine? :P Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:168: size_t padded_length = (kPanSize - kBinSize + kCvnSize); On 2012/11/30 19:55:52, Dan Beam wrote: > nit: these () aren't necessary, and the C++ style guide recommends unnecessary > () for at least return values > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Return_Values) > so I'd say that's probably applicable... Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:30: ~FullWallet(); On 2012/11/30 19:55:52, Dan Beam wrote: > nit: \n before each method with a description Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:35: // pad (OTP) On 2012/11/30 19:55:52, Dan Beam wrote: > nit: finish comments (even if sentence fragments, IMO) with a . Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:55: int expiration_month_; On 2012/11/30 19:55:52, Dan Beam wrote: > I assume the compiler will optimize this for us (as we only essentially need the > equivalent of an unsigned short or smaller). Not sure. +abodenha? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:60: std::string encrypted_rest_; On 2012/11/30 19:55:52, Dan Beam wrote: > if I were just looking at this file, I'd have no good idea what iin_, pan_, cvn, > encrypted_rest_ meant... Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet_unittest.cc:15: " \"expiration_month\":12," On 2012/11/30 19:55:52, Dan Beam wrote: > nit: 3 \s -> 2 \s Do you mean the JSON? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet_unittest.cc:411: FullWallet fullWallet(12, On 2012/11/30 19:55:52, Dan Beam wrote: > nit: s/fullWallet/full_wallet/ Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:15: const std::string& recipient_name, On 2012/11/30 19:55:52, Dan Beam wrote: > nit: line up with (, i.e. > > Address::Address(const std::string& country_name_code, > const std::string& recipient_name, Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:33: Address::~Address() {} On 2012/11/30 19:55:52, Dan Beam wrote: > nit: make wallet::Cart::~Cart() into > > Cart::~Cart() {} > > or make this > > Address::~Address() { > } > > IMO Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:35: Address* Address::CreateIdedAddress( On 2012/11/30 19:55:52, Dan Beam wrote: > can this be CreateAddressWithID to avoid strange "Ided"? Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:46: VLOG(1) << "Response from Google Wallet missing phone number"; On 2012/11/30 19:55:52, Dan Beam wrote: > nit: why is this VLOG(1) vs DLOG()? also, why not DVLOG()? Now DVLOG. It's not DLOG because it's good for debugging to know if this happens but not necessary all the time. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:86: } On 2012/11/30 19:55:52, Dan Beam wrote: > nit: needs moar \n's, IMO Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:101: base::DictionaryValue* dictionary) { On 2012/11/30 19:55:52, Dan Beam wrote: > nit: is there any reason these params can't be const ref and these methods can't > be const? Params can be const ref but the methods are static. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:147: if (country_name_code_.compare(other.country_name_code_) != 0) On 2012/11/30 19:55:52, Dan Beam wrote: > nit: why are you using compare instead of ==? this seems to be shorter and > easier to understand (and not mess up), IMO > > return country_name_code_ == other.country_name_code_ && > recipient_name_ == other.recipient_name_ && > address_line_1_ == other.address_line_1_ && > address_line_2_ == other.address_line_2_ && > administrative_area_name_ == other.administrative_area_name_ && > postal_code_number_ == other.postal_code_number_ && > phone_number_ == other.phone_number_ && > object_id_ == other.object_id_; Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:22: // is_minimal_address, is_valid, or is_default On 2012/11/30 19:55:52, Dan Beam wrote: > nit: end with . Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:76: // Google Wallet use. Caller owns returned pointer. On 2012/11/30 19:55:52, Dan Beam wrote: > nit: 1 \s between sentences in comments (if the rest of your code does). Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:161: if (value.get() && value->IsType(Value::TYPE_DICTIONARY)) { On 2012/11/30 19:55:52, Dan Beam wrote: > are these checks expected to ever fail? if not, let's make them DCHECK()s rather > than if() Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:26: namespace { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:41: request_type_(NO_PENDING_REQUEST) {} On 2012/12/01 01:19:49, Dan Beam wrote: > nit: > > : context_getter_(context_getter), > delegate_(NULL), > request_type_(NO_PENDING_REQUEST) {} Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:42: void AcceptLegalDocuments(const std::vector<std::string> document_ids, On 2012/12/01 01:19:49, Dan Beam wrote: > nit: is this supposed to be > > void AcceptLegalDocuments(const std::vector<std::string>& document_ids, > ^ > > ? Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:53: Delegate* delegate); On 2012/12/01 01:19:49, Dan Beam wrote: > if this class already has |delegate_| why does it need |delegate| as a param? > what's the difference? To set delegate_ for the duration of the request. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:80: const std::string& content_type = "application/json"); On 2012/12/01 01:19:49, Dan Beam wrote: > according to the style guide, optional params aren't allowed in this case - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul... Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:88: RequestType request_type_; On 2012/12/01 01:19:49, Dan Beam wrote: > does this need a DISALLOW_COPY_AND_ASSIGN()? Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:104: ++it) On 2012/12/01 01:19:49, Dan Beam wrote: > nit: curlies (i.e. {}) around the body of this for Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:151: request_dict.Set("cart", cart_dict.release()); On 2012/12/01 01:19:49, Dan Beam wrote: > nit: why not card.ToDictionary().release() ? Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:185: if (!success) On 2012/12/01 01:19:49, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:200: delegate_ = delegate; On 2012/12/01 01:19:49, Dan Beam wrote: > you should DCHECK(delegate) or add a lot of > > if (delegate_) > delegate_->DoSomething(); > > if it's valid to send a NULL delegate Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:213: bool should_retry = false; On 2012/12/01 01:19:49, Dan Beam wrote: > I think a caller should not have to set an out param before passing it to the > function, i.e. > > bool should_retry; > HandleResponse(source, &should_retry); > > and in HandleResponse() > > *should_retry_request = false; > > as currently if somebody calls HandleResponse() with a boolean that was > previously true and 404, there will be an endless loop of retry + 404s, right? Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:213: bool should_retry = false; On 2012/12/01 01:19:49, Dan Beam wrote: > I think a caller should not have to set an out param before passing it to the > function, i.e. > > bool should_retry; > HandleResponse(source, &should_retry); > > and in HandleResponse() > > *should_retry_request = false; > > as currently if somebody calls HandleResponse() with a boolean that was > previously true and 404, there will be an endless loop of retry + 404s, right? The current code has no retry logic but once I upload the next version it will retry up to 3 times in that case. Should I special case 404s? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:221: // We must set our context_getter_ again because On 2012/12/01 01:19:49, Dan Beam wrote: > nit: some don't like pronouns in comments > (https://groups.google.com/a/chromium.org/forum/?fromgroups#%21topic/chromium-...) > and this is generally discouraged Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:222: // URLFetcher::Core::RetryOrCompleteUrlFetch resets it to NULL... On 2012/12/01 01:19:49, Dan Beam wrote: > s/..././ Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:239: delegate_->OnWalletError(); On 2012/12/01 01:19:49, Dan Beam wrote: > nit: if you don't DCHECK() delegate_, > > if (delegate_) > delegate_->OnWalletError(); > else > // Some fallback if there is one. Added DCHECK. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:256: // TODO(ahutter): this condition sucks. needs to be simplified. On 2012/12/01 01:19:49, Dan Beam wrote: > nit: capitalize the start of sentences Gone https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:262: if ((source->GetMaxRetries() != -1)) { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: if (()) -> if () Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:277: case ACCEPT_LEGAL_DOCUMENTS: On 2012/12/01 01:19:49, Dan Beam wrote: > nit: I'd just use { } around here even though you're not introducing any new > variables for style consistency and for future authors, IMO Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:286: delegate_->OnWalletError(); On 2012/12/01 01:19:49, Dan Beam wrote: > nit: no curlies (here + below x 2) Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:288: delegate_->OnEncryptOtp(splits[1], splits[0]); On 2012/12/01 01:19:49, Dan Beam wrote: > nit: I'd make this: > > if (splits.size() == 2) > delegate_->OnEncryptOtp(splits[1], splits[0]); > else > delegate_->OnWalletError(); > > to match the: > > if success > good response, have a cookie > else > bad response, go to your room > > pattern Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.cc:321: net::URLRequestContextGetter* context_getter) { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: one assumes |content_getter| can never be NULL? Added a DCHECK. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:23: On 2012/12/01 01:19:49, Dan Beam wrote: > darth dan finds your lack of comments disturbing Added a number of comments. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:28: // Invoked on response to AcceptLegalDocuments request. On 2012/11/30 19:55:52, Dan Beam wrote: > nit: all these comments read a bit strangely to me... > > // Called when an AcceptLegalDocuments request finishes. > > or something like this might be easier to read. If this is the norm, ignore me. Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:29: virtual void OnAcceptLegalDocuments() = 0; On 2012/11/30 19:55:52, Dan Beam wrote: > nit: \n after each method with a description Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:30: // Invoked on reponse to encrypt otp request. On 2012/11/30 19:55:52, Dan Beam wrote: > nit: s/abbr/ABBR/ in comments, IMO (i.e. OTP instead of otp) Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:41: // Invoked when there is a network erro or upon receiving an invalid On 2012/11/30 19:55:52, Dan Beam wrote: > s/erro/error/ Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:51: void AcceptLegalDocuments(const std::vector<std::string> document_ids, On 2012/11/30 18:14:07, Albert Bodenhamer wrote: > Since other code will call into this it would be good to add comments on > expected usage to these prototypes. Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever.h:54: void EncryptOtp(const void* otp, size_t length, Delegate* delegate); On 2012/11/30 19:55:52, Dan Beam wrote: > add doc comments Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:176: MessageLoop message_loop_; On 2012/12/01 01:19:49, Dan Beam wrote: > nit: unused? intentionally overriding a different |message_loop_|? Supposedly required for TestURLFetcherFactory to work. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:181: : public wallet::WalletDataRetriever::Delegate { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: not totally sure, but I think this should be 4 \s, not 2 \s Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:208: fetcher->set_response_code(response_code); On 2012/12/01 01:19:49, Dan Beam wrote: > nit: why not just inline |response_code| throughout these tests? i.e. > > fetcher->set_response_code(nit::HTTP_INTERNAL_SERVER_ERROR); Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:246: // TODO(ahutter): make this more specific On 2012/12/01 01:19:49, Dan Beam wrote: > nit: . Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_data_retriever_unittest.cc:293: // TODO(ahutter): make this more specific On 2012/12/01 01:19:49, Dan Beam wrote: > nit: . Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:37: WalletItems::MaskedInstrument::CreateMaskedInstrument( On 2012/12/01 01:19:49, Dan Beam wrote: > nit: I think this should be 4 \s indented ? Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:56: if (supported_currency_list->GetString(i, ¤cy)) On 2012/12/01 01:19:49, Dan Beam wrote: > how would this fail? If the element at the i-th position wasn't a string. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:98: } On 2012/12/01 01:19:49, Dan Beam wrote: > 60 lines without a \n makes baby seals cry :_( Sorry. I'm fixing it I swear. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:153: if (type_string.compare("VISA") == 0) On 2012/12/01 01:19:49, Dan Beam wrote: > I think my == comment applies to here, as well (== > .compare() == 0) Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:166: return SWITCH; On 2012/12/01 01:19:49, Dan Beam wrote: > nit: should there be a NOTREACHED() here? do we expect to get here often? UNKNOWN is an actual type coming from Wallet so it's possible. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:184: return BILLING_INCOMPLETE; On 2012/12/01 01:19:49, Dan Beam wrote: > nit: same question about NOTREACHED() Same answer. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:190: const std::string& document_body) On 2012/12/01 01:19:49, Dan Beam wrote: > nit: indent is off Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:235: WalletItems::WalletItems(const std::vector<std::string> required_actions, On 2012/12/01 01:19:49, Dan Beam wrote: > nit: same question about passing this vector by value vs. const ref Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:258: if (required_action.size() > 0) { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:290: if (instrument) { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:306: if (address) { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:322: if (legal_doc) { On 2012/12/01 01:19:49, Dan Beam wrote: > same nit Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.h (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:21: On 2012/12/01 01:19:49, Dan Beam wrote: > doc comement Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:64: const std::string get_descriptive_name() const { return descriptive_name_; } On 2012/12/01 01:19:49, Dan Beam wrote: > nit: s/get_// for all simple dumb unix_hacker_getters() Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:105: On 2012/12/01 01:19:49, Dan Beam wrote: > nit: same nit re: removing get_ Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:122: // Returns null if input is invalid, an empty wallet items with required On 2012/12/01 01:19:49, Dan Beam wrote: > nit: this looks like a run-on sentence, use ; or . or change the wording, IMO Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:339: Address* address = new Address("country_code", On 2012/12/01 01:19:49, Dan Beam wrote: > does ~MaskedInstrument delete |address|? else the tests are leaking. Yeah, address goes into a scoped_ptr in the constructor of MaskedInstrument. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:391: WalletItems expected(required_actions, On 2012/12/01 01:19:49, Dan Beam wrote: > nit: > > WalletItems expected(required_actions, "", "", ""); Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:433: // expected.AddAddress(address); On 2012/12/01 01:19:49, Dan Beam wrote: > nit: don't keep commented out source code, IMO Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:21: std::string GetBaseWalletUrl() { On 2012/12/01 01:19:49, Dan Beam wrote: > nit: return a GURL Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:25: if (baseWalletServiceUrl.empty()) On 2012/12/01 01:19:49, Dan Beam wrote: > nit: ternary, IMO, i.e. > > return !baseWalletServiceUrl.empty() ? baseWalletServiceUrl : > kDefaultWalletServiceUrl; > > or reverse the logic: > > if (!baseWalletServiceUrl.empty()) > return baseWalletServiceUrl; > > return kDefaultWalletServiceUrl; Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:33: // TODO(ahutter): Should this live here? On 2012/12/01 01:19:49, Dan Beam wrote: > no, this will probably need to go into a different file that internal google > developers can use locally and chromium builders will have some default Who should I ask about that? https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:38: return GURL(baseWalletServiceUrl + "getWalletItemsJwtless"); On 2012/12/01 01:19:49, Dan Beam wrote: > once GetBaseWalletUrl() returns a GURL > > return GetBaseWalletUrl().resolve("getWalletItemsJwtless"); Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:64: if (walletSecureUrl.empty()) On 2012/12/01 01:19:49, Dan Beam wrote: > same nit re: ternary or order Done. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url_unittest.cc:11: TEST(WalletServiceUrl, CheckDefaultUrls) { On 2012/12/01 01:19:49, Dan Beam wrote: > I think this test isn't that useful... Yeah... we have a similar one for autofill_download_url so I went with it. https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1349: // Secure service url for Online Wallet Service On 2012/12/01 01:19:49, Dan Beam wrote: > nit: . at end of comments, s/url/URL/, also I think "online" is implied by it > being a service URL ;) Agreed, but Online Wallet is actually the name of the product. https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1350: const char kWalletSecureServiceUrl[] = "wallet-secure-service-url"; On 2012/11/30 19:55:52, Dan Beam wrote: > nit: line up = with = above (make some ASCII art) Done. https://codereview.chromium.org/11293078/diff/32002/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1353: const char kWalletServiceUrl[] = "wallet-service-url"; On 2012/12/01 01:19:49, Dan Beam wrote: > nit: are we ever talking to wallet *insecurely*? if so - why?! The secure url is used for escrowing credit card number and encrypting OTPs, stuff we can't do on our regular servers.
More fixes from code reviews. I think this is ready for a final review.
Mac builder is complaining about uninitialized variable use. See the build log at: http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/79557/... A few other (mostly nit) things below. I'm not done yet. I just wanted to give you a heads up on the build issue quickly. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:20: // is required to request a FullWallet from Online Wallet in order to set Callers of requestAutcomplete MIGHT not have this available. Should we just pass a default value in that case? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:22: class Cart { I understand the need to match the vocabulary to Sugar terms. Perhaps add a comment explaining why this is called "Cart" even though it's just a price? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:36: || (billing_address_ && shipping_address_)); nit: move the || to the line above and align the second term of the DCHECK with the first. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:25: FullWallet(int expiration_month, It looks like CreateFullWallet is the correct way to mint one of these. Should the constructor be private? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:18: // TODO(ahutter): This address is a lot like chrome/browser/autofill/address.h. Probably worth filing a crbug to track the cleanup and reference it from the todo.
lgtm https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:287: class WalletItemsTest : public testing::Test { Optional: May be not in this CL, but we would need a API compatibility test. I am not sure about where to implement this ? (chrome code base doesn't look like good option as whenever that API becomes incompatible, we will break chrome without giving them ability to fix it).
https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:26: // actions may be required before a purchase can be completing using Online typo: completed, not completing https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:287: class WalletItemsTest : public testing::Test { On 2012/12/05 19:02:22, Raman Kakilate wrote: > Optional: May be not in this CL, but we would need a API compatibility test. I > am not sure about where to implement this ? (chrome code base doesn't look like > good option as whenever that API becomes incompatible, we will break chrome > without giving them ability to fix it). Good idea. In general it's best to have compat tests on BOTH sides of an API. That way if either side tries to introduce a breaking change the test will catch it.
https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:40: FullWallet* FullWallet::CreateFullWallet(DictionaryValue* dictionary) { On 2012/12/01 04:06:51, ahutter wrote: > On 2012/11/30 19:55:52, Dan Beam wrote: > > nit: why isn't this also a scoped_ptr<FullWallet> or taking an out param (i.e. > > bool FullWallet::CreateFullWallet(Dictionary*, FullWallet*))? put in another > way > > - who owns or deletes this memory? > > Header file comment specifies that the caller owns the memory. Is there a > different style you would prefer to make that more clear. > I guess if it's called Create*() that's implied enough. https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet_unittest.cc (right): https://codereview.chromium.org/11293078/diff/32002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet_unittest.cc:15: " \"expiration_month\":12," On 2012/12/01 04:06:51, ahutter wrote: > On 2012/11/30 19:55:52, Dan Beam wrote: > > nit: 3 \s -> 2 \s > > Do you mean the JSON? Yes, basically -1 \s from all the JSON you've put in these tests. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:13: size_t kPanSize = 16; const x 3 https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:155: return country_name_code_ == other.country_name_code_ nit: && should be at the end of each line (operators should go at the end) https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:18: // TODO(ahutter): This address is a lot like chrome/browser/autofill/address.h. On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > Probably worth filing a crbug to track the cleanup and reference it from the > todo. +1 https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:12: namespace { nit: \n https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:13: static const char kAddressMissingObjectId[] = nit: static in an anonymous namespace does nothing, please remove these in all files https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:245: if (source->GetMaxRetries() != -1 nit: && on end of top line instead of start of bottom https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:263: DVLOG(1) << "Hitting " << source->GetOriginalURL(); nit: s/Hitting/Got response from/, maybe? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:128: "\"api_key\":\"abcdefg\"," nit: why did you indent this JSON differently than above and other tests? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:167: nit: s/\n\n/\n/ https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:186: TestingProfile profile_; nit: \n https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:298: EXPECT_CALL(observer, OnGetFullWallet(testing::NotNull())).Times(1); cool, didn't know about testing::NotNull() https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:162: WalletItems::MaskedInstrument::TypeFromString( nit: + 4 \s indent, I think https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:236: && document_body_ == other.document_body_; nit: && at ends https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:293: MaskedInstrument::CreateMaskedInstrument(*instrument_dict); nit: I think this is a bit risky for somebody to come along later and change this to something like if (instrument && instrument->something()) which is subtlely different as it leaks memory. Can you change this code to: scoped_ptr<MaskedInstrument> instrument( MaskedInstrument::CreateMaskedInstrument(*instrument_dict)); if (instrument.get()) wallet_items->AddInstrument(instrument.release()); and count on ~scoped_ptr self-deleting? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:332: "Google wallet"; nit: fix indent DLOG(ERROR) << "Malformed legal document in response from " "Google wallet"; https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:333: delete wallet_items; same nit re: using scoped_ptr::~scoped_ptr vs. delete and just .release()ing at the end. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:350: && required_actions_ == required_actions_; nit: ops at end https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:42: SWITCH, nit: is there a rationale to the order of these enums? https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:51: // correspond to any other inapplicable reasons that do not listed above nit: fix this comment https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:55: Type type, nit: why are |type| and |status| not const-ref?
btw, you'll need a base/OWNERS review, candidates are: mark@chromium.org darin@chromium.org brettw@chromium.org willchan@chromium.org jar@chromium.org
Adding darin@ for base/ and chrome/ OWNERS approval. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:20: // is required to request a FullWallet from Online Wallet in order to set On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > Callers of requestAutcomplete MIGHT not have this available. Should we just > pass a default value in that case? Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:22: class Cart { On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > I understand the need to match the vocabulary to Sugar terms. Perhaps add a > comment explaining why this is called "Cart" even though it's just a price? Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:13: size_t kPanSize = 16; On 2012/12/05 19:34:59, Dan Beam wrote: > const x 3 Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:36: || (billing_address_ && shipping_address_)); On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > nit: move the || to the line above and align the second term of the DCHECK with > the first. Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:25: FullWallet(int expiration_month, On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > It looks like CreateFullWallet is the correct way to mint one of these. Should > the constructor be private? Public for unit testing. Added a comment. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:155: return country_name_code_ == other.country_name_code_ On 2012/12/05 19:34:59, Dan Beam wrote: > nit: && should be at the end of each line (operators should go at the end) Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:18: // TODO(ahutter): This address is a lot like chrome/browser/autofill/address.h. On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > Probably worth filing a crbug to track the cleanup and reference it from the > todo. Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:18: // TODO(ahutter): This address is a lot like chrome/browser/autofill/address.h. On 2012/12/05 18:56:45, Albert Bodenhamer wrote: > Probably worth filing a crbug to track the cleanup and reference it from the > todo. Ack. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:12: namespace { On 2012/12/05 19:34:59, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:13: static const char kAddressMissingObjectId[] = On 2012/12/05 19:34:59, Dan Beam wrote: > nit: static in an anonymous namespace does nothing, please remove these in all > files Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:245: if (source->GetMaxRetries() != -1 On 2012/12/05 19:34:59, Dan Beam wrote: > nit: && on end of top line instead of start of bottom Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:263: DVLOG(1) << "Hitting " << source->GetOriginalURL(); On 2012/12/05 19:34:59, Dan Beam wrote: > nit: s/Hitting/Got response from/, maybe? Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:128: "\"api_key\":\"abcdefg\"," On 2012/12/05 19:34:59, Dan Beam wrote: > nit: why did you indent this JSON differently than above and other tests? ws matters here since I'm testing the request payload against this. Added a comment. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:167: On 2012/12/05 19:34:59, Dan Beam wrote: > nit: s/\n\n/\n/ Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:186: TestingProfile profile_; On 2012/12/05 19:34:59, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client_unittest.cc:298: EXPECT_CALL(observer, OnGetFullWallet(testing::NotNull())).Times(1); On 2012/12/05 19:34:59, Dan Beam wrote: > cool, didn't know about testing::NotNull() Ack. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:162: WalletItems::MaskedInstrument::TypeFromString( On 2012/12/05 19:34:59, Dan Beam wrote: > nit: + 4 \s indent, I think Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:236: && document_body_ == other.document_body_; On 2012/12/05 19:34:59, Dan Beam wrote: > nit: && at ends Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:293: MaskedInstrument::CreateMaskedInstrument(*instrument_dict); On 2012/12/05 19:34:59, Dan Beam wrote: > nit: I think this is a bit risky for somebody to come along later and change > this to something like > > if (instrument && instrument->something()) > > which is subtlely different as it leaks memory. Can you change this code to: > > scoped_ptr<MaskedInstrument> instrument( > MaskedInstrument::CreateMaskedInstrument(*instrument_dict)); > if (instrument.get()) > wallet_items->AddInstrument(instrument.release()); > > and count on ~scoped_ptr self-deleting? Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:332: "Google wallet"; On 2012/12/05 19:34:59, Dan Beam wrote: > nit: fix indent > > DLOG(ERROR) << "Malformed legal document in response from " > "Google wallet"; Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:333: delete wallet_items; On 2012/12/05 19:34:59, Dan Beam wrote: > same nit re: using scoped_ptr::~scoped_ptr vs. delete and just .release()ing at > the end. Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:350: && required_actions_ == required_actions_; On 2012/12/05 19:34:59, Dan Beam wrote: > nit: ops at end Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.h (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:26: // actions may be required before a purchase can be completing using Online On 2012/12/05 19:22:29, Albert Bodenhamer wrote: > typo: completed, not completing Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:42: SWITCH, On 2012/12/05 19:34:59, Dan Beam wrote: > nit: is there a rationale to the order of these enums? Nope. Now alphabetized. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:51: // correspond to any other inapplicable reasons that do not listed above On 2012/12/05 19:34:59, Dan Beam wrote: > nit: fix this comment Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:55: Type type, On 2012/12/05 19:34:59, Dan Beam wrote: > nit: why are |type| and |status| not const-ref? Done. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:287: class WalletItemsTest : public testing::Test { On 2012/12/05 19:22:29, Albert Bodenhamer wrote: > On 2012/12/05 19:02:22, Raman Kakilate wrote: > > Optional: May be not in this CL, but we would need a API compatibility test. I > > am not sure about where to implement this ? (chrome code base doesn't look > like > > good option as whenever that API becomes incompatible, we will break chrome > > without giving them ability to fix it). > > Good idea. In general it's best to have compat tests on BOTH sides of an API. > That way if either side tries to introduce a breaking change the test will catch > it. Added a comment in wallet_unit_tests.cc and a bug. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items_unittest.cc:287: class WalletItemsTest : public testing::Test { On 2012/12/05 19:02:22, Raman Kakilate wrote: > Optional: May be not in this CL, but we would need a API compatibility test. I > am not sure about where to implement this ? (chrome code base doesn't look like > good option as whenever that API becomes incompatible, we will break chrome > without giving them ability to fix it). Ack.
lgtm Please wait for Dan and Darin before committing. https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://codereview.chromium.org/11293078/diff/47002/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:13: static const char kAddressMissingObjectId[] = On 2012/12/05 19:34:59, Dan Beam wrote: > nit: static in an anonymous namespace does nothing, please remove these in all > files Not ENTIRELY nothing, but close enough. It'll result in a slightly smaller link table. Resulting binary will probably be identical. That said, yeah, best to keep the code simple. https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:35: DCHECK(required_actions_.size() > 0 || billing_address_); You're no longer DCHECKing shipping_address_ here. Did you have to make other changes? Or was it just not needed? https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:156: recipient_name_ == other.recipient_name_ && nit: 4 space indent.
https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.cc:156: recipient_name_ == other.recipient_name_ && On 2012/12/06 00:36:43, Albert Bodenhamer wrote: > nit: 4 space indent. I thought ASCII ident prevailed in Chrome? see new Address() above ^
https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:35: DCHECK(required_actions_.size() > 0 || billing_address_); On 2012/12/06 00:36:43, Albert Bodenhamer wrote: > You're no longer DCHECKing shipping_address_ here. Did you have to make other > changes? Or was it just not needed? Wasn't actually needed.
lgtm https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:51: return NULL; nit: I'd personally group all critical failures at the end of each Create*() method (or anything that can return NULL and bail), as there's no reason to do more work if a critical failure will waste all of it later.
https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:51: return NULL; On 2012/12/06 21:59:24, Dan Beam wrote: > nit: I'd personally group all critical failures at the end of each Create*() > method (or anything that can return NULL and bail), as there's no reason to do > more work if a critical failure will waste all of it later. whooops! I meant group all critical failures at the *beginning*.
Rebased and fixed linter issues and latest code review comments. https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/37010/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:51: return NULL; On 2012/12/06 22:00:14, Dan Beam wrote: > On 2012/12/06 21:59:24, Dan Beam wrote: > > nit: I'd personally group all critical failures at the end of each Create*() > > method (or anything that can return NULL and bail), as there's no reason to do > > more work if a critical failure will waste all of it later. > > whooops! I meant group all critical failures at the *beginning*. Done.
Adding Brett for base/ and chrome/ OWNER approval.
On 2012/12/06 22:46:27, ahutter wrote: > Adding Brett for base/ and chrome/ OWNER approval. In particular, chrome/chrome_browser.gypi and base/string_number_conversion*.
did you clear the CC field intentionally? https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:21: // spending limits on the generated proxy card. If the actual amount is not please make # of spaces after periods consistent (I prefer 1 but 2 is ok as long as it's consistent). https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:34: std::string total_price_; what is the format of the price? You should document that here. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:35: std::string currency_code_; ditto https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:25: Address* billing_address, you can (and should) make these a scoped_ptr, which makes the ownership model more clear/harder to screw up https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:110: bool FullWallet::operator==(const FullWallet& other) const { more vertical whitespace in this fn https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:164: uint8* result = new uint8[length]; wrap this with a scoped_ptr https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:169: // There is no uint8* to int64 so I convert the encrypted data to hex and then I think it's best to avoid using "I" in comments. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:45: // time pad (OTP). please use variable names in your documentation, like |bytes| if the OTP is stored in |bytes|. Why not pass a std::vector? Why is it void* instead of uint8*? https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:64: std::string pan_; docs on the format they're stored in https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:66: std::string cvn_; why is this not an int of some kind https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:73: std::vector<std::string> required_actions_; docs https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:37: class WalletClient::Core class level comment. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:41: explicit Core(net::URLRequestContextGetter* context_getter) i believe this should take a scoped_refptr https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:95: scoped_refptr<net::URLRequestContextGetter> context_getter_; docs for all these https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:169: WalletClient::WalletClientObserver* observer) { more vertical whitespace in this fn, and pretty much every fn in this file https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:350: } \n https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:23: std::string baseWalletServiceUrl = command_line.GetSwitchValueASCII( variable names go_like_this also, break the line after = instead of in the middle of a function call https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:26: GURL(kDefaultWalletServiceUrl); indent this to match the first GURL if possible https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:41: } \n https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:58: return !walletSecureUrl.empty() ? GURL(walletSecureUrl) : looks like a there's an extra space here https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:59: GURL(kDefaultWalletSecureServiceUrl); indent this to match the first GURL if possible https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.h:20: } // end wallet namespace normally this would be just } // namespace wallet https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.h:23: extra newline?
Some high-level comments: (1) When uploading a Chromium CL, please don't clear out the automatic CC list. (2) Thanks for writing clear and thorough test code. (3) Most of my comments below are on issues that are repeated in many places in this CL. I've noted them in a few places, but please fix all similar instances throughout. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.cc:44: for (size_t i = 0; i < required_actions_list->GetSize(); i++) { nit: ++i https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.cc:164: uint8* result = new uint8[length]; On 2012/12/07 19:57:44, Evan Stade wrote: > wrap this with a scoped_ptr Rather, scoped_array https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.cc:166: for (size_t i = 0; i < length; i++) nit: ++i https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/full_wallet.h (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.h:25: // This constructor is only public for unit testing. nit: Please instead make it private (or at most protected), and friend any tests if needed. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.h:32: const std::vector<std::string> required_actions); nit: Pass by reference. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.h:38: static FullWallet* CreateFullWallet(const base::DictionaryValue& dictionary); This method should return a scoped_ptr<> rather than a raw pointer to heap-allocated memory. The unit tests for this code currently leak. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/full_wallet.h:52: const Address* shipping_address() const { return shipping_address_.get(); } nit: Please return by const-reference instead of const pointer. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/wallet_address.h (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_address.h:82: static Address* CreateAddressWithID(const base::DictionaryValue& dictionary); This method should return a scoped_ptr<>, rather than a raw pointer to heap-allocated memory. The test code for this class currently leaks. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_address.h:86: static Address* CreateDisplayAddress(const base::DictionaryValue& dictionary); Ditto. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_address_unittest.cc:159: } nit: If you keep this method, it should be annotated with OVERRIDE. However, it currently does nothing: the object is constructed afresh for each test, so you're resetting an already empty scoped_ptr. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_client.cc:83: void MakeWalletRequest(const GURL url, nit: Pass by const-reference. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/wallet_client.h (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_client.h:113: scoped_refptr<Core> core_; Why does this need to be reference counted? https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... File chrome/browser/autofill/wallet/wallet_items.h (right): https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:54: // This constructor is only public for unit testing. Please reduce the visibility of this constructor and friend tests as needed instead. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:57: std::vector<std::string> supported_currencies, nit: Pass by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:62: Address* address, nit: Pass a scoped_ptr https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:76: const std::string descriptive_name() const { return descriptive_name_; } nit: Return by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:80: } nit: Return by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:81: const std::string last_four_digits() const { return last_four_digits_; } nit: Return by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:84: const std::string brand() const { return brand_; } nit: Return by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:85: const Address* address() const { return address_.get(); } nit: Return by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:87: const std::string object_id() const { return object_id_; } nit: Return by const reference https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:91: static Status StatusFromString(const std::string& status_string); nit: Please tuck these into an anonymous namespace in the implementation file. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:145: // owns returned pointer. Please return a scoped_ptr<> rather than a raw pointer. https://chromiumcodereview.appspot.com/11293078/diff/57004/chrome/browser/aut... chrome/browser/autofill/wallet/wallet_items.h:152: void AddInstrument(MaskedInstrument* instrument) { Please pass this in as a scoped_ptr<>. Ditto for the two methods below.
btw, when you upload an empty CL then add to it via repeated `git cl upload` with many more files I don't think the CC list is repopulated from watchlists (encountered this today)
https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/cart.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:21: // spending limits on the generated proxy card. If the actual amount is not On 2012/12/07 19:57:44, Evan Stade wrote: > please make # of spaces after periods consistent (I prefer 1 but 2 is ok as long > as it's consistent). Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:34: std::string total_price_; On 2012/12/07 19:57:44, Evan Stade wrote: > what is the format of the price? You should document that here. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/cart.h:35: std::string currency_code_; On 2012/12/07 19:57:44, Evan Stade wrote: > ditto Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:25: Address* billing_address, On 2012/12/07 19:57:44, Evan Stade wrote: > you can (and should) make these a scoped_ptr, which makes the ownership model > more clear/harder to screw up Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:44: for (size_t i = 0; i < required_actions_list->GetSize(); i++) { On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: ++i Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:110: bool FullWallet::operator==(const FullWallet& other) const { On 2012/12/07 19:57:44, Evan Stade wrote: > more vertical whitespace in this fn Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:164: uint8* result = new uint8[length]; On 2012/12/07 19:57:44, Evan Stade wrote: > wrap this with a scoped_ptr Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:164: uint8* result = new uint8[length]; On 2012/12/14 04:56:43, Ilya Sherman wrote: > On 2012/12/07 19:57:44, Evan Stade wrote: > > wrap this with a scoped_ptr > > Rather, scoped_array Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:166: for (size_t i = 0; i < length; i++) On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: ++i Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:169: // There is no uint8* to int64 so I convert the encrypted data to hex and then On 2012/12/07 19:57:44, Evan Stade wrote: > I think it's best to avoid using "I" in comments. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:25: // This constructor is only public for unit testing. On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Please instead make it private (or at most protected), and friend any tests > if needed. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:32: const std::vector<std::string> required_actions); On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Pass by reference. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:38: static FullWallet* CreateFullWallet(const base::DictionaryValue& dictionary); On 2012/12/14 04:56:43, Ilya Sherman wrote: > This method should return a scoped_ptr<> rather than a raw pointer to > heap-allocated memory. The unit tests for this code currently leak. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:45: // time pad (OTP). On 2012/12/07 19:57:44, Evan Stade wrote: > please use variable names in your documentation, like |bytes| if the OTP is > stored in |bytes|. Done. > > Why not pass a std::vector? Why is it void* instead of uint8*? To match the return value of crypto::RandBytes. Adding a comment to EncryptOtp to that affect. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:52: const Address* shipping_address() const { return shipping_address_.get(); } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Please return by const-reference instead of const pointer. They could be NULL. What should I do in that case? https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:64: std::string pan_; On 2012/12/07 19:57:44, Evan Stade wrote: > docs on the format they're stored in Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:66: std::string cvn_; On 2012/12/07 19:57:44, Evan Stade wrote: > why is this not an int of some kind There can be leading zeros. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:73: std::vector<std::string> required_actions_; On 2012/12/07 19:57:44, Evan Stade wrote: > docs Done. Added a TODO and bug to clean this up a little. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:82: static Address* CreateAddressWithID(const base::DictionaryValue& dictionary); On 2012/12/14 04:56:43, Ilya Sherman wrote: > This method should return a scoped_ptr<>, rather than a raw pointer to > heap-allocated memory. The test code for this class currently leaks. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:86: static Address* CreateDisplayAddress(const base::DictionaryValue& dictionary); On 2012/12/14 04:56:43, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address_unittest.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address_unittest.cc:159: } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: If you keep this method, it should be annotated with OVERRIDE. However, it > currently does nothing: the object is constructed afresh for each test, so > you're resetting an already empty scoped_ptr. Removed. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:37: class WalletClient::Core On 2012/12/07 19:57:44, Evan Stade wrote: > class level comment. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:41: explicit Core(net::URLRequestContextGetter* context_getter) On 2012/12/07 19:57:44, Evan Stade wrote: > i believe this should take a scoped_refptr I'm not seeing any other users of RequestContextGetter that do so. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:83: void MakeWalletRequest(const GURL url, On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Pass by const-reference. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:95: scoped_refptr<net::URLRequestContextGetter> context_getter_; On 2012/12/07 19:57:44, Evan Stade wrote: > docs for all these Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.cc:169: WalletClient::WalletClientObserver* observer) { On 2012/12/07 19:57:44, Evan Stade wrote: > more vertical whitespace in this fn, and pretty much every fn in this file Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.h:113: scoped_refptr<Core> core_; On 2012/12/14 04:56:43, Ilya Sherman wrote: > Why does this need to be reference counted? I was emulating other users of URLFetcherDelegate. Should I get rid of it? https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.cc:350: } On 2012/12/07 19:57:44, Evan Stade wrote: > \n Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_items.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:54: // This constructor is only public for unit testing. On 2012/12/14 04:56:43, Ilya Sherman wrote: > Please reduce the visibility of this constructor and friend tests as needed > instead. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:57: std::vector<std::string> supported_currencies, On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Pass by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:62: Address* address, On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Pass a scoped_ptr Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:76: const std::string descriptive_name() const { return descriptive_name_; } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Return by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:80: } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Return by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:81: const std::string last_four_digits() const { return last_four_digits_; } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Return by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:84: const std::string brand() const { return brand_; } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Return by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:85: const Address* address() const { return address_.get(); } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Return by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:87: const std::string object_id() const { return object_id_; } On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Return by const reference Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:91: static Status StatusFromString(const std::string& status_string); On 2012/12/14 04:56:43, Ilya Sherman wrote: > nit: Please tuck these into an anonymous namespace in the implementation file. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:145: // owns returned pointer. On 2012/12/14 04:56:43, Ilya Sherman wrote: > Please return a scoped_ptr<> rather than a raw pointer. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_items.h:152: void AddInstrument(MaskedInstrument* instrument) { On 2012/12/14 04:56:43, Ilya Sherman wrote: > Please pass this in as a scoped_ptr<>. Ditto for the two methods below. Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:23: std::string baseWalletServiceUrl = command_line.GetSwitchValueASCII( On 2012/12/07 19:57:44, Evan Stade wrote: > variable names go_like_this > > also, break the line after = instead of in the middle of a function call Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:26: GURL(kDefaultWalletServiceUrl); On 2012/12/07 19:57:44, Evan Stade wrote: > indent this to match the first GURL if possible Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:41: } On 2012/12/07 19:57:44, Evan Stade wrote: > \n Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:58: return !walletSecureUrl.empty() ? GURL(walletSecureUrl) : On 2012/12/07 19:57:44, Evan Stade wrote: > looks like a there's an extra space here Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.cc:59: GURL(kDefaultWalletSecureServiceUrl); On 2012/12/07 19:57:44, Evan Stade wrote: > indent this to match the first GURL if possible Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.h:20: } // end wallet namespace On 2012/12/07 19:57:44, Evan Stade wrote: > normally this would be just > > } // namespace wallet Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.h:23: On 2012/12/07 19:57:44, Evan Stade wrote: > extra newline? If the file doesn't end in a newline the linter complains.
https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:52: const Address* shipping_address() const { return shipping_address_.get(); } On 2012/12/15 01:06:31, ahutter wrote: > On 2012/12/14 04:56:43, Ilya Sherman wrote: > > nit: Please return by const-reference instead of const pointer. > > They could be NULL. What should I do in that case? Ah, in that case const pointer is ok, but please document that they can be NULL. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_client.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_client.h:113: scoped_refptr<Core> core_; On 2012/12/15 01:06:31, ahutter wrote: > On 2012/12/14 04:56:43, Ilya Sherman wrote: > > Why does this need to be reference counted? > > I was emulating other users of URLFetcherDelegate. Should I get rid of it? If that's the only reason, then yes, please get rid of it. Also, if this is the only reason that you have the private Core class, then IMO it would be better to drop that class as well, and instead directly add that functionality to the private interface for this class. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.h:23: On 2012/12/15 01:06:31, ahutter wrote: > On 2012/12/07 19:57:44, Evan Stade wrote: > > extra newline? > > If the file doesn't end in a newline the linter complains. Which linter? The one on the code review site is very out of date, and should pretty much always be ignored.
https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1367: const char kWalletServiceUrl[] = "wallet-service-url"; Why do we have a non-secure URL (that we're willing to use) for the service? These flags should probably be documented better to explain what they mean.
On 2012/12/15 01:23:59, Ilya Sherman wrote: > https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... > File chrome/browser/autofill/wallet/full_wallet.h (right): > > https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... > chrome/browser/autofill/wallet/full_wallet.h:52: const Address* > shipping_address() const { return shipping_address_.get(); } > On 2012/12/15 01:06:31, ahutter wrote: > > On 2012/12/14 04:56:43, Ilya Sherman wrote: > > > nit: Please return by const-reference instead of const pointer. > > > > They could be NULL. What should I do in that case? > > Ah, in that case const pointer is ok, but please document that they can be NULL. > > Done. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... > File chrome/browser/autofill/wallet/wallet_client.h (right): > > https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... > chrome/browser/autofill/wallet/wallet_client.h:113: scoped_refptr<Core> core_; > On 2012/12/15 01:06:31, ahutter wrote: > > On 2012/12/14 04:56:43, Ilya Sherman wrote: > > > Why does this need to be reference counted? > > > > I was emulating other users of URLFetcherDelegate. Should I get rid of it? > > If that's the only reason, then yes, please get rid of it. Also, if this is the > only reason that you have the private Core class, then IMO it would be better to > drop that class as well, and instead directly add that functionality to the > private interface for this class. > > Removed. https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... > File chrome/browser/autofill/wallet/wallet_service_url.h (right): > > https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... > chrome/browser/autofill/wallet/wallet_service_url.h:23: > On 2012/12/15 01:06:31, ahutter wrote: > > On 2012/12/07 19:57:44, Evan Stade wrote: > > > extra newline? > > > > If the file doesn't end in a newline the linter complains. > > Which linter? The one on the code review site is very out of date, and should > pretty much always be ignored. Argh. I'll do that in the future.
https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1367: const char kWalletServiceUrl[] = "wallet-service-url"; On 2012/12/15 01:40:12, Ilya Sherman wrote: > Why do we have a non-secure URL (that we're willing to use) for the service? > These flags should probably be documented better to explain what they mean. Done.
lgtm https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:66: std::string cvn_; On 2012/12/15 01:06:31, ahutter wrote: > On 2012/12/07 19:57:44, Evan Stade wrote: > > why is this not an int of some kind > > There can be leading zeros. I don't see how that leads to ambiguity. You know it's 3 digits, if you need to make it user-visible, you can force the formatting. But I guess it doesn't matter too much how you store this since you won't be doing arithmetic with it (I assume). https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_service_url.h (right): https://codereview.chromium.org/11293078/diff/57004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_service_url.h:23: On 2012/12/15 01:23:59, Ilya Sherman wrote: > On 2012/12/15 01:06:31, ahutter wrote: > > On 2012/12/07 19:57:44, Evan Stade wrote: > > > extra newline? > > > > If the file doesn't end in a newline the linter complains. > > Which linter? The one on the code review site is very out of date, and should > pretty much always be ignored. the file should end in a newline, but this looks like 2 newlines. I think that rietveld screws this up for new files though, so nevermind.
https://codereview.chromium.org/11293078/diff/22004/base/string_number_conver... File base/string_number_conversions.cc (right): https://codereview.chromium.org/11293078/diff/22004/base/string_number_conver... base/string_number_conversions.cc:306: class BaseHexIteratorRangeToInt64Traits Tests? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:16: size_t kCvnSize = 3; Are these constants always true? Some credit card has <16 digits (Diners Club has 14 digits). A credit card can have upto 19 digits. Bin is not always 6. Cvn is not always 3 (AMEX has 4) https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:28: std::vector<std::string> required_actions) this should be const reference https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:45: const std::vector<std::string> get_required_actions() const { should this return reference? https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:33: const std::string get_country_name_code() const { return country_name_code_; } should const getters return const references? https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1367: const char kWalletServiceUrl[] = "wallet-service-url"; I would suggest to use a different word other than "secure", as people leaning to think about "https:" when they saw "secure" and "url" in the constant name. On 2012/12/15 02:22:22, ahutter wrote: > On 2012/12/15 01:40:12, Ilya Sherman wrote: > > Why do we have a non-secure URL (that we're willing to use) for the service? > > These flags should probably be documented better to explain what they mean. > > Done.
Adding Sky for chrome/chrome_browser.gypi changes. https://codereview.chromium.org/11293078/diff/22004/base/string_number_conver... File base/string_number_conversions.cc (right): https://codereview.chromium.org/11293078/diff/22004/base/string_number_conver... base/string_number_conversions.cc:306: class BaseHexIteratorRangeToInt64Traits On 2012/12/15 02:48:59, benquan wrote: > Tests? Covered by tests for HexStringToInt64. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.cc (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:16: size_t kCvnSize = 3; On 2012/12/15 02:48:59, benquan wrote: > Are these constants always true? > Some credit card has <16 digits (Diners Club has 14 digits). A credit card can > have upto 19 digits. > Bin is not always 6. > Cvn is not always 3 (AMEX has 4) These are specific to our proxy cards. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.cc:28: std::vector<std::string> required_actions) On 2012/12/15 02:48:59, benquan wrote: > this should be const reference It is in the most recent CL. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/full_wallet.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/full_wallet.h:45: const std::vector<std::string> get_required_actions() const { On 2012/12/15 02:48:59, benquan wrote: > should this return reference? It does in the most recent CL. https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... File chrome/browser/autofill/wallet/wallet_address.h (right): https://codereview.chromium.org/11293078/diff/22004/chrome/browser/autofill/w... chrome/browser/autofill/wallet/wallet_address.h:33: const std::string get_country_name_code() const { return country_name_code_; } On 2012/12/15 02:48:59, benquan wrote: > should const getters return const references? They do in the most recent CL. https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11293078/diff/70001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:1367: const char kWalletServiceUrl[] = "wallet-service-url"; On 2012/12/15 02:48:59, benquan wrote: > I would suggest to use a different word other than "secure", as people leaning > to think about "https:" when they saw "secure" and "url" in the constant name. Added some comments. > > On 2012/12/15 02:22:22, ahutter wrote: > > On 2012/12/15 01:40:12, Ilya Sherman wrote: > > > Why do we have a non-secure URL (that we're willing to use) for the service? > > > > These flags should probably be documented better to explain what they mean. > > > > Done. >
*gypi LGTM
Ping. Need OWNERs approval for changes in base/.
Adding willchan@ for OWNERS approval of changes in base/.
I'll be back later to take a closer look, but please make sure there are the appropriate overflow tests in both the existing int and the new int64 string conversion test cases. It wasn't obvious to me if there was, so maybe a simple comment for the appropriate test cases would be good enough.
I fixed the [chromium-style] issue and the CL is now building locally using clang.
lgtm once my nits are addressed https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... File base/string_number_conversions_unittest.cc (right): https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... base/string_number_conversions_unittest.cc:234: {"100000000", -1, false}, // Testing that overflow fails. How about underflow? https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... base/string_number_conversions_unittest.cc:291: {"10000000000000000", -1, false}, // Testing that overflow fails. Underflow too? https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... base/string_number_conversions_unittest.cc:306: std::string input_string(input, arraysize(input) - 1); Why is this needed? HexStringToInt64 takes a StringPiece, right?
On 2012/12/18 19:08:41, willchan wrote: > lgtm once my nits are addressed > > It looks like the HexStringToInt doesn't behave as expected in under/overflow situations. Can I add a TODO in the interest of getting this CL in? https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... > File base/string_number_conversions_unittest.cc (right): > > https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... > base/string_number_conversions_unittest.cc:234: {"100000000", -1, false}, // > Testing that overflow fails. > How about underflow? > > https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... > base/string_number_conversions_unittest.cc:291: {"10000000000000000", -1, > false}, // Testing that overflow fails. > Underflow too? > > https://codereview.chromium.org/11293078/diff/87001/base/string_number_conver... > base/string_number_conversions_unittest.cc:306: std::string input_string(input, > arraysize(input) - 1); > Why is this needed? HexStringToInt64 takes a StringPiece, right? Test fails with this conversion.
Fixing hex string sounds like it would take an hour or so. If you don't fix it now, I'm not sure if anybody will ever fix it, so I'd encourage you to do it!
Fixed Will's nits.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahutter@chromium.org/11293078/92001
Message was sent while issue was closed.
Change committed as 174078 |
