Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11230060:
Adding commandline switch and user pref for autofill server url. (Closed)
|
Created:
8 years, 2 months ago by ahutter Modified:
8 years, 1 month ago CC:
chromium-reviews, dhollowa+watch_chromium.org, browser-components-watch_chromium.org, dyu1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding commandline switch and user pref for autofill server url.
BUG=157601
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164706
Patch Set 1 #Patch Set 2 : Removing DEPS changes. #Patch Set 3 : Fixing unit test. #Patch Set 4 : #
Total comments: 24
Patch Set 5 : Fixes from code review #Patch Set 6 : More fixes from code review #
Total comments: 13
Patch Set 7 : Even more fixes from code review #Patch Set 8 : Changing include after rebasing #
Total comments: 16
Patch Set 9 : Removing pref for autofill server url #
Total comments: 12
Patch Set 10 : Simplifying AutofillDownloadUrl further #
Total comments: 6
Patch Set 11 : Fixing final (I hope) style nits #
Messages
Total messages: 26 (0 generated)
A few bits of quick feedback: File a bug at crbug.com and reference it. Make sure each of your files ends in a newline. A few nits below. I'll do a more thorough review in a bit. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download_unittest.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_unittest.cc:19: #include "chrome/test/base/testing_pref_service.h" Alphabetize http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download_url.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_url.cc:14: const char kDefaultAutofillServiceUrl[] = "https://clients1.google.com/tbproxy/af/"; 80 chars. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download_url.h (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_url.h:14: public: 1 space on access specifiers. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_url.h:15: explicit AutofillDownloadUrl(PrefServiceBase* pref_service) 1 space after access specifier.
Drive-by: Why do we need a user pref for this? It seems like reading the value from the command-line flag, or else using the default value, should be good enough. Also, all new flags should generally be exposed in about:flags (see about_flags.cc). Otherwise, there is no reasonably convenient way to set the flags on ChromeOS, Android, or iOS. OTOH, this might require adding support for a freeform string input on the about:flags page, which might not be worth the effort for this CL. Just keep in mind that if you want to use this flag on any of the aforementioned platforms, you'll want to be able to set it via the about:flags UI. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.cc:53: autofill_download_url_ = new AutofillDownloadUrl(preferences); This will leak... http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.h (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.h:141: AutofillDownloadUrl* autofill_download_url_; Why is this stored as a pointer? http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download_unittest.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_unittest.cc:91: } nit: Since this code is testing the AutofillDownloadUrl class, it should be in a separate test file, named autofill_download_url_unittest.cc http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download_url.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_url.cc:32: std::string baseAutoFillServiceUrl = command_line.GetSwitchValueASCII( nit: "AutoFill" -> "Autofill" http://codereview.chromium.org/11230060/diff/7001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:126: // Flag used to tell Chrome the base url of the autofill service. nit: "autofill" -> "Autofill"
On Tue, Oct 23, 2012 at 4:45 PM, <isherman@chromium.org> wrote: > Drive-by: Why do we need a user pref for this? It seems like reading the > value > from the command-line flag, or else using the default value, should be good > enough. > > Also, all new flags should generally be exposed in about:flags (see > about_flags.cc). Otherwise, there is no reasonably convenient way to set > the > flags on ChromeOS, Android, or iOS. OTOH, this might require adding > support for > a freeform string input on the about:flags page, which might not be worth > the > effort for this CL. Just keep in mind that if you want to use this flag > on any > of the aforementioned platforms, you'll want to be able to set it via the > about:flags UI. > That was my suggestion. Other services do similar things in order to allow easy debug/test. > > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download.cc<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download.cc> > File chrome/browser/autofill/**autofill_download.cc (right): > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download.cc#newcode53<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download.cc#newcode53> > chrome/browser/autofill/**autofill_download.cc:53: autofill_download_url_ > = new AutofillDownloadUrl(**preferences); > This will leak... > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download.h<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download.h> > File chrome/browser/autofill/**autofill_download.h (right): > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download.h#newcode141<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download.h#newcode141> > chrome/browser/autofill/**autofill_download.h:141: AutofillDownloadUrl* > autofill_download_url_; > Why is this stored as a pointer? > > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download_unittest.cc<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download_unittest.cc> > File chrome/browser/autofill/**autofill_download_unittest.cc (right): > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download_unittest.cc#newcode91<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download_unittest.cc#newcode91> > chrome/browser/autofill/**autofill_download_unittest.cc:**91: } > nit: Since this code is testing the AutofillDownloadUrl class, it should > be in a separate test file, named autofill_download_url_**unittest.cc > > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download_url.cc<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download_url.cc> > File chrome/browser/autofill/**autofill_download_url.cc (right): > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > browser/autofill/autofill_**download_url.cc#newcode32<http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/autofill_download_url.cc#newcode32> > chrome/browser/autofill/**autofill_download_url.cc:32: std::string > baseAutoFillServiceUrl = command_line.**GetSwitchValueASCII( > nit: "AutoFill" -> "Autofill" > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > common/chrome_switches.cc<http://codereview.chromium.org/11230060/diff/7001/chrome/common/chrome_switches.cc> > File chrome/common/chrome_switches.**cc (right): > > http://codereview.chromium.**org/11230060/diff/7001/chrome/** > common/chrome_switches.cc#**newcode126<http://codereview.chromium.org/11230060/diff/7001/chrome/common/chrome_switches.cc#newcode126> > chrome/common/chrome_switches.**cc:126: // Flag used to tell Chrome the > base url of the autofill service. > nit: "autofill" -> "Autofill" > > http://codereview.chromium.**org/11230060/<http://codereview.chromium.org/112... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
On 2012/10/23 23:49:18, Albert Bodenhamer wrote: > On Tue, Oct 23, 2012 at 4:45 PM, <mailto:isherman@chromium.org> wrote: > > > Drive-by: Why do we need a user pref for this? It seems like reading the > > value > > from the command-line flag, or else using the default value, should be good > > enough. > > > > Also, all new flags should generally be exposed in about:flags (see > > about_flags.cc). Otherwise, there is no reasonably convenient way to set > > the > > flags on ChromeOS, Android, or iOS. OTOH, this might require adding > > support for > > a freeform string input on the about:flags page, which might not be worth > > the > > effort for this CL. Just keep in mind that if you want to use this flag > > on any > > of the aforementioned platforms, you'll want to be able to set it via the > > about:flags UI. > > > > That was my suggestion. Other services do similar things in order to allow > easy debug/test. Hmm, I don't really like the idea of every service rolling this functionality on its own. It's not a lot of code, but it's also not really a normal user pref, so I'd rather that we not treat it as though it were. Sticky flags is one of the things that about:flags just provides for free. I think we should not add a pref here, and instead just extend about:flags to support arbitrary string values for flags (probably as a separate CL).
And a few more. You'll also need OWNERS approval for adding the pref and command line flags. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.cc (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.cc:53: autofill_download_url_ = new AutofillDownloadUrl(preferences); Currently there's a leak here. Most consumers of classes like this allocate them on the stack. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download.h (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.h:18: #include "chrome/browser/autofill/autofill_download_url.h" Forward declare rather than include. http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download.h:141: AutofillDownloadUrl* autofill_download_url_; scoped_ptr<> http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_download_url.h (right): http://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_download_url.h:24: PrefServiceBase* pref_service_; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download.cc (right): https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download.cc:53: autofill_download_url_ = new AutofillDownloadUrl(preferences); On 2012/10/23 23:57:11, Albert Bodenhamer wrote: > Currently there's a leak here. Most consumers of classes like this allocate > them on the stack. Done. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download.h (right): https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download.h:18: #include "chrome/browser/autofill/autofill_download_url.h" On 2012/10/23 23:57:11, Albert Bodenhamer wrote: > Forward declare rather than include. Done. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download.h:141: AutofillDownloadUrl* autofill_download_url_; On 2012/10/23 23:57:11, Albert Bodenhamer wrote: > scoped_ptr<> Done. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_unittest.cc (right): https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_unittest.cc:19: #include "chrome/test/base/testing_pref_service.h" On 2012/10/23 23:34:17, Albert Bodenhamer wrote: > Alphabetize Reverting all changes to this file. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_unittest.cc:91: } On 2012/10/23 23:45:13, Ilya Sherman wrote: > nit: Since this code is testing the AutofillDownloadUrl class, it should be in a > separate test file, named autofill_download_url_unittest.cc Reverting all changes to this file. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.cc (right): https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:14: const char kDefaultAutofillServiceUrl[] = "https://clients1.google.com/tbproxy/af/"; On 2012/10/23 23:34:17, Albert Bodenhamer wrote: > 80 chars. Done. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:32: std::string baseAutoFillServiceUrl = command_line.GetSwitchValueASCII( On 2012/10/23 23:45:13, Ilya Sherman wrote: > nit: "AutoFill" -> "Autofill" Done. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.h (right): https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:14: public: On 2012/10/23 23:34:17, Albert Bodenhamer wrote: > 1 space on access specifiers. I think I fixed this. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:15: explicit AutofillDownloadUrl(PrefServiceBase* pref_service) On 2012/10/23 23:34:17, Albert Bodenhamer wrote: > 1 space after access specifier. I think I fixed this. https://codereview.chromium.org/11230060/diff/7001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:24: PrefServiceBase* pref_service_; On 2012/10/23 23:57:11, Albert Bodenhamer wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/11230060/diff/7001/chrome/common/chrome_switc... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/11230060/diff/7001/chrome/common/chrome_switc... chrome/common/chrome_switches.cc:126: // Flag used to tell Chrome the base url of the autofill service. On 2012/10/23 23:45:13, Ilya Sherman wrote: > nit: "autofill" -> "Autofill" Done.
http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download.cc (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download.cc:54: autofill_download_url_.reset(new AutofillDownloadUrl(preferences)); I still think it would be a bit better to just allocate an AutofillDownloadUrl on the stack in StartRequest rather than keeping one around forever. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.cc (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:9: #include "chrome/browser/api/prefs/pref_service_base.h" it looks like you should use: #include "chrome/browser/prefs/pref_service.h" http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:15: "https://clients1.google.com/tbproxy/af/"; 4 spaces on continuation of a line. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:28: kDefaultAutofillServiceUrl); 1 more space on the second line. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.h (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:16: public: Single space indent on public:. It should be: class Blah { public: explicit Blah() {} http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:17: explicit AutofillDownloadUrl(PrefServiceBase* pref_service) Should be: explicit Blah (Stuff* stuff) : BlahsGuts(stuff) {
http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download.cc (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download.cc:54: autofill_download_url_.reset(new AutofillDownloadUrl(preferences)); On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > I still think it would be a bit better to just allocate an AutofillDownloadUrl > on the stack in StartRequest rather than keeping one around forever. Done. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.cc (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:9: #include "chrome/browser/api/prefs/pref_service_base.h" On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > it looks like you should use: > #include "chrome/browser/prefs/pref_service.h" I don't think I can based on the current restrictions in the DEPS file and pref_service_base.h is consistent with the rest of the files in chrome/browser/autofill (I think). http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:15: "https://clients1.google.com/tbproxy/af/"; On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > 4 spaces on continuation of a line. Done. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:28: kDefaultAutofillServiceUrl); On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > 1 more space on the second line. Done. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.h (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:16: public: On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > Single space indent on public:. > > It should be: > class Blah { > public: > explicit Blah() {} Done. http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.h:17: explicit AutofillDownloadUrl(PrefServiceBase* pref_service) On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > Should be: > explicit Blah (Stuff* stuff) > : BlahsGuts(stuff) { Done.
http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_download_url.cc (right): http://codereview.chromium.org/11230060/diff/18001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_download_url.cc:9: #include "chrome/browser/api/prefs/pref_service_base.h" The rest of AF is using #include "base/prefs/public/pref_service_base.h" I only mentioned it because it looks like what you currently have here failed on the try bots I started on one of your earlier patches. On 2012/10/24 21:29:45, ahutter wrote: > On 2012/10/24 19:51:56, Albert Bodenhamer wrote: > > it looks like you should use: > > #include "chrome/browser/prefs/pref_service.h" > > I don't think I can based on the current restrictions in the DEPS file and > pref_service_base.h is consistent with the rest of the files in > chrome/browser/autofill (I think).
Adding brettw for OWNER approval.
LGTM for the comments I made. Ilya, would you like to take another look? On Thu, Oct 25, 2012 at 9:51 AM, <ahutter@chromium.org> wrote: > Adding brettw for OWNER approval. > > http://codereview.chromium.**org/11230060/<http://codereview.chromium.org/112... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
I still think that it is neither necessary nor wise to include a new preference for this. Looking at the code, there are lots of issues with how we write that pref, both stylistic and functional. It's certainly feasible to fix them, but I think it's more appropriate to simply remove all code dealing with prefs from this CL. Otherwise LG. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:26: return; nit: Does anything actually go wrong if we remove this line? https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:28: kDefaultAutofillServiceUrl); It looks like the default value is the only value that we'll ever write into the preferences. What's the point? That is, why not just keep it as a constant in this file, and drop all the mucking about with preferences? https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:35: if (ContainsOnlyWhitespaceASCII(baseAutofillServiceUrl)) Optional nit: Can we just check for baseAutofillServiceUrl.empty() instead? https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:37: pref_service_->GetString(prefs::kAutofillServiceUrl); nit: Please add curly braces to this if-stmt, since the body spans multiple lines (even though it is only a single statement). https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.h (right): https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:20: GURL GetAutofillRequestUrl(); nit: s/Request/Query https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:21: GURL GetAutofillUploadUrl(); nit: Please leave a blank line after this one. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:23: void RegisterPreferences(); nit: If we keep this, the common style is for this to be a static method with the signature "void RegisterPrefs(PrefService* prefs)" (or, in some cases, local_state rather than prefs). We shouldn't diverge from that style unless we have good reason to do so -- do we? https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:26: PrefServiceBase* pref_service_; nit: If we keep this, "PrefServiceBase* const pref_service_;" https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url_unittest.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:35: EXPECT_THAT(request_url, HasSubstr("?client=")); nit: This does look tidy, but why not just check that request_url contains the prefix "https://clients1.google.com/tbproxy/af/query?client="? That's a slightly stronger test, as it doesn't allow the URL fragments to be re-arranged arbitrarily w.r.t. one another. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:37: nit: Spurious newline.
https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:28: kDefaultAutofillServiceUrl); On 2012/10/25 22:52:02, Ilya Sherman wrote: > It looks like the default value is the only value that we'll ever write into the > preferences. What's the point? That is, why not just keep it as a constant in > this file, and drop all the mucking about with preferences? Removed. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:35: if (ContainsOnlyWhitespaceASCII(baseAutofillServiceUrl)) On 2012/10/25 22:52:02, Ilya Sherman wrote: > Optional nit: Can we just check for baseAutofillServiceUrl.empty() instead? Done. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.h (right): https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:20: GURL GetAutofillRequestUrl(); On 2012/10/25 22:52:02, Ilya Sherman wrote: > nit: s/Request/Query Done. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:21: GURL GetAutofillUploadUrl(); On 2012/10/25 22:52:02, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url_unittest.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:35: EXPECT_THAT(request_url, HasSubstr("?client=")); On 2012/10/25 22:52:02, Ilya Sherman wrote: > nit: This does look tidy, but why not just check that request_url contains the > prefix "https://clients1.google.com/tbproxy/af/query?client="? That's a > slightly stronger test, as it doesn't allow the URL fragments to be re-arranged > arbitrarily w.r.t. one another. Done. https://chromiumcodereview.appspot.com/11230060/diff/28002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:37: On 2012/10/25 22:52:02, Ilya Sherman wrote: > nit: Spurious newline. Done.
Thanks, looks much better. Just a bit more cleanup, then should be good to go :) https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.h (right): https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:8: #include <string> nit: Once GetBaseAutofillUrl() is moved into the implementation file, this import can be moved as well. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:14: class AutofillDownloadUrl { This class is now stateless, which means that the methods it exposes can now simply be free functions (probably enclosed in an "autofill" namespace). https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:21: std::string GetBaseAutofillUrl(); nit: This can now be tucked into an anonymous namespace in the implementation file. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url_unittest.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:15: }; nit: I believe that you can omit this class, and just use TEST rather than TEST_F below. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:22: StartsWith("https://clients1.google.com/tbproxy/af/query?client=")); nit: 80-col https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:28: StartsWith("https://clients1.google.com/tbproxy/af/upload?client=")); nit: 80-col
Nico, can you grant OWNERS approval for the changes to chrome/ ?
https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... chrome/chrome_browser.gypi:229: 'browser/autofill/autofill_download_url.h', $ ls -l chrome/browser/autofill/ | wc -l 115 Do you guys have plans to make chrome/browser/autofill a component?
On 2012/10/26 22:33:28, Nico wrote: > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... > File chrome/chrome_browser.gypi (right): > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... > chrome/chrome_browser.gypi:229: 'browser/autofill/autofill_download_url.h', > $ ls -l chrome/browser/autofill/ | wc -l > 115 > > Do you guys have plans to make chrome/browser/autofill a component? Yes, Jói and Avi are currently working on it.
(This CL lgtm btw) On 2012/10/26 22:40:40, Ilya Sherman wrote: > On 2012/10/26 22:33:28, Nico wrote: > > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... > > File chrome/chrome_browser.gypi (right): > > > > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... > > chrome/chrome_browser.gypi:229: 'browser/autofill/autofill_download_url.h', > > $ ls -l chrome/browser/autofill/ | wc -l > > 115 > > > > Do you guys have plans to make chrome/browser/autofill a component? > > Yes, Jói and Avi are currently working on it. Do you have a link to the bug for that?
On 2012/10/26 22:41:11, Nico wrote: > (This CL lgtm btw) > > On 2012/10/26 22:40:40, Ilya Sherman wrote: > > On 2012/10/26 22:33:28, Nico wrote: > > > > > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... > > > File chrome/chrome_browser.gypi (right): > > > > > > > > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... > > > chrome/chrome_browser.gypi:229: 'browser/autofill/autofill_download_url.h', > > > $ ls -l chrome/browser/autofill/ | wc -l > > > 115 > > > > > > Do you guys have plans to make chrome/browser/autofill a component? > > > > Yes, Jói and Avi are currently working on it. > > Do you have a link to the bug for that? Aye: http://crbug.com/140037
On Fri, Oct 26, 2012 at 3:51 PM, <isherman@chromium.org> wrote: > On 2012/10/26 22:41:11, Nico wrote: >> >> (This CL lgtm btw) > > >> On 2012/10/26 22:40:40, Ilya Sherman wrote: >> > On 2012/10/26 22:33:28, Nico wrote: >> > > >> > > > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... >> >> > > File chrome/chrome_browser.gypi (right): >> > > >> > > >> > > > > https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/chrome_brow... >> >> > > chrome/chrome_browser.gypi:229: > > 'browser/autofill/autofill_download_url.h', >> >> > > $ ls -l chrome/browser/autofill/ | wc -l >> > > 115 >> > > >> > > Do you guys have plans to make chrome/browser/autofill a component? >> > >> > Yes, Jói and Avi are currently working on it. > > >> Do you have a link to the bug for that? > > > Aye: http://crbug.com/140037 Thanks! > > https://chromiumcodereview.appspot.com/11230060/
https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.h (right): https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:8: #include <string> On 2012/10/26 00:10:49, Ilya Sherman wrote: > nit: Once GetBaseAutofillUrl() is moved into the implementation file, this > import can be moved as well. Done. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:14: class AutofillDownloadUrl { On 2012/10/26 00:10:49, Ilya Sherman wrote: > This class is now stateless, which means that the methods it exposes can now > simply be free functions (probably enclosed in an "autofill" namespace). Done. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:21: std::string GetBaseAutofillUrl(); On 2012/10/26 00:10:49, Ilya Sherman wrote: > nit: This can now be tucked into an anonymous namespace in the implementation > file. Done. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url_unittest.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:15: }; On 2012/10/26 00:10:49, Ilya Sherman wrote: > nit: I believe that you can omit this class, and just use TEST rather than > TEST_F below. Done. https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/aut... chrome/browser/autofill/autofill_download_url_unittest.cc:22: StartsWith("https://clients1.google.com/tbproxy/af/query?client=")); On 2012/10/26 00:10:49, Ilya Sherman wrote: > nit: 80-col Just wondering how you catch this? Is there a presubmit script I'm not running?
LGTM If you look at the code review tool there's a link on the right on each file that says "lint". 80-cols issues are also pretty clear in the diff view since the lines wrap. On Mon, Oct 29, 2012 at 8:38 AM, <ahutter@chromium.org> wrote: > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url.h<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url.h> > File chrome/browser/autofill/**autofill_download_url.h (right): > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url.h#**newcode8<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url.h#newcode8> > chrome/browser/autofill/**autofill_download_url.h:8: #include <string> > On 2012/10/26 00:10:49, Ilya Sherman wrote: > >> nit: Once GetBaseAutofillUrl() is moved into the implementation file, >> > this > >> import can be moved as well. >> > > Done. > > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url.h#**newcode14<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url.h#newcode14> > chrome/browser/autofill/**autofill_download_url.h:14: class > AutofillDownloadUrl { > On 2012/10/26 00:10:49, Ilya Sherman wrote: > >> This class is now stateless, which means that the methods it exposes >> > can now > >> simply be free functions (probably enclosed in an "autofill" >> > namespace). > > Done. > > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url.h#**newcode21<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url.h#newcode21> > chrome/browser/autofill/**autofill_download_url.h:21: std::string > GetBaseAutofillUrl(); > On 2012/10/26 00:10:49, Ilya Sherman wrote: > >> nit: This can now be tucked into an anonymous namespace in the >> > implementation > >> file. >> > > Done. > > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url_**unittest.cc<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url_unittest.cc> > File chrome/browser/autofill/**autofill_download_url_**unittest.cc > (right): > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url_** > unittest.cc#newcode15<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url_unittest.cc#newcode15> > chrome/browser/autofill/**autofill_download_url_**unittest.cc:15: }; > On 2012/10/26 00:10:49, Ilya Sherman wrote: > >> nit: I believe that you can omit this class, and just use TEST rather >> > than > >> TEST_F below. >> > > Done. > > > https://chromiumcodereview.**appspot.com/11230060/diff/** > 32002/chrome/browser/autofill/**autofill_download_url_** > unittest.cc#newcode22<https://chromiumcodereview.appspot.com/11230060/diff/32002/chrome/browser/autofill/autofill_download_url_unittest.cc#newcode22> > chrome/browser/autofill/**autofill_download_url_**unittest.cc:22: > StartsWith("https://clients1.**google.com/tbproxy/af/query?**client=<https://clients1.google.com/tbproxy/af/query?client=> > ")); > On 2012/10/26 00:10:49, Ilya Sherman wrote: > >> nit: 80-col >> > Just wondering how you catch this? Is there a presubmit script I'm not > running? > > https://chromiumcodereview.**appspot.com/11230060/<https://chromiumcodereview... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
LGTM (with a last few nits) -- thanks :) https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:13: namespace { nit: Leave a blank line after this one. https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:33: } nit: Please change this line to "} // anonymous namespace" https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.h (right): https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:12: GURL GetAutofillUploadUrl(); nit: The more typical indentation for these lines is to leave a blank line after line 10 and before line 13, and to not indent the method declarations.
https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.cc (right): https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:13: namespace { On 2012/10/29 16:47:54, Ilya Sherman wrote: > nit: Leave a blank line after this one. Done. https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.cc:33: } On 2012/10/29 16:47:54, Ilya Sherman wrote: > nit: Please change this line to "} // anonymous namespace" Done. https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... File chrome/browser/autofill/autofill_download_url.h (right): https://chromiumcodereview.appspot.com/11230060/diff/37001/chrome/browser/aut... chrome/browser/autofill/autofill_download_url.h:12: GURL GetAutofillUploadUrl(); On 2012/10/29 16:47:54, Ilya Sherman wrote: > nit: The more typical indentation for these lines is to leave a blank line after > line 10 and before line 13, and to not indent the method declarations. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahutter@chromium.org/11230060/33003
Change committed as 164706 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
