1180596 - [Presentation WebAPI] make service registration configurable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gary Chen [:xeonchen]]

the service name should be configurable, and there should be a switch to turn service registration off.

Comment 1

[Gary Chen [:xeonchen]]

Attachment: make customizable settings

Comment 2

[Gary Chen [:xeonchen]]

(In reply to Gary Chen [:xeonchen] from comment #1)
> Created attachment 8633965 [details] [diff] [review]
> make customizable settings

The mDNS provider was removed in Bug 1171827, I bring it back in this patch.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8633965 [details] [diff] [review]
make customizable settings

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

I'll expect some modification in test cases are required because service registration is disabled by default.

::: b2g/chrome/content/settings.js
@@ +556,5 @@
>      defaultValue: false
>    },
> +  'devtools.discovery.device': {
> +    prefName: 'dom.presentation.device.name',
> +    defaultValue: 'Firefox OS'

This looks like a temporary workaround for synchronizing device name between devtool and mDNS. Suggest to open a follow-up bug for providing a common device name and put some comment here.

::: dom/presentation/interfaces/nsIPresentationDeviceProvider.idl
@@ +8,5 @@
>  
>  %{C++
>  #define PRESENTATION_DEVICE_PROVIDER_CATEGORY "presentation-device-provider"
> +#define PREF_PRESENTATION_DISCOVERY "dom.presentation.discovery.enabled"
> +#define PREF_PRESENTATION_DISCOVERABLE "dom.presentation.discoverable"

These two preferences are actually for mDNS provider only. We can move it to MulticastDNSDeviceProvider.cpp.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +108,5 @@
>  
> +  const char* keys[] = { PREF_PRESENTATION_DISCOVERY,
> +                         PREF_PRESENTATION_DISCOVERABLE,
> +                         PREF_PRESENTATION_DEVICE_NAME,
> +                         nullptr };

Prefer to make this array an static local variable, like https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp#589

@@ +163,4 @@
>  
>    nsresult rv;
>  
> +  UnregisterService();

invoke UnregisterService() after printing out the "RegisterService" log will make people hard to debug via log file. Can we just assert here if service is already registered.

@@ +170,5 @@
> +
> +  /**
> +   * Create presentation server.
> +   */
> +  mPresentationServer = do_CreateInstance("@mozilla.org/presentation-device/tcp-presentation-server;1", &rv);

Put the contractId in static const variable or move it to IDL.

@@ +202,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv = serviceInfo->SetPort(servicePort)))) {
> +    return rv;
> +  }
> +
> +  return mMulticastDNS->RegisterService(serviceInfo, mWrappedListener, getter_AddRefs(mRegisterRequest));

nit: break into multiple lines.

