Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16975004:
Finished DNS-SD server. Finished Privet-specified DNS-SD server. (Closed)
|
Created:
7 years, 6 months ago by maksymb Modified:
7 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFinished DNS-SD server. Finished Privet-specified DNS-SD server.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208064
Patch Set 1 #Patch Set 2 : Fixed Lint errors.wq #
Total comments: 16
Patch Set 3 : Fixed some syntax-style mistakes. #Patch Set 4 : Made Builder as specifier of DNS-SD server. #Patch Set 5 : Fixed Lint errors. #
Total comments: 64
Patch Set 6 : Deleted commnets from DnsResponseBuilder. #Patch Set 7 : Fixed style mistakes. #Patch Set 8 : Fixed Lint error. #
Total comments: 18
Patch Set 9 : Added strong control of size in PrivetDnsResponseBuilder. #
Total comments: 46
Patch Set 10 : Reindentation of function calls. #
Total comments: 6
Patch Set 11 : Now builder is only builder and all info about service is now in DnsSdServer class. #Patch Set 12 : Fixed Lint errors. #
Total comments: 24
Patch Set 13 : Reorganized metadata (TXT) storing and building responses. #
Total comments: 10
Patch Set 14 : Reverdet String as parameter to AddResponse. #Patch Set 15 : Fixed Lint errors. #
Total comments: 9
Patch Set 16 : Replaced is_online_ with socket_. #Patch Set 17 : Replaced int with size_t in one place because of warning at win compile. Added comparison with NULL… #Patch Set 18 : Added static_cast for size_t. #Patch Set 19 : One more socket_ != NULL #Patch Set 20 : Added support Getting IP for Windows (now using net_util for this). #Patch Set 21 : Fixel little mistake in GetLocalIp. #
Total comments: 18
Patch Set 22 : Deleted strong #include depedency. #Patch Set 23 : Formalized IpAddressSize constant. #Messages
Total messages: 41 (0 generated)
Please take a look at my CL.
https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.cc:32: if (reader.ReadU16(&out->qtype) && if should fit one one https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:26: // Construct an uninitialized iterator. Construct -> Constructs https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:29: // Construct an iterator to process the |packet| of given |length|. same https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:49: unsigned ReadName(const void* pos, std::string* out) const { Read name does not need arg (pos) https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:69: net::DnsRecordParser record_parser; record_parser_ https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/privet_dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/privet_dns_sd_server.h:3: // found in the LICENSE file. Why did you split privet_dns_sd_server.h and dns_sd_server.h ?
Explained why I've made splitting. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.cc:32: if (reader.ReadU16(&out->qtype) && On 2013/06/13 22:26:36, Vitaly Buka wrote: > if should fit one one Done. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:26: // Construct an uninitialized iterator. On 2013/06/13 22:26:36, Vitaly Buka wrote: > Construct -> Constructs Done. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:29: // Construct an iterator to process the |packet| of given |length|. On 2013/06/13 22:26:36, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:49: unsigned ReadName(const void* pos, std::string* out) const { On 2013/06/13 22:26:36, Vitaly Buka wrote: > Read name does not need arg (pos) No, it does. I have to pass this value to method of encapsulated class. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:69: net::DnsRecordParser record_parser; On 2013/06/13 22:26:36, Vitaly Buka wrote: > record_parser_ Done. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/privet_dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/privet_dns_sd_server.h:3: // found in the LICENSE file. On 2013/06/13 22:26:36, Vitaly Buka wrote: > Why did you split privet_dns_sd_server.h and dns_sd_server.h ? I want to have general DNS-SD server and Privet-specified one.
https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:49: unsigned ReadName(const void* pos, std::string* out) const { On 2013/06/13 22:43:45, maksymb wrote: > On 2013/06/13 22:26:36, Vitaly Buka wrote: > > Read name does not need arg (pos) > > No, it does. I have to pass this value to method of encapsulated class. but you always call that with GetOffset() https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/privet_dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/privet_dns_sd_server.h:3: // found in the LICENSE file. On 2013/06/13 22:43:45, maksymb wrote: > On 2013/06/13 22:26:36, Vitaly Buka wrote: > > Why did you split privet_dns_sd_server.h and dns_sd_server.h ? > > I want to have general DNS-SD server and Privet-specified one. but DnsSdServer does not looks like complete DNS-SD server
https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/dns_packet_parser.h:49: unsigned ReadName(const void* pos, std::string* out) const { On 2013/06/13 22:54:10, Vitaly Buka wrote: > On 2013/06/13 22:43:45, maksymb wrote: > > On 2013/06/13 22:26:36, Vitaly Buka wrote: > > > Read name does not need arg (pos) > > > > No, it does. I have to pass this value to method of encapsulated class. > > but you always call that with GetOffset() Done. https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... File cloud_print/gcp20/prototype/privet_dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/2005/cloud_print/gcp20/prototyp... cloud_print/gcp20/prototype/privet_dns_sd_server.h:3: // found in the LICENSE file. On 2013/06/13 22:54:10, Vitaly Buka wrote: > On 2013/06/13 22:43:45, maksymb wrote: > > On 2013/06/13 22:26:36, Vitaly Buka wrote: > > > Why did you split privet_dns_sd_server.h and dns_sd_server.h ? > > > > I want to have general DNS-SD server and Privet-specified one. > > but DnsSdServer does not looks like complete DNS-SD server Done: now DnsSdServer requires object of class DnsResponseBuilder to build responses and announcements. This builder is service-specified and server is not.
https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.cc:21: : packet_(packet), length_(length), 4 spaces before : and one initializer per line. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:229: void DnsSdServer::DoLoop(int rv) { rename please to ReadFromSocket https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:250: if (CommandLine::ForCurrentProcess()->HasSwitch("no-announcement") == false) { if (!CommandLine::ForCurrentProcess()->HasSwitch("no-announcement")) { https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:257: net::CompletionCallback callback = base::Bind(DoNothing); &DoNothing but better just ti remove DoNothing and SendTo(...., net::CompletionCallback()) https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:270: base::Closure update_callback = base::Bind(&DnsSdServer::Update, remove update_callback and just put bind inside PostDalayedTask call https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:70: const uint32 full_ttl) WARN_UNUSED_RESULT; no need const here https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:87: int len, const scoped_refptr<net::IOBufferWithSize>& buf); const scoped_refptr<net::IOBufferWithSize>& buf -> const net::IOBufferWithSize& buf https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_txt_builder.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_txt_builder.h:25: void Build(std::string* out) const; Remove void Build(std::string* out) https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/gcp20_device.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:30: at_exit.RegisterTask(callback); at_exit.RegisterTask(base::Bind(StopPrinter)); https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:52: base::Closure callback = base::Bind(StartPrinter); Same https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/gcp20_device.gyp (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.gyp:18: { Probably you will need library with most of the code and keep main() only in executable usefull for unittests https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:22: uint32 http_port_tmp; always initialize variables if it has not default constructors https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:24: GetSwitchValueASCII("http-port"), &http_port_tmp)) use temp variable for CommandLine::ForCurrentProcess()->GetSwitchValueASCII("http-port") to fix this strange line break https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:25: http_port_tmp = kDefaultHttpPort; If you can't fit "if" in one line please use {} for body https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:34: VLOG(1) << "HTTP port for responses: " << http_port; you can do that without http_port variable https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:39: uint32 ttl; initialize https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:41: GetSwitchValueASCII("ttl"), &ttl)) same https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:42: ttl = kDefaultTTL; same for {} https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:51: const char* interface_name) { Header fits one line https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:52: struct ifaddrs *ifaddr; uninitialized pointer and datastructor char host[NI_MAXHOST] = {0} https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:62: for (struct ifaddrs* ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) { don't need struct here https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:62: for (struct ifaddrs* ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) { ifa != NULL -> ifa https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:67: int rv = getnameinfo(ifa->ifa_addr, missaligned https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:68: sizeof(struct sockaddr_in), sizeof(*ifa->ifa_addr) https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:78: : strcmp(ifa->ifa_name, interface_name) == 0) { just "interface_name ? " and add () in ? (...) : (...) https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.h:25: private: Empty line before private: https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:51: bool PrivetDnsResponseBuilder::CheckSrv(const std::string& qname) { Check* is not correct here IsSame* https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:95: net::dns_protocol::kTypeSRV, does it fit one line? https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:147: iter != responses_.end(); ++iter) { align on ( https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.h:34: const uint16 http_port); no need const here for http_port https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.h:54: void AppendA(const uint32 ttl) OVERRIDE; no need const here https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.h:77: net::IPAddressNumber http_ip_; http_ip_ and http_port_ need const here
All's done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.cc:21: : packet_(packet), length_(length), On 2013/06/14 19:27:07, Vitaly Buka wrote: > 4 spaces before : > and one initializer per line. Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:229: void DnsSdServer::DoLoop(int rv) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > rename please to ReadFromSocket Template: https://codereview.chromium.org/15733008/diff/17001/net/dns/mdns_listener_imp... https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:250: if (CommandLine::ForCurrentProcess()->HasSwitch("no-announcement") == false) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > if (!CommandLine::ForCurrentProcess()->HasSwitch("no-announcement")) { Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:257: net::CompletionCallback callback = base::Bind(DoNothing); On 2013/06/14 19:27:07, Vitaly Buka wrote: > &DoNothing > > but better just ti remove DoNothing and > > SendTo(...., net::CompletionCallback()) Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:270: base::Closure update_callback = base::Bind(&DnsSdServer::Update, On 2013/06/14 19:27:07, Vitaly Buka wrote: > remove update_callback and just put bind inside PostDalayedTask call Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:70: const uint32 full_ttl) WARN_UNUSED_RESULT; On 2013/06/14 19:27:07, Vitaly Buka wrote: > no need const here Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:87: int len, const scoped_refptr<net::IOBufferWithSize>& buf); On 2013/06/14 19:27:07, Vitaly Buka wrote: > const scoped_refptr<net::IOBufferWithSize>& buf > -> > const net::IOBufferWithSize& buf Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_txt_builder.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_txt_builder.h:25: void Build(std::string* out) const; On 2013/06/14 19:27:07, Vitaly Buka wrote: > Remove void Build(std::string* out) Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/gcp20_device.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:30: at_exit.RegisterTask(callback); On 2013/06/14 19:27:07, Vitaly Buka wrote: > at_exit.RegisterTask(base::Bind(StopPrinter)); Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:52: base::Closure callback = base::Bind(StartPrinter); On 2013/06/14 19:27:07, Vitaly Buka wrote: > Same Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/gcp20_device.gyp (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.gyp:18: { On 2013/06/14 19:27:07, Vitaly Buka wrote: > Probably you will need library with most of the code and keep main() only in > executable > usefull for unittests Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:22: uint32 http_port_tmp; On 2013/06/14 19:27:07, Vitaly Buka wrote: > always initialize variables if it has not default constructors Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:24: GetSwitchValueASCII("http-port"), &http_port_tmp)) On 2013/06/14 19:27:07, Vitaly Buka wrote: > use temp variable for > CommandLine::ForCurrentProcess()->GetSwitchValueASCII("http-port") > to fix this strange line break Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:25: http_port_tmp = kDefaultHttpPort; On 2013/06/14 19:27:07, Vitaly Buka wrote: > If you can't fit "if" in one line please use {} for body Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:34: VLOG(1) << "HTTP port for responses: " << http_port; On 2013/06/14 19:27:07, Vitaly Buka wrote: > you can do that without http_port variable Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:39: uint32 ttl; On 2013/06/14 19:27:07, Vitaly Buka wrote: > initialize Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:41: GetSwitchValueASCII("ttl"), &ttl)) On 2013/06/14 19:27:07, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:42: ttl = kDefaultTTL; On 2013/06/14 19:27:07, Vitaly Buka wrote: > same for {} Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:51: const char* interface_name) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > Header fits one line Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:52: struct ifaddrs *ifaddr; On 2013/06/14 19:27:07, Vitaly Buka wrote: > uninitialized pointer and datastructor > > char host[NI_MAXHOST] = {0} Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:62: for (struct ifaddrs* ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > don't need struct here Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:62: for (struct ifaddrs* ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > ifa != NULL -> ifa Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:67: int rv = getnameinfo(ifa->ifa_addr, On 2013/06/14 19:27:07, Vitaly Buka wrote: > missaligned Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:68: sizeof(struct sockaddr_in), On 2013/06/14 19:27:07, Vitaly Buka wrote: > sizeof(*ifa->ifa_addr) Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:78: : strcmp(ifa->ifa_name, interface_name) == 0) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > just "interface_name ? " > > and add () in ? (...) : (...) > > Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.h:25: private: On 2013/06/14 19:27:07, Vitaly Buka wrote: > Empty line before private: Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:51: bool PrivetDnsResponseBuilder::CheckSrv(const std::string& qname) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > Check* is not correct here > IsSame* Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:95: net::dns_protocol::kTypeSRV, On 2013/06/14 19:27:07, Vitaly Buka wrote: > does it fit one line? Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:147: iter != responses_.end(); ++iter) { On 2013/06/14 19:27:07, Vitaly Buka wrote: > align on ( Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.h (right): https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.h:34: const uint16 http_port); On 2013/06/14 19:27:07, Vitaly Buka wrote: > no need const here for http_port Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.h:54: void AppendA(const uint32 ttl) OVERRIDE; On 2013/06/14 19:27:07, Vitaly Buka wrote: > no need const here Done. https://codereview.chromium.org/16975004/diff/36001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.h:77: net::IPAddressNumber http_ip_; On 2013/06/14 19:27:07, Vitaly Buka wrote: > http_ip_ and http_port_ need const here Done.
https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:147: scoped_refptr<net::IOBufferWithSize> buffer( fix https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/gcp20_device.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:28: Bind(& https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:51: base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(StartPrinter)); same https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:41: base::StringToUint( CommandLine::ForCurrentProcess()->GetSwitchValueASCII("ttl"), &ttl); https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:53: ifaddrs* ifaddr = NULL; move both this closer where it used first time https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:57: if (getifaddrs(&ifaddr) == -1) { ifaddr -> first_address ifa -> address https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:116: AddResponse(service_name_, one line? https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:134: std::string name_in_dns_format; move inside of loop https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:181: size_ += name.size() + 2 /*two dots: first and last*/ + remove size_
All's done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:147: scoped_refptr<net::IOBufferWithSize> buffer( On 2013/06/14 23:46:52, Vitaly Buka wrote: > fix Done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/gcp20_device.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:28: On 2013/06/14 23:46:52, Vitaly Buka wrote: > Bind(& Done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/gcp20_device.cc:51: base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(StartPrinter)); On 2013/06/14 23:46:52, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:41: On 2013/06/14 23:46:52, Vitaly Buka wrote: > base::StringToUint( > CommandLine::ForCurrentProcess()->GetSwitchValueASCII("ttl"), > &ttl); Done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:53: ifaddrs* ifaddr = NULL; On 2013/06/14 23:46:52, Vitaly Buka wrote: > move both this closer where it used first time Done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:57: if (getifaddrs(&ifaddr) == -1) { On 2013/06/14 23:46:52, Vitaly Buka wrote: > ifaddr -> first_address > ifa -> address I think "ifa" means "interface". Done. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:116: AddResponse(service_name_, On 2013/06/14 23:46:52, Vitaly Buka wrote: > one line? It'll be 84 characters long. https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:134: std::string name_in_dns_format; On 2013/06/14 23:46:52, Vitaly Buka wrote: > move inside of loop http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_... https://codereview.chromium.org/16975004/diff/54001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:181: size_ += name.size() + 2 /*two dots: first and last*/ + On 2013/06/14 23:46:52, Vitaly Buka wrote: > remove size_ Done.
lgtm https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:62: for (ifaddrs* interface = first_interface; interface fits on the first line https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:116: net::dns_protocol::kTypeTXT, then proper way is AddResponse(service_name_, net::dns_protocol::kTypeTXT, ttl, txt_builder.Build()); https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:180: current_respond.begin(), Check http://www.corp.google.com/eng/doc/cppguide.xml#Function_Calls
All's done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:62: for (ifaddrs* interface = first_interface; On 2013/06/15 01:20:58, Vitaly Buka wrote: > interface fits on the first line Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/privet_dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:116: net::dns_protocol::kTypeTXT, On 2013/06/15 01:20:58, Vitaly Buka wrote: > then proper way is > > AddResponse(service_name_, net::dns_protocol::kTypeTXT, ttl, > txt_builder.Build()); Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/privet_dns_response_builder.cc:180: current_respond.begin(), On 2013/06/15 01:20:58, Vitaly Buka wrote: > Check http://www.corp.google.com/eng/doc/cppguide.xml#Function_Calls Done.
Not complete. I'll take more look at printer.* and privet_dns_response_builder.* later https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.cc:34: return record_parser_.SkipQuestion(); // instead of |cur_ = reader.ptr();| nit: it is a little bit better to read to internal variable, and only when everything is ok, copy data to the return value. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:14: DnsQueryRecord(); why do you need constructor/destructor here? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:27: DnsPacketParser(); Is there a value in having uninitialized iterator? I don't see a way to initialize it after contruction. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:48: bool SkipQuestion() { return record_parser_.SkipQuestion(); } Do you need SkipQuestion() in the public section of this class? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:54: // Parses a (possibly compressed) DNS name from the packet starting at I think you mean "decompress" or "expand" here https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:70: }; Could you please use Chrome macro here to disable copy constructors? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:28: // TODO(maksymb): This function is not reached and never used. It is callback If this should never be reached, use NOTREACHED() macro here https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:44: delete response_builder_factory_; This may crash :), if I construct and delete the instance of this class. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:131: DnsPacketParser parser(buf->data(), suggestion: since DnsPacketParser understands the whole Dns packet, it might understand header as well. You may consider moving header parsing to DnsPacketParser https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:137: bool success = reader.ReadU16(&header.id) && I am probably missing something. Where are you checking if this is a query? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:174: current_ttl = 0; That is usually a bad idea (client will treat this as no service), at least send 1. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:184: if (success) { This whole if() is asking to go to a different function that process the single query, e.g. ProcessQuery() https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:191: if ((responded = builder->IsSamePtr(query.qname))) Usually, it is property of the DnsServer to serve a particular service, while builder should be the same for everyone. Could you please change code to follow this paradigm? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:210: log = "Unknown query type"; if you want this log, please add query type as number here. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:222: return builder->Build(); Builder may return an empty buffer if it has no answers. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:25: virtual bool IsSamePtr(const std::string& qname) = 0; can IsSameXXX() functions modify members or they should be const? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:31: virtual void AppendPtr(uint32 ttl) = 0; Where do you get metadata (for example service name for PTR) for all AppendXXX() functions? https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:58: // Constructs not started server. "Constructs not started server." -> "Constructor does not start server." https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:66: bool Start(DnsResponseBuilderFactoryInterface* response_builder_factory, Usually factory is a singleton type of object. Unless you see different ways to build DnsResponse https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:89: DnsPacketParser* parser) const; I am confused what is parser doing in the CreateResponsePacket() funtion, but I'll check implementation. Also, you have BuildAnnouncement(). I believe they are doing similar things, could you please call them both CreateXXX or BuildXXX https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_txt_builder.cc (right): https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_txt_builder.cc:20: record += '\0'; // Allocate space for length byte. it looks like you can add length directly here: int len = strlen(str); DCHECK_LT(len, 256); record += static_cast<char>(len); https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:16: const char* kServiceDomainName = "my.privet.device.local"; "my.privet.device.local" -> "my-privet-device.local" https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.h (right): https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.h:32: }; dissallow copy constructor macro here?
All's done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.cc:34: return record_parser_.SkipQuestion(); // instead of |cur_ = reader.ptr();| On 2013/06/15 02:13:23, gene wrote: > nit: it is a little bit better to read to internal variable, and only when > everything is ok, copy data to the return value. Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:14: DnsQueryRecord(); On 2013/06/15 02:13:23, gene wrote: > why do you need constructor/destructor here? constructor now initializes |qtype| and |qclass| values with zeros. destructor was deleted. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:27: DnsPacketParser(); On 2013/06/15 02:13:23, gene wrote: > Is there a value in having uninitialized iterator? > I don't see a way to initialize it after contruction. No reason. Deleted. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:48: bool SkipQuestion() { return record_parser_.SkipQuestion(); } On 2013/06/15 02:13:23, gene wrote: > Do you need SkipQuestion() in the public section of this class? Deleted. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:54: // Parses a (possibly compressed) DNS name from the packet starting at On 2013/06/15 02:13:23, gene wrote: > I think you mean "decompress" or "expand" here Left as is. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:70: }; On 2013/06/15 02:13:23, gene wrote: > Could you please use Chrome macro here to disable copy constructors? Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:28: // TODO(maksymb): This function is not reached and never used. It is callback On 2013/06/15 02:13:23, gene wrote: > If this should never be reached, use NOTREACHED() macro here Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:44: delete response_builder_factory_; On 2013/06/15 02:13:23, gene wrote: > This may crash :), if I construct and delete the instance of this class. Factory was deleted. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:131: DnsPacketParser parser(buf->data(), On 2013/06/15 02:13:23, gene wrote: > suggestion: since DnsPacketParser understands the whole Dns packet, it might > understand header as well. You may consider moving header parsing to > DnsPacketParser Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:137: bool success = reader.ReadU16(&header.id) && On 2013/06/15 02:13:23, gene wrote: > I am probably missing something. Where are you checking if this is a query? Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:174: current_ttl = 0; On 2013/06/15 02:13:23, gene wrote: > That is usually a bad idea (client will treat this as no service), at least send > 1. Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:184: if (success) { On 2013/06/15 02:13:23, gene wrote: > This whole if() is asking to go to a different function that process the single > query, e.g. ProcessQuery() Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:191: if ((responded = builder->IsSamePtr(query.qname))) On 2013/06/15 02:13:23, gene wrote: > Usually, it is property of the DnsServer to serve a particular service, while > builder should be the same for everyone. > Could you please change code to follow this paradigm? Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:210: log = "Unknown query type"; On 2013/06/15 02:13:23, gene wrote: > if you want this log, please add query type as number here. Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:222: return builder->Build(); On 2013/06/15 02:13:23, gene wrote: > Builder may return an empty buffer if it has no answers. Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:25: virtual bool IsSamePtr(const std::string& qname) = 0; On 2013/06/15 02:13:23, gene wrote: > can IsSameXXX() functions modify members or they should be const? Deleted functions at all. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:31: virtual void AppendPtr(uint32 ttl) = 0; On 2013/06/15 02:13:23, gene wrote: > Where do you get metadata (for example service name for PTR) for all AppendXXX() > functions? Now it is in parameters. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:58: // Constructs not started server. On 2013/06/15 02:13:23, gene wrote: > "Constructs not started server." -> "Constructor does not start server." Done. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:66: bool Start(DnsResponseBuilderFactoryInterface* response_builder_factory, On 2013/06/15 02:13:23, gene wrote: > Usually factory is a singleton type of object. Unless you see different ways to > build DnsResponse Deleted. https://codereview.chromium.org/16975004/diff/67001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:89: DnsPacketParser* parser) const; On 2013/06/15 02:13:23, gene wrote: > I am confused what is parser doing in the CreateResponsePacket() funtion, but > I'll check implementation. > > Also, you have BuildAnnouncement(). I believe they are doing similar things, > could you please call them both CreateXXX or BuildXXX Method was deleted. https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_txt_builder.cc (right): https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_txt_builder.cc:20: record += '\0'; // Allocate space for length byte. On 2013/06/15 02:13:23, gene wrote: > it looks like you can add length directly here: > int len = strlen(str); > DCHECK_LT(len, 256); > record += static_cast<char>(len); Sure. This strange code appeared because of double rewriting parts of it :) https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:16: const char* kServiceDomainName = "my.privet.device.local"; On 2013/06/15 02:13:23, gene wrote: > "my.privet.device.local" -> "my-privet-device.local" Done. https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.h (right): https://codereview.chromium.org/16975004/diff/76001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.h:32: }; On 2013/06/15 02:13:23, gene wrote: > dissallow copy constructor macro here? Done.
https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:39: const net::dns_protocol::Header& header() { return header_; } const? https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.cc:111: uint32 ttl, const std::string& rdata) { It looks through the code, it might be better to pass "void*, int size" instead of "const std:string& rdata" here. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_response_builder.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.h:27: void AppendPtr(uint32 ttl, const ServiceParameters& serv_params); I would suggest having record specific input params here, something like this: AppendPtr(const string& service_type, uint32 ttl, const string& service_name); AppendSrv(const string& service_name, uint32 ttl, const string& domain, int port, int weight, int priority); AppendA(const string& domain, uint32 ttl, int ip_v4); AppendTxt(const string& service_name, uint32 ttl, const vector<string>& metadata); https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.h:37: scoped_refptr<net::IOBufferWithSize> BuildAnnouncement(uint32 ttl); const? https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.h:46: std::vector<uint8> responses_; Usually it is easier to collect all records in something like vector<DnsRecord>, and only during the Build() call, construct the buffer and return it. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:198: if ((responded = (query.qname == serv_params_.service_type_))) it will be easier to read: if (query.qname == serv_params_.service_type_) if builder has a function to give you a number of replies, you can check if query was processed https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:267: kTimeToNextAnnouncement*full_ttl_))); spaces around * https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:31: uint32 full_ttl, const std::string& txt_data) WARN_UNUSED_RESULT; TXT data will be easier to represent as vector<std::string>, and you can call it metadata https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:43: void UpdateTxtData(const std::string& txt_data); UpdateMetadata ? https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_txt_builder.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_txt_builder.h:13: class DnsTxtBuilder { Instead of TxtBuilder, you can just make Dns builder to accept vector<std::string> https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:31: if (http_port_tmp > kuint32max) { I think max port number in TCP and UDP is 64k https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/service_parameters.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/service_parameters.h:14: ServiceParameters(); do you need a default constructor?
https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_packet_parser.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_packet_parser.h:39: const net::dns_protocol::Header& header() { return header_; } On 2013/06/19 05:40:51, gene wrote: > const? Done. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.cc:111: uint32 ttl, const std::string& rdata) { On 2013/06/19 05:40:51, gene wrote: > It looks through the code, it might be better to pass "void*, int size" instead > of "const std:string& rdata" here. Done. But still I store rdata as std::vector<uint8> in DnsRespondRecord. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_response_builder.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.h:27: void AppendPtr(uint32 ttl, const ServiceParameters& serv_params); On 2013/06/19 05:40:51, gene wrote: > I would suggest having record specific input params here, something like this: > AppendPtr(const string& service_type, uint32 ttl, const string& service_name); > AppendSrv(const string& service_name, uint32 ttl, const string& domain, int > port, int weight, int priority); > AppendA(const string& domain, uint32 ttl, int ip_v4); > AppendTxt(const string& service_name, uint32 ttl, const vector<string>& > metadata); Done. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.h:37: scoped_refptr<net::IOBufferWithSize> BuildAnnouncement(uint32 ttl); On 2013/06/19 05:40:51, gene wrote: > const? Sorry, I forgot to delete declaration. This method currently is deleted. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_response_builder.h:46: std::vector<uint8> responses_; On 2013/06/19 05:40:51, gene wrote: > Usually it is easier to collect all records in something like vector<DnsRecord>, > and only during the Build() call, construct the buffer and return it. Done. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:198: if ((responded = (query.qname == serv_params_.service_type_))) On 2013/06/19 05:40:51, gene wrote: > it will be easier to read: > if (query.qname == serv_params_.service_type_) > > if builder has a function to give you a number of replies, you can check if > query was processed I don't have this method. I hope this will be more clear: if (query.qname == serv_params_.service_type_) { builder->AppendPtr(current_ttl, serv_params_); responded = true; } https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:267: kTimeToNextAnnouncement*full_ttl_))); On 2013/06/19 05:40:51, gene wrote: > spaces around * http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo... v = w * x + y / z; // Binary operators usually have spaces around them, v = w*x + y/z; // but it's okay to remove spaces around factors. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:31: uint32 full_ttl, const std::string& txt_data) WARN_UNUSED_RESULT; On 2013/06/19 05:40:51, gene wrote: > TXT data will be easier to represent as vector<std::string>, and you can call it > metadata Done. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.h:43: void UpdateTxtData(const std::string& txt_data); On 2013/06/19 05:40:51, gene wrote: > UpdateMetadata ? Done. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_txt_builder.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_txt_builder.h:13: class DnsTxtBuilder { On 2013/06/19 05:40:51, gene wrote: > Instead of TxtBuilder, you can just make Dns builder to accept > vector<std::string> Done. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/printer.cc:31: if (http_port_tmp > kuint32max) { On 2013/06/19 05:40:51, gene wrote: > I think max port number in TCP and UDP is 64k Yes, this is right. Fixed. https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/service_parameters.h (right): https://codereview.chromium.org/16975004/diff/68014/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/service_parameters.h:14: ServiceParameters(); On 2013/06/19 05:40:51, gene wrote: > do you need a default constructor? Yes, DnsSdServer receives info about service only when Start() is called. So, I have to initialize ServiceParameters with some values when DnsSdServer constructor is called.
https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:56: scoped_ptr<char[]> rdata(new char[rdata_size]); please use vector<char> rdata(rdata_size, 0);
Reverted string as parameter. Please take a look. https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:56: scoped_ptr<char[]> rdata(new char[rdata_size]); On 2013/06/19 23:03:21, Vitaly Buka wrote: > please use vector<char> rdata(rdata_size, 0); Done.
almost done :) some comments below: https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_packet_parser.cc:10: size_t length) will it fit one line? https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:38: bool success = net::DNSDomainFromDot(service_name, will fit on one line https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.h (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.h:26: std::vector<uint8> rdata; // points to the original response buffer is comment still valid here? https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.h:47: const std::vector<std::string>& txt); txt -> metadata? https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:104: you can add header_.ancount = responses_.size(); here https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:119: DCHECK(success); I would put DCHECK(name_in_dns_format.size() == iter->name.size() + 2) check here, to make sure you size accumulation logic works. https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:138: ++header_.ancount; you can remove this, and use responses_.size() instead. https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:41: : is_online_(false), instead of is_online_ you can check socket_ pointer, and reset it in shutdown https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/service_parameters.h (right): https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/service_parameters.h:14: ServiceParameters(); do you need default constructor here?
All's done. https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_packet_parser.cc (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_packet_parser.cc:10: size_t length) On 2013/06/20 00:56:52, gene wrote: > will it fit one line? Done. https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:38: bool success = net::DNSDomainFromDot(service_name, On 2013/06/20 00:56:52, gene wrote: > will fit on one line Done. https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.h (right): https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.h:26: std::vector<uint8> rdata; // points to the original response buffer On 2013/06/20 00:56:52, gene wrote: > is comment still valid here? It was deleted at patch 14. https://codereview.chromium.org/16975004/diff/100001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.h:47: const std::vector<std::string>& txt); On 2013/06/20 00:56:52, gene wrote: > txt -> metadata? Done. https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_response_builder.cc (right): https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:104: On 2013/06/20 00:56:52, gene wrote: > you can add header_.ancount = responses_.size(); here Done. https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:119: DCHECK(success); On 2013/06/20 00:56:52, gene wrote: > I would put DCHECK(name_in_dns_format.size() == iter->name.size() + 2) check > here, to make sure you size accumulation logic works. Done. https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_response_builder.cc:138: ++header_.ancount; On 2013/06/20 00:56:52, gene wrote: > you can remove this, and use responses_.size() instead. Done. https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/119002/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:41: : is_online_(false), On 2013/06/20 00:56:52, gene wrote: > instead of is_online_ you can check socket_ pointer, and reset it in shutdown Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/125001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/125001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/147001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/86002
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/170001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
lgtm https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:51: if (socket_.get() != NULL) if (socket_) or IsOnline() https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:73: void DnsSdServer::Update() { same https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:81: if (socket_.get() == NULL) same https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:91: if (socket_.get() == NULL) same https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:221: serv_params_.http_ipv4_); IP must be different depending on source of query https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:267: serv_params_.http_ipv4_); same https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.h:15: #include "net/udp/udp_socket.h" put forward declaration of net::UDPSocket here and move #include "net/udp/udp_socket.h" to cc https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.h:40: bool IsOnline() { return socket_.get() != NULL; } return socket_; https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/printer.cc:57: bool return_ipv6_number) { Why just one IP? what about the rest
https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:51: if (socket_.get() != NULL) On 2013/06/21 01:03:39, Vitaly Buka wrote: > if (socket_) or IsOnline() Done. https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:73: void DnsSdServer::Update() { On 2013/06/21 01:03:39, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:81: if (socket_.get() == NULL) On 2013/06/21 01:03:39, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:91: if (socket_.get() == NULL) On 2013/06/21 01:03:39, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:221: serv_params_.http_ipv4_); On 2013/06/21 01:03:39, Vitaly Buka wrote: > IP must be different depending on source of query TODO added https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.cc:267: serv_params_.http_ipv4_); On 2013/06/21 01:03:39, Vitaly Buka wrote: > same TODO added https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/dns_sd_server.h (right): https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.h:15: #include "net/udp/udp_socket.h" On 2013/06/21 01:03:39, Vitaly Buka wrote: > put forward declaration of net::UDPSocket here and move > #include "net/udp/udp_socket.h" to cc Done. https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/dns_sd_server.h:40: bool IsOnline() { return socket_.get() != NULL; } On 2013/06/21 01:03:39, Vitaly Buka wrote: > return socket_; Done. https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... File cloud_print/gcp20/prototype/printer.cc (right): https://codereview.chromium.org/16975004/diff/186001/cloud_print/gcp20/protot... cloud_print/gcp20/prototype/printer.cc:57: bool return_ipv6_number) { On 2013/06/21 01:03:39, Vitaly Buka wrote: > Why just one IP? what about the rest TODO added.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/193001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/207001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/207001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/207001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/207001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16975004/207001
Message was sent while issue was closed.
Change committed as 208064 |
