1216398 - [Presentation WebAPI] provide device ID to TCPPresentationServer while device discoverable is turned off

Status: RESOLVED FIXED

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

Description

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

TCPPresentationServer needs a ID for initiating session request, however in current code base we only set ID after service is registered. In this case, we cannot make a pure controller (enable discovery but disable discoverable).

We'll need to init TCPPresentationServer before service registeration.

Comment 1

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

Attachment: bug1216398-support-non-discoverable-device.patch

TCPPresentationServer.id is only used for controller scenario and TCPPresentationServer.startService is only used for receiver scenario.

MDNSDeviceProvider might correct the Id after service registration but it doesn't affect any receiver side functionality.

Comment 2

[Gary Chen [:xeonchen]]

Comment on attachment 8676099 [details] [diff] [review]
bug1216398-support-non-discoverable-device.patch

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

lgtm, thank you!

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +185,5 @@
>    mDiscveryTimeoutMs = Preferences::GetUint(PREF_PRESENTATION_DISCOVERY_TIMEOUT_MS);
>    mDiscoverable = Preferences::GetBool(PREF_PRESENTATION_DISCOVERABLE);
>    mServiceName = Preferences::GetCString(PREF_PRESENTATION_DEVICE_NAME);
>  
> +  mPresentationServer->SetId(mServiceName);

the return value should be checked?

Comment 3

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

Comment on attachment 8676099 [details] [diff] [review]
bug1216398-support-non-discoverable-device.patch

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

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +185,5 @@
>    mDiscveryTimeoutMs = Preferences::GetUint(PREF_PRESENTATION_DISCOVERY_TIMEOUT_MS);
>    mDiscoverable = Preferences::GetBool(PREF_PRESENTATION_DISCOVERABLE);
>    mServiceName = Preferences::GetCString(PREF_PRESENTATION_DEVICE_NAME);
>  
> +  mPresentationServer->SetId(mServiceName);

|SetId| will no fail after this modification, I'll use |unused| to capture the return value.

Comment 4

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

Attachment: bug1216398-support-non-discoverable-device.patch

Address previous review comment and use device name as default service name on Fennec.

Comment 5

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

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

Comment 6

[Junior [inactive]]

Comment on attachment 8676687 [details] [diff] [review]
bug1216398-support-non-discoverable-device.patch

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

::: dom/presentation/interfaces/nsITCPPresentationServer.idl
@@ +87,5 @@
>    readonly attribute uint16_t port;
>  
>    /**
> +   * The id of the TCP presentation server. The TCP presentation server won't
> +   * work until the |id| is set.

This comment seems not able to elaborate the intention of this patch. Should |id| always be set before use? 

|responseSession| doesn't check the existence of |id| anymore. Does it mean TCPPresentationServer is able to without id as a presenter?

Whatever it is, make it clear on the comment.

::: dom/presentation/provider/TCPPresentationServer.js
@@ +65,5 @@
>      } catch (e) {
>        // NS_ERROR_SOCKET_ADDRESS_IN_USE
>        DEBUG && log("TCPPresentationServer - init server socket fail: " + e);
>        throw Cr.NS_ERROR_FAILURE;
>      }

idl should mention this throw

@@ +128,5 @@
>                                   aUrl);
>    },
>  
>    responseSession: function(aDeviceInfo, aSocketTransport) {
> +    if (!this._isServiceInit()) {

I guess we never reach here. Modify the log message to specify it.

Comment 7

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

Comment on attachment 8676687 [details] [diff] [review]
bug1216398-support-non-discoverable-device.patch

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

::: dom/presentation/interfaces/nsITCPPresentationServer.idl
@@ +87,5 @@
>    readonly attribute uint16_t port;
>  
>    /**
> +   * The id of the TCP presentation server. The TCP presentation server won't
> +   * work until the |id| is set.

Yeah, |id| is only required for controller action, i.e. requestSession. I'll rephrase the comment.

::: dom/presentation/provider/TCPPresentationServer.js
@@ +65,5 @@
>      } catch (e) {
>        // NS_ERROR_SOCKET_ADDRESS_IN_USE
>        DEBUG && log("TCPPresentationServer - init server socket fail: " + e);
>        throw Cr.NS_ERROR_FAILURE;
>      }

Oh my bad, I shouldn't remove the |throw| section in IDL.

Comment 8

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

Attachment: bug1216398-support-non-discoverable-device.patch

update according to review comments, carry r+.

Comment 9

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

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d340e3a25b9c

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/d340e3a25b9c