@@ +538,5 @@
> +{
> +  NS_ConvertUTF16toUTF8 data(aData);
> +  LOG_I("Observe: topic = %s, data = %s", aTopic, data.get());
> +
> +  if (!nsCRT::strcmp(aTopic, "nsPref:changed")) {

use NS_PREFBRANCH_PREFCHANGE_TOPIC_ID in https://dxr.mozilla.org/mozilla-central/source/modules/libpref/nsIPrefBranch.idl#407

@@ +549,2 @@
>      }
> +  } else if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {

Handling xpcom-shutdown event in PresentationDeviceManager will make the Provider lifecycle much easier to implement.

@@ +561,5 @@
> +  LOG_I("DiscoveryEnabled = %d\n", mDiscoveryEnabled);
> +  if (mDiscoveryEnabled) {
> +    return ForceDiscovery();
> +  }
> +  return StopDiscovery();

nit: need some line breaks in this function.

@@ +572,5 @@
> +  LOG_I("Discoverable = %d\n", mDiscoverable);
> +  if (mDiscoverable) {
> +    return RegisterService();
> +  }
> +  return UnregisterService();

nit: need some line breaks in this function.

::: dom/presentation/provider/MulticastDNSDeviceProvider.h
@@ +47,5 @@
>  private:
>    virtual ~MulticastDNSDeviceProvider();
> +  nsresult RegisterService();
> +  nsresult UnregisterService(nsresult aReason = NS_OK);
> +  nsresult StopDiscovery(nsresult aReason = NS_OK);

I'm not a big fan of default parameter because it increase reviewers' effort while skimming through code segments.

::: modules/libpref/init/all.js
@@ +4817,5 @@
>  pref("dom.presentation.enabled", false);
>  pref("dom.presentation.tcp_server.debug", false);
> +pref("dom.presentation.discovery.enabled", true);
> +pref("dom.presentation.discoverable", false);
> +pref("dom.presentation.device.name", "Firefox OS");

all.js is for all products, should provide a general name here or move it to b2g.js.

Comment 4

[Eric Tsai (no more review request accepted)]

If dom.presentation.device.name uses default value but name conflict occurs, how could we let user know the new name in UI?

Comment 5

[Gary Chen [:xeonchen]]

(In reply to Eric Tsai from comment #4)
> If dom.presentation.device.name uses default value but name conflict occurs,
> how could we let user know the new name in UI?

Currently there's no way to display the new name.
If this is necessary, maybe we should open another API?

Comment 6

[Gary Chen [:xeonchen]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #3)
> Comment on attachment 8633965 [details] [diff] [review]
> make customizable settings
> 
> Review of attachment 8633965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/settings.js
> @@ +556,5 @@
> >      defaultValue: false
> >    },
> > +  'devtools.discovery.device': {
> > +    prefName: 'dom.presentation.device.name',
> > +    defaultValue: 'Firefox OS'
> 
> This looks like a temporary workaround for synchronizing device name between
> devtool and mDNS. Suggest to open a follow-up bug for providing a common
> device name and put some comment here.
>

open Bug 1185806 for follow-up.

Comment 7

[Gary Chen [:xeonchen]]

Attachment: Part 1: make add/remove/get devices available without init

Since discovery/discoverable may be switched on/off separately, operations to device container should be accessible without listening a port.

Comment 8

[Gary Chen [:xeonchen]]

Attachment: Part 2: make customizable settings of Presentation API

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Gary Chen [:xeonchen] from comment #5)
> (In reply to Eric Tsai from comment #4)
> > If dom.presentation.device.name uses default value but name conflict occurs,
> > how could we let user know the new name in UI?
> 
> Currently there's no way to display the new name.
> If this is necessary, maybe we should open another API?

Eventually we are going to use a common device name for all the connectivity protocols (Bluetooth, mDNS, WebIDE, etc.). Two approaches I have in mind while detecting name conflict:
  1. Create a new common name and notify all connectivity protocols. It'll make sure the name consistency for all protocols, however, it might generate ping-pong effect if the new name still conflict theoretically.
  2. Create a new name for protocols that detect conflict. In theory it can find a unique name faster, however, it'll be hard to provide a UI that not confuse user.

I'll suggest to move this discussion to bug 1185806.

Comment 10

[Gary Chen [:xeonchen]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf83c74f80d3

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8636499 [details] [diff] [review]
Part 2: make customizable settings of Presentation API

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

r+ with my comment addressed.

::: dom/presentation/PresentationDeviceManager.cpp
@@ +246,5 @@
>                                     const char16_t *aData)
>  {
>    if (!strcmp(aTopic, "profile-after-change")) {
> +    Init();
> +  } else if (!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {

simply use |strcmp| and remove the |#include "nsCRT.h"|.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +168,2 @@
>  
>    nsresult rv;

nit: move this declaration closer to its usage.

@@ +171,5 @@
> +  if (!mDiscoverable) {
> +    return NS_OK;
> +  }
> +
> +  MOZ_ASSERT(!mRegisterRequest && !mRegisterRequest);

Why need check |mRegisterRequest| twice? Should one be |mDiscoveryRequest|?

@@ +185,5 @@
> +    return rv;
> +  }
> +
> +  /**
> +   * Create service info.

make this comment more meaningful, e.g. register the presentation control channel server as an mDNS service.

@@ +188,5 @@
> +  /**
> +   * Create service info.
> +   */
> +  nsCOMPtr<nsIDNSServiceInfo> serviceInfo = do_CreateInstance(
> +      DNSSERVICEINFO_CONTRACT_ID, &rv);

I'll suggest to break line after "=", i.e.

nsCOMPtr<nsIDNSServiceInfo> serviceInfo =
  do_CreateInstance(DNSSERVICEINFO_CONTRACT_ID, &rv);

@@ +524,5 @@
>  MulticastDNSDeviceProvider::OnClose(nsresult aReason)
>  {
>    LOG_I("OnClose: %x", aReason);
>  
> +  nsresult rv;

nit: move declaration closer to its first usage.

@@ +548,5 @@
> +{
> +  NS_ConvertUTF16toUTF8 data(aData);
> +  LOG_I("Observe: topic = %s, data = %s", aTopic, data.get());
> +
> +  if (!nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {

simply use |strcmp| and remove |#include "nsCRT.h"|.

@@ +593,5 @@
> +nsresult
> +MulticastDNSDeviceProvider::OnServiceNameChanged(const nsCString& aServiceName)
> +{
> +  mServiceName = aServiceName;
> +  LOG_I("serviceName = %s\n", mServiceName.get());

Make logging as the first line of this function. It'll be more consistent and easier for future modification.

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +216,5 @@
> +
> +  let mockObj = {
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIDNSServiceDiscovery]),
> +    startDiscovery: function(serviceType, listener) {},
> +    registerService: function(serviceInfo, listener) {

|registerService| should never be invoked if not discoverable, right? You can simply do |Assert.ok(false, "should not register service if not discoverable");|. And you don't need the serviceRegistered/serviceUnregistered attributes.

@@ +235,5 @@
> +  Assert.equal(mockObj.serviceRegistered, 0);
> +  let provider = Cc[PROVIDER_CONTRACT_ID].createInstance(Ci.nsIPresentationDeviceProvider);
> +  Assert.equal(mockObj.serviceRegistered, 0);
> +  provider.listener = {
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDeviceListener, Ci.nsISupportsWeakReference]),

nit: break into multiple lines.

@@ +319,5 @@
> +
> +  let provider = Cc[PROVIDER_CONTRACT_ID].createInstance(Ci.nsIPresentationDeviceProvider);
> +  let listener = {
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDeviceListener, Ci.nsISupportsWeakReference]),
> +    addDevice: function(device) { this.devices.push(device); },

|Assert.ok(false, "shouldn't perform any device discovery");| here.

@@ +342,5 @@
>    let infoHook = new ContractHook(INFO_CONTRACT_ID, MockDNSServiceInfo);
>  
> +  do_register_cleanup(() => {
> +    Services.prefs.setBoolPref(PREF_DISCOVERABLE, gOldPrefDiscovery);
> +    Services.prefs.setBoolPref(PREF_DISCOVERABLE, gOldPrefDiscoverable);

Use |Services.prefs.clearUserPref| to restore the default setting, in this case you don't need to remember the original value.

BTW, one of the pref name should be PREF_DISCOVERY.

Comment 12

[Gary Chen [:xeonchen]]

Attachment: Part 2: make customizable settings of Presentation API

add more test cases:

* registerServiceDynamically
* addDeviceDynamically
* serverClosed

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8638374 [details] [diff] [review]
Part 2: make customizable settings of Presentation API

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

r+ with my comment addressed.

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +593,5 @@
> +
> +nsresult
> +MulticastDNSDeviceProvider::OnServiceNameChanged(const nsCString& aServiceName)
> +{
> +  LOG_I("serviceName = %s\n", mServiceName.get());

You might want to log the |aServiceName| here for printing out the new name.

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +377,5 @@
>    let contractHook = new ContractHook(SD_CONTRACT_ID, mockObj);
>  
>    let provider = Cc[PROVIDER_CONTRACT_ID].createInstance(Ci.nsIPresentationDeviceProvider);
>    let listener = {
>      QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDeviceListener, Ci.nsISupportsWeakReference]),

nit: break into multiple lines.

Comment 14

[Gary Chen [:xeonchen]]

Attachment: Part 2: make customizable settings of Presentation API

fix nit parts, carry r+

Comment 15

[Gary Chen [:xeonchen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aef7e04cc660

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b18caf96c537
https://hg.mozilla.org/integration/mozilla-inbound/rev/be21dfb89dc7

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b18caf96c537
https://hg.mozilla.org/mozilla-central/rev/be21dfb89dc7