[Presentation WebAPI] built-in device discovery and control protocol

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: schien, Assigned: xeonchen)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(4 attachments, 41 obsolete attachments)

49 bytes, text/x-github-pull-request
mwu
: review+
Details | Review | Splinter Review
46 bytes, text/x-github-pull-request
johnhu
: review+
Details | Review | Splinter Review
67.26 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
27.53 KB, patch
xeonchen
: review+
Details | Diff | Splinter Review
We need a built-in device protocol implementation for Firefox and Firefox OS. Per discussion during Portland workweek, we'll use mDNS/DNS-SD for device discovery and a plain TCP socket as control channel.
We are going to use the mDNS JS module in bug 1011340.
Assignee: juhsu → xeonchen

Updated

3 years ago
Depends on: 1120308
(Assignee)

Comment 2

3 years ago
Created attachment 8547471 [details] [diff] [review]
part 1, import-mDNS-JS-module-in-bug-1011340.patch

[WIP] this is imported from Bug 1011340, and fix some error.
(Assignee)

Comment 3

3 years ago
Created attachment 8547477 [details] [diff] [review]
part 2, add-mDNS-provider.patch

[WIP] add a mDNS XPCOM component.
(Assignee)

Comment 4

3 years ago
Created attachment 8547480 [details] [diff] [review]
[test-only] modify-to-test-in-browser.patch

[WIP] use this patch to test mDNS provider in browser
(Assignee)

Comment 5

3 years ago
There's lots of details about mDNS & device discovery, and the MulticastDNS.jsm only implements basic functions of discovery.

To support more features, we decide to use mDNSResponder, the mDNS/DNS-SD implementation by Apple, instead of extending MulticastDNS.jsm since the specs (RFC-1035, RFC-6762, RFC-6763) contain a lot of works.

Our first step is to bring up mDNSResponder on b2g devices, make sure it works, then implement the module on other platforms (OSX, Linux, Windows, Android) later.
(Assignee)

Comment 6

3 years ago
Created attachment 8552253 [details] [diff] [review]
implement-nsIPresentationDeviceProvider-with-mDNS.patch

update previous work on mDNS response.
Attachment #8547471 - Attachment is obsolete: true
Attachment #8547477 - Attachment is obsolete: true
Attachment #8547480 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8554397 [details] [diff] [review]
add MulticastDNSProvider

[WIP] use mDNSReponder to get discovery responses.
However, the client-server domain socket model seems not working on B2G (got kDNSServiceErr_ServiceNotRunning) by running /system/bin/mdnsd.
Using the embedded model in [1] is the next step.

