[Presentation WebAPI] make service registration configurable

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 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)

(Assignee)

Description

4 years ago
the service name should be configurable, and there should be a switch to turn service registration off.
(Assignee)

Updated

4 years ago
Assignee: nobody → xeonchen
(Assignee)

Comment 1

4 years ago
Posted patch make customizable settings (obsolete) — Splinter Review
Attachment #8633965 - Flags: review?(schien)
(Assignee)

Comment 2

4 years ago
(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?
(Assignee)

Comment 5

4 years ago
(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)
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
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)
(Assignee)

Comment 8

4 years ago
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)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 12

4 years ago
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+
(Assignee)

Comment 14

4 years ago
fix nit parts, carry r+
Attachment #8638374 - Attachment is obsolete: true
Attachment #8641417 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Depends on: 1190000
https://hg.mozilla.org/mozilla-central/rev/b18caf96c537
https://hg.mozilla.org/mozilla-central/rev/be21dfb89dc7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

4 years ago
Depends on: 1188935

Updated

4 years ago
Depends on: 1191330
Depends on: 1190791
Depends on: 1200271
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.