Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16272006:
In-browser DNS-based service discovery system (Closed)
|
Created:
7 years, 6 months ago by Noam Samuel Modified:
7 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@mdns_implementation Visibility:
Public. |
DescriptionThis is a DNS-based service discovery system, implemented to work in the browser process behind a flag.
This contains two classes. The first, ServiceTypeWatcher, lets users observe services of a specific type and query the network for them. The second, ServiceReader, lets the user receive more in-depth data about the service itself.
BUG=249896
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207583
Patch Set 1 : #
Total comments: 27
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #
Total comments: 33
Patch Set 4 : #
Total comments: 17
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #
Total comments: 1
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client.cc (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.cc:13: g_instance = new ServiceDiscoveryClientImpl(); maybe return g_instance ? g_instance : Singleton<ServiceDiscoveryClient>::get(); https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:41: std::vector<std::string>* services) const = 0; one line? https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:83: MetadataCallback; Maybe move all callback into delegate? https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:120: bool force_refresh) = 0; missaligned https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:52: if (!listener_->Start()) return false; Chromium coding styles requires line break after condition. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:83: if (active) transaction_flags |= net::MDnsTransaction::QUERY_NETWORK; same https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:186: if (!srv_listener_->Start()) return false; again https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:224: AsWeakPtr())); don't need to break bind line https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:267: if (first_period == std::string::npos) return ""; more https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:317: address_ = net::HostPortPair(); same question as for has_metadata_ below https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:331: has_metadata_ = false; can you just use metadata_.empty() instead of has_metadata_ https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:340: metadata_); one line https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:369: for (std::vector<ServiceReader::AddressCallback>::iterator i = SrvRecordTransactionResponse and TxtRecordTransactionResponse are very similar https://codereview.chromium.org/16272006/diff/7001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/16272006/diff/7001/chrome/chrome_browser.gypi... chrome/chrome_browser.gypi:3132: 'browser/local_discovery/service_discovery_client.cc', Why do you need this? we have run time flag
https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:83: MetadataCallback; On 2013/06/15 00:22:30, Vitaly Buka wrote: > Maybe move all callback into delegate? Hm. It would definitely provide some simplicity. The concept behind using callbacks for these is to make them similar to other asynchronous data requests, whereas the delegate signifies changes in state. Not sure if the distinction is that useful, though. I could add On*RequestComplete separate from the On*Changed functions in the delegate, which would be called whenever a request is finished rather than being called whenever the value changes, or I could try merging the two which might be messy from the API user's side. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:331: has_metadata_ = false; On 2013/06/15 00:22:30, Vitaly Buka wrote: > can you just use metadata_.empty() instead of has_metadata_ I think so. Technically, a service could have empty metadata, but I don't see the distinction between having empty metadata and no metadata at all ever being important. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:369: for (std::vector<ServiceReader::AddressCallback>::iterator i = On 2013/06/15 00:22:30, Vitaly Buka wrote: > SrvRecordTransactionResponse and TxtRecordTransactionResponse are very similar Since they contain slightly different logic and types, I'm not sure how to merge them short of using templates, which seems like overkill for this case.
https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:21: class ServiceTypeWatcher { May be just a ServiceWatcher, instead of ServiceTypeWatcher ? https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:44: virtual void DiscoverNewServices() = 0; Can we have a parameter here to force discover, instead of ForceUpdateServices()? https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:54: virtual bool IsActive() const = 0; In what cases do we need an active watcher? https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:59: class ServiceReader { I usually saw name Resolve for this type of operation in other DNS-SD libraries https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:67: class Delegate { This is a bit confusing. It looks like this class is both resolver and watcher. Will it be better to separate those 2? https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:103: virtual bool GetMetadata(std::vector<std::string>* metadata) const = 0; We have Get and Read functionality for metadata and address. If we have a separate process for mdns/dns-sd, will we still be able to implement synchronous access to cache?
Hey, I've refactored the interface considerably, so now the following is true: * ServiceTypeWatcher has been renamed to ServiceWatcher, and its functionality has expanded to include notifying its user of whether a service's SRV or TXT record have been updated. * ServiceReader has been replaced by ServiceResolver, a class that resolves a service to a Service object, which contains its current address, metadata, and (optionally) address. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:41: std::vector<std::string>* services) const = 0; On 2013/06/15 00:22:30, Vitaly Buka wrote: > one line? Doesn't fit. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:83: MetadataCallback; Made irrelevant by refactor. On 2013/06/15 00:42:06, Noam Samuel wrote: > On 2013/06/15 00:22:30, Vitaly Buka wrote: > > Maybe move all callback into delegate? > > Hm. It would definitely provide some simplicity. The concept behind using > callbacks for these is to make them similar to other asynchronous data requests, > whereas the delegate signifies changes in state. Not sure if the distinction is > that useful, though. > > I could add On*RequestComplete separate from the On*Changed functions in the > delegate, which would be called whenever a request is finished rather than being > called whenever the value changes, or I could try merging the two which might be > messy from the API user's side. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:120: bool force_refresh) = 0; On 2013/06/15 00:22:30, Vitaly Buka wrote: > missaligned Code removed. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:52: if (!listener_->Start()) return false; On 2013/06/15 00:22:30, Vitaly Buka wrote: > Chromium coding styles requires line break after condition. Done. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:83: if (active) transaction_flags |= net::MDnsTransaction::QUERY_NETWORK; On 2013/06/15 00:22:30, Vitaly Buka wrote: > same Done. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:186: if (!srv_listener_->Start()) return false; On 2013/06/15 00:22:30, Vitaly Buka wrote: > again Done. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:224: AsWeakPtr())); On 2013/06/15 00:22:30, Vitaly Buka wrote: > don't need to break bind line It doesn't fit by a couple of characters (at least, it doesn't now that it's been renamed from Reader to Resolver). https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:267: if (first_period == std::string::npos) return ""; On 2013/06/15 00:22:30, Vitaly Buka wrote: > more Done. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:317: address_ = net::HostPortPair(); On 2013/06/15 00:22:30, Vitaly Buka wrote: > same question as for has_metadata_ below Code path removed. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:340: metadata_); On 2013/06/15 00:22:30, Vitaly Buka wrote: > one line Deleted. https://codereview.chromium.org/16272006/diff/2001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client_impl.cc:369: for (std::vector<ServiceReader::AddressCallback>::iterator i = On 2013/06/15 00:42:06, Noam Samuel wrote: > On 2013/06/15 00:22:30, Vitaly Buka wrote: > > SrvRecordTransactionResponse and TxtRecordTransactionResponse are very similar > > Since they contain slightly different logic and types, I'm not sure how to merge > them short of using templates, which seems like overkill for this case. Deleted. https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:21: class ServiceTypeWatcher { On 2013/06/15 00:58:19, gene wrote: > May be just a ServiceWatcher, instead of ServiceTypeWatcher ? Done. https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:44: virtual void DiscoverNewServices() = 0; On 2013/06/15 00:58:19, gene wrote: > Can we have a parameter here to force discover, instead of > ForceUpdateServices()? Done. https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:54: virtual bool IsActive() const = 0; On 2013/06/15 00:58:19, gene wrote: > In what cases do we need an active watcher? Removed. https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:59: class ServiceReader { On 2013/06/15 00:58:19, gene wrote: > I usually saw name Resolve for this type of operation in other DNS-SD libraries Done. https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:67: class Delegate { On 2013/06/15 00:58:19, gene wrote: > This is a bit confusing. It looks like this class is both resolver and watcher. > Will it be better to separate those 2? Done. https://codereview.chromium.org/16272006/diff/7001/chrome/browser/local_disco... chrome/browser/local_discovery/service_discovery_client.h:103: virtual bool GetMetadata(std::vector<std::string>* metadata) const = 0; On 2013/06/15 00:58:19, gene wrote: > We have Get and Read functionality for metadata and address. If we have a > separate process for mdns/dns-sd, will we still be able to implement synchronous > access to cache? Removed. https://codereview.chromium.org/16272006/diff/7001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/16272006/diff/7001/chrome/chrome_browser.gypi... chrome/chrome_browser.gypi:3132: 'browser/local_discovery/service_discovery_client.cc', On 2013/06/15 00:22:30, Vitaly Buka wrote: > Why do you need this? we have run time flag Changed to enable_mdns (since we depend on enable_mdns being true).
lgtm https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:19: struct Service { Service -> ServiceDescription ? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:76: if (force_update) services_.clear(); please break if https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:98: std::map<std::string, ServiceListeners*> services_; Please define typedef for td::map<std::string, ServiceListeners*> and use everywhere https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_unittest.cc (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_unittest.cc:261: ServiceDiscoveryTest() : socket_factory_(new MockDatagramSocketFactory), maybe break before :
https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:24: // The address (in host/port format) for the service (from SRV record). Do you need service name as a part of this class? Also, it might be nice to have function here to get service type. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:47: virtual void OnServiceChanged(const std::string& service_name) = 0; Do you need to pass Service/ServiceDescription object here? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:81: // A callback for recieving the last time the PTR record has been seen. Do you mean SRV record here? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:82: typedef base::Callback<void(RequestStatus)> ResolveCompleteCallback; Do you need to pass Service/ServiceDescription here in callback? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:109: // service type |service_type|. If a service type watcher is |active|, you can remove part about active listener here https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:136: DCHECK(services_.find(record->name()) != services_.end()); PTR may easily arrive after SRV, will it cause problems here? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:153: DCHECK(rdata); Do you want to have DCHECK() here, or if() and politely handle wrong response? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:178: return txt_listener_->Start(); Is there are problems if src listener will start and txt don't? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:183: std::pair<std::map<std::string, ServiceListeners*>::iterator, bool> found = consider typedef std::map<std::string, ServiceListeners*> to simplify this line https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:184: services_.insert(make_pair(service, services_.find() can make this code a bit more readable https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:187: found.first->second = new ServiceListeners(service, this); Do you want to keep found.first->second as a scoped_ptr vs. ServiceListeners* ? https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:32: // Create a service object listening for DNS-SD service announcements on Comment is for a listener here.
https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:19: struct Service { On 2013/06/18 23:01:46, Vitaly Buka wrote: > Service -> ServiceDescription ? Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:24: // The address (in host/port format) for the service (from SRV record). On 2013/06/18 23:46:53, gene wrote: > Do you need service name as a part of this class? > Also, it might be nice to have function here to get service type. Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:47: virtual void OnServiceChanged(const std::string& service_name) = 0; On 2013/06/18 23:46:53, gene wrote: > Do you need to pass Service/ServiceDescription object here? Users will need to resolve the service separately using their own service resolver. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:81: // A callback for recieving the last time the PTR record has been seen. On 2013/06/18 23:46:53, gene wrote: > Do you mean SRV record here? Whoops. This is completely the wrong comment. That's actually the callback for when the resolve is finished. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:82: typedef base::Callback<void(RequestStatus)> ResolveCompleteCallback; On 2013/06/18 23:46:53, gene wrote: > Do you need to pass Service/ServiceDescription here in callback? Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:109: // service type |service_type|. If a service type watcher is |active|, On 2013/06/18 23:46:53, gene wrote: > you can remove part about active listener here Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:76: if (force_update) services_.clear(); On 2013/06/18 23:01:46, Vitaly Buka wrote: > please break if Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:136: DCHECK(services_.find(record->name()) != services_.end()); On 2013/06/18 23:46:53, gene wrote: > PTR may easily arrive after SRV, will it cause problems here? The service won't be notified as "updated" if that's the case since there will be no listener for the SRV record when it arrives. I might posit it's a problem if SRV arrives immediately after PTR and causes a spurious update notification. It's not a huge problem, though, so I'm OK deferring it to another CL. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:153: DCHECK(rdata); On 2013/06/18 23:46:53, gene wrote: > Do you want to have DCHECK() here, or if() and politely handle wrong response? Since the only transaction we have is a PTR record transaction, and the MDns layer should be passing only PTR records as responses, I'd posit a DCHECK is perfectly legitimate here. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:178: return txt_listener_->Start(); On 2013/06/18 23:46:53, gene wrote: > Is there are problems if src listener will start and txt don't? It seems unlikely that would happen. If it does, the service watcher will not deliver change notifications for changes in the TXT records of that service. Ideally we should tunnel this error upwards. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:183: std::pair<std::map<std::string, ServiceListeners*>::iterator, bool> found = On 2013/06/18 23:46:53, gene wrote: > consider typedef std::map<std::string, ServiceListeners*> to simplify this line Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:184: services_.insert(make_pair(service, On 2013/06/18 23:46:53, gene wrote: > services_.find() can make this code a bit more readable Services_.insert() will do one lookup instead of two. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:187: found.first->second = new ServiceListeners(service, this); On 2013/06/18 23:46:53, gene wrote: > Do you want to keep found.first->second as a scoped_ptr vs. ServiceListeners* ? scoped_ptr doesn't play nice with STL containers. Using linked_ptr. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:32: // Create a service object listening for DNS-SD service announcements on On 2013/06/18 23:46:53, gene wrote: > Comment is for a listener here. Removed comments on implementation and added references to originals, matching the style in the mDNS library. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:98: std::map<std::string, ServiceListeners*> services_; On 2013/06/18 23:01:46, Vitaly Buka wrote: > Please define typedef for td::map<std::string, ServiceListeners*> > and use everywhere > Done. https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_unittest.cc (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_unittest.cc:261: ServiceDiscoveryTest() : socket_factory_(new MockDatagramSocketFactory), On 2013/06/18 23:01:46, Vitaly Buka wrote: > maybe break before : Done.
https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:84: STATUS_TIMEOUT = 1, STATUS_TIMEOUT conflicts with define in WinNT.h
almost lgtm. a few nits below: https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/17001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:136: DCHECK(services_.find(record->name()) != services_.end()); Could you please add a comment here, explaining that service must be present in the services_ list already, if we get TXT/SRV record update? On 2013/06/19 18:46:22, Noam Samuel wrote: > On 2013/06/18 23:46:53, gene wrote: > > PTR may easily arrive after SRV, will it cause problems here? > > The service won't be notified as "updated" if that's the case since there will > be no listener for the SRV record when it arrives. I might posit it's a problem > if SRV arrives immediately after PTR and causes a spurious update notification. > It's not a huge problem, though, so I'm OK deferring it to another CL. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:51: virtual void OnServiceStatusChanged(bool available, May be join those two functions to something like OnServiceChanged(status, service_name), where status = {available, changed, removed} https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:90: ResolveCompleteCallback; indentation https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:220: if (!CreateTxtTransaction()) Can result of the transaction arrive within this call? If so, does it make sense to move is_resolving_, address_resolved_, metadata_resolved_ initialization before starting transactions? https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:288: is_resolving_ = false; Do you need to reset address_resolved_ and metadata_resolved_ as well here? You can have a separate function ServiceNotFound() in this case. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:42: public net::MDnsListener::Delegate { not sure what is the correct indentation here https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:179: ServiceDescription services_[2]; This is a bit confusing, a few alternatives are: 1. Have staging_service_ and final_service_. Implement swap() function in ServiceDescription (string and vector already supports swap) 2. Have staging_service_ and final_service_. Implement StagingToFinal() function in this class. 3. Have scoped_ptr<ServiceDescription> staging_service_ and final_service_. scoped_ptr already supports swap().
https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:348: NOTREACHED(); no default return value
https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:42: public net::MDnsListener::Delegate { all publics must be aligned
https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client.h (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:51: virtual void OnServiceStatusChanged(bool available, On 2013/06/19 22:48:03, gene wrote: > May be join those two functions to something like > OnServiceChanged(status, service_name), where status = {available, changed, > removed} Done. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:84: STATUS_TIMEOUT = 1, On 2013/06/19 21:02:02, Vitaly Buka wrote: > STATUS_TIMEOUT conflicts with define in WinNT.h Done. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client.h:90: ResolveCompleteCallback; On 2013/06/19 22:48:03, gene wrote: > indentation Done. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:220: if (!CreateTxtTransaction()) On 2013/06/19 22:48:03, gene wrote: > Can result of the transaction arrive within this call? > If so, does it make sense to move is_resolving_, address_resolved_, > metadata_resolved_ initialization before starting transactions? Done. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:288: is_resolving_ = false; On 2013/06/19 22:48:03, gene wrote: > Do you need to reset address_resolved_ and metadata_resolved_ as well here? > You can have a separate function ServiceNotFound() in this case. address_resolved_ and metadata_resolved_ are reset when the resolution starts (in StartResolving). Moved to separate function. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:348: NOTREACHED(); On 2013/06/19 22:51:24, Vitaly Buka wrote: > no default return value Done. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:42: public net::MDnsListener::Delegate { On 2013/06/19 22:55:21, Vitaly Buka wrote: > all publics must be aligned Done. https://codereview.chromium.org/16272006/diff/31001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:179: ServiceDescription services_[2]; On 2013/06/19 22:48:03, gene wrote: > This is a bit confusing, a few alternatives are: > 1. Have staging_service_ and final_service_. Implement swap() function in > ServiceDescription (string and vector already supports swap) > > 2. Have staging_service_ and final_service_. Implement StagingToFinal() function > in this class. > > 3. Have scoped_ptr<ServiceDescription> staging_service_ and final_service_. > scoped_ptr already supports swap(). > Done.
lgtm tiny nit below: https://codereview.chromium.org/16272006/diff/45001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.cc (right): https://codereview.chromium.org/16272006/diff/45001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.cc:355: case net::MDnsTransaction::RESULT_DONE: add comment // pass through
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noamsml@chromium.org/16272006/53001
Message was sent while issue was closed.
Change committed as 207583
Message was sent while issue was closed.
https://codereview.chromium.org/16272006/diff/53001/chrome/browser/local_disc... File chrome/browser/local_discovery/service_discovery_client_impl.h (right): https://codereview.chromium.org/16272006/diff/53001/chrome/browser/local_disc... chrome/browser/local_discovery/service_discovery_client_impl.h:85: typedef std::map<std::string, linked_ptr<ServiceListeners>> Please fix. ">>" is a syntax with our build flags (-Werror=c++0x-compat). Neither the tryserver nor the waterfall bots compile or execute any of this code, so I suggest you consider running tryjobs with a gclient override enable_mdns=1. (You might be able to do this without uploading such a change to rietveld by using "git try" rather than "git cl try".) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
