chrome.mdns API

chrome.mdns API

BUG=

[justinlin, 2013-08-19 17:06:16]

On 2013/08/19 17:04:21, justinlin wrote:

The mDNS implementation part is pretty much the same as yours and does much of
the same thing as the privet listener. I removed a bunch of it because it was
mostly copy-pasta from the private listener, and things like the resolver I
think should be refactored and shared.

The IDL differences may be worth discussing. I used much of the same format as
for NSD Network Services.

[mark a. foltz, 2013-08-19 18:25:25]

Looks nice.  Looks like we ended up in a similar place with the implementation. 
Most of the remaining work is to hash out the API.

https://codereview.chromium.org/22870011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/dial.idl (left):

https://codereview.chromium.org/22870011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/dial.idl:48: interface Events {
It makes more sense to add an event here to fire when a previously-returned
NetworkService has changed. The event listener API could take the same service
type parameter as getNetworkServices(); is that possible in the extensions API
framework?

If not, we'll have to keep track of which service(s) will fire events.  Will
calling GetNetworkServices once for a service type register it?  I feel like
that's a weird coupling between the two APIs, although my patch has the same
problem.

Let's discuss more.

https://codereview.chromium.org/22870011/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/dial.idl (right):

https://codereview.chromium.org/22870011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/dial.idl:41: // TODO(justinlin): Add event
interfaces?
Not sure if it make sense on an individual service object.   For example if you
add a listener to a service, and get an update event, would you then have to
unregister the listener on the old NS object and re-register it on the new
object?

The NSD spec has a NetworkServices object that has the event API for a given
services query, but even then that design has some issues.

https://codereview.chromium.org/22870011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/dial.idl:71: callback GetNetworkServicesCallback =
void (NetworkService[] deviceInfo);
s/deviceInfo/networkServices/

