Initial chrome.mdns API.

Initial chrome.mdns API.

BUG=280900
TBR=erg

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221619

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221738

[justinlin, 2013-08-29 00:48:05]

Hi, ptal.

Much of the work was done in this review, just made a new one to clean things up
a bit:
https://codereview.chromium.org/22870011/

Will look into using event filters instead, per suggestion since that seems
feasible now. It seems at first sight that a manifest key is a bit cleaner and
easier to use.

Thanks.

[Matt Perry, 2013-08-30 00:43:29]

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:23: const
std::vector<std::string> GetServiceTypesForExtension(
you can return a reference here

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:24: Profile* profile, std::string
extension_id) {
pass strings by const ref

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:87: if (!extensions.size()) {
nit: size is not a boolean. use either size() == 0 or empty()

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:91:
service_type_to_extensions_map_.erase(service_type);
This will invalidate your iterator. You should do
  service_type_to_extensions_map_.erase(it++);
and not increment |it| in the next for-loop iteration.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:97: void
MDnsAPI::OnMDnsEvent(const std::string& service_type,
This is where filtered events would come in really handy. You would just set up
the right filter, then BroadcastEvent and the EventRouter will take care of
sending it to the right extensions.

(And you wouldn't need service_type_to_extensions_map_ anymore, I believe.)

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api_factory.h (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api_factory.h:17: class MDnsAPIFactory :
public BrowserContextKeyedServiceFactory {
Use ProfileKeyedAPI instead, and you can get rid of this file. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
for example.

[mark a. foltz, 2013-08-30 01:35:52]

Mostly minor comments / doc nitpicks

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:28: NOTREACHED();
Include a message

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:31: const MDnsInfo& info =
MDnsInfo::GetMDnsInfo(extension);
Combine this with the return statement?

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:49: void
MDnsAPI::SetNsdRegistryForTesting(NsdRegistry* nsd_registry) {
Make clear (in a comment or the .h) that the MDnsAPI takes ownership of the
argument.  Maybe take a scoped_ptr<> and release() it?

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:73:
service_type_to_extensions_map_[service_type].insert(extension_id);
Check return value?
I guess we don't care about duplicate insertions?

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:112: // TODO(justinlin, mfoltz):
Correctly fill in service info.
The registry should already have correctly filled mdns::MDnsService objects.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api.h (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:23: // register listeners for the
MDNS extension API. It will use an NSD registry
s/MDNS/chrome.mdns/

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:23: // register listeners for the
MDNS extension API. It will use an NSD registry
You might explain what 'NSD' means in this context.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:23: // register listeners for the
MDNS extension API. It will use an NSD registry
extensions that register DNS-SD service types with the ... manifest key.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:24: // class to start discovery
and observe new service events to dispatch them to
... to start the mDNS listener process (if necessary) and ...

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:34: void
SetNsdRegistryForTesting(NsdRegistry* registry);
Since nsd_registry() is virtual would it be easier to override it in tests?

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:55: base::ThreadChecker
thread_checker_;
Add a comment that this API is normally only called from the UI thread.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:57: scoped_ptr<NsdRegistry>
nsd_registry_;
Add a comment?  Note that it is created lazily and lifetime tied to this object?

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/nsd_registry.h (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/nsd_registry.h:15: // Network Service
Discovery registry class for keeping track of local network
remove the first 'local' as it seems redundant

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/nsd_registry.h:16: // services discovered on
the local network.
You may want to drop a URL to the NSD spec as a point of reference, even though
this is a subset of the full functionality in the spec

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/mdns.idl (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:8: // Represents a MDNS/DNS-SD service.
mDNS/DNS-SD

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:21: DOMString[] serviceData;
Need to check that DOMString[] is the right thing here.  I am not sure it's
required that the contents of the TXT record are valid UTF-8 or any other
character encoding.  Treating it as binary data may be safest.

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:25: // Event fired to inform clients of
the current complete set of responsive
s/responsive/known available/

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:27: // event. The MDNS watcher is started
when the first listener is added. The
I would remove the sentence about the watcher since it seems to be a
implementation detail that may change.

[justinlin, 2013-09-03 21:21:56]

Thanks for the reviews. Looking into switching to using a filter instead, but
haven't figured out how to use them in an idl file yet. Checking with koz@..

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:23: const
std::vector<std::string> GetServiceTypesForExtension(
On 2013/08/30 00:43:29, Matt Perry wrote:
> you can return a reference here

Done. Have to use a lazy instance to return if extension is not found, but not a
big deal since it should not be hit usually.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:24: Profile* profile, std::string
extension_id) {
On 2013/08/30 00:43:29, Matt Perry wrote:
> pass strings by const ref

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:28: NOTREACHED();
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Include a message

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:31: const MDnsInfo& info =
MDnsInfo::GetMDnsInfo(extension);
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Combine this with the return statement?

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:49: void
MDnsAPI::SetNsdRegistryForTesting(NsdRegistry* nsd_registry) {
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Make clear (in a comment or the .h) that the MDnsAPI takes ownership of the
> argument.  Maybe take a scoped_ptr<> and release() it?

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:73:
service_type_to_extensions_map_[service_type].insert(extension_id);
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Check return value?
> I guess we don't care about duplicate insertions?

Added DCHECK, we should never get duplicate insertions I think. I only see this
event once per extension no matter how many listeners are added.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:87: if (!extensions.size()) {
On 2013/08/30 00:43:29, Matt Perry wrote:
> nit: size is not a boolean. use either size() == 0 or empty()

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:91:
service_type_to_extensions_map_.erase(service_type);
On 2013/08/30 00:43:29, Matt Perry wrote:
> This will invalidate your iterator. You should do
>   service_type_to_extensions_map_.erase(it++);
> and not increment |it| in the next for-loop iteration.

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:97: void
MDnsAPI::OnMDnsEvent(const std::string& service_type,
On 2013/08/30 00:43:29, Matt Perry wrote:
> This is where filtered events would come in really handy. You would just set
up
> the right filter, then BroadcastEvent and the EventRouter will take care of
> sending it to the right extensions.
> 
> (And you wouldn't need service_type_to_extensions_map_ anymore, I believe.)

Looking into using this. Does seem promising, but I haven't figured out how to
use filters in an .idl file yet. It seems they're only ever using in the
webnavigation API right now.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:112: // TODO(justinlin, mfoltz):
Correctly fill in service info.
On 2013/08/30 01:35:52, mark a. foltz wrote:
> The registry should already have correctly filled mdns::MDnsService objects.

The TODO is to copy the information from a NsdServiceList::Service object to the
base::Dictionary object.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api.h (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:23: // register listeners for the
MDNS extension API. It will use an NSD registry
On 2013/08/30 01:35:52, mark a. foltz wrote:
> s/MDNS/chrome.mdns/

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:23: // register listeners for the
MDNS extension API. It will use an NSD registry
On 2013/08/30 01:35:52, mark a. foltz wrote:
> extensions that register DNS-SD service types with the ... manifest key.
Not addressing this for now since we might use filters instead.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:23: // register listeners for the
MDNS extension API. It will use an NSD registry
On 2013/08/30 01:35:52, mark a. foltz wrote:
> You might explain what 'NSD' means in this context.

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:24: // class to start discovery
and observe new service events to dispatch them to
On 2013/08/30 01:35:52, mark a. foltz wrote:
> ... to start the mDNS listener process (if necessary) and ...

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:34: void
SetNsdRegistryForTesting(NsdRegistry* registry);
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Since nsd_registry() is virtual would it be easier to override it in tests?

The problem is that in browser_tests, I can't see to find a way to do anything
before this class is created since it's initialized earlier than the test hooks,
so I have to override the registry in this function.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:55: base::ThreadChecker
thread_checker_;
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Add a comment that this API is normally only called from the UI thread.

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.h:57: scoped_ptr<NsdRegistry>
nsd_registry_;
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Add a comment?  Note that it is created lazily and lifetime tied to this
object?

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api_factory.h (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api_factory.h:17: class MDnsAPIFactory :
public BrowserContextKeyedServiceFactory {
On 2013/08/30 00:43:29, Matt Perry wrote:
> Use ProfileKeyedAPI instead, and you can get rid of this file. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> for example.

Done. Wow, this is great!

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/nsd_registry.h (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/nsd_registry.h:15: // Network Service
Discovery registry class for keeping track of local network
On 2013/08/30 01:35:52, mark a. foltz wrote:
> remove the first 'local' as it seems redundant

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/nsd_registry.h:16: // services discovered on
the local network.
On 2013/08/30 01:35:52, mark a. foltz wrote:
> You may want to drop a URL to the NSD spec as a point of reference, even
though
> this is a subset of the full functionality in the spec

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/mdns.idl (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:8: // Represents a MDNS/DNS-SD service.
On 2013/08/30 01:35:52, mark a. foltz wrote:
> mDNS/DNS-SD

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:21: DOMString[] serviceData;
On 2013/08/30 01:35:52, mark a. foltz wrote:
> Need to check that DOMString[] is the right thing here.  I am not sure it's
> required that the contents of the TXT record are valid UTF-8 or any other
> character encoding.  Treating it as binary data may be safest.

OK, the alternative is an ArrayBuffer, I think, but that makes it a bit less
convenient to handle in js. Maybe we can change this when more of the service is
implemented and see what works best?

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:25: // Event fired to inform clients of
the current complete set of responsive
On 2013/08/30 01:35:52, mark a. foltz wrote:
> s/responsive/known available/

Done.

https://codereview.chromium.org/23437015/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/mdns.idl:27: // event. The MDNS watcher is started
when the first listener is added. The
On 2013/08/30 01:35:52, mark a. foltz wrote:
> I would remove the sentence about the watcher since it seems to be a
> implementation detail that may change.

Done.

[Matt Perry, 2013-09-03 22:24:04]

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/mdns/mdns_api.cc:97: void
MDnsAPI::OnMDnsEvent(const std::string& service_type,
On 2013/09/03 21:21:56, justinlin wrote:
> On 2013/08/30 00:43:29, Matt Perry wrote:
> > This is where filtered events would come in really handy. You would just set
> up
> > the right filter, then BroadcastEvent and the EventRouter will take care of
> > sending it to the right extensions.
> > 
> > (And you wouldn't need service_type_to_extensions_map_ anymore, I believe.)
> 
> Looking into using this. Does seem promising, but I haven't figured out how to
> use filters in an .idl file yet. It seems they're only ever using in the
> webnavigation API right now.

Oh you're right, we don't have support for filters in .idl files yet. Also,
you'll have to add a new filter type (see EventFilteringInfo and related data
structures) for service type.

As for the .idl, you can either use a .json file instead, or add support to the
.idl compiler to handle your new filter type.

https://codereview.chromium.org/23437015/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:78: LOG(ERROR) << "LISTENER
ADDED";
fyi

[justinlin, 2013-09-05 05:14:46]

Switched to using a new event filter "serviceType" instead. Definitely much
cleaner this way, thanks. I tested registering multiple listeners with different
service type filters, and that works at well. PTAL

Note: One question that might come up might be why I'm looping through all the
listeners every time one listener is added. I considered adding a filters field
to the EventListenerInfo object, but it seems unnecessary to do that just for
this API and does not end up that much cleaner since you have to manipulate
base:: objects anyway.

https://codereview.chromium.org/23437015/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:78: LOG(ERROR) << "LISTENER
ADDED";
On 2013/09/03 22:24:04, Matt Perry wrote:
> fyi

Done...

[gene, 2013-09-05 05:28:32]

Hi Justin,
One small question. Are you sure using mDNS name and not DNS-SD everywhere in
this CL?
mDNS - is a generic DNS support in the local network through muticast
DNS-SD - is a service discovery protocol on top of mDNS

For example, RegisterMDnsListener(service_name) - does not make much sense
because mDNS spec covers so many DNS records. But
RegisterDnsSdListener(service_name) sounds much better because DnsSd spec
describes service type.
Another example, if I look at the mDNS name, I assume the full spec of the mDNS
records are supported, for example CNAME, and I should be able to request them.

I would like you to consider switching from mDNS to DNS-SD to avoid such
confusions.
I know it requires to change almost every line :(.

[justinlin, 2013-09-05 05:38:52]

Hi Gene, thanks for the comment. I think that does make sense. No worries, it's
an easy change to make :). I do think at least from the NsdRegistry class should
be changed like you suggested. Will check with mfoltz@ whether we want to also
rename the API to chrome.dnssd or maybe just leave the API name as is for now.
This will eventually migrate to NSD anyway.

[Matt Perry, 2013-09-05 20:14:17]

LGTM with nits

https://codereview.chromium.org/23437015/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:146:
filter_info.SetServiceType(service_type);
nit: why not set this directly on event, e.g.
event->filter_info.SetServiceType(etc)

https://codereview.chromium.org/23437015/diff/31001/extensions/common/event_f...
File extensions/common/event_filtering_info.h (right):

https://codereview.chromium.org/23437015/diff/31001/extensions/common/event_f...
extensions/common/event_filtering_info.h:41: const std::string &service_type()
const { return service_type_; }
nit: space after &, not before.

https://codereview.chromium.org/23437015/diff/31001/extensions/common/event_f...
extensions/common/event_filtering_info.h:41: const std::string &service_type()
const { return service_type_; }
I'd also add a has_service_type() that just checks for empty(), for symmetry
with the other fields.

https://codereview.chromium.org/23437015/diff/47001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/mdns/api/register_listener.js (right):

https://codereview.chromium.org/23437015/diff/47001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/mdns/api/register_listener.js:18: },
{'serviceType': '_googlecast._tcp.local'});
What happpens if a listener leaves the serviceType out? I think that means it
will receive events for all serviceTypes. Is that what you want?

[justinlin, 2013-09-05 21:44:04]

thanks!

https://codereview.chromium.org/23437015/diff/31001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/31001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:146:
filter_info.SetServiceType(service_type);
On 2013/09/05 20:14:18, Matt Perry wrote:
> nit: why not set this directly on event, e.g.
> event->filter_info.SetServiceType(etc)

Done.

https://codereview.chromium.org/23437015/diff/31001/extensions/common/event_f...
File extensions/common/event_filtering_info.h (right):

https://codereview.chromium.org/23437015/diff/31001/extensions/common/event_f...
extensions/common/event_filtering_info.h:41: const std::string &service_type()
const { return service_type_; }
On 2013/09/05 20:14:18, Matt Perry wrote:
> nit: space after &, not before.

Done.

https://codereview.chromium.org/23437015/diff/31001/extensions/common/event_f...
extensions/common/event_filtering_info.h:41: const std::string &service_type()
const { return service_type_; }
On 2013/09/05 20:14:18, Matt Perry wrote:
> I'd also add a has_service_type() that just checks for empty(), for symmetry
> with the other fields.

Done.

https://codereview.chromium.org/23437015/diff/47001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/mdns/api/register_listener.js (right):

https://codereview.chromium.org/23437015/diff/47001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/mdns/api/register_listener.js:18: },
{'serviceType': '_googlecast._tcp.local'});
On 2013/09/05 20:14:18, Matt Perry wrote:
> What happpens if a listener leaves the serviceType out? I think that means it
> will receive events for all serviceTypes. Is that what you want?

Yes, that is what happens. I think that's acceptable for now. One way to address
this later is I think adding a new property where an event is forced to have
filters (something like [supportsFilters=required]).

[mark a. foltz, 2013-09-06 00:15:49]

LGTM, minor comments

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:24: bool
IsServiceTypeWhitelisted(std::string service_type) {
const std::string&

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:48: static
base::LazyInstance<ProfileKeyedAPIFactory<MDnsAPI> > g_factory =
Is g_factory the naming convention for statics?

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:69: bool GetURLFromValue(const
base::Value* in_value, std::string* out_value) {
Put in anonymous { } namespace?

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.h (right):

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:26: // managing discovery
protocols.
The way this is described it sounds more like an NsdApi since it exposes the
NsdRegistry to extensions.  Perhaps this is the right separation of concerns but
I feel like the naming should be made a bit more consistent.  Maybe NsdRegistry
should be DnsSdRegistry?

I don't feel strongly.

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:54: virtual void OnMDnsEvent(
OnDnsSdEvent?

[justinlin, 2013-09-06 00:39:29]

Thanks! Made the rename.

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.cc (right):

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:24: bool
IsServiceTypeWhitelisted(std::string service_type) {
On 2013/09/06 00:15:49, mark a. foltz wrote:
> const std::string&

Done.

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:48: static
base::LazyInstance<ProfileKeyedAPIFactory<MDnsAPI> > g_factory =
On 2013/09/06 00:15:49, mark a. foltz wrote:
> Is g_factory the naming convention for statics?

Yes, that's what used for ProfileKeyedApiFactory lazy instances.

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:69: bool GetURLFromValue(const
base::Value* in_value, std::string* out_value) {
On 2013/09/06 00:15:49, mark a. foltz wrote:
> Put in anonymous { } namespace?

Oops, this was leftover, removed.

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api.h (right):

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:26: // managing discovery
protocols.
On 2013/09/06 00:15:49, mark a. foltz wrote:
> The way this is described it sounds more like an NsdApi since it exposes the
> NsdRegistry to extensions.  Perhaps this is the right separation of concerns
but
> I feel like the naming should be made a bit more consistent.  Maybe
NsdRegistry
> should be DnsSdRegistry?
> 
> I don't feel strongly.

Done. Yes, I think that makes sense. Renamed NsdRegistry to DnsSdRegistry. The
api name is still mdns for now since mdns is more general.

https://codereview.chromium.org/23437015/diff/57001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:54: virtual void OnMDnsEvent(
On 2013/09/06 00:15:49, mark a. foltz wrote:
> OnDnsSdEvent?

Done.

[commit-bot: I haz the power, 2013-09-06 00:39:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/23437015/64001

[commit-bot: I haz the power, 2013-09-06 00:53:27]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-06 01:00:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/23437015/4057

[commit-bot: I haz the power, 2013-09-06 02:19:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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.

[commit-bot: I haz the power, 2013-09-06 04:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/23437015/90001

[commit-bot: I haz the power, 2013-09-06 07:21:10]

Change committed as 221619

[commit-bot: I haz the power, 2013-09-06 15:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/23437015/105001

[commit-bot: I haz the power, 2013-09-06 18:26:03]

Change committed as 221738