[Presentation WebAPI] make service registration configurable

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: xeonchen, Assigned: xeonchen)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

the service name should be configurable, and there should be a switch to turn service registration off.
Assignee: nobody → xeonchen
Posted patch make customizable settings (obsolete) — Splinter Review
Attachment #8633965 - Flags: review?(schien)
(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 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.
Attachment #8633965 - Flags: review?(schien)
If dom.presentation.device.name uses default value but name conflict occurs, how could we let user know the new name in UI?
(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?
Flags: needinfo?(schien)
(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.
Since discovery/discoverable may be switched on/off separately, operations to device container should be accessible without listening a port.
Attachment #8636498 - Flags: review?(fabrice)
Attachment #8633965 - Attachment is obsolete: true
Attachment #8636499 - Flags: review?(schien)
(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.
Flags: needinfo?(schien)
Blocks: 1175387
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.
Attachment #8636499 - Flags: review?(schien) → review+
add more test cases:

* registerServiceDynamically
* addDeviceDynamically
* serverClosed
Attachment #8636499 - Attachment is obsolete: true
Attachment #8638374 - Flags: review?(schien)
Attachment #8636498 - Flags: review?(fabrice) → review+
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.
Attachment #8638374 - Flags: review?(schien) → review+
fix nit parts, carry r+
Attachment #8638374 - Attachment is obsolete: true
Attachment #8641417 - Flags: review+
Keywords: checkin-needed
Depends on: 1190000
https://hg.mozilla.org/mozilla-central/rev/b18caf96c537
https://hg.mozilla.org/mozilla-central/rev/be21dfb89dc7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1188935
Depends on: 1191330
Depends on: 1190791
Depends on: 1200271
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.