https://codereview.chromium.org/22870011/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/dial.idl:80: static void
getNetworkServices(GetNetworkServicesCallback callback);
There should be a query parameter to indicate the kind of service being queried
for, for example zeroconf:googlecast_.tcp_.  I don't think we want to include
all services by default.

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/dial_api.cc (right):

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_api.cc:35: const char
kCastDefaultDeviceType[] = "_privet._tcp.local";
_googlecast._tcp

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_api.cc:62: // TODO(justinlin): Any
creation/access thread constraints?
I think this should be fine, the host side of the Privet libraries is called
directly from the UI thread in the code I've seen.

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_api.cc:199:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
I think this should be okay to be a synchronous function on the UI thread (see
my patch), although not sure what the policy is on IPCs from the UI thread.
Let's chat about it.

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/dial_api.h (right):

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/dial_api.h:105: // must be returned on the UI
thread.
Leftover comments?

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/mdns_service.cc (right):

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/mdns_service.cc:51: //   // If there is
already a resolver working on this service, don't add one.
Yeah, if we want to use the device we have to fire up a task to resolve the
service name.  This might not be necessary on Macs because the mDNS listener is
built into the default resolver chain at the OS level.

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/dial/mdns_service.h (right):

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/mdns_service.h:14: class MDNSService {
This is a bit simpler.  Did you see an advantage to removing the ::Delegate
pattern?

https://codereview.chromium.org/22870011/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/dial/mdns_service.h:16: // TODO(justinlin): Should
be singleton.
Singleton per service_type? Agreed, but seems like that can be enforced in
DialApi.

https://codereview.chromium.org/22870011/diff/5001/chrome/common/extensions/a...
File chrome/common/extensions/api/dial.idl (right):

https://codereview.chromium.org/22870011/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/dial.idl:55: // origin's URL whitelist.
The URL whitelist comment doesn't make as much sense in an extensions context, I
think that will ultimately be controlled by the extension's permissions and CSP.

https://codereview.chromium.org/22870011/diff/5001/chrome/common/extensions/a...
chrome/common/extensions/api/dial.idl:65: // Whether the service is online.
I am not sure this is directly available from the existing backend.  It will
tell us when the record has been removed because of TTL expiry.  Need to check
the NSD spec to see what it says about this for zeroconf.

[scheib, 2013-08-20 23:33:50]

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
File chrome/common/extensions/api/dial.idl (right):

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
chrome/common/extensions/api/dial.idl:28: // Set when discoveryType is mdns,
unset otherwise.
discoveryType is not defined elsewhere in this idl other than these comments.

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
chrome/common/extensions/api/dial.idl:67: static void
registerNetworkService(DOMString serviceType);
Perhaps registerInterestInNetworkService?

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
chrome/common/extensions/api/dial.idl:67: static void
registerNetworkService(DOMString serviceType);
Reverence what a serviceType string could be.

[mark a. foltz, 2013-08-23 23:27:22]

Looks good so far.  Fleshing out the api -> NsdRegistry -> MDnsService flow is
the next logical step.  This is a pretty big patch - maybe the manifest specific
changes could be split out?

I'll update the doc with the manifest based proposal.

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/dial/dial_api.cc (right):

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_api.cc:157: dial_ =
DialAPIFactory::GetForProfile(profile()).get();
Maybe the dial-* changes should be a separate patch?

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

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:28:
ExtensionSystem::Get(profile)->event_router()->RegisterObserver(
DCHECK(profile_)

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:42:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Use base::NonThreadSafe or base::TheadChecker instead?

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:52:
nsd_registry->RegisterMDnsListener(*it);
When an extension is unloaded or disabled, do we have any way of unregistering
for service types?

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

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:36: Profile* profile_;
Profile *const profile_;

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api_factory.cc (right):

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.cc:18: MDnsAPI*
MDnsAPIFactory::GetForProfile(Profile* profile) {
I wonder if profile can be const Profile* in these methods that only use it for
lookup.

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_service.h (right):

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_service.h:80: // TODO: I think this
should be a reference.
SGTM if the delegate will always outlive this object.
Otherwise we may need a more explicit observer type pattern.

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/nsd_registry.cc (right):

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/nsd_registry.cc:23: mdns_service()->Start();
We should probably enforce the service type whitelist here (as well as check the
syntax of the service_name).

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

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/nsd_registry.h:42: void
RegisterMDnsListener(std::string serviceName);
Where to we track which extension(s) are watching which service(s)?

[justinlin, 2013-08-27 00:09:24]

Addressed most comments. Splitting files out now..

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/dial/dial_api.cc (right):

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/dial/dial_api.cc:157: dial_ =
DialAPIFactory::GetForProfile(profile()).get();
On 2013/08/23 23:27:23, mark a. foltz wrote:
> Maybe the dial-* changes should be a separate patch?

Done.

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

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:28:
ExtensionSystem::Get(profile)->event_router()->RegisterObserver(
On 2013/08/23 23:27:23, mark a. foltz wrote:
> DCHECK(profile_)

Done.

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:42:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2013/08/23 23:27:23, mark a. foltz wrote:
> Use base::NonThreadSafe or base::TheadChecker instead?

Done.

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:52:
nsd_registry->RegisterMDnsListener(*it);
On 2013/08/23 23:27:23, mark a. foltz wrote:
> When an extension is unloaded or disabled, do we have any way of unregistering
> for service types?

Done.

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

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:36: Profile* profile_;
On 2013/08/23 23:27:23, mark a. foltz wrote:
> Profile *const profile_;

Done.

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/nsd_registry.cc (right):

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/nsd_registry.cc:23: mdns_service()->Start();
On 2013/08/23 23:27:23, mark a. foltz wrote:
> We should probably enforce the service type whitelist here (as well as check
the
> syntax of the service_name).

It's already done in mdns_manifest_handler.cc, so we shouldn't need to check it
again. That file handles convert the manifest array into an object that we can
use and does all the validation as well. Well, right now, only 1 string is
valid..

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

https://codereview.chromium.org/22870011/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/nsd_registry.h:42: void
RegisterMDnsListener(std::string serviceName);
On 2013/08/23 23:27:23, mark a. foltz wrote:
> Where to we track which extension(s) are watching which service(s)?

There's now a map in MDnsApi

[mark a. foltz, 2013-08-27 00:35:42]

Thanks, will do a round on the doc tonight.

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

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:46: const extensions::Extension*
extension = ExtensionSystem::Get(profile_)->
extension guaranteed to be non-NULL?

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:48: MDnsInfo* info =
static_cast<MDnsInfo*>(
Guaranteed to be non-NULL?

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:50: NsdRegistry* nsd_registry =
Why not use nsd_registry()?

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:51:
MDnsAPIFactory::GetForProfile(profile_)->nsd_registry();
Maybe a method to return the service_types vector for the current extension
would be a clean factoring.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:72: NsdRegistry* nsd_registry =
nsd_registry()?

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

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:42: ServiceTypeToExtensionsMap
service_type_to_extension_map_;
service_type_to_extensions_map_?

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api_factory.cc (right):

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.cc:12: // TODO(justinlin):
Instead my NsdRegistry the LazyInstance.
Didn't quite follow this one.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.cc:13: // TODO(justinlin):
Just need somehow to access profile to dispatch events.
Is there another option besides grabbing a ptr at the time the API instance is
constructed?  e.g., is there a global to the "current" profile?  That seems a
little scary.

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

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.h:13: // TODO(justinlin):
Evaluate if NSD Registry can be the singleton instead.
I think the factory is okay as a singleton since it produces instances for any
profile...?

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_histogram_value.h:614:
MDNS_REGISTERNETWORKSERVICE,
Do we still need this?

[justinlin, 2013-08-27 01:50:20]

thanks for the review! addressed comments and removed all the service classes.
needs more comments and tests, will do that tomorrow.

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

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:46: const extensions::Extension*
extension = ExtensionSystem::Get(profile_)->
On 2013/08/27 00:35:42, mark a. foltz wrote:
> extension guaranteed to be non-NULL?

Done. Should be.. but added check.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:48: MDnsInfo* info =
static_cast<MDnsInfo*>(
On 2013/08/27 00:35:42, mark a. foltz wrote:
> Guaranteed to be non-NULL?

Done. Now it is.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:50: NsdRegistry* nsd_registry =
On 2013/08/27 00:35:42, mark a. foltz wrote:
> Why not use nsd_registry()?

Done.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:51:
MDnsAPIFactory::GetForProfile(profile_)->nsd_registry();
On 2013/08/27 00:35:42, mark a. foltz wrote:
> Maybe a method to return the service_types vector for the current extension
> would be a clean factoring.

Done.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.cc:72: NsdRegistry* nsd_registry =
On 2013/08/27 00:35:42, mark a. foltz wrote:
> nsd_registry()?

Done. Made it private too. Oh right, we only needed it to be public when we
actually have functions.

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

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:42: ServiceTypeToExtensionsMap
service_type_to_extension_map_;
On 2013/08/27 00:35:42, mark a. foltz wrote:
> service_type_to_extensions_map_?

Done.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/api/mdns/mdns_api_factory.cc (right):

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.cc:12: // TODO(justinlin):
Instead my NsdRegistry the LazyInstance.
On 2013/08/27 00:35:42, mark a. foltz wrote:
> Didn't quite follow this one.

Ignore :). This is just related to NsdRegistry being made a global singleton
instead.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.cc:13: // TODO(justinlin):
Just need somehow to access profile to dispatch events.
On 2013/08/27 00:35:42, mark a. foltz wrote:
> Is there another option besides grabbing a ptr at the time the API instance is
> constructed?  e.g., is there a global to the "current" profile?  That seems a
> little scary.

I don't remember what this comment was about, but that's not an issue now.

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

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api_factory.h:13: // TODO(justinlin):
Evaluate if NSD Registry can be the singleton instead.
On 2013/08/27 00:35:42, mark a. foltz wrote:
> I think the factory is okay as a singleton since it produces instances for any
> profile...?

I was thinking of making NsdRegistry a singleton for the whole browser instead
of per-profile, but I'm not sure how that would work out yet. Just using MDnsApi
as an owner to NsdRegistry should be fine for now.

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/22870011/diff/50001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_histogram_value.h:614:
MDNS_REGISTERNETWORKSERVICE,
On 2013/08/27 00:35:42, mark a. foltz wrote:
> Do we still need this?

Done.

[justinlin, 2013-08-28 16:53:10]

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
File chrome/common/extensions/api/dial.idl (right):

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
chrome/common/extensions/api/dial.idl:28: // Set when discoveryType is mdns,
unset otherwise.
On 2013/08/20 23:33:50, scheib wrote:
> discoveryType is not defined elsewhere in this idl other than these comments.

service type no longer needed.

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
chrome/common/extensions/api/dial.idl:67: static void
registerNetworkService(DOMString serviceType);
On 2013/08/20 23:33:50, scheib wrote:
> Perhaps registerInterestInNetworkService?

Removed this method, we use a manifest field now.

https://codereview.chromium.org/22870011/diff/17001/chrome/common/extensions/...
chrome/common/extensions/api/dial.idl:67: static void
registerNetworkService(DOMString serviceType);
On 2013/08/20 23:33:50, scheib wrote:
> Reverence what a serviceType string could be.

serviceType no longer needed.

[mark a. foltz, 2013-09-05 23:08:14]

One minor comment, LGTM

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

https://codereview.chromium.org/22870011/diff/62001/chrome/browser/extensions...
chrome/browser/extensions/api/mdns/mdns_api.h:41: // Mapping from MDNS service
types to currently listening extensions.
"to the set of currently listening extensions (by extension id)"