Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16389002:
GCP2.0 Device: mDNS basics (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. |
DescriptionGCP2.0 Device: mDNS basics
Socket binding and sending empty announcements
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204754
Patch Set 1 #
Total comments: 18
Patch Set 2 : Replaced dns_io_routines with BigEndianWriter. Corrected TTL variables type. #Patch Set 3 : Added a lot of comments. #Patch Set 4 : #
Total comments: 12
Patch Set 5 : #Patch Set 6 : Checked Lint errors. #Patch Set 7 : Added trailing newlines. #
Total comments: 8
Patch Set 8 : Corrected "Binds". #
Total comments: 14
Patch Set 9 : Renamed DnsSd to DnsSdServer and cleared response-making functionality (will be added later) #Patch Set 10 : Corrected file rename issues #
Total comments: 2
Patch Set 11 : Deleted AppendXXX() and UpdateHeader(). #
Total comments: 2
Patch Set 12 : Checked nits. #
Total comments: 4
Patch Set 13 : Removed defines and unnecessary dependencies from hyp file. #Patch Set 14 : #
Messages
Total messages: 28 (0 generated)
Please take a look on my CL
https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_io_routines.cc (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_io_routines.cc:15: PutUint8(buf, (val >> 8) & 0xFF); Please use BigEndianWriter instead of this files, see src\net\dns\dns_query.cc https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_io_routines.h (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_io_routines.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. We don't put "(c)" in new files. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_io_routines.h:10: class DnsIoRoutines { Can you please add some docs?
https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_sd.cc (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.cc:14: put all consts in anonymous namespace { ... } // namespace https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:10: class DnsSd { comments https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:18: bool IsOnline() { return online_; } is_online() https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:26: void AppendSrv(std::vector<uint8>* answer, uint16 ttl); output args should go after input args https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:31: bool online_; is_online_ https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:32: net::UDPSocket* socket_; scoped_ptr<net::UDPSocket>
Done all comments. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_io_routines.cc (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_io_routines.cc:15: PutUint8(buf, (val >> 8) & 0xFF); On 2013/06/05 20:55:17, Vitaly Buka corp wrote: > Please use BigEndianWriter instead of this files, see src\net\dns\dns_query.cc Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_io_routines.h (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_io_routines.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/06/05 20:55:17, Vitaly Buka corp wrote: > We don't put "(c)" in new files. Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_io_routines.h:10: class DnsIoRoutines { On 2013/06/05 20:55:17, Vitaly Buka corp wrote: > Can you please add some docs? Done (Now it does not matter for this deleted file) https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_sd.cc (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.cc:14: On 2013/06/05 21:01:39, Vitaly Buka corp wrote: > put all consts in anonymous > > > namespace { > ... > > } // namespace Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:10: class DnsSd { On 2013/06/05 21:01:39, Vitaly Buka corp wrote: > comments Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:18: bool IsOnline() { return online_; } On 2013/06/05 21:01:39, Vitaly Buka corp wrote: > is_online() Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:26: void AppendSrv(std::vector<uint8>* answer, uint16 ttl); On 2013/06/05 21:01:39, Vitaly Buka corp wrote: > output args should go after input args Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:31: bool online_; On 2013/06/05 21:01:39, Vitaly Buka corp wrote: > is_online_ Done. https://codereview.chromium.org/16389002/diff/1/cloud_print/gcp20/prototype/d... cloud_print/gcp20/prototype/dns_sd.h:32: net::UDPSocket* socket_; On 2013/06/05 21:01:39, Vitaly Buka corp wrote: > scoped_ptr<net::UDPSocket> Done.
Please also fix all Lint error. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.cc (right): https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:158: bool success = Put the first writer. on the same line as = and align the rest on it https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:10: // Class for sending multicast announcements, receiving queries andanswering on andanswering? https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:11: // them. Somebody should call |ProccessMessages| periodically to make server Somebody -> Client https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:13: No need empty line here https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:22: // Tries to start the server. Returns |true| if server works. Also sends "Tries to start" -> "Starts" https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:36: // Tries to bind a socket to multicast address. Remove Tries
Checked Lint errors and done all comments. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.cc (right): https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:158: bool success = On 2013/06/05 23:08:11, Vitaly Buka corp wrote: > Put the first writer. on the same line as = and align the rest on it Done. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:10: // Class for sending multicast announcements, receiving queries andanswering on On 2013/06/05 23:08:11, Vitaly Buka corp wrote: > andanswering? Done. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:11: // them. Somebody should call |ProccessMessages| periodically to make server On 2013/06/05 23:08:11, Vitaly Buka corp wrote: > Somebody -> Client Done. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:13: On 2013/06/05 23:08:11, Vitaly Buka corp wrote: > No need empty line here Done. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:22: // Tries to start the server. Returns |true| if server works. Also sends On 2013/06/05 23:08:11, Vitaly Buka corp wrote: > "Tries to start" -> "Starts" Done. https://codereview.chromium.org/16389002/diff/13001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:36: // Tries to bind a socket to multicast address. On 2013/06/05 23:08:11, Vitaly Buka corp wrote: > Remove Tries Done.
lgtm gene? https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:36: // Bind a socket to multicast address. Returns |true| on success. Binds
give me a few minutes, I'll take a look On Wed, Jun 5, 2013 at 4:52 PM, <vitalybuka@google.com> wrote: > lgtm > > gene? > > > https://codereview.chromium.**org/16389002/diff/25001/cloud_** > print/gcp20/prototype/dns_sd.h<https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototype/dns_sd.h> > File cloud_print/gcp20/prototype/**dns_sd.h (right): > > https://codereview.chromium.**org/16389002/diff/25001/cloud_** > print/gcp20/prototype/dns_sd.**h#newcode36<https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototype/dns_sd.h#newcode36> > cloud_print/gcp20/prototype/**dns_sd.h:36: // Bind a socket to multicast > address. Returns |true| on success. > Binds > > https://codereview.chromium.**org/16389002/<https://codereview.chromium.org/1... >
Correct "Binds": Done. https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:36: // Bind a socket to multicast address. Returns |true| on success. On 2013/06/05 23:52:12, Vitaly Buka corp wrote: > Binds Done.
nothing serious, just a few nits below: https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:14: class DnsSd { DnsSd -> DnsSdServer ? https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:70: extra empty line https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:78: extra empty line https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.cc (right): https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:64: bool DnsSd::BindSocket() { BindSocket -> CreateSocket ? https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:117: question_count_ = 0; I think it is better not to make question_count_ and answers_count_ as members. Since you have not implement any AppendXXX functions, I suggest you remove them from the class declaration. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:124: UpdateHeader(false/*no question*/, message.get()); It is better to create header here, or pass all parameters necessary to create header in the message https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:126: extra empty line https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:169: // write code "// write code" -> "NOTIMPLEMENTED(); // implement this" https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:13: // them. Client should call |ProccessMessages| periodically to make server work. Add a ProcessMessage() function, make it NOTIMPLEMENTED()? https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:77: }; add chrome macro to disable copy contructor
Renamed and checked mistakes. Created dump for DNS header. https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:14: class DnsSd { On 2013/06/06 00:13:51, gene wrote: > DnsSd -> DnsSdServer ? Done. https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:70: On 2013/06/06 00:13:51, gene wrote: > extra empty line Done. https://codereview.chromium.org/16389002/diff/25001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:78: On 2013/06/06 00:13:51, gene wrote: > extra empty line Done. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.cc (right): https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:64: bool DnsSd::BindSocket() { On 2013/06/06 00:13:51, gene wrote: > BindSocket -> CreateSocket ? Done. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:117: question_count_ = 0; On 2013/06/06 00:13:51, gene wrote: > I think it is better not to make question_count_ and answers_count_ as members. > Since you have not implement any AppendXXX functions, I suggest you remove them > from the class declaration. Done. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:124: UpdateHeader(false/*no question*/, message.get()); On 2013/06/06 00:13:51, gene wrote: > It is better to create header here, or pass all parameters necessary to create > header in the message Cleared. Created temporary dump. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:126: On 2013/06/06 00:13:51, gene wrote: > extra empty line Done. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.cc:169: // write code On 2013/06/06 00:13:51, gene wrote: > "// write code" -> "NOTIMPLEMENTED(); // implement this" Done. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd.h (right): https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:13: // them. Client should call |ProccessMessages| periodically to make server work. On 2013/06/06 00:13:51, gene wrote: > Add a ProcessMessage() function, make it NOTIMPLEMENTED()? Done. https://codereview.chromium.org/16389002/diff/32001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd.h:77: }; On 2013/06/06 00:13:51, gene wrote: > add chrome macro to disable copy contructor Do you mean DISALLOW_COPY_AND_ASSIGN? Then done.
lgtm a couple of nits below https://codereview.chromium.org/16389002/diff/39001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16389002/diff/39001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:24: DnsSdServer::DnsSdServer(): is_online_(false) { nit: please add space between () and : https://codereview.chromium.org/16389002/diff/46001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16389002/diff/46001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:121: // TODO(maksymb): Create and implement DnsResponce class DnsResponce -> DnsResponse
All is done. https://codereview.chromium.org/16389002/diff/39001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16389002/diff/39001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:24: DnsSdServer::DnsSdServer(): is_online_(false) { On 2013/06/06 02:41:20, gene wrote: > nit: please add space between () and : Done. https://codereview.chromium.org/16389002/diff/46001/cloud_print/gcp20/prototy... File cloud_print/gcp20/prototype/dns_sd_server.cc (right): https://codereview.chromium.org/16389002/diff/46001/cloud_print/gcp20/prototy... cloud_print/gcp20/prototype/dns_sd_server.cc:121: // TODO(maksymb): Create and implement DnsResponce class On 2013/06/06 02:41:20, gene wrote: > DnsResponce -> DnsResponse Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16389002/50001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&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/16389002/50001
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/16389002/50001
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/16389002/50001
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/16389002/50001
https://chromiumcodereview.appspot.com/16389002/diff/50001/cloud_print/gcp20/... File cloud_print/gcp20/prototype/gcp20_device.gyp (right): https://chromiumcodereview.appspot.com/16389002/diff/50001/cloud_print/gcp20/... cloud_print/gcp20/prototype/gcp20_device.gyp:20: '_ATL_APARTMENT_THREADED', try to remove all defines https://chromiumcodereview.appspot.com/16389002/diff/50001/cloud_print/gcp20/... cloud_print/gcp20/prototype/gcp20_device.gyp:47: 'UACExecutionLevel': '2', # /level='requireAdministrator' -UACExecutionLevel
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.
Checked. https://chromiumcodereview.appspot.com/16389002/diff/50001/cloud_print/gcp20/... File cloud_print/gcp20/prototype/gcp20_device.gyp (right): https://chromiumcodereview.appspot.com/16389002/diff/50001/cloud_print/gcp20/... cloud_print/gcp20/prototype/gcp20_device.gyp:20: '_ATL_APARTMENT_THREADED', On 2013/06/06 21:40:40, Vitaly Buka corp wrote: > try to remove all defines Done. https://chromiumcodereview.appspot.com/16389002/diff/50001/cloud_print/gcp20/... cloud_print/gcp20/prototype/gcp20_device.gyp:47: 'UACExecutionLevel': '2', # /level='requireAdministrator' On 2013/06/06 21:40:40, Vitaly Buka corp wrote: > -UACExecutionLevel Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16389002/76002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maksymb@chromium.org/16389002/101001
Message was sent while issue was closed.
Change committed as 204754 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
