Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11867025:
Download autocheckout whitelist and enable autocheckout for whitelisted sites only. (Closed)
|
Created:
7 years, 11 months ago by benquan Modified:
7 years, 10 months ago CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDownload autocheckout whitelist and enable autocheckout for whitelisted sites only.
BUG=170945
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181800
Patch Set 1 #
Total comments: 38
Patch Set 2 : User timer to schedule downloads. #Patch Set 3 : fix unittestwq #
Total comments: 2
Patch Set 4 : revert code change for manual testing/:wq. #
Total comments: 88
Patch Set 5 : Address comments. #Patch Set 6 : update FormStructure constructor and stop friending unittest class from WhitelistManager. #Patch Set 7 : fix memory leak in AutofillMetricsTest. #
Total comments: 42
Patch Set 8 : Fix Ilya's review comments, mostly styling. #
Total comments: 2
Patch Set 9 : change WhitelistManager back to inherit from base::SupportsUserData::Data #Patch Set 10 : Pass autocheckout url prefix to FormStructure's constructor. #
Total comments: 32
Patch Set 11 : Fix lints. #Patch Set 12 : gclient runhooks #Patch Set 13 : last patch #Patch Set 14 : Fix a build error. #Patch Set 15 : Patch the fix that was not picked up in the last git commit. #Patch Set 16 : sync to head #Messages
Total messages: 37 (0 generated)
PTAL
https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:23: const int kWhitelistDownloadBackoffDelay = 86400; Can you add a readable time here? Like 1 day or something like that. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:25: const char kWhiteListKeyName[] = "autocheckout_whitelist_manager"; new line. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:60: // The feature is not enabled: do not do the request. did you mean ";" instead of ":" https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:65: // We are in back-off mode: do not do the request. ditto. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:106: if (url.spec().compare(0, it->size(), *it) == 0) { no curlies https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:114: // TODO(benquan) find a better way to parse csv data. missing ":" https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.h:11: #include "base/gtest_prod_util.h" Where are you using this? https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.h:30: // WhitelistManager is responsible for download and caching autocheckout nit: Autocheckout https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.h:33: public base::SupportsUserData::Data { Comments in SupportsUserData say you need a virtual destructor. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:46: bool DownloadWhitelist(int response_code, const char* response) { Why not a const std::string reference? https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:54: DCHECK(fetcher); you should probably do an ASSERT_TRUE since you'll get a serious error if this isn't here. Also, no DCHECK's in tests only EXPECT_* or ASSERT_*. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:67: const std::vector<std::string> & GetUrlPrefixes() { No space before & https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:82: ASSERT_TRUE(DownloadWhitelist(net::HTTP_OK, kDownloadWhitelistResponse)); EXPECT is preferred unless ASSERT is necessary. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_url.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_url.cc:18: } // anonymous namespace just "namespace" https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:844: bool AutofillManager::IsAutocheckoutEnabled() const{ missing space https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:845: if (!web_contents()) return false; newline https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:849: return wm->IsAutocheckoutEnabled(web_contents()->GetURL()); does web_contents() always refer to url in the nav bar? https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/form_... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/form_... chrome/browser/autofill/form_structure.h:46: FormStructure(const FormData& form, bool autocheckout_enabled = false); Can't use default arguments like this according to the style guide. https://codereview.chromium.org/11867025/diff/1/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/common/chrome_switches... chrome/common/chrome_switches.cc:125: // Flag used to tell Chrome where to download autocheckout whitelist. Maybe more like "Flag to specify the URL Chrome should download the Autocheckout whitelist from.
I changed to use timer to schedule downloads. PTAL It works fine when run chrome directly, but I have problem with running timer in unittests, looking into the issue now. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:23: const int kWhitelistDownloadBackoffDelay = 86400; On 2013/01/19 02:07:16, ahutter wrote: > Can you add a readable time here? Like 1 day or something like that. added comment here. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:25: const char kWhiteListKeyName[] = "autocheckout_whitelist_manager"; On 2013/01/19 02:07:16, ahutter wrote: > new line. Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:60: // The feature is not enabled: do not do the request. On 2013/01/19 02:07:16, ahutter wrote: > did you mean ";" instead of ":" Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:65: // We are in back-off mode: do not do the request. On 2013/01/19 02:07:16, ahutter wrote: > ditto. Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:106: if (url.spec().compare(0, it->size(), *it) == 0) { On 2013/01/19 02:07:16, ahutter wrote: > no curlies Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.cc:114: // TODO(benquan) find a better way to parse csv data. On 2013/01/19 02:07:16, ahutter wrote: > missing ":" Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.h:11: #include "base/gtest_prod_util.h" On 2013/01/19 02:07:16, ahutter wrote: > Where are you using this? Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.h:30: // WhitelistManager is responsible for download and caching autocheckout On 2013/01/19 02:07:16, ahutter wrote: > nit: Autocheckout Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager.h:33: public base::SupportsUserData::Data { On 2013/01/19 02:07:16, ahutter wrote: > Comments in SupportsUserData say you need a virtual destructor. That comment seems not necessary, because virtual-ness propagates to derived classes automatically. base::SupportsUserData::Data already has a virtual dtor, so the any derived class's dtor is virtual automatically, even if you do not specifically defined one. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:46: bool DownloadWhitelist(int response_code, const char* response) { On 2013/01/19 02:07:16, ahutter wrote: > Why not a const std::string reference? Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:54: DCHECK(fetcher); ah, fixed! BTW, this was copied from https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/autofill/w... On 2013/01/19 02:07:16, ahutter wrote: > you should probably do an ASSERT_TRUE since you'll get a serious error if this > isn't here. Also, no DCHECK's in tests only EXPECT_* or ASSERT_*. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:67: const std::vector<std::string> & GetUrlPrefixes() { On 2013/01/19 02:07:16, ahutter wrote: > No space before & Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:82: ASSERT_TRUE(DownloadWhitelist(net::HTTP_OK, kDownloadWhitelistResponse)); On 2013/01/19 02:07:16, ahutter wrote: > EXPECT is preferred unless ASSERT is necessary. Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... File chrome/browser/autofill/autocheckout/whitelist_url.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autoc... chrome/browser/autofill/autocheckout/whitelist_url.cc:18: } // anonymous namespace On 2013/01/19 02:07:16, ahutter wrote: > just "namespace" Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:844: bool AutofillManager::IsAutocheckoutEnabled() const{ On 2013/01/19 02:07:16, ahutter wrote: > missing space Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:845: if (!web_contents()) return false; On 2013/01/19 02:07:16, ahutter wrote: > newline Done. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/autof... chrome/browser/autofill/autofill_manager.cc:849: return wm->IsAutocheckoutEnabled(web_contents()->GetURL()); On 2013/01/19 02:07:16, ahutter wrote: > does web_contents() always refer to url in the nav bar? it refers to the tab the AutofillManager associated with, not the url in the nav. https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/form_... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/1/chrome/browser/autofill/form_... chrome/browser/autofill/form_structure.h:46: FormStructure(const FormData& form, bool autocheckout_enabled = false); On 2013/01/19 02:07:16, ahutter wrote: > Can't use default arguments like this according to the style guide. Done. https://codereview.chromium.org/11867025/diff/1/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11867025/diff/1/chrome/common/chrome_switches... chrome/common/chrome_switches.cc:125: // Flag used to tell Chrome where to download autocheckout whitelist. On 2013/01/19 02:07:16, ahutter wrote: > Maybe more like "Flag to specify the URL Chrome should download the Autocheckout > whitelist from. Done.
+Ilya
I'm assuming this isn't yet ready for a full review from me -- let me know if that's not so -- but one thing caught my eye: https://codereview.chromium.org/11867025/diff/13001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11867025/diff/13001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:84: const size_t kMaxRecentFormSignaturesToRemember = 0; Why did you change this variable?
Hi Ilya, this CL is ready for you to review. https://codereview.chromium.org/11867025/diff/13001/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11867025/diff/13001/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:84: const size_t kMaxRecentFormSignaturesToRemember = 0; On 2013/01/24 06:36:48, Ilya Sherman wrote: > Why did you change this variable? ah, this change was for testing, reverted.
https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:11: #include "base/supports_user_data.h" nit: Remove this; it's already present in the header. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:27: const int kInitialDownloadDelaySeconds = 3; nit: I think you want a slightly larger delay for this. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:39: DCHECK(context); nit: Omit this. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:40: WhitelistManager* wm = static_cast<WhitelistManager*>( nit: Please avoid abbreviations in variable names. Instead, use a name like manager_ or whitelist_manager_ https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:60: bool WhitelistManager::ScheduleDownload(int interval_seconds) { nit: The return value from this method doesn't seem to ever be used, so I'd recommend changing this to a void function. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:67: DVLOG(1) << "Autocheckout DownloadWhitelist scheduler is already running."; Will this DVLOG and others in this file be useful after this CL lands? (It's fine if they are; they just strike me as being more like debugging code than long-term logging code.) https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:88: return; nit: Omit this. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:102: return; So, if the download ever fails, you'll never schedule a new one? Are you sure that's the behavior you want? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:114: // The feature is not enabled, return false. nit: This comment is redundant with the code; please remove it. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:126: if (url.spec().compare(0, it->size(), *it) == 0) nit: Use StartsWith() from base/string_util.h for this check. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:136: std::stringstream dataStream(data); nit: Use base::SplitString rather than using a stringstream. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:143: // prefix. What are the remaining columns? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:27: namespace autocheckout { nit: Please also wrap this in the autofill namespace, so that the full namespace is autofill::autocheckout. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::SupportsUserData::Data { nit: I don't think you need to inherit from SupportsUserData. Instead, use base::UserDataAdapter. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:34: nit: Omit this newline https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:43: bool ScheduleDownload(int interval_seconds); Why is this public? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:43: bool ScheduleDownload(int interval_seconds); nit: Use size_t rather than int, since you never want negative values. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:54: // Parse whitelist data and build whitelist nit: End comment with a period. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:54: // Parse whitelist data and build whitelist Please describe (at a high level) what format the data has. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:58: // The pointer value is const, so this can only be set in the constructor. nit: Omit this line; it's redundant with the code. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:63: bool callback_pending_; nit: Perhaps name this has_callback_pending_ or callback_is_pending_ or something like that? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:76: friend class WhitelistManagerTest; Rather than friending the test, which allows the test to make all sorts of calls that break invariants, create a TestWhitelistManager that inherits from WhitelistManager, and provide a minimal set of protected methods that the test manager can override. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:19: nit: Please omit the second newline. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:50: } Why not just do this in the constructor? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:60: ASSERT_TRUE(fetcher != NULL); nit: You can drop the "!= NULL" https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:71: const std::vector<std::string>& GetUrlPrefixes() { nit: const https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:89: EXPECT_EQ((size_t)2, GetUrlPrefixes().size()); nit: Use C++-style casts rather than C-style casts, i.e. static_cast<size_t>(2). But in this specific case, you can just write 2U (U stands for "unsigned") instead of 2. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:89: EXPECT_EQ((size_t)2, GetUrlPrefixes().size()); nit: ASSERT_EQ, since the test code below this will crash if there aren't enough elements. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:138: EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(GURL(""))); nit: Prefer std::string() to "" https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:165: GURL("a random string"))); Doesn't calling spec() on an invalid URL cause an assertion failure? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:172: } Please add test cases for http://, and for no specified protocol. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_url.h (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_url.h:10: namespace autocheckout { nit: Please also wrap this in the autofill namespace, so that the full namespace is autofill::autocheckout. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/autofill_manager.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autofill_manager.cc:899: autocheckout::WhitelistManager* wm = nit: Please avoid using abbreviations for variable names. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autofill_manager.cc:907: IsAutocheckoutEnabled(); Hmm, I don't think we should override the Autofill enabled setting; at least, not without more data for the initial launch. Most users don't disable Autofill, so this doesn't buy us much, but it might anger some users who've intentionally disabled Autofill. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/autofill_manager.h (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/autofill_manager.h:129: bool IsAutocheckoutEnabled() const; nit: Can this have private rather than protected visibility? https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/form_structure.cc (left): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/form_structure.cc:421: nit: Please revert this diff. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... File chrome/browser/autofill/form_structure.h (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/browser/aut... chrome/browser/autofill/form_structure.h:47: FormStructure(const FormData& form, bool autocheckout_enabled); Please replace the previous constructor, rather than adding a new one. https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/common/chro... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/14015/chrome/common/chro... chrome/common/chrome_switches.cc:123: const char kAutocheckoutWhitelistUrl[] = "autocheckout-whitelist-url"; Is this actually something that's useful to override for testing? It doesn't seem like a very complex server.
PTAL https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:11: #include "base/supports_user_data.h" On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Remove this; it's already present in the header. removed it from the header and kept this line. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:27: const int kInitialDownloadDelaySeconds = 3; On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: I think you want a slightly larger delay for this. Download will be triggered only when we create WhitelistManager instance in GetForBrowserContext() which is called from AutofillManager, I think we already have some delay there already. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:39: DCHECK(context); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Omit this. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:40: WhitelistManager* wm = static_cast<WhitelistManager*>( On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Please avoid abbreviations in variable names. Instead, use a name like > manager_ or whitelist_manager_ Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:60: bool WhitelistManager::ScheduleDownload(int interval_seconds) { On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: The return value from this method doesn't seem to ever be used, so I'd > recommend changing this to a void function. The unittest uses it to check if a new download was scheduled by this function. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:67: DVLOG(1) << "Autocheckout DownloadWhitelist scheduler is already running."; On 2013/01/24 22:01:55, Ilya Sherman wrote: > Will this DVLOG and others in this file be useful after this CL lands? (It's > fine if they are; they just strike me as being more like debugging code than > long-term logging code.) removed https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:88: return; On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Omit this. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:102: return; fixed On 2013/01/24 22:01:55, Ilya Sherman wrote: > So, if the download ever fails, you'll never schedule a new one? Are you sure > that's the behavior you want? https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:114: // The feature is not enabled, return false. On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: This comment is redundant with the code; please remove it. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:126: if (url.spec().compare(0, it->size(), *it) == 0) On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Use StartsWith() from base/string_util.h for this check. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:136: std::stringstream dataStream(data); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Use base::SplitString rather than using a stringstream. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:143: // prefix. On 2013/01/24 22:01:55, Ilya Sherman wrote: > What are the remaining columns? We only have one column right now. It's for backward compatibility, if we decide to add additional metadata as more columns, the old version of chrome may not be impacted. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:27: namespace autocheckout { On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Please also wrap this in the autofill namespace, so that the full namespace > is autofill::autocheckout. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::SupportsUserData::Data { On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: I don't think you need to inherit from SupportsUserData. Instead, use > base::UserDataAdapter. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:34: On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Omit this newline Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:43: bool ScheduleDownload(int interval_seconds); On 2013/01/24 22:01:55, Ilya Sherman wrote: > Why is this public? Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:43: bool ScheduleDownload(int interval_seconds); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Use size_t rather than int, since you never want negative values. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:54: // Parse whitelist data and build whitelist On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: End comment with a period. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:54: // Parse whitelist data and build whitelist On 2013/01/24 22:01:55, Ilya Sherman wrote: > Please describe (at a high level) what format the data has. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:58: // The pointer value is const, so this can only be set in the constructor. On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Omit this line; it's redundant with the code. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:63: bool callback_pending_; On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Perhaps name this has_callback_pending_ or callback_is_pending_ or > something like that? Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:76: friend class WhitelistManagerTest; On 2013/01/24 22:01:55, Ilya Sherman wrote: > Rather than friending the test, which allows the test to make all sorts of calls > that break invariants, create a TestWhitelistManager that inherits from > WhitelistManager, and provide a minimal set of protected methods that the test > manager can override. TestWhitelistManager still can not access WhitelistManager's private members, should I friend TestWhitelistManager or make the members protected rather than private? https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:19: On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Please omit the second newline. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:50: } On 2013/01/24 22:01:55, Ilya Sherman wrote: > Why not just do this in the constructor? WhitelistManager's constructor checks chrome_switches, some of the testes in this file needs to override switches before make an instance of WhitelistManager. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:60: ASSERT_TRUE(fetcher != NULL); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: You can drop the "!= NULL" Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:71: const std::vector<std::string>& GetUrlPrefixes() { On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: const Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:89: EXPECT_EQ((size_t)2, GetUrlPrefixes().size()); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Use C++-style casts rather than C-style casts, i.e. static_cast<size_t>(2). > But in this specific case, you can just write 2U (U stands for "unsigned") > instead of 2. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:89: EXPECT_EQ((size_t)2, GetUrlPrefixes().size()); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: ASSERT_EQ, since the test code below this will crash if there aren't enough > elements. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:138: EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(GURL(""))); On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Prefer std::string() to "" Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:165: GURL("a random string"))); No, it doesn't, spec() returns an empty string. On 2013/01/24 22:01:55, Ilya Sherman wrote: > Doesn't calling spec() on an invalid URL cause an assertion failure? https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:172: } On 2013/01/24 22:01:55, Ilya Sherman wrote: > Please add test cases for http://, and for no specified protocol. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_url.h (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_url.h:10: namespace autocheckout { On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Please also wrap this in the autofill namespace, so that the full namespace > is autofill::autocheckout. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autofill_manager.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:899: autocheckout::WhitelistManager* wm = On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Please avoid using abbreviations for variable names. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autofill_manager.cc:907: IsAutocheckoutEnabled(); Ok, we can revisit this later and enabled it when needed. On 2013/01/24 22:01:55, Ilya Sherman wrote: > Hmm, I don't think we should override the Autofill enabled setting; at least, > not without more data for the initial launch. Most users don't disable > Autofill, so this doesn't buy us much, but it might anger some users who've > intentionally disabled Autofill. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.cc (left): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.cc:421: On 2013/01/24 22:01:55, Ilya Sherman wrote: > nit: Please revert this diff. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:47: FormStructure(const FormData& form, bool autocheckout_enabled); I was told we do not want default value for parameters. Do you think it is ok? On 2013/01/24 22:01:55, Ilya Sherman wrote: > Please replace the previous constructor, rather than adding a new one. https://codereview.chromium.org/11867025/diff/14015/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:123: const char kAutocheckoutWhitelistUrl[] = "autocheckout-whitelist-url"; No, it is not. Do you mean define a constant rather than a switch? On 2013/01/24 22:01:55, Ilya Sherman wrote: > Is this actually something that's useful to override for testing? It doesn't > seem like a very complex server.
https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:27: const int kInitialDownloadDelaySeconds = 3; On 2013/01/25 00:55:31, benquan wrote: > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > nit: I think you want a slightly larger delay for this. > > Download will be triggered only when we create WhitelistManager instance in > GetForBrowserContext() which is called from AutofillManager, I think we already > have some delay there already. Ok. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:60: bool WhitelistManager::ScheduleDownload(int interval_seconds) { On 2013/01/25 00:55:31, benquan wrote: > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > nit: The return value from this method doesn't seem to ever be used, so I'd > > recommend changing this to a void function. > > The unittest uses it to check if a new download was scheduled by this function. That means that the unittest is not testing the production code. If you were to reverse what the code actually does, but keep the return values the same, the unit test should fail; but if it's relying on the returned values to be meaningful, it might pass instead. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:143: // prefix. On 2013/01/25 00:55:31, benquan wrote: > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > What are the remaining columns? > > We only have one column right now. It's for backward compatibility, if we decide > to add additional metadata as more columns, the old version of chrome may not be > impacted. > Please mention that in the comment. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:76: friend class WhitelistManagerTest; On 2013/01/25 00:55:31, benquan wrote: > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > Rather than friending the test, which allows the test to make all sorts of > calls > > that break invariants, create a TestWhitelistManager that inherits from > > WhitelistManager, and provide a minimal set of protected methods that the test > > manager can override. > > TestWhitelistManager still can not access WhitelistManager's private members, > should I friend TestWhitelistManager or make the members protected rather than > private? The members should remain private, but you can add protected accessors to them. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:50: } On 2013/01/25 00:55:31, benquan wrote: > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > Why not just do this in the constructor? > > WhitelistManager's constructor checks chrome_switches, some of the testes in > this file needs to override switches before make an instance of > WhitelistManager. Hmm, fair enough. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:165: GURL("a random string"))); In Debug mode, it will trigger a DCHECK: https://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exa... What am I missing? On 2013/01/25 00:55:31, benquan wrote: > No, it doesn't, spec() returns an empty string. > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > Doesn't calling spec() on an invalid URL cause an assertion failure? https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:47: FormStructure(const FormData& form, bool autocheckout_enabled); Yes, that's correct: parameters should not have default values. By overloading the constructor, you're essentially providing a default value. Hence, you shouldn't overload the constructor. On 2013/01/25 00:55:31, benquan wrote: > I was told we do not want default value for parameters. Do you think it is ok? > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > Please replace the previous constructor, rather than adding a new one. > https://codereview.chromium.org/11867025/diff/14015/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:123: const char kAutocheckoutWhitelistUrl[] = "autocheckout-whitelist-url"; Yes, my question is whether you really need this switch. If not, it's better to omit it and just use a fixed constant. On 2013/01/25 00:55:31, benquan wrote: > No, it is not. Do you mean define a constant rather than a switch? > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > Is this actually something that's useful to override for testing? It doesn't > > seem like a very complex server. >
https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:60: bool WhitelistManager::ScheduleDownload(int interval_seconds) { On 2013/01/25 01:22:40, Ilya Sherman wrote: > On 2013/01/25 00:55:31, benquan wrote: > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > > nit: The return value from this method doesn't seem to ever be used, so I'd > > > recommend changing this to a void function. > > > > The unittest uses it to check if a new download was scheduled by this > function. > > That means that the unittest is not testing the production code. If you were to > reverse what the code actually does, but keep the return values the same, the > unit test should fail; but if it's relying on the returned values to be > meaningful, it might pass instead. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:143: // prefix. On 2013/01/25 01:22:40, Ilya Sherman wrote: > On 2013/01/25 00:55:31, benquan wrote: > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > > What are the remaining columns? > > > > We only have one column right now. It's for backward compatibility, if we > decide > > to add additional metadata as more columns, the old version of chrome may not > be > > impacted. > > > > Please mention that in the comment. Done. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:76: friend class WhitelistManagerTest; Some functions needed by tests are also lifted to protected section. On 2013/01/25 01:22:40, Ilya Sherman wrote: > On 2013/01/25 00:55:31, benquan wrote: > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > > Rather than friending the test, which allows the test to make all sorts of > > calls > > > that break invariants, create a TestWhitelistManager that inherits from > > > WhitelistManager, and provide a minimal set of protected methods that the > test > > > manager can override. > > > > TestWhitelistManager still can not access WhitelistManager's private members, > > should I friend TestWhitelistManager or make the members protected rather than > > private? > > The members should remain private, but you can add protected accessors to them. https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:165: GURL("a random string"))); On 2013/01/25 01:22:40, Ilya Sherman wrote: > In Debug mode, it will trigger a DCHECK: > https://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exa... > > What am I missing? Oh, in my WhitelistManager::IsAutocheckoutEnabled(), I check url.is_empty() first, it returns true for invalid urls, and it will not call spec(). > > On 2013/01/25 00:55:31, benquan wrote: > > No, it doesn't, spec() returns an empty string. > > > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > > Doesn't calling spec() on an invalid URL cause an assertion failure? > https://codereview.chromium.org/11867025/diff/14015/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11867025/diff/14015/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:123: const char kAutocheckoutWhitelistUrl[] = "autocheckout-whitelist-url"; On 2013/01/25 01:22:40, Ilya Sherman wrote: > Yes, my question is whether you really need this switch. If not, it's better to > omit it and just use a fixed constant. > > On 2013/01/25 00:55:31, benquan wrote: > > No, it is not. Do you mean define a constant rather than a switch? > > > > On 2013/01/24 22:01:55, Ilya Sherman wrote: > > > Is this actually something that's useful to override for testing? It > doesn't > > > seem like a very complex server. > > > Done.
Benquan, please ping this thread once you update a revised patch set. Thanks.
https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:131: for (std::vector<std::string>::iterator line = lines.begin(); nit: Can this be a const_iterator? https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:138: // chrome can ignore them and continue to work. nit: "chrome" -> "Chrome" https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::RefCounted<WhitelistManager> { Why is this class reference counted? It almost certainly should not be. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:46: // Start the download timer. nit: Please mention that this is called by ScheduleDownload(), and exposed as a separate method for mocking out in tests. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:62: } nit: Please write this all on one line, as: bool callback_is_pending() const { return callback_is_pending_; } https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:62: } nit: Please move this method to be before set_callback_is_pending. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:64: base::OneShotTimer<WhitelistManager>& download_timer() { nit: Please return a const reference from this function. You can also expose a ResetDownloadTimer() function if needed. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:66: } nit: Include a blank line after this one. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:35: : WhitelistManager(context_getter), called_timer_start_(false) {} nit: Format this as: explicit TestWhitelistManager(net::URLRequestContextGetter* context_getter) : WhitelistManager(context_getter), called_timer_start_(false) {} https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:47: bool CalledTimerStart() const { nit: Name this method called_timer_start(), since it's just a simple accessor. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:49: } nit: Please leave a blank line after this one. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:51: bool called_timer_start_; Optional nit: Perhaps name this "did_start_download_timer_" or something along those lines? https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:53: friend class WhitelistManagerTest; nit: Don't use a friend declaration here. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_url.h (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_url.h:16: } // namespace autofill nit: It doesn't really seem worth defining a whole new header for just a single string constant + conversion to a GURL. I'd just inline all of this code into the anonymous namespace in whitelist_manager.cc https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/form_structure.h (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); nit: Rather than passing in a boolean, which is hard to understand semantically at the call-site, please define an enumerated type with two constants, AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not mentioning that earlier, prior to you adding the false param to all of the FormData constructor call sites.
PTAL https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:131: for (std::vector<std::string>::iterator line = lines.begin(); On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Can this be a const_iterator? Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:138: // chrome can ignore them and continue to work. On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: "chrome" -> "Chrome" Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::RefCounted<WhitelistManager> { On 2013/01/28 23:21:39, Ilya Sherman wrote: > Why is this class reference counted? It almost certainly should not be. UserDataAdapter requires it to be reference counted: // Adapter class that releases a refcounted object when the // SupportsUserData::Data object is deleted. template <typename T> class UserDataAdapter : public base::SupportsUserData::Data https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:46: // Start the download timer. On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Please mention that this is called by ScheduleDownload(), and exposed as a > separate method for mocking out in tests. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:62: } On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Please write this all on one line, as: > > bool callback_is_pending() const { return callback_is_pending_; } Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:62: } On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Please move this method to be before set_callback_is_pending. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:64: base::OneShotTimer<WhitelistManager>& download_timer() { On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Please return a const reference from this function. You can also expose a > ResetDownloadTimer() function if needed. Added StopDownloadTimer(), and this function is no longer needed. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:66: } On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Include a blank line after this one. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:35: : WhitelistManager(context_getter), called_timer_start_(false) {} On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Format this as: > > explicit TestWhitelistManager(net::URLRequestContextGetter* context_getter) > : WhitelistManager(context_getter), > called_timer_start_(false) {} Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:47: bool CalledTimerStart() const { On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Name this method called_timer_start(), since it's just a simple accessor. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:49: } On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:51: bool called_timer_start_; On 2013/01/28 23:21:39, Ilya Sherman wrote: > Optional nit: Perhaps name this "did_start_download_timer_" or something along > those lines? Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:53: friend class WhitelistManagerTest; On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Don't use a friend declaration here. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_url.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_url.h:16: } // namespace autofill On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: It doesn't really seem worth defining a whole new header for just a single > string constant + conversion to a GURL. I'd just inline all of this code into > the anonymous namespace in whitelist_manager.cc Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/28 23:21:39, Ilya Sherman wrote: > nit: Rather than passing in a boolean, which is hard to understand semantically > at the call-site, please define an enumerated type with two constants, > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not mentioning > that earlier, prior to you adding the false param to all of the FormData > constructor call sites. It will not be consistent with other methods in this file which take bool as parameters.
https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::RefCounted<WhitelistManager> { On 2013/01/29 03:28:30, benquan wrote: > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > Why is this class reference counted? It almost certainly should not be. > > UserDataAdapter requires it to be reference counted: > > // Adapter class that releases a refcounted object when the > // SupportsUserData::Data object is deleted. > template <typename T> > class UserDataAdapter : public base::SupportsUserData::Data Ah, I didn't realize that. In that case, let's go back to having this class inherit from base::SupportsUserData::Data. Sorry for the run-around. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/29 03:28:30, benquan wrote: > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > nit: Rather than passing in a boolean, which is hard to understand > semantically > > at the call-site, please define an enumerated type with two constants, > > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not > mentioning > > that earlier, prior to you adding the false param to all of the FormData > > constructor call sites. > > It will not be consistent with other methods in this file which take bool as > parameters. That's ok. It's worthwhile to move code in that direction, and definitely to implement new code this way, for the readability benefits,
https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::RefCounted<WhitelistManager> { On 2013/01/29 05:56:35, Ilya Sherman wrote: > On 2013/01/29 03:28:30, benquan wrote: > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > Why is this class reference counted? It almost certainly should not be. > > > > UserDataAdapter requires it to be reference counted: > > > > // Adapter class that releases a refcounted object when the > > // SupportsUserData::Data object is deleted. > > template <typename T> > > class UserDataAdapter : public base::SupportsUserData::Data > > Ah, I didn't realize that. In that case, let's go back to having this class > inherit from base::SupportsUserData::Data. Or, actually, it looks like inheriting Sorry for the run-around. Or possibly inherit from content::WebContentsUserData, as e.g. the AutofillExternalDelegate class does. https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_url.h (right): https://chromiumcodereview.appspot.com/11867025/diff/29001/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_url.h:16: } // namespace autofill On 2013/01/29 03:28:30, benquan wrote: > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > nit: It doesn't really seem worth defining a whole new header for just a > single > > string constant + conversion to a GURL. I'd just inline all of this code into > > the anonymous namespace in whitelist_manager.cc > > Done. This doesn't look to be done in the latest patch set. https://chromiumcodereview.appspot.com/11867025/diff/36002/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/36002/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:64: } nit: Place this in the same relative order in the implementation file as in the header.
https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::RefCounted<WhitelistManager> { On 2013/01/29 06:15:09, Ilya Sherman wrote: > On 2013/01/29 05:56:35, Ilya Sherman wrote: > > On 2013/01/29 03:28:30, benquan wrote: > > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > > Why is this class reference counted? It almost certainly should not be. > > > > > > UserDataAdapter requires it to be reference counted: > > > > > > // Adapter class that releases a refcounted object when the > > > // SupportsUserData::Data object is deleted. > > > template <typename T> > > > class UserDataAdapter : public base::SupportsUserData::Data > > > > Ah, I didn't realize that. In that case, let's go back to having this class > > inherit from base::SupportsUserData::Data. Or, actually, it looks like > inheriting Sorry for the run-around. > > Or possibly inherit from content::WebContentsUserData, as e.g. the > AutofillExternalDelegate class does. I want one WhitelistManager instance per WebContext, not per WebContent, so it won't work for this. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:32: public base::RefCounted<WhitelistManager> { On 2013/01/29 05:56:35, Ilya Sherman wrote: > On 2013/01/29 03:28:30, benquan wrote: > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > Why is this class reference counted? It almost certainly should not be. > > > > UserDataAdapter requires it to be reference counted: > > > > // Adapter class that releases a refcounted object when the > > // SupportsUserData::Data object is deleted. > > template <typename T> > > class UserDataAdapter : public base::SupportsUserData::Data > > Ah, I didn't realize that. In that case, let's go back to having this class > inherit from base::SupportsUserData::Data. Sorry for the run-around. Done. https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_url.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_url.h:16: } // namespace autofill On 2013/01/29 06:15:09, Ilya Sherman wrote: > On 2013/01/29 03:28:30, benquan wrote: > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > nit: It doesn't really seem worth defining a whole new header for just a > > single > > > string constant + conversion to a GURL. I'd just inline all of this code > into > > > the anonymous namespace in whitelist_manager.cc > > > > Done. > > This doesn't look to be done in the latest patch set. sorry, I missed this. Done https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/29 05:56:35, Ilya Sherman wrote: > On 2013/01/29 03:28:30, benquan wrote: > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > nit: Rather than passing in a boolean, which is hard to understand > > semantically > > > at the call-site, please define an enumerated type with two constants, > > > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not > > mentioning > > > that earlier, prior to you adding the false param to all of the FormData > > > constructor call sites. > > > > It will not be consistent with other methods in this file which take bool as > > parameters. > > That's ok. It's worthwhile to move code in that direction, and definitely to > implement new code this way, for the readability benefits, Is it a new coding style guide requires to replace bool with specific enums or something? Personally I don't think this won't gain much on readability. There is also a class memeber named autocheckout_enabled_, how could we name it properly? Also, we can not check the field like this: if (autocheckout_enabled_) { // do something } instead we have to use this form: if (autocheckout_enabled_ == AUTOCHECKOUT_ENABLED) { // do someting } Overall I think a simple bool fit this usecase naturally and much easier. https://codereview.chromium.org/11867025/diff/36002/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/36002/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:64: } On 2013/01/29 06:15:09, Ilya Sherman wrote: > nit: Place this in the same relative order in the implementation file as in the > header. Done.
https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/30 00:48:42, benquan wrote: > On 2013/01/29 05:56:35, Ilya Sherman wrote: > > On 2013/01/29 03:28:30, benquan wrote: > > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > > nit: Rather than passing in a boolean, which is hard to understand > > > semantically > > > > at the call-site, please define an enumerated type with two constants, > > > > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not > > > mentioning > > > > that earlier, prior to you adding the false param to all of the FormData > > > > constructor call sites. > > > > > > It will not be consistent with other methods in this file which take bool as > > > parameters. > > > > That's ok. It's worthwhile to move code in that direction, and definitely to > > implement new code this way, for the readability benefits, > > Is it a new coding style guide requires to replace bool with specific enums or > something? Specifically, booleans as parameters to functions, where the meaning of the boolean is not clear at the callsite. > Personally I don't think this won't gain much on readability. There is also a > class memeber named autocheckout_enabled_, how could we name it properly? Also, > we can not check the field like this: > > if (autocheckout_enabled_) { > // do something > } > > instead we have to use this form: > if (autocheckout_enabled_ == AUTOCHECKOUT_ENABLED) { > // do someting > } > > Overall I think a simple bool fit this usecase naturally and much easier. You can keep the boolean member variable, and initialize it as """autocheckout_enabled_(autocheckout_enabled == AUTOCHECKOUT_ENABLED)""" in the initializer list.
https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/30 00:52:48, Ilya Sherman wrote: > On 2013/01/30 00:48:42, benquan wrote: > > On 2013/01/29 05:56:35, Ilya Sherman wrote: > > > On 2013/01/29 03:28:30, benquan wrote: > > > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > > > nit: Rather than passing in a boolean, which is hard to understand > > > > semantically > > > > > at the call-site, please define an enumerated type with two constants, > > > > > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not > > > > mentioning > > > > > that earlier, prior to you adding the false param to all of the FormData > > > > > constructor call sites. > > > > > > > > It will not be consistent with other methods in this file which take bool > as > > > > parameters. > > > > > > That's ok. It's worthwhile to move code in that direction, and definitely > to > > > implement new code this way, for the readability benefits, > > > > Is it a new coding style guide requires to replace bool with specific enums or > > something? > > Specifically, booleans as parameters to functions, where the meaning of the > boolean is not clear at the callsite. Bascially we have the same issue with all constants not just for booleans. For example, foo(2), the meaning of number 1 is not clear at the caller site either. > > > Personally I don't think this won't gain much on readability. There is also a > > class memeber named autocheckout_enabled_, how could we name it properly? > Also, > > we can not check the field like this: > > > > if (autocheckout_enabled_) { > > // do something > > } > > > > instead we have to use this form: > > if (autocheckout_enabled_ == AUTOCHECKOUT_ENABLED) { > > // do someting > > } > > > > Overall I think a simple bool fit this usecase naturally and much easier. > > You can keep the boolean member variable, and initialize it as > """autocheckout_enabled_(autocheckout_enabled == AUTOCHECKOUT_ENABLED)""" in the > initializer list. I guess you have read item 40 in http://go/effectivejava, "Prefer two-element enum types to boolean parameters". It makes sense for a lot of things when you may need a third value. The example the author gave was: use TemperatureScale enum: public enum TemperatureScale { FAHRENHEIT, CELSIUS } rather than : bool is_fahrenheit; The reason is that you may want to include KELVIN as the third scale. But if you use it for a bi-state flag, you breaks the attitude of a boolean that is nothing more than representing a bi-state of true and false and converse with each other. What does !AUTOCHECKOUT_ENABLED mean? It should be equivalent to AUTOCHECKOUT_DISABLED, but it is illegal in syntax. I think it is not ideal to convert autocheckout_enabled_ to an enum, but if you insist that this is the direction we want to take, I can make the code change.
https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/30 18:39:00, benquan wrote: > On 2013/01/30 00:52:48, Ilya Sherman wrote: > > On 2013/01/30 00:48:42, benquan wrote: > > > On 2013/01/29 05:56:35, Ilya Sherman wrote: > > > > On 2013/01/29 03:28:30, benquan wrote: > > > > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > > > > nit: Rather than passing in a boolean, which is hard to understand > > > > > semantically > > > > > > at the call-site, please define an enumerated type with two constants, > > > > > > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for not > > > > > mentioning > > > > > > that earlier, prior to you adding the false param to all of the > FormData > > > > > > constructor call sites. > > > > > > > > > > It will not be consistent with other methods in this file which take > bool > > as > > > > > parameters. > > > > > > > > That's ok. It's worthwhile to move code in that direction, and definitely > > to > > > > implement new code this way, for the readability benefits, > > > > > > Is it a new coding style guide requires to replace bool with specific enums > or > > > something? > > > > Specifically, booleans as parameters to functions, where the meaning of the > > boolean is not clear at the callsite. > Bascially we have the same issue with all constants not just for booleans. For > example, foo(2), the meaning of number 1 is not clear at the caller site either. > > > > > Personally I don't think this won't gain much on readability. There is also > a > > > class memeber named autocheckout_enabled_, how could we name it properly? > > Also, > > > we can not check the field like this: > > > > > > if (autocheckout_enabled_) { > > > // do something > > > } > > > > > > instead we have to use this form: > > > if (autocheckout_enabled_ == AUTOCHECKOUT_ENABLED) { > > > // do someting > > > } > > > > > > Overall I think a simple bool fit this usecase naturally and much easier. > > > > You can keep the boolean member variable, and initialize it as > > """autocheckout_enabled_(autocheckout_enabled == AUTOCHECKOUT_ENABLED)""" in > the > > initializer list. > > > I guess you have read item 40 in http://go/effectivejava, "Prefer two-element > enum types to boolean parameters". It makes sense for a lot of things when you > may need a third value. The example the author gave was: use TemperatureScale > enum: > public enum TemperatureScale { FAHRENHEIT, CELSIUS } > rather than : > bool is_fahrenheit; > The reason is that you may want to include KELVIN as the third scale. > > But if you use it for a bi-state flag, you breaks the attitude of a boolean that > is nothing more than representing a bi-state of true and false and converse with > each other. What does !AUTOCHECKOUT_ENABLED mean? It should be equivalent to > AUTOCHECKOUT_DISABLED, but it is illegal in syntax. > > I think it is not ideal to convert autocheckout_enabled_ to an enum, but if you > insist that this is the direction we want to take, I can make the code change. I do insist that if you keep this as a parameter to the constructor, the parameter should be an enumerated constant rather than a boolean.
PTAL https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.h (right): https://codereview.chromium.org/11867025/diff/29001/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.h:48: FormStructure(const FormData& form, bool autocheckout_enabled); On 2013/01/30 21:05:31, Ilya Sherman wrote: > On 2013/01/30 18:39:00, benquan wrote: > > On 2013/01/30 00:52:48, Ilya Sherman wrote: > > > On 2013/01/30 00:48:42, benquan wrote: > > > > On 2013/01/29 05:56:35, Ilya Sherman wrote: > > > > > On 2013/01/29 03:28:30, benquan wrote: > > > > > > On 2013/01/28 23:21:39, Ilya Sherman wrote: > > > > > > > nit: Rather than passing in a boolean, which is hard to understand > > > > > > semantically > > > > > > > at the call-site, please define an enumerated type with two > constants, > > > > > > > AUTOCHECKOUT_ENABLED and AUTOCHECKOUT_DISABLED. My apologies for > not > > > > > > mentioning > > > > > > > that earlier, prior to you adding the false param to all of the > > FormData > > > > > > > constructor call sites. > > > > > > > > > > > > It will not be consistent with other methods in this file which take > > bool > > > as > > > > > > parameters. > > > > > > > > > > That's ok. It's worthwhile to move code in that direction, and > definitely > > > to > > > > > implement new code this way, for the readability benefits, > > > > > > > > Is it a new coding style guide requires to replace bool with specific > enums > > or > > > > something? > > > > > > Specifically, booleans as parameters to functions, where the meaning of the > > > boolean is not clear at the callsite. > > Bascially we have the same issue with all constants not just for booleans. For > > example, foo(2), the meaning of number 1 is not clear at the caller site > either. > > > > > > > Personally I don't think this won't gain much on readability. There is > also > > a > > > > class memeber named autocheckout_enabled_, how could we name it properly? > > > Also, > > > > we can not check the field like this: > > > > > > > > if (autocheckout_enabled_) { > > > > // do something > > > > } > > > > > > > > instead we have to use this form: > > > > if (autocheckout_enabled_ == AUTOCHECKOUT_ENABLED) { > > > > // do someting > > > > } > > > > > > > > Overall I think a simple bool fit this usecase naturally and much easier. > > > > > > You can keep the boolean member variable, and initialize it as > > > """autocheckout_enabled_(autocheckout_enabled == AUTOCHECKOUT_ENABLED)""" in > > the > > > initializer list. > > > > > > I guess you have read item 40 in http://go/effectivejava, "Prefer two-element > > enum types to boolean parameters". It makes sense for a lot of things when you > > may need a third value. The example the author gave was: use TemperatureScale > > enum: > > public enum TemperatureScale { FAHRENHEIT, CELSIUS } > > rather than : > > bool is_fahrenheit; > > The reason is that you may want to include KELVIN as the third scale. > > > > But if you use it for a bi-state flag, you breaks the attitude of a boolean > that > > is nothing more than representing a bi-state of true and false and converse > with > > each other. What does !AUTOCHECKOUT_ENABLED mean? It should be equivalent to > > AUTOCHECKOUT_DISABLED, but it is illegal in syntax. > > > > I think it is not ideal to convert autocheckout_enabled_ to an enum, but if > you > > insist that this is the direction we want to take, I can make the code change. > > I do insist that if you keep this as a parameter to the constructor, the > parameter should be an enumerated constant rather than a boolean. Replaced autochout_enabled with autocheckout_url_prefix, which is the matched url prefix in the whitelist. The hashcode of the prefix wil be passed down to autofillserver in the other CL: https://codereview.chromium.org/11953100/
LGTM with nits: https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:12: #include "base/supports_user_data.h" nit: This is already included in the header. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:114: return ""; nit: Prefer std::string() to "". https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.cc:131: // TODO(benquan): find a better way to parse csv data. nit: Is this TODO still relevant? https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:12: #include "base/time.h" nit: Move this to the .cc file. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:24: class URLFetcher; nit: Alphabetize https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:31: // whitelist from the server. nit: Suggested tweaked wording: "Downloads and caches the list of URL prefixes whitelisted for use with Autocheckout." https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:62: bool callback_is_pending() const { return callback_is_pending_; } nit: This doesn't seem to be used. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager.h:66: } nit: You could omit this method by setting callback_is_pending_ to false when StopDownloadTimer() is called. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:84: whitelist_manager_.reset(); nit: This doesn't seem like it ought to be needed. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:90: void EnsureCreateWhitelistManager() { nit: Omit "Create" https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:93: profile_.GetRequestContext())); nit: Please add curly braces, since the body of the if-stmt spans multiple lines. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:115: const std::vector<std::string>& GetUrlPrefixes() const { nit: Name this method get_url_prefixes() https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:142: whitelist_manager_->ScheduleDownload(3); nit: Please define a named constant for "3". https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:231: GURL("http://www.Merchant1.com/checkout/"))); nit: Please use lowercase Merchant here, since what you're really testing is the scheme. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:234: GURL("www.Merchant1.com/checkout/"))); nit: Ditto. https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... File chrome/browser/autofill/form_structure.cc (right): https://chromiumcodereview.appspot.com/11867025/diff/32025/chrome/browser/aut... chrome/browser/autofill/form_structure.cc:554: return IsAutocheckoutEnabled()? 0 : kRequiredFillableFields; nit: Please remove two of the spaces after "return" and add a space before "?".
https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.cc (right): https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:12: #include "base/supports_user_data.h" On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: This is already included in the header. Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:114: return ""; On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Prefer std::string() to "". Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.cc:131: // TODO(benquan): find a better way to parse csv data. On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Is this TODO still relevant? Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager.h (right): https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:12: #include "base/time.h" On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Move this to the .cc file. Looks like this is not needed anymore, deleted. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:24: class URLFetcher; On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Alphabetize Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:31: // whitelist from the server. On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Suggested tweaked wording: "Downloads and caches the list of URL prefixes > whitelisted for use with Autocheckout." Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:62: bool callback_is_pending() const { return callback_is_pending_; } On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: This doesn't seem to be used. Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager.h:66: } On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: You could omit this method by setting callback_is_pending_ to false when > StopDownloadTimer() is called. Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... File chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc (right): https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:84: whitelist_manager_.reset(); On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: This doesn't seem like it ought to be needed. Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:90: void EnsureCreateWhitelistManager() { On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Omit "Create" Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:93: profile_.GetRequestContext())); On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Please add curly braces, since the body of the if-stmt spans multiple > lines. Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:115: const std::vector<std::string>& GetUrlPrefixes() const { On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Name this method get_url_prefixes() Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:142: whitelist_manager_->ScheduleDownload(3); On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Please define a named constant for "3". Done. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:231: GURL("http://www.Merchant1.com/checkout/"))); On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Please use lowercase Merchant here, since what you're really testing is the > scheme. Done https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/a... chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc:234: GURL("www.Merchant1.com/checkout/"))); On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Ditto. Ditto. https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/f... File chrome/browser/autofill/form_structure.cc (right): https://codereview.chromium.org/11867025/diff/32025/chrome/browser/autofill/f... chrome/browser/autofill/form_structure.cc:554: return IsAutocheckoutEnabled()? 0 : kRequiredFillableFields; On 2013/01/31 05:01:29, Ilya Sherman wrote: > nit: Please remove two of the spaces after "return" and add a space before "?". Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/33006
Failed to trigger a try job on android_clang_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/35009
Failed to trigger a try job on android_clang_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/50003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/50003
Presubmit check for 11867025-50003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 2.2s to calculate.
Hi sky, Need your approval for the .gypi change. Thanks, Benquan
*gyp* LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/50003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/50003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/50003
Failed to apply patch for chrome/browser/autofill/autofill_manager_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/autofill/autofill_manager_unittest.cc
Hunk #1 FAILED at 617.
Hunk #2 succeeded at 648 with fuzz 1.
Hunk #3 succeeded at 751 (offset 4 lines).
Hunk #4 succeeded at 779 (offset 17 lines).
Hunk #5 succeeded at 2695 (offset 17 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/autofill/autofill_manager_unittest.cc.rej
Patch: chrome/browser/autofill/autofill_manager_unittest.cc
Index: chrome/browser/autofill/autofill_manager_unittest.cc
diff --git a/chrome/browser/autofill/autofill_manager_unittest.cc
b/chrome/browser/autofill/autofill_manager_unittest.cc
index
c9932bdb4f5c263a1ac6d991c773fcf1160245f4..ba8a8bc60f6659f6231bda0789db08582e4db7be
100644
--- a/chrome/browser/autofill/autofill_manager_unittest.cc
+++ b/chrome/browser/autofill/autofill_manager_unittest.cc
@@ -617,19 +617,22 @@ class AutofillManagerTest : public
ChromeRenderViewHostTestHarness {
AutofillManagerTest()
: ChromeRenderViewHostTestHarness(),
ui_thread_(BrowserThread::UI, &message_loop_),
- file_thread_(BrowserThread::FILE) {
+ file_thread_(BrowserThread::FILE),
+ io_thread_(BrowserThread::IO) {
}
virtual ~AutofillManagerTest() {
}
virtual void SetUp() OVERRIDE {
- Profile* profile = new TestingProfile();
+ TestingProfile* profile = new TestingProfile();
+ profile->CreateRequestContext();
browser_context_.reset(profile);
PersonalDataManagerFactory::GetInstance()->SetTestingFactory(
profile, TestPersonalDataManager::Build);
ChromeRenderViewHostTestHarness::SetUp();
+ io_thread_.StartIOThread();
TabAutofillManagerDelegate::CreateForWebContents(web_contents());
personal_data_.SetBrowserContext(profile);
autofill_manager_ = new TestAutofillManager(
@@ -648,6 +651,7 @@ class AutofillManagerTest : public
ChromeRenderViewHostTestHarness {
autofill_manager_ = NULL;
file_thread_.Stop();
ChromeRenderViewHostTestHarness::TearDown();
+ io_thread_.Stop();
}
void UpdatePasswordGenerationState(bool new_renderer) {
@@ -746,6 +750,7 @@ class AutofillManagerTest : public
ChromeRenderViewHostTestHarness {
protected:
content::TestBrowserThread ui_thread_;
content::TestBrowserThread file_thread_;
+ content::TestBrowserThread io_thread_;
scoped_refptr<TestAutofillManager> autofill_manager_;
TestPersonalDataManager personal_data_;
@@ -760,7 +765,8 @@ class AutofillManagerTest : public
ChromeRenderViewHostTestHarness {
class TestFormStructure : public FormStructure {
public:
- explicit TestFormStructure(const FormData& form) : FormStructure(form) {}
+ explicit TestFormStructure(const FormData& form)
+ : FormStructure(form, std::string()) {}
virtual ~TestFormStructure() {}
void SetFieldTypes(const std::vector<AutofillFieldType>& heuristic_types,
@@ -2675,7 +2681,7 @@ TEST_F(AutofillManagerTest,
FormSubmittedWithDifferentFields) {
FormsSeen(forms);
// Cache the expected form signature.
- std::string signature = FormStructure(form).FormSignature();
+ std::string signature = FormStructure(form, std::string()).FormSignature();
// Change the structure of the form prior to submission.
// Websites would typically invoke JavaScript either on page load or on form
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benquan@chromium.org/11867025/59001
Message was sent while issue was closed.
Change committed as 181800 |