[1] http://www.opensource.apple.com/source/mDNSResponder/mDNSResponder-561.1.1/mDNSPosix/ReadMe.txt
Attachment #8552253 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
(In reply to Liang-Heng Chen [:xeonchen] from comment #7)
> Created attachment 8554397 [details] [diff] [review]
> add MulticastDNSProvider
> 
> [WIP] use mDNSReponder to get discovery responses.
> However, the client-server domain socket model seems not working on B2G (got
> kDNSServiceErr_ServiceNotRunning) by running /system/bin/mdnsd.
> Using the embedded model in [1] is the next step.
> 
> [1]
> http://www.opensource.apple.com/source/mDNSResponder/mDNSResponder-561.1.1/
> mDNSPosix/ReadMe.txt

Finally I got mdnsd running by removing "disabled" and "oneshot" in the mdnsd segment of $B2G/system/core/rootdir/init.rc, therefore embedded model will be dropped.
(Assignee)

Comment 9

3 years ago
Created attachment 8557799 [details] [diff] [review]
0001-add-MulticastDNSProvider.patch

[WIP] support Browse device via mDNS protocol using mDNSResponder.
Attachment #8554397 - Attachment is obsolete: true
Created attachment 8557834 [details] [diff] [review]
0001-enable-mDNS-daemon.patch

This patch launches /system/bin/mdnsd at boot time, which is originally launched on demand via NsdManager[1] in Android.
We don't have an interface to communicate with NsdManager because it's Java-based.

I'm not sure if it's a good idea to modify init.rc (external repository in [2]), would you give some advices?

[1] http://developer.android.com/reference/android/net/nsd/NsdManager.html
[2] git://codeaurora.org/platform/system/core
Attachment #8557834 - Flags: feedback?(fabrice)
Sure we can modify init.rc 
The one you patched already has gonk changes.
Attachment #8557834 - Flags: feedback?(fabrice) → feedback+
Created attachment 8561826 [details] [diff] [review]
0001-add-mDNSResponder-XPCOM-module.patch

XPCOM module of mDNSResponder
Created attachment 8561827 [details] [diff] [review]
mDNS-provider
Attachment #8557799 - Attachment is obsolete: true
Created attachment 8561828 [details] [diff] [review]
mDNSResponder XPCOM module
Attachment #8561826 - Attachment is obsolete: true
Created attachment 8562575 [details] [diff] [review]
mDNSResponder XPCOM module

[WIP] feature complete, going to write test cases.
Attachment #8561828 - Attachment is obsolete: true
(In reply to Liang-Heng Chen [:xeonchen] from comment #15)
> Created attachment 8562575 [details] [diff] [review]
> mDNSResponder XPCOM module
> 
> [WIP] feature complete, going to write test cases.

I found that it is difficult to test this module since it's tight coupling with DNSService*() APIs defined in dns_sd.h provided by mDNSResponder.

Fabrice, do we have any conventional way to test such modules, or do you have any suggestion to refine the design?
Flags: needinfo?(fabrice)
Created attachment 8563297 [details] [diff] [review]
add mDNSResponder XPCOM module

This patch creates an XPCOM module of mDNS implemetation using Android built-in mDNSResponder.

TXT records are not supported yet, but it's not a necessary part for our Presentation API implementation. Maybe we should create another bug to implement this feature in the future.

This patch also obsoletes previous modification of init.rc since it can be launched via |property_set| API.
Attachment #8557834 - Attachment is obsolete: true
Attachment #8562575 - Attachment is obsolete: true
Flags: needinfo?(fabrice)
Attachment #8563297 - Flags: review?(fabrice)
Comment on attachment 8563297 [details] [diff] [review]
add mDNSResponder XPCOM module

This patch doesn't pass on some Desktop and Emulator platform, I'll fix it.
Attachment #8563297 - Flags: review?(fabrice)
Created attachment 8565281 [details] [diff] [review]
add mDNSResponder XPCOM module

This patch creates an XPCOM module of mDNS implemetation using Android built-in mDNSResponder.

TXT records are not supported yet since it's not a necessary part for our Presentation API implementation. Maybe we should create another bug to implement this feature in the future.

This patch also obsoletes previous modification of init.rc since it can be launched via |property_set| API.
Attachment #8563297 - Attachment is obsolete: true
Attachment #8565281 - Flags: review?(fabrice)
Created attachment 8566441 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

This patch creates an XPCOM module of mDNS implemetation using Android built-in mDNSResponder.

TXT records are implemented in this patch.

This patch also obsoletes previous modification of init.rc since it can be launched via |property_set| API.
Attachment #8565281 - Attachment is obsolete: true
Attachment #8565281 - Flags: review?(fabrice)
Attachment #8566441 - Flags: review?(fabrice)
Comment on attachment 8566441 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Review of attachment 8566441 [details] [diff] [review]:
-----------------------------------------------------------------

I have mixed feelings about implementing this feature with platform specific code like that. I feel using a js implementation like the one from Justin at https://github.com/justindarc/dns-sd.js would be easier to support.
Attachment #8566441 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #21)
> Comment on attachment 8566441 [details] [diff] [review]
> add mDNSResponder XPCOM module
> 
> Review of attachment 8566441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have mixed feelings about implementing this feature with platform specific
> code like that. I feel using a js implementation like the one from Justin at
> https://github.com/justindarc/dns-sd.js would be easier to support.

We'll not be able to bind the mDNS port on platform with built-in mDNS service, e.g. Mac and Android.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #22)
> (In reply to Fabrice Desré [:fabrice] from comment #21)
> > Comment on attachment 8566441 [details] [diff] [review]
> > add mDNSResponder XPCOM module
> > 
> > Review of attachment 8566441 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I have mixed feelings about implementing this feature with platform specific
> > code like that. I feel using a js implementation like the one from Justin at
> > https://github.com/justindarc/dns-sd.js would be easier to support.
> 
> We'll not be able to bind the mDNS port on platform with built-in mDNS
> service, e.g. Mac and Android.

For the case S.C. mentioned, using mDNSResponder might be the only choice.
Flags: needinfo?(fabrice)
(In reply to Liang-Heng Chen [:xeonchen] from comment #23)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #22)
> > (In reply to Fabrice Desré [:fabrice] from comment #21)
> > > Comment on attachment 8566441 [details] [diff] [review]
> > > add mDNSResponder XPCOM module
> > > 
> > > Review of attachment 8566441 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I have mixed feelings about implementing this feature with platform specific
> > > code like that. I feel using a js implementation like the one from Justin at
> > > https://github.com/justindarc/dns-sd.js would be easier to support.
> > 
> > We'll not be able to bind the mDNS port on platform with built-in mDNS
> > service, e.g. Mac and Android.
> 
> For the case S.C. mentioned, using mDNSResponder might be the only choice.

Yeah, that's unfortunate... So the implementation you have now is gonk only, right? Or can it also be used on fennec? If it's gonk only, I'd still rather use a js implementation for gonk and other platforms that don't have a default mDNS service.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #24)
> (In reply to Liang-Heng Chen [:xeonchen] from comment #23)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #22)
> > > (In reply to Fabrice Desré [:fabrice] from comment #21)
> > > > Comment on attachment 8566441 [details] [diff] [review]
> > > > add mDNSResponder XPCOM module
> > > > 
> > > > Review of attachment 8566441 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > I have mixed feelings about implementing this feature with platform specific
> > > > code like that. I feel using a js implementation like the one from Justin at
> > > > https://github.com/justindarc/dns-sd.js would be easier to support.
> > > 
> > > We'll not be able to bind the mDNS port on platform with built-in mDNS
> > > service, e.g. Mac and Android.
> > 
> > For the case S.C. mentioned, using mDNSResponder might be the only choice.
> 
> Yeah, that's unfortunate... So the implementation you have now is gonk only,
> right? Or can it also be used on fennec? If it's gonk only, I'd still rather
> use a js implementation for gonk and other platforms that don't have a
> default mDNS service.

This is for gonk only. For fennec we'll need to use NsdManager in Android framework. mDNSResponder library also supports multiple platform and it has been widely used, which means all the protocol features and protocol compatibility issues has been resolved. We should be able to use it on Windows/Linux with slightly modification on library linking model. Is there any other strong reason to use JS library here?

@xeonchen, please also take a look at dns-sd.js to see if it has all the feature we need, e.g. TXT record.
Flags: needinfo?(xeonchen)
Flags: needinfo?(fabrice)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #25)
> @xeonchen, please also take a look at dns-sd.js to see if it has all the
> feature we need, e.g. TXT record.

TXT record seems not working.
In https://github.com/justindarc/dns-sd.js/blob/master/src/dns-sd.js#L193, the TXT related code is commented out, and I didn't see any log about it.
But TXT is for extensibility and we don't actually use it currently.

For simple registration/discovery, it works.
However, the responses by mDNSResponder and dns-sd.js are a little bit different by observing using Wireshark.
I'm not sure whether the differences affect compatibility or not, but some of my discovery clients (mdns-scan, avahi-discover) cannot display the service registered by dns-sd.js well.
Flags: needinfo?(xeonchen)
Ok, that makes me a bit sad but let's move on with the mDNSResponder library.
Flags: needinfo?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8566441 - Flags: review?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8566441 - Attachment description: add mDNSResponder XPCOM module → Part 1: implement XPCOM module for mDNSProvider
Created attachment 8572452 [details] [diff] [review]
Part 2: implement mDNS device provider
Attachment #8561827 - Attachment is obsolete: true
Attachment #8572452 - Flags: review?(fabrice)
Comment on attachment 8572452 [details] [diff] [review]
Part 2: implement mDNS device provider

I'll make some modifications on this patch
Attachment #8572452 - Flags: review?(fabrice)
Created attachment 8572534 [details] [diff] [review]
Part 2: implement mDNS device provider
Attachment #8572452 - Attachment is obsolete: true
Attachment #8572534 - Flags: review?(fabrice)
Shih-Chiang, do you mind doing the reviews?
Yeah I can do that, you want to transfer the r? to me?
Attachment #8566441 - Flags: review?(fabrice) → review?(schien)
Attachment #8572534 - Flags: review?(fabrice) → review?(schien)
Created attachment 8577052 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Fix code style, put function return type to single line
Attachment #8566441 - Attachment is obsolete: true
Attachment #8566441 - Flags: review?(schien)
Attachment #8577052 - Flags: review?(schien)
Created attachment 8577055 [details] [diff] [review]
Part 2: implement mDNS device provider

fix try server errors
Attachment #8572534 - Attachment is obsolete: true
Attachment #8572534 - Flags: review?(schien)
Attachment #8577055 - Flags: review?(schien)
Hi Steve, I discussed with you the other day about the Hazard Analysis Build failure.
My build criteria is: (in moz.build of Attachment #8577052 [details] [diff])
> if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] >= '16':
>   DIRS += ['gonk']
and the failure platform is B2G Desktop.

So I don't think it should be built.
Do you have any idea?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56
Flags: needinfo?(sphink)
(In reply to Liang-Heng Chen [:xeonchen] from comment #35)
> Hi Steve, I discussed with you the other day about the Hazard Analysis Build
> failure.
> My build criteria is: (in moz.build of Attachment #8577052 [details] [diff] [diff])
> > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] >= '16':
> >   DIRS += ['gonk']
> and the failure platform is B2G Desktop.
> 
> So I don't think it should be built.
> Do you have any idea?
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56

Ok, I'm going to need some help with this. Despite creating the b2g hazard build, I still have only a very basic understanding of the b2g build system.

In the hazard build, I see it trying to create libxul.so with -lmdnssd, and it can't find the mdnssd library.

In http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/gachen@mozilla.com-efe6a4feee56/try-linux64_gecko/try-linux64_gecko-bm76-try1-build735.txt.gz (for example), then when it creates libxul.so it does *not* have -lmdnssd. Is that expected? You said "So I don't think it should be built." What configurations should and should not build with this?

In general, I like the hazard build to build as much code as possible. Technically, it doesn't even need to link, though that's probably a useful sanity check and it's hard to suppress the link anyway. But in particular, the hazard build *needs* to compile any code that could conceivably use the JSAPI.

The hazard build currently just does |build.sh gecko|, except it also passes a custom mozconfig that looks like

  . gonk-misc/default-gecko-config
  ac_add_options --with-compiler-wrapper=...
  ac_add_options --without-ccache

Er... I don't even understand how that selects a device/whatever to build for, though in the logs I do see

  device/generic/armv7-a-neon/vendorsetup.sh

Oh. I see in http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/gachen@mozilla.com-efe6a4feee56/try-emulator-kk-debug/b2g_try_emulator-kk-debug_dep-bm75-try1-build681.txt.gz that it *does* pass in -lmdnssd. So I guess it's not supposed to link in on desktop? Which seems to say that the problem is that |./build.sh gecko| is linking it in when it shouldn't be?

Sorry for being so clueless here.
Flags: needinfo?(sphink)
(In reply to Steve Fink [:sfink, :s:] from comment #36)
> (In reply to Liang-Heng Chen [:xeonchen] from comment #35)
> > Hi Steve, I discussed with you the other day about the Hazard Analysis Build
> > failure.
> > My build criteria is: (in moz.build of Attachment #8577052 [details] [diff] [diff] [diff])
> > > if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and CONFIG['ANDROID_VERSION'] >= '16':
> > >   DIRS += ['gonk']
> > and the failure platform is B2G Desktop.
> > 
> > So I don't think it should be built.
> > Do you have any idea?
> > 
> > [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56
> 
> Ok, I'm going to need some help with this. Despite creating the b2g hazard
> build, I still have only a very basic understanding of the b2g build system.
> 
> In the hazard build, I see it trying to create libxul.so with -lmdnssd, and
> it can't find the mdnssd library.
> 
> In
> http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/gachen@mozilla.com-
> efe6a4feee56/try-linux64_gecko/try-linux64_gecko-bm76-try1-build735.txt.gz
> (for example), then when it creates libxul.so it does *not* have -lmdnssd.
> Is that expected? You said "So I don't think it should be built." What
> configurations should and should not build with this?
> 
> In general, I like the hazard build to build as much code as possible.
> Technically, it doesn't even need to link, though that's probably a useful
> sanity check and it's hard to suppress the link anyway. But in particular,
> the hazard build *needs* to compile any code that could conceivably use the
> JSAPI.
> 
> The hazard build currently just does |build.sh gecko|, except it also passes
> a custom mozconfig that looks like
> 
>   . gonk-misc/default-gecko-config
>   ac_add_options --with-compiler-wrapper=...
>   ac_add_options --without-ccache
> 
> Er... I don't even understand how that selects a device/whatever to build
> for, though in the logs I do see
> 
>   device/generic/armv7-a-neon/vendorsetup.sh
> 
> Oh. I see in
> http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/gachen@mozilla.com-
> efe6a4feee56/try-emulator-kk-debug/b2g_try_emulator-kk-debug_dep-bm75-try1-
> build681.txt.gz that it *does* pass in -lmdnssd. So I guess it's not
> supposed to link in on desktop? Which seems to say that the problem is that
> |./build.sh gecko| is linking it in when it shouldn't be?
> 
> Sorry for being so clueless here.

Sorry I didn't clarify the question clearly.

The sub-directory named 'gonk' is expected to be built only for some platforms, and

  CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk'

defines the criteria.

And that's why it didn't have -lmdnssd in [1], because it's 'B2G Desktop Linux x64 opt'.

I don't know why it tries to build the sub-directory 'gonk' in the hazard build since it doesn't in the normal build.

If it have to build the 'gonk' directory for some reason, I think it is supposed to provide the mdnssd library.
Actually, if mdnssd library is provided, I'll be happy to build it on all platforms.

[1] http://ftp.mozilla.org/pub/mozilla.org/b2g/try-builds/gachen@mozilla.com-efe6a4feee56/try-linux64_gecko/try-linux64_gecko-bm76-try1-build735.txt.gz
Flags: needinfo?(sphink)
Comment on attachment 8577055 [details] [diff] [review]
Part 2: implement mDNS device provider

Review of attachment 8577055 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the late response. Haven't looked at the detailed log but I think we can do that after the architecture/code clean up.

::: b2g/installer/package-manifest.in
@@ +266,5 @@
>  @RESPATH@/components/layout_xul_tree.xpt
>  @RESPATH@/components/layout_xul.xpt
>  @RESPATH@/components/locale.xpt
>  @RESPATH@/components/lwbrk.xpt
> +@RESPATH@/components/mdnsresponder.xpt

this change should belongs to patch part 1, right?

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +12,5 @@
> +
> +#ifdef LOG
> +#undef LOG
> +#endif
> +#define LOG printf_stderr

Use NSPR Logging mechanism.

@@ +16,5 @@
> +#define LOG printf_stderr
> +
> +#define SERVICE_TYPE "_mozilla_papi._tcp"
> +
> +using namespace mozilla::dom::presentation;

avoid |using namespace|.

@@ +18,5 @@
> +#define SERVICE_TYPE "_mozilla_papi._tcp"
> +
> +using namespace mozilla::dom::presentation;
> +
> +class nsIPresentationDevice;

nit: remove this because we already include nsIPresentationDevice.h

@@ +23,5 @@
> +
> +/**
> + * This wrapper is used to break circular-reference problem.
> + */
> +class mozilla::dom::presentation::DNSServiceWrappedListener

use NS_FORWARD_SAFE_XXX technique to reduce hand-crafting proxy object.

@@ +47,5 @@
> +private:
> +  nsIDNSServiceDiscoveryListener* mServiceDiscoveryListener;
> +  nsIDNSRegistrationListener* mRegistrationListener;
> +  nsIDNSServiceResolveListener* mServiceResolveListener;
> +  nsITCPPresentationServerListener* mPresentationServerListener;

MulticastDNSDeviceProvider and DNSSErviceWrappedListener should be 1-to-1 mapping, right? We should be able to use only one member for storing the relationship.

@@ +117,5 @@
> +  mInitialized = true;
> +  return NS_OK;
> +}
> +
> +nsresult

change the return value to "void" because we don't really use it.

@@ +136,5 @@
> +    nsCOMPtr<nsIDNSServiceDiscoveryListener> listener(do_QueryObject(mWrappedListener, &rv));
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +    if (NS_WARN_IF(NS_FAILED(rv = mDiscovery->StopDiscovery(listener)))) {

Can we use |mDiscovery->StopDiscovery(mWrappedListener)| directly? So does other do_QueryObject code in this file.

@@ +170,5 @@
> +MulticastDNSDeviceProvider::RegisterService(uint32_t port)
> +{
> +  nsresult rv;
> +
> +  mRegistration = do_CreateInstance("@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1", &rv);

Is there any possible to create only one instance of nsDNSServiceDiscovery for both registration and discovery? In addition, should we use do_GetService instead of do_CreateInstance because mdnsresponder should be a singleton?

@@ +457,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMPL_ISUPPORTS(DNSServiceWrappedListener,
> +  nsIDNSServiceDiscoveryListener,

nit: should align with DNSServiceWrappedListener

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: suggest to add vim/emacs format tag above, like in https://dxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1 (other new C++ files, too)

@@ +10,5 @@
> +
> +#include "nsIDNSServiceDiscovery.h"
> +#include "nsWeakPtr.h"
> +#include "nsIPresentationDeviceProvider.h"
> +#include "nsITCPPresentationServer.h"

nit: sort header in dictionary order.

@@ +12,5 @@
> +#include "nsWeakPtr.h"
> +#include "nsIPresentationDeviceProvider.h"
> +#include "nsITCPPresentationServer.h"
> +
> +#define MULTICAST_DNS_PROVIDER_CID_STR "814f947a-52f7-41c9-94a1-3684797284ac"

nit: remove unused define

@@ +17,5 @@
> +#define MULTICAST_DNS_PROVIDER_CID \
> +  {0x814f947a, 0x52f7, 0x41c9, \
> +    { 0x94, 0xa1, 0x36, 0x84, 0x79, 0x72, 0x84, 0xac }}
> +
> +#define MULTICAST_DNS_PROVIDER_CONTRACT_ID "@mozilla.org/dom/presentation/multicastdns-provider;1"

We don't expect CID and CONTRACT_ID will be used out side of MulticastDNSProviderModule.cpp, right? If so, we should move those define to MuldicastDNSProviderModule.cpp.

@@ +26,5 @@
> +
> +class DNSServiceWrappedListener;
> +class MulticastDNSService;
> +
> +class MulticastDNSDeviceProvider

Add MOZ_FINAL if we don't expect people to extend this class.

@@ +43,5 @@
> +  NS_DECL_NSITCPPRESENTATIONSERVERLISTENER
> +
> +  MulticastDNSDeviceProvider();
> +  virtual nsresult Init();
> +  virtual nsresult Uninit();

why virtual?

::: dom/presentation/provider/MulticastDNSProviderModule.cpp
@@ +4,5 @@
> +
> +#include "MulticastDNSDeviceProvider.h"
> +#include "mozilla/ModuleUtils.h"
> +
> +using namespace mozilla::dom::presentation;

avoid using namespace, explicitly list all the name you want like:

using mozilla::dom::presentation::MulticastDNSDeviceProvider;

@@ +31,5 @@
> +  kMulticastDNSDeviceProviderContracts,
> +  kMulticastDNSDeviceProviderCategories
> +};
> +
> +NSMODULE_DEFN(MulticastDNSDeviceProviderModule) = &kMulticastDNSDeviceProviderModule;

This file might be used by other device provider in the future. Rename to something more general. e.g. PresentationDeviceProviderModule. (File name should be changed as well)

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Use CC0 for test files.

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

@@ +223,5 @@
> +
> +  add_test(registerService);
> +  add_test(addDevice);
> +
> +  infoHook.cleanup();

use head/tail script for test setup/teardown. test framework will guarantee that teardown will be executed even if exception happened in test procedure.
Attachment #8577055 - Flags: review?(schien)
Comment on attachment 8577052 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Review of attachment 8577052 [details] [diff] [review]:
-----------------------------------------------------------------

I'd prefer moving all these files to netwerk folder since we are going to introduce a general network feature for future use. In this case, the namespace should change to mozilla::net::mdns as well.

::: config/system-headers
@@ +1358,5 @@
>  unicode/utypes.h
>  #endif
>  libutil.h
>  unwind.h
> +dns_sd.h

move it according to the dictionary order and wrap with corresponding #ifdef block

::: toolkit/components/mdnsresponder/gonk/MDNSResponderMonitor.cpp
@@ +6,5 @@
> +#include <unistd.h>
> +#include <sys/epoll.h>
> +#include "nsThreadUtils.h"
> +
> +using namespace mozilla::toolkit::components::mdnsresponder;

IIRC, "using namespace" is bad for unified build because it will pollute namespace. Use namespace block directly.

namspace mozilla {
namespace toolkit {
namespace components {
namespace mdsnresponder {

::: toolkit/components/mdnsresponder/gonk/MDNSResponderMonitor.h
@@ +21,5 @@
> + * mDNSResponder callbacks are invoked by DNSServiceProcessResult(),
> + * but it blocks until data is readable.
> + * This class utilizes another thread to trigger callback events.
> + */
> +class MDNSResponderMonitor

mark as final.

@@ +33,5 @@
> +
> +private:
> +  MDNSResponderMonitor();
> +  virtual ~MDNSResponderMonitor();
> +  NS_IMETHODIMP Init();

use nsresult directly since it is not a virtual function from any XPIDL interface.

::: toolkit/components/mdnsresponder/gonk/MDNSResponderOperator.cpp
@@ +20,5 @@
> +
> +#ifdef ANDROID
> +#include <android/log.h>
> +#define LOG(aFmt, ...) __android_log_print(ANDROID_LOG_INFO, "MulticastDNS", aFmt, ## __VA_ARGS__);
> +#endif

Use NSPR logging mechanism would be better. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/Logging

@@ +25,5 @@
> +
> +using namespace mozilla::toolkit::components::mdnsresponder;
> +
> +nsresult
> +CloneService(nsIDNSServiceInfo* src, nsIDNSServiceInfo** obj)

can we use a copy constructor?

::: toolkit/components/mdnsresponder/gonk/MDNSResponderReply.cpp
@@ +27,5 @@
> +
> +NS_IMETHODIMP
> +BrowseReplyRunnable::Run()
> +{
> +  BrowseOperator* obj = reinterpret_cast<BrowseOperator*>(mContext);

should be caller's responsibility to perform type casting. |mContext| should be |BrowseOperator*| instead of |void*|.

::: toolkit/components/mdnsresponder/gonk/MDNSResponderReply.h
@@ +22,5 @@
> +    uint32_t interfaceIndex,
> +    DNSServiceErrorType errorCode,
> +    const char* serviceName,
> +    const char* regType,
> +    const char* replyDomain,

I'll suggest to use Gecko string type if possible.

::: toolkit/components/mdnsresponder/gonk/MDNSService.cpp
@@ +5,5 @@
> +#define MDNS_SERVICE_STATUS "init.svc.mdnsd"
> +#define NAP_TIME 200 // 200 ms between polls
> +
> +static bool
> +check_property(const char* name, const char* desired_value)

nit: coding style, use camel case

@@ +34,5 @@
> +    return NS_ERROR_FAILURE;
> +}
> +
> +nsresult
> +mozilla::toolkit::components::mdnsresponder::StartService()

should use namespace and anonymous namespace.

::: toolkit/components/mdnsresponder/gonk/MDNSService.h
@@ +10,5 @@
> +namespace components {
> +namespace mdnsresponder {
> +
> +nsresult StartService();
> +nsresult StopService();

These functions are only used in nsDNSServiceDiscovery.cpp, can we move them into the anonymous namespace in that file?

::: toolkit/components/mdnsresponder/gonk/nsDNSServiceDiscovery.h
@@ +8,5 @@
> +#include <map>
> +#include "nsRefPtr.h"
> +#include "nsIDNSServiceDiscovery.h"
> +
> +#define DNSSERVICEDISCOVERY_CID_STR "8df43d23-d3f9-4dd5-b965-de2ca3f6a42c"

nit: remove unused define.

@@ +11,5 @@
> +
> +#define DNSSERVICEDISCOVERY_CID_STR "8df43d23-d3f9-4dd5-b965-de2ca3f6a42c"
> +#define DNSSERVICEDISCOVERY_CID \
> +  {0x8df43d23, 0xd3f9, 0x4dd5, \
> +    { 0xb9, 0x65, 0xde, 0x2c, 0xa3, 0xf6, 0xa4, 0x2c }}

move this into nsMulticastDNSResponderModule.cpp

@@ +13,5 @@
> +#define DNSSERVICEDISCOVERY_CID \
> +  {0x8df43d23, 0xd3f9, 0x4dd5, \
> +    { 0xb9, 0x65, 0xde, 0x2c, 0xa3, 0xf6, 0xa4, 0x2c }}
> +
> +#define DNSSERVICEDISCOVERY_CONTRACT_ID "@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1"

move this into nsIDNSServiceDiscovery.idl

@@ +18,5 @@
> +
> +namespace mozilla {
> +namespace toolkit {
> +namespace components {
> +namespace mdnsresponder {

too many namespace hierarchy. mozilla::mdnsresponder should be enough.

::: toolkit/components/mdnsresponder/gonk/nsDNSServiceInfo.cpp
@@ +7,5 @@
> +
> +NS_IMPL_ISUPPORTS(nsDNSServiceInfo, nsIDNSServiceInfo)
> +
> +nsDNSServiceInfo::nsDNSServiceInfo()
> +  : mHost(NS_LITERAL_CSTRING(""))

no need to assign empty string for nsCString

@@ +12,5 @@
> +  , mPort(0)
> +  , mServiceName(NS_LITERAL_CSTRING(""))
> +  , mServiceType(NS_LITERAL_CSTRING(""))
> +  , mDomainName(NS_LITERAL_CSTRING(""))
> +  , mAttributes(nullptr)

no need to assign nullptr for nsCOMPtr.

@@ +129,5 @@
> +  nsCOMPtr<nsIProperties> attributes(mAttributes);
> +  attributes.forget(aAttributes);
> +  return NS_OK;
> +}
> +NS_IMETHODIMP

nit: add empty line above

::: toolkit/components/mdnsresponder/gonk/nsDNSServiceInfo.h
@@ +7,5 @@
> +
> +#include "nsCOMPtr.h"
> +#include "nsIDNSServiceDiscovery.h"
> +
> +#define DNSSERVICEINFO_CID_STR "14a50f2b-7ff6-48a5-88e3-615fd111f5d3"

nit: remove this because it hasn't been used in any place.

@@ +10,5 @@
> +
> +#define DNSSERVICEINFO_CID_STR "14a50f2b-7ff6-48a5-88e3-615fd111f5d3"
> +#define DNSSERVICEINFO_CID \
> +  {0x14a50f2b, 0x7ff6, 0x48a5, \
> +    { 0x88, 0xe3, 0x61, 0x5f, 0xd1, 0x11, 0xf5, 0xd3 }}

move this to nsMulticastDNSResponderModule.cpp since it is only been used there.

@@ +12,5 @@
> +#define DNSSERVICEINFO_CID \
> +  {0x14a50f2b, 0x7ff6, 0x48a5, \
> +    { 0x88, 0xe3, 0x61, 0x5f, 0xd1, 0x11, 0xf5, 0xd3 }}
> +
> +#define DNSSERVICEINFO_CONTRACT_ID "@mozilla.org/toolkit/components/mdnsresponder/dns-info;1"

move this define to nsIDNSServiceDiscovery.idl, then you can use this macro in other place (with nsIDNSServiceDiscovery.h included)

::: toolkit/components/mdnsresponder/nsIDNSServiceDiscovery.idl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +#include "nsIProperties.idl"

replace with |interface nsIProperties;|.

@@ +43,5 @@
> +
> +  /**
> +   * The attributes of the service.
> +   */
> +  attribute nsIProperties attributes;

nsIPropertyBag2 has better interface than nsIProperties.
Attachment #8577052 - Flags: review?(schien)
@mcmanus, we're going to support MDNS as the device discovery protocol. Do you think it is appropriate to put the related code under netwerk folder?
Flags: needinfo?(mcmanus)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #38)
> @@ +117,5 @@
> > +  mInitialized = true;
> > +  return NS_OK;
> > +}
> > +
> > +nsresult
> 
> change the return value to "void" because we don't really use it.

It is used by |SetListener|.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #40)
> @mcmanus, we're going to support MDNS as the device discovery protocol. Do
> you think it is appropriate to put the related code under netwerk folder?

awesome! yes please.. under network/dns/mdns probly
Flags: needinfo?(mcmanus)
or even flat in netwerk/dns - I'm happy to review or sr
(In reply to Steve Fink [:sfink, :s:] from comment #36)
> The hazard build currently just does |build.sh gecko|, except it also passes
> a custom mozconfig that looks like
> 
>   . gonk-misc/default-gecko-config
>   ac_add_options --with-compiler-wrapper=...
>   ac_add_options --without-ccache
> 

The link error seems because it *only* does |build.sh gecko| instead of |build.sh|, therefore the libraries in the external/* are not built.
I'm going to find out how to avoid this situation.

Steve, do we have a document to build the hazard analysis process on my computer instead of try server?
Flags: needinfo?(sphink) → needinfo?
(Assignee)

Updated

3 years ago
Flags: needinfo? → needinfo?(sphink)
Created attachment 8584389 [details] [diff] [review]
Part 2: implement mDNS device provider

refinements according to review comments
Attachment #8577055 - Attachment is obsolete: true
Attachment #8584389 - Flags: review?(schien)
Comment on attachment 8584389 [details] [diff] [review]
Part 2: implement mDNS device provider

Review of attachment 8584389 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good but need some more modifications.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +6,5 @@
> +#include "MulticastDNSDeviceProvider.h"
> +#include "nsAutoPtr.h"
> +#include "nsComponentManagerUtils.h"
> +#include "nsIPresentationDevice.h"
> +#include "nsISupportsImpl.h"

Do we need to explicitly include nsISupportsImpl.h?

@@ +33,5 @@
> +
> +/**
> + * This wrapper is used to break circular-reference problem.
> + */
> +class DNSServiceWrappedListener

marks as final.

@@ +46,5 @@
> +  NS_FORWARD_SAFE_NSIDNSREGISTRATIONLISTENER(mListener)
> +  NS_FORWARD_SAFE_NSIDNSSERVICERESOLVELISTENER(mListener)
> +  NS_FORWARD_SAFE_NSITCPPRESENTATIONSERVERLISTENER(mListener)
> +
> +  DNSServiceWrappedListener() = default;

mark as explicit.

@@ +57,5 @@
> +
> +private:
> +  virtual ~DNSServiceWrappedListener() = default;
> +
> +private:

unnecessary private label.

@@ +83,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::Init()
> +{
> +  if (mInitialized) {
> +    return NS_ERROR_ALREADY_INITIALIZED;

simply return NS_OK;

@@ +92,5 @@
> +  mMulticastDNS = do_GetService("@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1", &rv);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +  mWrappedListener = new DNSServiceWrappedListener();

nit: need an empty line above, so does other places.

@@ +106,5 @@
> +  }
> +  if (NS_WARN_IF(NS_FAILED(mPresentationServer->SetListener(mWrappedListener)))) {
> +    return rv;
> +  }
> +  if (NS_WARN_IF(NS_FAILED(rv = mPresentationServer->Init(NS_LITERAL_CSTRING(""), 0)))) {

use EmptyCString() instead.

@@ +125,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::Uninit()
> +{
> +  if (!mInitialized) {
> +    return NS_ERROR_NOT_INITIALIZED;

simply return NS_OK?

@@ +137,5 @@
> +  }
> +
> +  if (mMulticastDNS) {
> +    if (NS_WARN_IF(NS_FAILED(rv = mMulticastDNS->StopDiscovery(mWrappedListener)))) {
> +      return rv;

don't need to early return during the uninit.

@@ +149,5 @@
> +  if (mWrappedListener) {
> +    mWrappedListener->SetListener(nullptr);
> +    mWrappedListener = nullptr;
> +  }
> +

forget |mInitialized = false| in this function?

@@ +178,5 @@
> +MulticastDNSDeviceProvider::GetListener(nsIPresentationDeviceListener** aListener)
> +{
> +  if (NS_WARN_IF(!aListener)) {
> +    return NS_ERROR_INVALID_POINTER;
> +  }

NS_ENSURE_ARG_POINTER(aListener);

@@ +203,5 @@
> +  if (!mInitialized && mDeviceListener) {
> +    if (NS_WARN_IF(NS_FAILED(rv = Init()))) {
> +      return rv;
> +    }
> +  }

The logic of this function seems a bit odd to me. Could we simply do uninit if aListener is null and do init otherwise?

@@ +213,5 @@
> +MulticastDNSDeviceProvider::ForceDiscovery()
> +{
> +  if (!mInitialized) {
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }

|MOZ_ASSERT(mInitialized);| should be enough.

@@ +216,5 @@
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +  if (!mMulticastDNS) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

MOZ_ASSERT here, too?

@@ +248,5 @@
> +MulticastDNSDeviceProvider::OnServiceFound(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +252,5 @@
> +  }
> +
> +  nsresult rv ;
> +
> +  if (mMulticastDNS) {

MOZ_ASSERT here?

@@ +266,5 @@
> +MulticastDNSDeviceProvider::OnServiceLost(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +289,5 @@
> +  }
> +
> +  nsCOMPtr<nsIPresentationDevice> device;
> +  if (NS_FAILED(mPresentationServer->GetTCPDevice(serviceName, getter_AddRefs(device)))) {
> +    return NS_OK; // ignore non-exist device;

s/non-exist/non-existing

@@ +312,5 @@
> +
> +NS_IMETHODIMP
> +MulticastDNSDeviceProvider::OnStartDiscoveryFailed(const nsACString& serviceType, int32_t errorCode)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

Do we ever need those unimplemented callbacks? need follow-up bugs?

@@ +327,5 @@
> +MulticastDNSDeviceProvider::OnServiceRegistered(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +368,5 @@
> +MulticastDNSDeviceProvider::OnServiceResolved(nsIDNSServiceInfo* serviceInfo)
> +{
> +  if (NS_WARN_IF(!serviceInfo)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }

NS_ENSURE_ARG(serviceInfo);

@@ +372,5 @@
> +  }
> +
> +  nsresult rv;
> +
> +  nsAutoCString host;

use nsCString directly since we are going to pass it to CreateTCPDevice, should be able to prevent one string copy.

@@ +400,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIPresentationDeviceListener> listener;
> +  if (NS_WARN_IF(NS_FAILED(rv = GetListener(getter_AddRefs(listener))))) {

GetListener should be free from returning error here, the error checking is redundant.

@@ +405,5 @@
> +    return rv;
> +  }
> +  if (listener) {
> +    if (NS_WARN_IF(NS_FAILED(rv = listener->AddDevice(device)))) {
> +      return rv;

We don't need to propagate error along the callback chain.

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +34,5 @@
> +  NS_DECL_NSIDNSREGISTRATIONLISTENER
> +  NS_DECL_NSIDNSSERVICERESOLVELISTENER
> +  NS_DECL_NSITCPPRESENTATIONSERVERLISTENER
> +
> +  MulticastDNSDeviceProvider() = default;

mark as explicit.

@@ +40,5 @@
> +  nsresult Uninit();
> +
> +private:
> +  virtual ~MulticastDNSDeviceProvider();
> +  nsresult RegisterService(uint32_t port);

nit: argument should be named with "a" prefix, e.g. aPort. All other files should fix this as well.

@@ +42,5 @@
> +private:
> +  virtual ~MulticastDNSDeviceProvider();
> +  nsresult RegisterService(uint32_t port);
> +
> +private:

this label is unnecessary, empty line between private method and attributes should be enough.
Attachment #8584389 - Flags: review?(schien)
Created attachment 8586528 [details] [review]
Part 1: Add libmdnssd.so to gecko library dependent list

To avoid link error of hazard build on try server.
Flags: needinfo?(sphink)
Attachment #8586528 - Flags: review?(mwu)
Created attachment 8586624 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

implement XPCOM module for mDNSProvider
Attachment #8577052 - Attachment is obsolete: true
Attachment #8586624 - Flags: review?(mcmanus)
Created attachment 8586625 [details] [diff] [review]
Part 3: implement mDNS device provider

refinements according to review comments
Attachment #8584389 - Attachment is obsolete: true
Attachment #8586625 - Flags: review?(schien)
Comment on attachment 8586624 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

Review of attachment 8586624 [details] [diff] [review]:
-----------------------------------------------------------------

This is very nicely developed code. thanks!

I was hoping that this was going to be a cross platform implementation :(.. but it seems that while it is described as being gonk specific, its really (mostly) libmbdnssd specific - which should work on a bunch of platforms, right?

I think we can make some baby steps here in unifying it because I think the other platforms look a lot like this one!
 - bring mdns/gonk/* into mdns/libmdns/*
 - use the nsSocketTransportService code for cross platform management of poll instead of just going to epoll with the montior service.. that's the only obvious thing I see that is gonk specific. you can use the libmdnssd api to get a FD to give to the STS. Among other things that will centralize our shutdown handling around polling which has been a problem in the past (and is timeout based in your patch)
 - perhaps separately, get the mdns lib into other platform builds

I'm trying to figure out the reason for the new thread other than epoll (which can go away).. It looks like we spin on get_property()? but we do that on the main thread (that would have to be changed) Is there no alternative to that? It would be nice if that (or whatever the real reason is) was documented in the thread management code as the rationale..

::: netwerk/dns/mdns/gonk/MDNSResponderMonitor.cpp
@@ +9,5 @@
> +#include <unistd.h>
> +
> +namespace mozilla {
> +namespace net {
> +namespace mdns {

just mozilla::net please. (many think it should just be mozilla, but mozilla::net is established)

::: netwerk/dns/mdns/nsIDNSServiceDiscovery.idl
@@ +144,5 @@
> +/**
> + * The callback interface for service resolve
> + */
> +[scriptable, uuid(24ee6408-648e-421d-accf-c6e5adeccf97)]
> +interface nsIDNSServiceResolveListener : nsISupports

you can also make this derived from nsICancelable

@@ +179,5 @@
> +   */
> +  void startDiscovery(in AUTF8String aServiceType, in nsIDNSServiceDiscoveryListener aListener);
> +
> +  /**
> +   * Terminate a previously started discovery.

you don't need this if the listener is cancelable
Attachment #8586624 - Flags: review?(mcmanus)
Comment on attachment 8586624 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

Review of attachment 8586624 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/dns/mdns/gonk/MDNSResponderOperator.h
@@ +120,5 @@
> +  nsCOMPtr<nsIDNSServiceInfo> mServiceInfo;
> +  nsCOMPtr<nsIDNSServiceResolveListener> mListener;
> +
> +  // hold self until callback is made.
> +  nsRefPtr<ResolveOperator> mKungFuDeathGrip;

this is a nit you can ignore if you like, but I dislike that member name. mDeleteProtector is preferred.

::: netwerk/dns/mdns/gonk/nsDNSServiceDiscovery.cpp
@@ +29,5 @@
> +
> +nsresult
> +WaitForProperty(const char* aName, const char* aDesiredValue, int aMaxWait)
> +{
> +    int maxNaps = (aMaxWait * 1000) / NAP_TIME;

so nap_time is 200ms, but the callers seem to be asking for 5ms max which seems incongruous

@@ +36,5 @@
> +        maxNaps = 1;
> +    }
> +
> +    while (maxNaps-- > 0) {
> +        usleep(NAP_TIME * 1000);

assert not main thread

@@ +37,5 @@
> +    }
> +
> +    while (maxNaps-- > 0) {
> +        usleep(NAP_TIME * 1000);
> +        if (CheckProperty(aName, aDesiredValue)) {

can we sleep after we check?

@@ +74,5 @@
> +nsresult
> +nsDNSServiceDiscovery::Init()
> +{
> +  nsresult rv;
> +  if (NS_WARN_IF(NS_FAILED(rv = StartService()))) {

seems to me like this janks the main thread
(In reply to Patrick McManus [:mcmanus] from comment #50)
> I was hoping that this was going to be a cross platform implementation :(..
> but it seems that while it is described as being gonk specific, its really
> (mostly) libmbdnssd specific - which should work on a bunch of platforms,
> right?

I think it's should work on most platforms that's compatible with mDNSResponder library.
However, some platforms have their own mDNS service (e.g. NsdManager of Android), and we are supposed to deal with their mDNS related APIs.

> I think we can make some baby steps here in unifying it because I think the
> other platforms look a lot like this one!
>  - bring mdns/gonk/* into mdns/libmdns/*
>  - use the nsSocketTransportService code for cross platform management of
> poll instead of just going to epoll with the montior service.. that's the
> only obvious thing I see that is gonk specific. you can use the libmdnssd
> api to get a FD to give to the STS. Among other things that will centralize
> our shutdown handling around polling which has been a problem in the past
> (and is timeout based in your patch)
>  - perhaps separately, get the mdns lib into other platform builds

Sounds good, I'll work on it.

> I'm trying to figure out the reason for the new thread other than epoll
> (which can go away).. It looks like we spin on get_property()? but we do
> that on the main thread (that would have to be changed) Is there no
> alternative to that? It would be nice if that (or whatever the real reason
> is) was documented in the thread management code as the rationale..

A better solution is to launch mdnsd via netd, I'm studying about it.

> ::: netwerk/dns/mdns/nsIDNSServiceDiscovery.idl
> @@ +144,5 @@
> > +/**
> > + * The callback interface for service resolve
> > + */
> > +[scriptable, uuid(24ee6408-648e-421d-accf-c6e5adeccf97)]
> > +interface nsIDNSServiceResolveListener : nsISupports
> 
> you can also make this derived from nsICancelable
> 
> @@ +179,5 @@
> > +   */
> > +  void startDiscovery(in AUTF8String aServiceType, in nsIDNSServiceDiscoveryListener aListener);
> > +
> > +  /**
> > +   * Terminate a previously started discovery.
> 
> you don't need this if the listener is cancelable

I'm not sure the reason to make a callback interface derived from |nsICancelable|?
|nsIDNSServiceDiscovery| cannot be notified when |nsICancelable::Cancel| is called.

Do you mean returning a |nsICancelable| like the way [1] does?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNSService.idl#45
Flags: needinfo?(mcmanus)
> 
> Do you mean returning a |nsICancelable| like the way [1] does?

you're right - that's what I meant. And thinking about it more its not a big deal either way.
Flags: needinfo?(mcmanus)
(In reply to Liang-Heng Chen [:xeonchen] from comment #47)
> Created attachment 8586528 [details] [review]
> Part 1: Add libmdnssd.so to gecko library dependent list
> 
> To avoid link error of hazard build on try server.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd97884079a4
Comment on attachment 8586625 [details] [diff] [review]
Part 3: implement mDNS device provider

Review of attachment 8586625 [details] [diff] [review]:
-----------------------------------------------------------------

overall look good!

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +85,5 @@
> +  }
> +
> +  nsresult rv;
> +
> +  mMulticastDNS = do_GetService("@mozilla.org/toolkit/components/mdnsresponder/dns-sd;1", &rv);

use DNSSERVICEDISCOVERY_CONTRACT_ID

@@ +156,5 @@
> +MulticastDNSDeviceProvider::RegisterService(uint32_t aPort)
> +{
> +  nsresult rv;
> +
> +  nsCOMPtr<nsIDNSServiceInfo> serviceInfo = do_CreateInstance("@mozilla.org/toolkit/components/mdnsresponder/dns-info;1", &rv);

use DNSSERVICEINFO_CONTRACT_ID instead since nsIDNSServiceDiscovery.h is included.

::: dom/presentation/provider/PresentationDeviceProviderModule.cpp
@@ +9,5 @@
> +#define MULTICAST_DNS_PROVIDER_CID \
> +  {0x814f947a, 0x52f7, 0x41c9, \
> +    { 0x94, 0xa1, 0x36, 0x84, 0x79, 0x72, 0x84, 0xac }}
> +
> +#define MULTICAST_DNS_PROVIDER_CONTRACT_ID "@mozilla.org/dom/presentation/multicastdns-provider;1"

suggest to use "@mozilla.org/presentation-device/multicastdns-provider;1" corresponding to the contractID of PresentationDeviceManager [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationDeviceManager.idl#11

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +195,5 @@
> +      Assert.equal(serviceInfo.serviceType, mockDevice.serviceType);
> +      listener.onServiceResolved(createDevice(mockDevice.host, mockDevice.port, mockDevice.serviceName, mockDevice.serviceType));
> +    }
> +  };
> +  let contractHook = new ContractHook(SD_CONTRACT_ID, mockObj);

SD_CONTRACT_ID has been hooked twice so there will be two cleanup function been hooked. However the |_oldFactory| for the second ContractHook will be the first MockFactory, which mean the factory for SD_CONTRACT_ID will be overwritten.
Attachment #8586625 - Flags: review?(schien)
Created attachment 8590775 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

[WIP] 
Since we have to make sure all MDNS operations are running after the service launched without blocking main thread, the MDNS operations are moved into another thread to avoid blocking main thread.

Patrick, do you have any suggestion on this solution?
Attachment #8586624 - Attachment is obsolete: true
Attachment #8590775 - Flags: feedback?(mcmanus)
Created attachment 8590776 [details] [diff] [review]
Part 3: implement mDNS device provider
Attachment #8586625 - Attachment is obsolete: true
Attachment #8590776 - Flags: review?(schien)
Comment on attachment 8590776 [details] [diff] [review]
Part 3: implement mDNS device provider

Review of attachment 8590776 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm! Thanks for the hard work!

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +47,5 @@
> +
> +  this.init();
> +}
> +
> +ContractHook.hookedMap = new Map(); // remember only the most original factory.

you can put |hookedMap| initialization into ContractHook.prototype.

@@ +53,5 @@
> +ContractHook.prototype = {
> +  init: function() {
> +    this.reset();
> +
> +    Assert.ok(ContractHook.hookedMap.has(this._contractID));

mock object setup/cleanup is not part of our test target. It's ok to remove these |Assert.ok|.

@@ +68,5 @@
> +  cleanup: function() {
> +    this.reset();
> +
> +    Assert.ok(ContractHook.hookedMap.has(this._contractID));
> +    Assert.ok(ContractHook.hookedMap.get(this._contractID).length > 0);

here too.

@@ +217,5 @@
> +      listener.onServiceFound(createDevice("", 0, mockDevice.serviceName, mockDevice.serviceType));
> +      return {
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
> +        cancel: function() {
> +        }.bind(this)

bind to an empty function seems useless.

@@ +229,5 @@
> +    }
> +  };
> +  let contractHook = new ContractHook(SD_CONTRACT_ID, mockObj);
> +
> +  let provider = Cc["@mozilla.org/presentation-device/multicastdns-provider;1"].createInstance(Ci.nsIPresentationDeviceProvider);

suggest to make this contract Id as a const variable like others.
Attachment #8590776 - Flags: review?(schien) → review+
Created attachment 8591454 [details] [diff] [review]
Part 3: implement mDNS device provider

refine and carry r+
Attachment #8590776 - Attachment is obsolete: true
Attachment #8591454 - Flags: review+
(In reply to Gary Chen [:xeonchen] from comment #44)
> (In reply to Steve Fink [:sfink, :s:] from comment #36)
> > The hazard build currently just does |build.sh gecko|, except it also passes
> > a custom mozconfig that looks like
> > 
> >   . gonk-misc/default-gecko-config
> >   ac_add_options --with-compiler-wrapper=...
> >   ac_add_options --without-ccache
> > 
> 
> The link error seems because it *only* does |build.sh gecko| instead of
> |build.sh|, therefore the libraries in the external/* are not built.
> I'm going to find out how to avoid this situation.

Ok. I saw your pull request. If that fixes it, fine. Otherwise, you can add whatever you like into the mozharness build script mozharness/scripts/spidermonkey/build.b2g

> Steve, do we have a document to build the hazard analysis process on my
> computer instead of try server?

No. Theoretically, you could run the mozharness script hazard_build.py with the right config settings, but in practice it's a total PITA because it requires access to a tooltool server that requires LDAP credentials and there's something broken in that setup anyway. But now there's a public tooltool server. I need to get back to cleaning that up.

(To be sure, I *do* occasionally run the analysis on my local machine, but I have my own tooltool server set up and I have to spend a while to fix it every time I do this.)
Created attachment 8592658 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

[WIP] trying to solve main thread blocking issue.

Dispatch only |StartMDNSService()| into another thread.
Any API call before service will get a failure callback with error code |ERROR_SERVICE_NOT_RUNNING|.
Attachment #8590775 - Attachment is obsolete: true
Attachment #8590775 - Flags: feedback?(mcmanus)
Attachment #8592658 - Flags: feedback?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #50)
>  - use the nsSocketTransportService code for cross platform management of
> poll instead of just going to epoll with the montior service.. that's the
> only obvious thing I see that is gonk specific. you can use the libmdnssd
> api to get a FD to give to the STS. Among other things that will centralize
> our shutdown handling around polling which has been a problem in the past
> (and is timeout based in your patch)

STS requires |PRFileDesc| type, I see transformation from osfd to PRFileDesc in [1],
but I feel it might not be a good idea.

Instead, |FdWatcher| in [2], which uses libevent, seems an alternative solution.

[1] https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptio.c#3300
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsDumpUtils.h#35
Flags: needinfo?(mcmanus)
Created attachment 8593278 [details] [diff] [review]
Part 2: implement XPCOM module for mDNSProvider

use |MessageLoopForIO| to read data instead of creating another thread.
Attachment #8592658 - Attachment is obsolete: true
Attachment #8592658 - Flags: feedback?(mcmanus)
Flags: needinfo?(mcmanus)
Attachment #8593278 - Flags: review?(mcmanus)
Created attachment 8593279 [details] [diff] [review]
Part 3: implement mDNS device provider

handle registration event failure in |OnRegistrationFailed| callback.
Attachment #8593279 - Flags: review?(schien)
(Assignee)

Updated

3 years ago
Attachment #8591454 - Attachment is obsolete: true
Comment on attachment 8593279 [details] [diff] [review]
Part 3: implement mDNS device provider

Review of attachment 8593279 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +169,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv = serviceInfo->SetPort(aPort)))) {
> +    return rv;
> +  }
> +
> +  mRegisterRequest = nullptr;

need to cancel previous request?

@@ +209,5 @@
> +  MOZ_ASSERT(mMulticastDNS);
> +
> +  nsresult rv;
> +
> +  mDiscoveryRequest = nullptr;

need cancel, too?

@@ +210,5 @@
> +
> +  nsresult rv;
> +
> +  mDiscoveryRequest = nullptr;
> +  if (NS_WARN_IF(NS_FAILED(rv = mMulticastDNS->StartDiscovery(NS_LITERAL_CSTRING(SERVICE_TYPE), mWrappedListener, getter_AddRefs(mDiscoveryRequest))))) {

nit: too long.
Created attachment 8593708 [details] [diff] [review]
Part 3: implement mDNS device provider

refinement
Attachment #8593279 - Attachment is obsolete: true
Attachment #8593279 - Flags: review?(schien)
Attachment #8593708 - Flags: review?(schien)
Attachment #8593708 - Flags: review?(schien) → review+
Created attachment 8593781 [details] [diff] [review]
Part 3: implement mDNS device provider

fix try error, carry r+
Attachment #8593708 - Attachment is obsolete: true
Attachment #8593781 - Flags: review+

Comment 68

3 years ago
Comment on attachment 8586528 [details] [review]
Part 1: Add libmdnssd.so to gecko library dependent list

Avoid the shell call and check if we're on ICS (15) instead. We don't/won't support anything older than ICS.
Attachment #8586528 - Flags: review?(mwu)
Created attachment 8595097 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

fix try server warnings of public destructors
Attachment #8593278 - Attachment is obsolete: true
Attachment #8593278 - Flags: review?(mcmanus)
Attachment #8595097 - Flags: review?(mcmanus)
Created attachment 8595099 [details] [diff] [review]
Part 2: implement mDNS device provider

fix mochitest error because the provider does not exist on some platforms.
Attachment #8593781 - Attachment is obsolete: true
Attachment #8595099 - Flags: review?(schien)
(Assignee)

Updated

3 years ago
Attachment #8595099 - Attachment description: 0002-Bug-1115480-Part-2-implement-mDNS-device-provider-r-.patch → Part 3: implement mDNS device provider
(Assignee)

Updated

3 years ago
Attachment #8595099 - Attachment description: Part 3: implement mDNS device provider → Part 2: implement mDNS device provider
Created attachment 8595124 [details] [review]
Part 0: Add libmdnssd.so to gecko library dependent list
Attachment #8586528 - Attachment is obsolete: true
Attachment #8595124 - Flags: review?(mwu)
Comment on attachment 8595099 [details] [diff] [review]
Part 2: implement mDNS device provider

Review of attachment 8595099 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/provider/PresentationDeviceProviderModule.cpp
@@ +27,5 @@
> +  { nullptr }
> +};
> +
> +static const mozilla::Module::CategoryEntry kPresentationDeviceProviderCategories[] = {
> +#if MOZ_WIDGET_GONK && ANDROID_VERSION >= 16

#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 16
Attachment #8595099 - Flags: review?(schien) → review+
Comment on attachment 8595097 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Review of attachment 8595097 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to be making great progress! Can we get at least one non-gonk platform enabled?

::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
@@ +48,5 @@
> +  }
> +
> +  void Init()
> +  {
> +    if (mService) {

typically we format this as

if (!mService) {
 return;
}

normal-case-code-minimally indented;
return;

@@ +50,5 @@
> +  void Init()
> +  {
> +    if (mService) {
> +      int fd = DNSServiceRefSockFD(mService);
> +      XRE_GetIOMessageLoop()->PostTask(FROM_HERE, NewRunnableMethod(this, &ServiceWatcher::StartWatching, fd));

I really want this done with the SocketTransportService .. other uses of the ServiceWatcher aren't really for networking (and if they are, they ought to be consolidated to the STS as well) - its important to have a central point to manage shutdown from, track file descriptor use from, and be able to do reporting in about:networking on. Potentially we will want to be able to do things like control routing on app-id basis for various vpns as well, so its important to have a centralized dispatching layer.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +40,5 @@
> +    while (maxNaps-- > 0) {
> +        if (CheckProperty(aName, aDesiredValue)) {
> +          return NS_OK;
> +        }
> +        usleep(NAP_TIME * 1000);

really importnant here to assert that you aren't on the main thread or the STS thread - janking either of those with a blocking sleep is something we need to be careful of.

@@ +157,5 @@
> +{
> +  NS_NewNamedThread("mDNS Starter", getter_AddRefs(mThread));
> +
> +  nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(this, &nsDNSServiceDiscovery::StartService);
> +  mThread->Dispatch(event, NS_DISPATCH_NORMAL);

as far as I understand, all of the methods of this class are meant to run on this new thread. That should be documented in the h file each method should assert it. This is especially important to help future implementors understand the implicit locking of the hash tables.
Attachment #8595097 - Flags: review?(mcmanus) → review-

Updated

3 years ago
Attachment #8595124 - Flags: review?(mwu) → review+
Created attachment 8595688 [details] [diff] [review]
Part 2: implement mDNS device provider

fix suggestion, carry r+
Attachment #8595099 - Attachment is obsolete: true
Attachment #8595688 - Flags: review+
(Assignee)

Updated

3 years ago
Blocks: 1158029
Created attachment 8597090 [details] [diff] [review]
Part 2: implement mDNS device provider

enable log only when PR_LOGGING is define.
Attachment #8595688 - Attachment is obsolete: true
Attachment #8597090 - Flags: review+
Created attachment 8597093 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

use STS to check readable
Attachment #8595097 - Attachment is obsolete: true
Attachment #8597093 - Flags: review?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #73)
> Comment on attachment 8595097 [details] [diff] [review]
> Part 1: implement XPCOM module for mDNSProvider
> 
> Review of attachment 8595097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems to be making great progress! Can we get at least one non-gonk
> platform enabled?
> 
Yes, I've created Bug 1158029 to follow-up.

> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
> @@ +40,5 @@
> > +    while (maxNaps-- > 0) {
> > +        if (CheckProperty(aName, aDesiredValue)) {
> > +          return NS_OK;
> > +        }
> > +        usleep(NAP_TIME * 1000);
> 
> really importnant here to assert that you aren't on the main thread or the
> STS thread - janking either of those with a blocking sleep is something we
> need to be careful of.
> 
> @@ +157,5 @@
> > +{
> > +  NS_NewNamedThread("mDNS Starter", getter_AddRefs(mThread));
> > +
> > +  nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(this, &nsDNSServiceDiscovery::StartService);
> > +  mThread->Dispatch(event, NS_DISPATCH_NORMAL);
> 
> as far as I understand, all of the methods of this class are meant to run on
> this new thread. That should be documented in the h file each method should
> assert it. This is especially important to help future implementors
> understand the implicit locking of the hash tables.

Of course it guarantees server is ready if we put |StartService| and following
API calls into another thread, but it's a little bit complicated to cancel an
unstarted task in the queue (maybe set a cancel flag, I guess).
So I've removed this checking and thread since it's not really helpful.

The default behavior is that it will invoke an error callback if an API is called
before server is ready, and it's caller's responsibility to handle the error.
Created attachment 8597760 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

fix build error on some platforms
Attachment #8597093 - Attachment is obsolete: true
Attachment #8597093 - Flags: review?(mcmanus)
Attachment #8597760 - Flags: review?(mcmanus)
Created attachment 8601286 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

fix build error on Windows platform.
Attachment #8597760 - Attachment is obsolete: true
Attachment #8597760 - Flags: review?(mcmanus)
Attachment #8601286 - Flags: review?(mcmanus)
Comment on attachment 8601286 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Review of attachment 8601286 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/dns/mdns/libmdns/MDNSResponderReply.cpp
@@ +188,5 @@
> +ResolveReplyRunnable::Reply(DNSServiceRef aSdRef,
> +                            DNSServiceFlags aFlags,
> +                            uint32_t aInterfaceIndex,
> +                            DNSServiceErrorType aErrorCode,
> +                            const char* fullname,

nit: aFullname / aHostTarget / etc...

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +179,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv = UnregisterService(aListener)))) {
> +    return rv;
> +  }
> +
> +  nsRefPtr<RegisterOperator> registerOp(new RegisterOperator(aServiceInfo,

nit: use | foo = new Foo()| instead.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices
Created attachment 8603698 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

fit nit parts
Attachment #8601286 - Attachment is obsolete: true
Attachment #8601286 - Flags: review?(mcmanus)
Attachment #8603698 - Flags: review?(mcmanus)
Comment on attachment 8603698 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Review of attachment 8603698 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/dns/mdns/libmdns/MDNSResponderOperator.cpp
@@ +31,5 @@
> +  static PRLogModuleInfo* log = PR_NewLogModule("MDNSResponderOperator");
> +  return log;
> +}
> +
> +#ifdef PR_LOGGING

PR_LOGGING is going to be removed, see Bug 1161238.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.cpp
@@ +39,5 @@
> +}
> +
> +} // namespace anonymous
> +
> +namespace {

only need one anonymous namespace.

@@ +66,5 @@
> +  , mListener(aListener)
> +{
> +}
> +
> +NS_IMETHODIMP DiscoveryRequest::Cancel(nsresult aReason)

nit: split return value to a separate line.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.h
@@ +32,5 @@
> +  **/
> +  nsresult Init();
> +
> +  nsresult StopDiscovery(nsIDNSServiceDiscoveryListener *aListener);
> +  nsresult UnregisterService(nsIDNSRegistrationListener *aListener);

nit: nsIDNSServiceDiscoveryListener* aListener / nsIDNSRegistrationListener* aListener.

::: netwerk/dns/mdns/libmdns/nsDNSServiceInfo.cpp
@@ +23,5 @@
> +  nsAutoCString str;
> +  uint16_t value;
> +
> +  if (NS_SUCCEEDED(aServiceInfo->GetHost(str))) {
> +    MOZ_ASSERT(NS_SUCCEEDED(SetHost(str)));

MOZ_ASSERT shouldn't be use on an expression with side effect.

::: netwerk/dns/mdns/moz.build
@@ +9,5 @@
> +XPIDL_SOURCES += [
> +    'nsIDNSServiceDiscovery.idl',
> +]
> +
> +XPIDL_MODULE = 'mdns'

The XPIDL_MODULE under netwerk folder all have "necko_" prefix, maybe we should use "necko_mdns" here.
Created attachment 8605787 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

fix some logic errors
Attachment #8603698 - Attachment is obsolete: true
Attachment #8603698 - Flags: review?(mcmanus)
Attachment #8605787 - Flags: review?(mcmanus)
(Assignee)

Updated

3 years ago
Attachment #8605787 - Attachment description: 0001-Bug-1115480-Part-1-implement-XPCOM-module-for-mDNSPr.patch → Part 1: implement XPCOM module for mDNSProvider
Created attachment 8605788 [details] [diff] [review]
Part 2: implement mDNS device provider

remove PR_LOGGING and add some messages
Attachment #8597090 - Attachment is obsolete: true
Attachment #8605788 - Flags: review+

Comment 85

3 years ago
Created attachment 8606081 [details] [review]
Part 3: UI test of Presentation API
Created attachment 8606082 [details] [review]
Part 3: UI test of Presentation API
Attachment #8606082 - Flags: review?(im)
(Assignee)

Updated

3 years ago
Attachment #8606082 - Attachment is obsolete: true
Attachment #8606082 - Flags: review?(im)
(Assignee)

Updated

3 years ago
Attachment #8606081 - Attachment description: [gaia] xeonchen:bug-1115480 > mozilla-b2g:master → Part 3: UI test of Presentation API
Attachment #8606081 - Flags: review?(im)
Comment on attachment 8606081 [details] [review]
Part 3: UI test of Presentation API

Looks good to me. Please check PR for the nits.

If you want to land this test code before gecko patch, please add code to detect the existence of mozPresentationDeviceInfo.
Attachment #8606081 - Flags: review?(im) → review+
Comment on attachment 8606081 [details] [review]
Part 3: UI test of Presentation API

check capability
Attachment #8606081 - Flags: review+ → review?(im)
Comment on attachment 8606081 [details] [review]
Part 3: UI test of Presentation API

Looks good to me. This version looks like that it can be landed before gecko patch. Thanks.
Attachment #8606081 - Flags: review?(im) → review+
Created attachment 8608516 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

update log to "mozilla/Logging.h"
Attachment #8605787 - Attachment is obsolete: true
Attachment #8605787 - Flags: review?(mcmanus)
Attachment #8608516 - Flags: review?(mcmanus)
Created attachment 8608517 [details] [diff] [review]
Part 2: implement mDNS device provider

update log to "mozilla/Logging.h"
Attachment #8605788 - Attachment is obsolete: true
Attachment #8608517 - Flags: review+
Comment on attachment 8608516 [details] [diff] [review]
Part 1: implement XPCOM module for mDNSProvider

Review of attachment 8608516 [details] [diff] [review]:
-----------------------------------------------------------------

I really appreciate the updates. lgtm
Attachment #8608516 - Flags: review?(mcmanus) → review+
try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc165d9f51cc
Part 1 depends on Part 0, so please land Part 0 and b2g/config/*/sources.xml should be updated first
Otherwise it may cause the problem in [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe6a4feee56&filter-searchStr=b2g_try_linux64-b2g-haz_dep
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gonk-misc/commit/5deaf27fd266316f27e68206cc3be0e6f47ded54

Master: https://github.com/mozilla-b2g/gaia/commit/084f9ef593d201fcc077c55309d945d9b378412a

Comment 96

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/dda118cb0bcc
https://hg.mozilla.org/integration/b2g-inbound/rev/7ada5429c80d
Keywords: checkin-needed
Not sure why you removed the head/tail lines from xpcshell.ini, but it blew up the remote xpcshell harness (and the failures were visible in your Try push as well). I pushed a follow-up fix since backing out would have been more problematic than just fixing it.
https://hg.mozilla.org/integration/b2g-inbound/rev/fa0c93ed6885

https://treeherder.mozilla.org/logviewer.html#?job_id=2075752&repo=b2g-inbound
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #97)
> Not sure why you removed the head/tail lines from xpcshell.ini, but it blew
> up the remote xpcshell harness (and the failures were visible in your Try
> push as well). I pushed a follow-up fix since backing out would have been
> more problematic than just fixing it.
> https://hg.mozilla.org/integration/b2g-inbound/rev/fa0c93ed6885
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=2075752&repo=b2g-
> inbound

Thank you, it's my fault.
https://hg.mozilla.org/mozilla-central/rev/dda118cb0bcc
https://hg.mozilla.org/mozilla-central/rev/7ada5429c80d
https://hg.mozilla.org/mozilla-central/rev/fa0c93ed6885
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Comment 100

3 years ago
Finally. Congrats!
Depends on: 1171827
(Assignee)

Updated

2 years ago
Blocks: 1180596
Depends on: 1190069

Updated

2 years ago
Depends on: 1172343
(Assignee)

Updated

2 years ago
See Also: → bug 1046244
You need to log in before you can comment on or make changes to this bug.