Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10386120:
Utility to resolve an hostname using Chromium's code in net/dns (Closed)
|
Created:
8 years, 7 months ago by Daniele Modified:
8 years, 6 months ago CC:
chromium-reviews, pam+watch_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org, mmenke Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUtility to resolve a hostname using Chromium's code in net/dns
R=szym@chromium.org
BUG=128212
TEST=build it and test manually
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140908
Patch Set 1 #
Total comments: 24
Patch Set 2 : Tried to address all the comments szym provided in the previous code review #
Total comments: 9
Patch Set 3 : MockDnsConfigService removed since we can now pass a DnsClient to the constructor of HostResolverIm… #Patch Set 4 : In the previous commit I accidentally removed base/dns_util_unittest.cc from net.gyp #Patch Set 5 : Create a standard DnsClient instead of a MockDnsClient to be passed to HostResolverImpl #
Total comments: 6
Patch Set 6 : Moved dns_config_service_.release() in a single place #
Total comments: 2
Patch Set 7 : Addressing the comments szym wrote in the previous code review #
Total comments: 33
Patch Set 8 : Using LOG instead of std::cout, fix includes and a few stylistic nits. #
Total comments: 1
Patch Set 9 : Removed logging, using an index to iterate the addrlist entries and an ifdef for windows. #Patch Set 10 : gclient sync'd and rebased because try bot can't merge net.gyp #Patch Set 11 : Added string_util include #
Total comments: 1
Patch Set 12 : Include files in alphabetical order #
Total comments: 10
Patch Set 13 : Added base dependency, rearranged include files, added a check when parsing a CL parameter #
Total comments: 4
Patch Set 14 : Changed sort order of dependencies in net.gpy. Added a line in include files. #Messages
Total messages: 22 (0 generated)
Thanks for creating this. http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver.h File net/base/host_resolver.h (right): http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver.h#newco... net/base/host_resolver.h:209: NET_EXPORT HostResolver* CreateHostResolver( I think you should just |#include "net/base/host_resolver_impl.h"| and construct the HostResolverImpl yourself. I think of gdig as the same as test code, and I'd like to avoid exposing CreateHostResolver in host_resolver.h http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver_impl.cc... net/base/host_resolver_impl.cc:1071: No need for this empty line. http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver_impl.cc... net/base/host_resolver_impl.cc:1560: dns_config_service_->Watch(base::Bind(&HostResolverImpl::OnDnsConfigChanged, Why did you change the order? I'm okay with the new order, but I'm wondering about the reasoning here. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode14 net/tools/gdig/gdig.cc:14: #include "net/base/net_errors.h" The lint tool will automatically point most style issues, ordering of #includes among other things. Please correct all listed issues (except for its incorrect complaint about no newline character at end). http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode24 net/tools/gdig/gdig.cc:24: void usage(const char* program_name) { Suggested function name: PrintUsage, probably should be a method of GDig. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode43 net/tools/gdig/gdig.cc:43: callback.Run(dns_config_); This is strongly discouraged. Rule-of-thumb: avoid synchronous calls to callbacks. DoSomething(completion_callback) should either complete synchronously (return value), or asynchronously (through completion_callback). Here, Watch cannot complete synchronously. In most cases, you'd MessageLoop::PostTask() to fix this, but in this case I think this can be avoided. I suggest creating HostResolverImpl with the following mock service: class MockDnsConfigService : public DnsConfigService { public: explicit MockDnsConfigService(GDig* gdig): gdig_(gdig) {} virtual void Watch(const CallbackType& callback) { callback_ = callback; real_service_ = DnsConfigService::CreateSystemService(); real_service_->Watch(base::Bind(&MockDnsConfigService::OnDnsConfig, Unretained(this)); } private: void OnDnsConfig(const DnsConfig& config) { callback_.Run(config); // Tell HostResolverImpl. gdig_->OnDnsConfig(); // Tell parent to run Resolve now. real_service_.reset(); // We don't need it anymore. } GDig* gdig_; CallbackType callback_; scoped_ptr<DnsConfigService> real_service_; }; http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode61 net/tools/gdig/gdig.cc:61: return -1; I suggest defining in GDig: enum { RESULT_NO_RESOLVE = -3, // I'm terrible with names -- this is a rough suggestion. RESULT_NO_CONFIG = -2, RESULT_WRONG_USAGE = -1, RESULT_OK = 0, }; http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode72 net/tools/gdig/gdig.cc:72: LoadDnsConfig(); I suggest: void Start() { resolver_ = new HostResolver(... new MockDnsConfigService(this)); // Wait until service provides DnsConfig. timeout_.Reset(base::Bind(&GDig::OnTimeout, Unretained(this), RESULT_NO_CONFIG)); MessageLoop::current()->PostDelayedTask(timeout_); // alternatively could use Timer from base/base_timer.h } void OnDnsConfig() { resolver_->Resolve(..., base::Bind(OnResolveComplete, ...)); timeout_.Reset(base::Bind(&GDig::OnTimeout, Unretained(this), RESULT_NO_RESOLVE)); // Although I think there's no need for this timeout. // HostResolverImpl has internal time outs configured by DnsConfig. } void OnResolveComplete(int rv) { timeout_.Cancel() // print result Finish(..); } void OnTimeout(int rv) { // rv might be not needed if the only timeout is on RESULT_NO_CONFIG // print that we're timing out Finish(rv); } void Finish(int rv) { result_ = rv; MessageLoop::current()->Quit(); } // then in Main all you need is: Start(); MessageLoop::current()->Run(); return result_; http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode90 net/tools/gdig/gdig.cc:90: void ResolverCallback(int val){ better name: OnResolveComplete(int rv) http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode134 net/tools/gdig/gdig.cc:134: base::TimeDelta::FromSeconds(5)); Ideally, make timeout value a command-line argument. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode140 net/tools/gdig/gdig.cc:140: MessageLoop::current()->PostTask(FROM_HERE, No need to post it on MessageLoop. Just let go of it here. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode156 net/tools/gdig/gdig.cc:156: if (parsed_command_line.HasSwitch("max_retry")) { These values are not going to be used with DnsConfig. They are for getaddrinfo only. You can stuff it with 1 for both. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode175 net/tools/gdig/gdig.cc:175: config_service_.reset(DnsConfigService::CreateSystemService().release()); scoped_ptr::operator= has the semantics you want: config_service_ = DnsConfigService::CreateSystemService();
I http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver.h File net/base/host_resolver.h (right): http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver.h#newco... net/base/host_resolver.h:209: NET_EXPORT HostResolver* CreateHostResolver( On 2012/05/14 18:48:32, szym wrote: > I think you should just |#include "net/base/host_resolver_impl.h"| and construct > the HostResolverImpl yourself. I think of gdig as the same as test code, and I'd > like to avoid exposing CreateHostResolver in host_resolver.h Done. http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver_impl.cc... net/base/host_resolver_impl.cc:1071: On 2012/05/14 18:48:32, szym wrote: > No need for this empty line. Done. http://codereview.chromium.org/10386120/diff/1/net/base/host_resolver_impl.cc... net/base/host_resolver_impl.cc:1560: dns_config_service_->Watch(base::Bind(&HostResolverImpl::OnDnsConfigChanged, I changed it because of the way I was calling the callback from DnsConfigServiceMock inside the Watch method. I needed dns_client_ to be created before calling the callback. Now I changed the code as you suggested and there is no need for this. On 2012/05/14 18:48:32, szym wrote: > Why did you change the order? I'm okay with the new order, but I'm wondering > about the reasoning here. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode24 net/tools/gdig/gdig.cc:24: void usage(const char* program_name) { Ok, even if I don't really see the necessity to make it a method, since it doesn't use any GDig member. On 2012/05/14 18:48:32, szym wrote: > Suggested function name: PrintUsage, probably should be a method of GDig. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode43 net/tools/gdig/gdig.cc:43: callback.Run(dns_config_); Quite clear, it solved other problems I had to work around as well. On 2012/05/14 18:48:32, szym wrote: > This is strongly discouraged. Rule-of-thumb: avoid synchronous calls to > callbacks. DoSomething(completion_callback) should either complete synchronously > (return value), or asynchronously (through completion_callback). Here, Watch > cannot complete synchronously. In most cases, you'd MessageLoop::PostTask() to > fix this, but in this case I think this can be avoided. > > I suggest creating HostResolverImpl with the following mock service: > > class MockDnsConfigService : public DnsConfigService { > public: > explicit MockDnsConfigService(GDig* gdig): gdig_(gdig) {} > virtual void Watch(const CallbackType& callback) { > callback_ = callback; > real_service_ = DnsConfigService::CreateSystemService(); > real_service_->Watch(base::Bind(&MockDnsConfigService::OnDnsConfig, > Unretained(this)); > } > private: > void OnDnsConfig(const DnsConfig& config) { > callback_.Run(config); // Tell HostResolverImpl. > gdig_->OnDnsConfig(); // Tell parent to run Resolve now. > real_service_.reset(); // We don't need it anymore. > } > > GDig* gdig_; > CallbackType callback_; > scoped_ptr<DnsConfigService> real_service_; > }; http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode61 net/tools/gdig/gdig.cc:61: return -1; Done, but the name you used seems ok to me, so I kept it. :) On 2012/05/14 18:48:32, szym wrote: > I suggest defining in GDig: > enum { > RESULT_NO_RESOLVE = -3, // I'm terrible with names -- this is a rough > suggestion. > RESULT_NO_CONFIG = -2, > RESULT_WRONG_USAGE = -1, > RESULT_OK = 0, > }; http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode72 net/tools/gdig/gdig.cc:72: LoadDnsConfig(); That's definitely nicer than what I did. On 2012/05/14 18:48:32, szym wrote: > I suggest: > > void Start() { > resolver_ = new HostResolver(... new MockDnsConfigService(this)); > // Wait until service provides DnsConfig. > timeout_.Reset(base::Bind(&GDig::OnTimeout, Unretained(this), > RESULT_NO_CONFIG)); > MessageLoop::current()->PostDelayedTask(timeout_); > // alternatively could use Timer from base/base_timer.h > } > > void OnDnsConfig() { > resolver_->Resolve(..., base::Bind(OnResolveComplete, ...)); > timeout_.Reset(base::Bind(&GDig::OnTimeout, Unretained(this), > RESULT_NO_RESOLVE)); // Although I think there's no need for this timeout. > // HostResolverImpl has internal time outs configured by DnsConfig. > } > > void OnResolveComplete(int rv) { > timeout_.Cancel() > // print result > Finish(..); > } > > void OnTimeout(int rv) { // rv might be not needed if the only timeout is on > RESULT_NO_CONFIG > // print that we're timing out > Finish(rv); > } > > void Finish(int rv) { > result_ = rv; > MessageLoop::current()->Quit(); > } > > // then in Main all you need is: > Start(); > MessageLoop::current()->Run(); > return result_; http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode90 net/tools/gdig/gdig.cc:90: void ResolverCallback(int val){ On 2012/05/14 18:48:32, szym wrote: > better name: OnResolveComplete(int rv) Done. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode134 net/tools/gdig/gdig.cc:134: base::TimeDelta::FromSeconds(5)); On 2012/05/14 18:48:32, szym wrote: > Ideally, make timeout value a command-line argument. Done. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode140 net/tools/gdig/gdig.cc:140: MessageLoop::current()->PostTask(FROM_HERE, Releasing the pointer here, caused a DCHECK to complain. This code was called from Main -> LoadDnsConfig -> Watch -> OnDnsConfigChanged. Now that this method is going to be call by a callback called by the messageloop, I can release it here without any problem. On 2012/05/14 18:48:32, szym wrote: > No need to post it on MessageLoop. Just let go of it here. http://codereview.chromium.org/10386120/diff/1/net/tools/gdig/gdig.cc#newcode156 net/tools/gdig/gdig.cc:156: if (parsed_command_line.HasSwitch("max_retry")) { On 2012/05/14 18:48:32, szym wrote: > These values are not going to be used with DnsConfig. They are for getaddrinfo > only. You can stuff it with 1 for both. Done.
Excellent! To fetch the updates to AddressList: git checkout master gclient sync git checkout <your branch> git rebase master https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:25: // It is used to intercept the OnDnsConfig and notify the main program Not clear which "OnDnsConfig." How about: "It will intercept the call to |callback| and notify GDig that it received DnsConfig." https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:27: class DnsConfigServiceMock : public DnsConfigService { nit: in src/net MockXxx is much more common than XxxMock. https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:34: void OnDnsConfig(const DnsConfig& dns_config_); Can be private. https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:62: friend void DnsConfigServiceMock::OnDnsConfig(const DnsConfig&); I think it's reasonable to friend the whole class. A bit less cognitive load. https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:108: std::cout << "usage: " << program_name << This is so basic, could consider removing this function entirely and simply print the usage in ParseCommandLine. This way if you change the name of the switch it will be in the same location. If you choose to keep it, add // static before it. https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:135: if (val < 0) { val != OK https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:140: const struct addrinfo* addrinf = addrlist_.head(); AddressList has been changed to std::vector<IPEndPoint> and this will not compile. https://chromiumcodereview.appspot.com/10386120/diff/5001/net/tools/gdig/gdig... net/tools/gdig/gdig.cc:198: if (parsed_command_line.HasSwitch("read_config_timeout")) { Perhaps just config_timeout
The change to HostResolverImpl constructor has landed (and set_dns_client_for_tests is gone). To fetch it: git checkout master gclient sync git checkout <your branch> git rebase master You can still use a MockDnsConfigService, but I think the one from net/dns/dns_test_util.cc should suffice. I suggest using a simple forwarder for DnsClient to determine when it receives a valid DnsConfig. http://codereview.chromium.org/10386120/diff/5001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/5001/net/tools/gdig/gdig.cc#newc... net/tools/gdig/gdig.cc:74: int timeout_seconds_; Use base::TimeDelta.
On 2012/06/01 17:54:49, szym wrote: > You can still use a MockDnsConfigService, but I think the one from > net/dns/dns_test_util.cc should suffice. I suggest using a simple forwarder for > DnsClient to determine when it receives a valid DnsConfig. Doh. Of course you need to use DnsConfigService::CreateSystemService. Sorry for the confusion.
http://codereview.chromium.org/10386120/diff/19001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/19001/net/net.gyp#newcode1614 net/net.gyp:1614: 'target_name': 'gdig', you have trailing whitespace here http://codereview.chromium.org/10386120/diff/19001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/19001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:55: std::string domain_name_; nit (personal taste): These two are configuration, so I'd move them up. http://codereview.chromium.org/10386120/diff/19001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:79: MessageLoop loop(MessageLoop::TYPE_IO); It's probably preferred to use MessageLoopForIO.
http://codereview.chromium.org/10386120/diff/20001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/20001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:84: dns_config_service_.reset(); Maybe add a comment: "Destroy it while MessageLoop is alive."
Thank you Szymon for the meticulous reviews, I think I addressed all the issues you pointed out. http://codereview.chromium.org/10386120/diff/19001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/19001/net/net.gyp#newcode1614 net/net.gyp:1614: 'target_name': 'gdig', On 2012/06/01 22:07:58, szym wrote: > you have trailing whitespace here Done. http://codereview.chromium.org/10386120/diff/19001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/19001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:55: std::string domain_name_; On 2012/06/01 22:07:58, szym wrote: > nit (personal taste): These two are configuration, so I'd move them up. Done. http://codereview.chromium.org/10386120/diff/19001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:79: MessageLoop loop(MessageLoop::TYPE_IO); On 2012/06/01 22:07:58, szym wrote: > It's probably preferred to use MessageLoopForIO. Done. http://codereview.chromium.org/10386120/diff/20001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/20001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:84: dns_config_service_.reset(); On 2012/06/01 22:10:23, szym wrote: > Maybe add a comment: "Destroy it while MessageLoop is alive." Done.
LGTM! A few stylistic nits, and maybe switch from std::cout to LOG. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:1: #include <stdio.h> Add the licence blurb. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:2: #include <iostream> Don't use iostream. Use base/logging.h and LOG(INFO)/LOG(ERROR) instead. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:12: Unneeded blank line. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:17: Ditto. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:95: std::cout << i->ToStringWithoutPort() << std::endl; nit: too much indent http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:148: return false; nit: too much indent http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:153: int timeout_seconds = 0; Ditto.
Fix missing includes and a few more nits. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:2: #include <iostream> On 2012/06/03 04:49:20, szym wrote: > Don't use iostream. Use base/logging.h and LOG(INFO)/LOG(ERROR) instead. Also, need to include <string> http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:4: #include "base/at_exit.h" Include "base/bind.h", "base/cancelable_callback.h", "base/logging.h" (for DCHECK) and "base/time.h". http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:13: #include "net/base/host_resolver_impl.h" Include "net/base/host_cache.h". http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:16: #include "net/base/sys_addrinfo.h" Include "net/base/address_list.h" instead. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:18: #include "net/dns/dns_client.h" Include "net/dns/dns_config_service.h". http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:71: // Without this there will be a mem leak on osx nit: Add "." at end of sentence. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:82: // Destroy it while MessageLoopForIO is alive Ditto. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:94: for (AddressList::iterator i=addrlist_.begin(); i!=addrlist_.end(); ++i) { No strong opinion about it, but I think preference in net is for "for (size_t i = 0; i < addrlist.size(); ++i)". Also, add spaces around "=" and "!=". http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:140: nit: Unneeded blank line. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:147: if (parsed_command_line.GetArgs().size() != 1) { No need for braces around single line body.
I used the LOG only for error and info messages, and switched to printf for the actual output, is that what you intended? http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:1: #include <stdio.h> On 2012/06/03 04:49:20, szym wrote: > Add the licence blurb. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:2: #include <iostream> On 2012/06/03 20:00:05, szym wrote: > On 2012/06/03 04:49:20, szym wrote: > > Don't use iostream. Use base/logging.h and LOG(INFO)/LOG(ERROR) instead. > > Also, need to include <string> Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:4: #include "base/at_exit.h" On 2012/06/03 20:00:05, szym wrote: > Include "base/bind.h", "base/cancelable_callback.h", "base/logging.h" (for > DCHECK) and "base/time.h". Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:13: #include "net/base/host_resolver_impl.h" On 2012/06/03 20:00:05, szym wrote: > Include "net/base/host_cache.h". Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:16: #include "net/base/sys_addrinfo.h" On 2012/06/03 20:00:05, szym wrote: > Include "net/base/address_list.h" instead. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:17: On 2012/06/03 04:49:20, szym wrote: > Ditto. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:18: #include "net/dns/dns_client.h" On 2012/06/03 20:00:05, szym wrote: > Include "net/dns/dns_config_service.h". Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:71: // Without this there will be a mem leak on osx On 2012/06/03 20:00:05, szym wrote: > nit: Add "." at end of sentence. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:82: // Destroy it while MessageLoopForIO is alive On 2012/06/03 20:00:05, szym wrote: > Ditto. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:94: for (AddressList::iterator i=addrlist_.begin(); i!=addrlist_.end(); ++i) { std::vector<IPEndPoint>::at is not exposed in AddressList, and I need it if we want to use an index to iterate, should I expose it? On 2012/06/03 20:00:05, szym wrote: > No strong opinion about it, but I think preference in net is for "for (size_t i > = 0; i < addrlist.size(); ++i)". > Also, add spaces around "=" and "!=". http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:95: std::cout << i->ToStringWithoutPort() << std::endl; On 2012/06/03 04:49:20, szym wrote: > nit: too much indent Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:140: On 2012/06/03 20:00:05, szym wrote: > nit: Unneeded blank line. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:147: if (parsed_command_line.GetArgs().size() != 1) { On 2012/06/03 20:00:05, szym wrote: > No need for braces around single line body. Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:148: return false; On 2012/06/03 04:49:20, szym wrote: > nit: too much indent Done. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:153: int timeout_seconds = 0; On 2012/06/03 04:49:20, szym wrote: > Ditto. Done.
LGTM. Up to you if you want to use cout or printf, but it looks logging.h is too much hassle for a tool like this. http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/27001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:94: for (AddressList::iterator i=addrlist_.begin(); i!=addrlist_.end(); ++i) { On 2012/06/04 20:01:14, Daniele wrote: > std::vector<IPEndPoint>::at is not exposed in AddressList, and I need it if we > want to use an index to iterate, should I expose it? > > On 2012/06/03 20:00:05, szym wrote: > > No strong opinion about it, but I think preference in net is for "for (size_t > i > > = 0; i < addrlist.size(); ++i)". > > Also, add spaces around "=" and "!=". > operator[] is exposed.
http://codereview.chromium.org/10386120/diff/31001/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/31001/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:163: domain_name_ = parsed_command_line.GetArgs().at(0); Use operator[] rather than .at() for consistency with rest of net/. This will not compile on Windows, because GetArgs()[0] is std::wstring. You'll need to use WideToASCII #if defined(OS_WIN).
I think it's ready to land. Thanks for all the hard work! https://chromiumcodereview.appspot.com/10386120/diff/45002/net/tools/gdig/gdi... File net/tools/gdig/gdig.cc (right): https://chromiumcodereview.appspot.com/10386120/diff/45002/net/tools/gdig/gdi... net/tools/gdig/gdig.cc:23: #include "net/base/address_list.h" nit: not in alphabetical order
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dcastagna@google.com/10386120/46002
Just a few random drive-by nits, mostly super-minor style. http://codereview.chromium.org/10386120/diff/46002/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/46002/net/net.gyp#newcode1616 net/net.gyp:1616: 'net', nit: You should depend on base explicitly, since you directly include those headers. http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:6: #include <string> nit: Line breaks between each of C headers, C++ headers, and project headers http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:13: #endif nit: platform-specific headers appear at the end of the include section (eg: line 26) See http://dev.chromium.org/developers/coding-style "#includes should be ordered per Google style, except"... http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:165: &timeout_seconds); nit: This returns a bool - check the result? Also negative values et-al, if they matter.
http://codereview.chromium.org/10386120/diff/46002/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/46002/net/net.gyp#newcode1616 net/net.gyp:1616: 'net', On 2012/06/06 15:54:58, Ryan Sleevi wrote: > nit: You should depend on base explicitly, since you directly include those > headers. I don't think it's a nit. I think it will fail on the "win" CQ bot, which is configured to do component=shared_library and will fail with LNK2019 (unresolved external symbol). Had similar issue: http://codereview.chromium.org/10441027/#ps19005
Thank you Ryan for the additional review. I added the dependency and fixed the includes. Even if now cpplint says they're not in alphabetical order. I also added a warning message if the config_timeout parameter parsing fails. http://codereview.chromium.org/10386120/diff/46002/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/46002/net/net.gyp#newcode1616 net/net.gyp:1616: 'net', On 2012/06/06 15:54:58, Ryan Sleevi wrote: > nit: You should depend on base explicitly, since you directly include those > headers. Done. http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:6: #include <string> On 2012/06/06 15:54:58, Ryan Sleevi wrote: > nit: Line breaks between each of C headers, C++ headers, and project headers Done. http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:13: #endif On 2012/06/06 15:54:58, Ryan Sleevi wrote: > nit: platform-specific headers appear at the end of the include section (eg: > line 26) > > See http://dev.chromium.org/developers/coding-style "#includes should be ordered > per Google style, except"... Now cpplint complains that it's not in alphabetic order, is that ok? http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:165: &timeout_seconds); On 2012/06/06 15:54:58, Ryan Sleevi wrote: > nit: This returns a bool - check the result? Also negative values et-al, if they > matter. Done.
lgtm http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/46002/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:13: #endif On 2012/06/06 17:43:42, Daniele wrote: > On 2012/06/06 15:54:58, Ryan Sleevi wrote: > > nit: platform-specific headers appear at the end of the include section (eg: > > line 26) > > > > See http://dev.chromium.org/developers/coding-style "#includes should be > ordered > > per Google style, except"... > > Now cpplint complains that it's not in alphabetic order, is that ok? Yes. The rules in cpplint.py currently reflect Google style, but this was an important deviation for Chromium given the cross-platform nature. http://codereview.chromium.org/10386120/diff/44004/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/44004/net/net.gyp#newcode1617 net/net.gyp:1617: '../base/base.gyp:base', nit: sort these (../ before a-z) http://codereview.chromium.org/10386120/diff/44004/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/44004/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:24: #if defined(OS_MACOSX) Newline here as well, before the #if defined
Thank you for the reviews, I'm going to try to commit this. http://codereview.chromium.org/10386120/diff/44004/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/10386120/diff/44004/net/net.gyp#newcode1617 net/net.gyp:1617: '../base/base.gyp:base', On 2012/06/06 23:20:29, Ryan Sleevi wrote: > nit: sort these (../ before a-z) Done. Ok, I just copied it from the dependencies of fetch_client, that has net before ../ http://codereview.chromium.org/10386120/diff/44004/net/tools/gdig/gdig.cc File net/tools/gdig/gdig.cc (right): http://codereview.chromium.org/10386120/diff/44004/net/tools/gdig/gdig.cc#new... net/tools/gdig/gdig.cc:24: #if defined(OS_MACOSX) On 2012/06/06 23:20:29, Ryan Sleevi wrote: > Newline here as well, before the #if defined Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dcastagna@google.com/10386120/47004
Change committed as 140908 |
