Closed Bug 1080474 Opened 10 years ago Closed 10 years ago

[Presentation WebAPI] Device discovery mechanism

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
2.2 S4 (23jan)

People

(Reporter: schien, Assigned: schien)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [ft:conndevices])

Attachments

(4 files, 32 obsolete files)

PresentationDevice Architecture.png
161.79 KB, image/png
fabrice
: feedback+
Details
Part 3, presentation-prompt-b2g.patch, v2.1
15.30 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 1, presentation-device-management.patch, v4.2
39.71 KB, patch
schien
: review+
Details | Diff | Splinter Review
Part 2, device-info-api.patch, v3.2
28.02 KB, patch
schien
: review+
Details | Diff | Splinter Review 
UA need to maintain a list of available devices for user to choose while establishing presentation session.
PresentationDeviceManager and PresentationDeviceHandle
Device list and device selection prompt for Firefox OS.
Attached image Presentation Architecture.png (obsolete) — 
proposed architecture for device management, allow to support multiple types of device.
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #3)
> Created attachment 8505424 [details]
> Presentation Architecture.png
> 
> proposed architecture for device management, allow to support multiple types
> of device.

The security/policy model for accepting remote session request is not decided yet.
previous one is incorrect patch file.
Attachment #8508381 - Attachment is obsolete: true
Whiteboard: [ft:conndevices]
@kilik, nsIPresentationSessionTransport.idl and nsIPresentationSessionRequest.idl is really depends on the requirement of Presentation WebIDL implementation. Can you provide the updated version for that matches your requirement?
Flags: needinfo?(kikuo)
@S.C.

I suppose there could be several members in nsIPresentationSessionRequest.idl
1). requestURL // For presenter to display (it might be either HTTP or app protocol)
2). sessionId // Generated & passed from controller (it could be used if Controller-UA StartSession with a URL-1 and an sessionId-1, but Presenter-UA is presenting session with same URL-1 but another sessionId-2, presenter should open a new window to display the content), for more detail, you could check the description
here [1]. And that's my rough thoughts, does that make sense to you ?

Regarding nsIPresentationSessionTransport.idl.
Per discussion in Bug 1069230,
|NotifyOpened| & |NotifyClosed| are necessary. 
What I'm curious about is that you mentioned "|NotifyClosed| should be triggered if error happened in the session transport instance", is there any possibility that an failure occurs while posting message this time, but succeed at the next time ?  
I guess, a occasional error occurs, it's not necessarily leading to a closing of connection, right ?


[1] http://webscreens.github.io/presentation-api/#starting-new-presentations-or-manually-connecting-to-existing-presentations
Flags: needinfo?(kikuo)
(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #9)
> @S.C.
> 
> I suppose there could be several members in nsIPresentationSessionRequest.idl
> 1). requestURL // For presenter to display (it might be either HTTP or app
> protocol)
In my patch part 1 I already have |url| in nsIPresentationSessionRequest. Is this not you want or you just want to have a nicer name?

> 2). sessionId // Generated & passed from controller (it could be used if
> Controller-UA StartSession with a URL-1 and an sessionId-1, but Presenter-UA
> is presenting session with same URL-1 but another sessionId-2, presenter
> should open a new window to display the content), for more detail, you could
> check the description
> here [1]. And that's my rough thoughts, does that make sense to you ?
The |sessionId| is given by client-side javascript, right? I suppose it should be passed through PresentationDevice.establishSessionTransport(url, sessionId) and device protocol implementer should transmit it, without modification, to the presenter endpoint. If so, two following IDL modifications are required:
1. Add one more parameter "sessionId" in nsIPresentationDevice.establishSessionTransport(url).
2. Add one more attribute "sessionId" in nsIPresentationSessionRequest.

Am I read it correctly?

> 
> Regarding nsIPresentationSessionTransport.idl.
> Per discussion in Bug 1069230,
> |NotifyOpened| & |NotifyClosed| are necessary. 
> What I'm curious about is that you mentioned "|NotifyClosed| should be
> triggered if error happened in the session transport instance", is there any
> possibility that an failure occurs while posting message this time, but
> succeed at the next time ?  
> I guess, a occasional error occurs, it's not necessarily leading to a
> closing of connection, right ?
I didn't see any exception handling for |postMessage| in both spec and our implementation on bug 1069230. That's why I assume any error in the transport layer will lead to close. Do you already have a plan on how to deal with the error?
nsIPresentationSessionTransport is the interface we ask any future device protocol implementer to follow. It will be superfluous to add |NotifyError| if we only print some log on it.

> 
> 
> [1]
> http://webscreens.github.io/presentation-api/#starting-new-presentations-or-
> manually-connecting-to-existing-presentations
Flags: needinfo?(kikuo)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #10)
> In my patch part 1 I already have |url| in nsIPresentationSessionRequest. Is
> this not you want or you just want to have a nicer name?

Oh, my bad, I applied the old patch, and didn't notice there's new update.
The name |url| is ok to me, thanks.

> The |sessionId| is given by client-side javascript, right? I suppose it
> should be passed through PresentationDevice.establishSessionTransport(url,
> sessionId) and device protocol implementer should transmit it, without
> modification, to the presenter endpoint. If so, two following IDL
> modifications are required:
> 1. Add one more parameter "sessionId" in
> nsIPresentationDevice.establishSessionTransport(url).
> 2. Add one more attribute "sessionId" in nsIPresentationSessionRequest.
> 
> Am I read it correctly?

Yes, : )

> 
> > 
> > Regarding nsIPresentationSessionTransport.idl.
> > Per discussion in Bug 1069230,
> > |NotifyOpened| & |NotifyClosed| are necessary. 
> > What I'm curious about is that you mentioned "|NotifyClosed| should be
> > triggered if error happened in the session transport instance", is there any
> > possibility that an failure occurs while posting message this time, but
> > succeed at the next time ?  
> > I guess, a occasional error occurs, it's not necessarily leading to a
> > closing of connection, right ?
> I didn't see any exception handling for |postMessage| in both spec and our
> implementation on bug 1069230. That's why I assume any error in the
> transport layer will lead to close. Do you already have a plan on how to
> deal with the error?
> nsIPresentationSessionTransport is the interface we ask any future device
> protocol implementer to follow. It will be superfluous to add |NotifyError|
> if we only print some log on it.
> 

Indeed, I still don't have plans on dealing with the errors yet.
For now, |NotifyClose(nsresult)| seems to be able to fit our need.
Flags: needinfo?(kikuo)
Device management and device protocol registration for Presentation WebAPI.
Attachment #8508682 - Attachment is obsolete: true
Attachment #8515506 - Flags: review?(fabrice)
UI glue for device selection and device list.
Attachment #8508383 - Attachment is obsolete: true
Attachment #8515507 - Flags: review?(fabrice)
Hi S.C.

I've modified the WebIDL patch (Bug 1069230), and because now it's able to send blob data via the interface. 
I'm wondering if it is possible to add a API (e.g. |postStream|) in |nsIPresentationSessionTransport| or another parameter |aIsBinary| for |postMessage| to indicate that what we're transmitting is a binary stream, not just a string.
Does that make sense to you ?
Flags: needinfo?(schien)
Comment on attachment 8515506 [details] [diff] [review]
part 1, presentation-device-management.patch

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

Looks mostly good, but I haven't looked at the tests yet.

::: dom/presentation/PresentationDeviceManager.cpp
@@ +20,5 @@
> +
> +namespace {
> +
> +
> +} // anonymous namespace

why do you need that?

@@ +148,5 @@
> +  NS_ENSURE_ARG(aProvider);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (NS_WARN_IF(mProviders.Contains(aProvider))) {
> +    return NS_ERROR_FAILURE;

is that really a failure?

::: dom/presentation/PresentationDeviceManager.h
@@ +39,5 @@
> +      Equals(const nsRefPtr<DeviceHandle>& a, const nsRefPtr<DeviceHandle>& b) const
> +      {
> +        return a->mDevice.get() == b->mDevice.get();
> +      }
> +    };

why not just override the == operator?

@@ +60,5 @@
> +  PresentationDeviceManager();
> +  virtual ~PresentationDeviceManager();
> +
> +  void
> +  LoadDeviceProviders();

nit: void LoadDeviceProviders(); on a single line.

@@ +69,5 @@
> +  void
> +  NotifyDeviceChange();
> +
> +  size_t
> +  FindDevice(nsIPresentationDevice* aDevice);

unless I missed something, it looks like this could return a boolean instead.

::: dom/presentation/PresentationSessionRequest.h
@@ +31,5 @@
> +
> +  nsCOMPtr<nsIPresentationDevice> mDevice;
> +  nsString mUrl;
> +  nsString mPresentationId;
> +  nsCOMPtr<nsIPresentationSessionTransport> mSessionTransport;

nit: align the mXXX and group the nsCOMPtr together.

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +57,5 @@
> +  /*
> +   * Unhook an event listener
> +   * @param listener The event listener to remove.
> +   */
> +  void unsetListener(in nsIPresentationDeviceEventListener listener);

Is it fine to only be able to set a single listener?

::: dom/presentation/interfaces/nsIPresentationDeviceManager.idl
@@ +39,5 @@
> +   */
> +  void forceDiscovery();
> +
> +  /*
> +   * Retrieve all available devices.

nit: document what the array items are.

::: dom/presentation/interfaces/nsIPresentationDevicePrompt.idl
@@ +19,5 @@
> +  // The origin which initiate the request.
> +  readonly attribute DOMString origin;
> +
> +  // The URL to be opened after selection.
> +  readonly attribute DOMString requestURL;

Should these be nsIURI instead?

::: dom/presentation/interfaces/nsIPresentationDeviceProvider.idl
@@ +16,5 @@
> +[scriptable, uuid(7f9f0514-d957-485a-90e8-57cc3acbf15b)]
> +interface nsIPresentationDeviceListener: nsISupports
> +{
> +  void addDevice(in nsIPresentationDevice device);
> +  void removeDevice(in nsIPresentationDevice device);

do we need the full device here? Or would the deviceID be enough?

::: dom/presentation/interfaces/nsIPresentationSessionRequest.idl
@@ +11,5 @@
> +#define PRESENTATION_SESSION_REQUEST_TOPIC "presentation-session-request"
> +%}
> +
> +/*
> + * The event of a device requesting for a presentation session. User can 

nit: trailing whitespace

::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
@@ +13,5 @@
> +  /*
> +   * Callback for receiving a message from remote endpoint.
> +   * @param msg The received message.
> +   */
> +  void onMessage(in DOMString msg);

string only? Don't we want any clonable object?

@@ +37,5 @@
> +  /*
> +   * Send a message to remote endpiont.
> +   * @param msg The message to send.
> +   */
> +  void postMessage(in DOMString msg);

and here too.
Attachment #8515506 - Flags: review?(fabrice) → feedback+
Comment on attachment 8515507 [details] [diff] [review]
part 2, presentation-prompt-b2g.patch

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

r- for the settings abuse!

::: b2g/chrome/content/presentation-device.js
@@ +27,5 @@
> +      type: device.type,
> +    });
> +  }
> +  log('update device: ' + JSON.stringify(deviceArray));
> +  navigator.mozSettings.createLock().set({"dom.presentation.available-devices": deviceArray});

ouch, no please! We can't misuse the settings api like that.

@@ +49,5 @@
> +
> +(function() {
> +  let devices = presentationDeviceManager.getAvailableDevices().QueryInterface(Ci.nsIArray);
> +  updateDeviceList(devices);
> +})();

Overall, I would slightly prefer that to be a jsm imported from shell.js.

::: b2g/components/B2GPresentationDevicePrompt.js
@@ +25,5 @@
> +  classID: kB2GPRESENTATIONDEVICEPROMPT_CID,
> +  contractID: kB2GPRESENTATIONDEVICEPROMPT_CONTRACTID,
> +  classDescription: "B2G Presentation Device Prompt",
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDevicePrompt]),
> +  _xpcom_factory: XPCOMUtils.generateSingletonFactory(B2GPresentationDevicePrompt),

you don't need that.

@@ +62,5 @@
> +    let detail = {
> +      type: "presentation-select-device",
> +      origin: request.origin,
> +      requestURL: request.requestURL,
> +      id: requestId,

since you can't use settings, you'll have to pass the device information here.
Attachment #8515507 - Flags: review?(fabrice) → review-
(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #14)
> Hi S.C.
> 
> I've modified the WebIDL patch (Bug 1069230), and because now it's able to
> send blob data via the interface. 
> I'm wondering if it is possible to add a API (e.g. |postStream|) in
> |nsIPresentationSessionTransport| or another parameter |aIsBinary| for
> |postMessage| to indicate that what we're transmitting is a binary stream,
> not just a string.
> Does that make sense to you ?
According to our offline discussion, we are trying to expose WebSocket-like interface to client-side JS. I'll suggest to expose following interface.
nsIPresentationSessionTransport
  .postMessage(in DOMString msg);
  .postBinaryMessage(in ACString data);
  .postBinaryStream(in nsIInputStream stream, in unsigned long length);

nsIPresentationSessionTransportListener
  .onMessage(in DOMString msg);
  .onBinaryMessage(in ACString data);
Flags: needinfo?(schien)
Comment on attachment 8515506 [details] [diff] [review]
part 1, presentation-device-management.patch

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

::: dom/presentation/PresentationDeviceManager.cpp
@@ +20,5 @@
> +
> +namespace {
> +
> +
> +} // anonymous namespace

Forgot to remove during my refactory process. Will remove it in next revision.

@@ +148,5 @@
> +  NS_ENSURE_ARG(aProvider);
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (NS_WARN_IF(mProviders.Contains(aProvider))) {
> +    return NS_ERROR_FAILURE;

It's just preventing duplicate provider registration, print a warning and return NS_OK should be enough.

::: dom/presentation/PresentationDeviceManager.h
@@ +39,5 @@
> +      Equals(const nsRefPtr<DeviceHandle>& a, const nsRefPtr<DeviceHandle>& b) const
> +      {
> +        return a->mDevice.get() == b->mDevice.get();
> +      }
> +    };

The comparison is between nsRefPtr<DeviceHandle> and I think overriding nsRefPtr's behavior is not necessary.

@@ +60,5 @@
> +  PresentationDeviceManager();
> +  virtual ~PresentationDeviceManager();
> +
> +  void
> +  LoadDeviceProviders();

Done.

@@ +69,5 @@
> +  void
> +  NotifyDeviceChange();
> +
> +  size_t
> +  FindDevice(nsIPresentationDevice* aDevice);

The index is used in |PresentationDeviceManager::RemoveDevice| to prevent extra list traverse. Do you prefer |bool FindDevice(nsIPresentationDevice* aDevice, size_t* index)|?

::: dom/presentation/PresentationSessionRequest.h
@@ +31,5 @@
> +
> +  nsCOMPtr<nsIPresentationDevice> mDevice;
> +  nsString mUrl;
> +  nsString mPresentationId;
> +  nsCOMPtr<nsIPresentationSessionTransport> mSessionTransport;

Done.

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +57,5 @@
> +  /*
> +   * Unhook an event listener
> +   * @param listener The event listener to remove.
> +   */
> +  void unsetListener(in nsIPresentationDeviceEventListener listener);

Yes, I also prefer single listener. Do you prefer |attribute nsIXXX listener| in all the IDLs?

::: dom/presentation/interfaces/nsIPresentationDeviceManager.idl
@@ +39,5 @@
> +   */
> +  void forceDiscovery();
> +
> +  /*
> +   * Retrieve all available devices.

Done, nsIPresentationDevice is used here.

::: dom/presentation/interfaces/nsIPresentationDeviceProvider.idl
@@ +16,5 @@
> +[scriptable, uuid(7f9f0514-d957-485a-90e8-57cc3acbf15b)]
> +interface nsIPresentationDeviceListener: nsISupports
> +{
> +  void addDevice(in nsIPresentationDevice device);
> +  void removeDevice(in nsIPresentationDevice device);

Passing full device or deviceID are both ok. I'm trying to provide a symmetric API for all add/remove/update event. Do you prefer to use deviceID in removeDevice()?

::: dom/presentation/interfaces/nsIPresentationSessionRequest.idl
@@ +11,5 @@
> +#define PRESENTATION_SESSION_REQUEST_TOPIC "presentation-session-request"
> +%}
> +
> +/*
> + * The event of a device requesting for a presentation session. User can 

Done.

::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
@@ +13,5 @@
> +  /*
> +   * Callback for receiving a message from remote endpoint.
> +   * @param msg The received message.
> +   */
> +  void onMessage(in DOMString msg);

Will add |onBinaryMessage(in ACString msg)| and WebIDL implementation will do the JS wrapping. see comment #14.

@@ +37,5 @@
> +  /*
> +   * Send a message to remote endpiont.
> +   * @param msg The message to send.
> +   */
> +  void postMessage(in DOMString msg);

Will add |postBinaryMessage(in ACString msg)| and |postBinaryStream(in nsIInputStream stream)|. see comment #14.
ni? fabrice about some question in comment #18
Flags: needinfo?(fabrice)
Comment on attachment 8515507 [details] [diff] [review]
part 2, presentation-prompt-b2g.patch

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

::: b2g/chrome/content/presentation-device.js
@@ +27,5 @@
> +      type: device.type,
> +    });
> +  }
> +  log('update device: ' + JSON.stringify(deviceArray));
> +  navigator.mozSettings.createLock().set({"dom.presentation.available-devices": deviceArray});

As our discussion, I'll provide a certified-app-only API for device info.

@@ +49,5 @@
> +
> +(function() {
> +  let devices = presentationDeviceManager.getAvailableDevices().QueryInterface(Ci.nsIArray);
> +  updateDeviceList(devices);
> +})();

Eventually this will be placed in a jsm after I put all the logic in certified-app-only API.

::: b2g/components/B2GPresentationDevicePrompt.js
@@ +25,5 @@
> +  classID: kB2GPRESENTATIONDEVICEPROMPT_CID,
> +  contractID: kB2GPRESENTATIONDEVICEPROMPT_CONTRACTID,
> +  classDescription: "B2G Presentation Device Prompt",
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDevicePrompt]),
> +  _xpcom_factory: XPCOMUtils.generateSingletonFactory(B2GPresentationDevicePrompt),

I'm just being paranoid about unexpected |createInstance()| for this XPCOM. I'm fine to remove this if we don't need this strong guarantee nowadays.

@@ +62,5 @@
> +    let detail = {
> +      type: "presentation-select-device",
> +      origin: request.origin,
> +      requestURL: request.requestURL,
> +      id: requestId,

Device info can still be retrieved from the API I'm going to add, so I think we still don't have to pass it here.
Comment on attachment 8515506 [details] [diff] [review]
part 1, presentation-device-management.patch

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

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +19,5 @@
> +   * @param sessionTransport The transport channel for this session.
> +   */
> +  void onSessionRequest(in DOMString url,
> +                        in DOMString presentationId,
> +                        in nsIPresentationSessionTransport sessionTransport);

By adding |nsIPresentationDevice| as a parameter to this callback, I can make PresentationDeviceManager a single listener to all devices. (which means we can get rid of those DeviceHandle objects). @fabrice, do you think it's a reasonable modification?
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #18)
> > +  size_t
> > +  FindDevice(nsIPresentationDevice* aDevice);
> 
> The index is used in |PresentationDeviceManager::RemoveDevice| to prevent
> extra list traverse. Do you prefer |bool FindDevice(nsIPresentationDevice*
> aDevice, size_t* index)|?

Ha, I missed this one. Feel free to keep your current version.
 
> ::: dom/presentation/interfaces/nsIPresentationDevice.idl
> @@ +57,5 @@
> > +  /*
> > +   * Unhook an event listener
> > +   * @param listener The event listener to remove.
> > +   */
> > +  void unsetListener(in nsIPresentationDeviceEventListener listener);
> 
> Yes, I also prefer single listener. Do you prefer |attribute nsIXXX
> listener| in all the IDLs?

That would be less ambiguous I think.
 
> ::: dom/presentation/interfaces/nsIPresentationDeviceProvider.idl
> @@ +16,5 @@
> > +[scriptable, uuid(7f9f0514-d957-485a-90e8-57cc3acbf15b)]
> > +interface nsIPresentationDeviceListener: nsISupports
> > +{
> > +  void addDevice(in nsIPresentationDevice device);
> > +  void removeDevice(in nsIPresentationDevice device);
> 
> Passing full device or deviceID are both ok. I'm trying to provide a
> symmetric API for all add/remove/update event. Do you prefer to use deviceID
> in removeDevice()?

No strong preference either.

> ::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
> @@ +13,5 @@
> > +  /*
> > +   * Callback for receiving a message from remote endpoint.
> > +   * @param msg The received message.
> > +   */
> > +  void onMessage(in DOMString msg);
> 
> Will add |onBinaryMessage(in ACString msg)| and WebIDL implementation will
> do the JS wrapping. see comment #14.

I'm not sure to understand. Why don't we use something similar to http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Window.webidl#85 here and for postMessage?
 

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #21)
> Comment on attachment 8515506 [details] [diff] [review]
> part 1, presentation-device-management.patch
> 
> Review of attachment 8515506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/presentation/interfaces/nsIPresentationDevice.idl
> @@ +19,5 @@
> > +   * @param sessionTransport The transport channel for this session.
> > +   */
> > +  void onSessionRequest(in DOMString url,
> > +                        in DOMString presentationId,
> > +                        in nsIPresentationSessionTransport sessionTransport);
> 
> By adding |nsIPresentationDevice| as a parameter to this callback, I can
> make PresentationDeviceManager a single listener to all devices. (which
> means we can get rid of those DeviceHandle objects). @fabrice, do you think
> it's a reasonable modification?

Yep!
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #22)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #18)
> > ::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
> > @@ +13,5 @@
> > > +  /*
> > > +   * Callback for receiving a message from remote endpoint.
> > > +   * @param msg The received message.
> > > +   */
> > > +  void onMessage(in DOMString msg);
> > 
> > Will add |onBinaryMessage(in ACString msg)| and WebIDL implementation will
> > do the JS wrapping. see comment #14.
> 
> I'm not sure to understand. Why don't we use something similar to
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Window.webidl#85
> here and for postMessage?
Hmm...allow passing transferable means platform need the ability to serialize/deserialize any DOM/JS object. Sounds like a super powerful API but hard to implement for cross-browser scenario. I'm not quit sure which direction we should take for this communication API (WebSocket-like or window.postMessage-like). Let me post this suggestion to bug 1069230.
update according to review comment.

Although we are still discussing the messaging API in nsIPresentationSessionTransport, we can continue the review because the messaging API is not used in this patch.
Attachment #8515506 - Attachment is obsolete: true
Attachment #8523107 - Flags: review?(fabrice)
Attached patch Part 2, device-info-api.patch (obsolete) — — Splinter Review 
Create a Chrome/Certified-app only API for managing device information. This can be used in browser chrome or System/Settings app for displaying available devices.
Attachment #8515507 - Attachment is obsolete: true
Attachment #8523110 - Flags: review?(khuey)
Attachment #8523110 - Flags: review?(fabrice)
UI Prompt for device selection while establishing presentation session.
Attachment #8523112 - Flags: review?(fabrice)
One memory leak is found while running test_presentation_device_info.html (leaking PresentationDeviceManager). I'm still finding the root cause but I don't want to hold the review process.
Attached image Presentation Architecture.png (obsolete) — 
update architecture diagram
Attachment #8505424 - Attachment is obsolete: true
found leak in StaticRefPtr and fix by not manually maintain singleton reference. Use PresentationDeviceManager as an XPCOM service is sufficient (let XPCOM to maintain the singleton reference).
Attachment #8523107 - Attachment is obsolete: true
Attachment #8523107 - Flags: review?(fabrice)
Attachment #8523193 - Flags: review?(fabrice)
fix deinit procedure in PresentationDeviceInfoManager.js.
Attachment #8523110 - Attachment is obsolete: true
Attachment #8523110 - Flags: review?(khuey)
Attachment #8523110 - Flags: review?(fabrice)
Attachment #8523316 - Flags: review?(khuey)
Attachment #8523316 - Flags: review?(fabrice)
Comment on attachment 8523316 [details] [diff] [review]
Part 2, device-info-api.patch, v1.1

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

Note: Forget to update dom/tests/mochitest/general/test_interfaces.html. Will include it in next revision.
Update DOM interface test cases.
Attachment #8523316 - Attachment is obsolete: true
Attachment #8523316 - Flags: review?(khuey)
Attachment #8523316 - Flags: review?(fabrice)
Attachment #8524057 - Flags: review?(khuey)
Attachment #8524057 - Flags: review?(fabrice)
fix the warning "PresentationDeviceManager does not implement nsIObserver" at start-up.
Attachment #8523193 - Attachment is obsolete: true
Attachment #8523193 - Flags: review?(fabrice)
Attachment #8525007 - Flags: review?(fabrice)
Should use the full permission name in WebIDL CheckPermissions.
Attachment #8524057 - Attachment is obsolete: true
Attachment #8524057 - Flags: review?(khuey)
Attachment #8524057 - Flags: review?(fabrice)
Attachment #8525012 - Flags: review?(khuey)
Attachment #8525012 - Flags: review?(fabrice)
device discovery protocol for demo only
necessary platform hack before Presentation WebIDL implementation is ready, used by Presentation API polyfill [1].

[1] http://people.mozilla.org/~schien/presentation-demo/js/
Target Milestone: --- → 2.2 S1 (5dec)
Comment on attachment 8525007 [details] [diff] [review]
Part 1, presentation-device-management.patch, v2.2

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

lgtm, except that I'm not sure about the session transport api.

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +14,5 @@
> +interface nsIPresentationDeviceEventListener : nsISupports
> +{
> +  /*
> +   * Callback while the remote device is requesting to start a presentation session.
> +   * @param url The URL requested to open by remote device. app:// is allowed.

nit: remove the "app:// is allowed"

@@ +45,5 @@
> +  attribute nsIPresentationDeviceEventListener listener;
> +
> +  /*
> +   * Establish a transport channel to this device.
> +   * @param url The URL requested to open by remote device. app:// is allowed.

here too.

::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
@@ +57,5 @@
> +   * Send a binary message to remote endpiont. |onBinaryMessage| should
> +   * be invoked on remote endpoint.
> +   * @param msg The binary message to send.
> +   */
> +  void postBinaryMessage(in ACString msg);

ACString is not safe if there's a null character right?
Attachment #8525007 - Flags: review?(fabrice) → feedback+
Comment on attachment 8525012 [details] [diff] [review]
Part 2, device-info-api.patch, v1.3

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

r- because I don't think the event is good enough, and I'd like to know why we use the settings permission here.

::: dom/presentation/PresentationDeviceInfoManager.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> +
> +function log(msg) {

nit: I won't insist, but we usually use aParam in chrome JS, like in c++.

@@ +40,5 @@
> +    log("receive msg: " + msg.name);
> +    switch (msg.name) {
> +      case "PresentationDeviceInfoManager:OnDeviceChange": {
> +        let event = new this._window.Event("devicechange");
> +        this.__DOM_IMPL__.dispatchEvent(event);

This would be better to have an event that actually sends the device that changed.

@@ +44,5 @@
> +        this.__DOM_IMPL__.dispatchEvent(event);
> +        break;
> +      }
> +      case "PresentationDeviceInfoManager:GetAll:Result:Ok": {
> +        if (!msg.json) {

Use msg.data instead, and keep a scoped ref.

::: dom/webidl/PresentationDeviceInfoManager.webidl
@@ +12,5 @@
> +
> +[NavigatorProperty="mozPresentationDeviceInfos",
> + JSImplementation="@mozilla.org/presentation-device/deviceInfos;1",
> + Pref="dom.presentation.enabled",
> + CheckPermissions="settings-read settings-write"]

why are we using these permissions?
Attachment #8525012 - Flags: review?(fabrice) → review-
So, reading again part1, when a device is removed or added we just trigger a "presentation-device-change" observer notification with no indication about the device. The notification uses the device manager as the subject, maybe we could use the device itself instead?
Comment on attachment 8523112 [details] [diff] [review]
Part 3, presentation-prompt-b2g.patch

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

::: b2g/components/B2GPresentationDevicePrompt.js
@@ +30,5 @@
> +  // nsIPresentationDevicePrompt
> +  promptDeviceSelection: function(request) {
> +    let self = this;
> +    let requestId = Cc["@mozilla.org/uuid-generator;1"]
> +                  .getService(Ci.nsIUUIDGenerator).generateUUID().toString();

nit: align .getService with ["@mozilla.org...]

@@ +62,5 @@
> +      type: "presentation-select-device",
> +      origin: request.origin,
> +      requestURL: request.requestURL,
> +      id: requestId,
> +    };

hm, so we don't send the device list here? Does that mean that the system app will use getAll() etc. from part 2 to show the device picker?

@@ +83,5 @@
> +    return null;
> +  },
> +};
> +
> +//module initialization

nit: remove this comment.

::: b2g/components/test/mochitest/test_presentation_device_prompt.html
@@ +70,5 @@
> +  });
> +}
> +
> +function testSelectedNotExisted() {
> +  info('test selected device is not existed');

nit: s/is not existed/doesn't exist

@@ +78,5 @@
> +      ok(true, 'receive user prompt for device selection');
> +      let response = {
> +        id: detail.id,
> +        type: 'presentation-select-deny',
> +        deviceId: undefined, // simulate device Id that is not existed

here too
Attachment #8523112 - Flags: review?(fabrice) → feedback+
Comment on attachment 8525012 [details] [diff] [review]
Part 2, device-info-api.patch, v1.3

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

::: dom/presentation/PresentationDeviceInfoManager.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
> +
> +function log(msg) {

Ok, will update in next revision.

@@ +40,5 @@
> +    log("receive msg: " + msg.name);
> +    switch (msg.name) {
> +      case "PresentationDeviceInfoManager:OnDeviceChange": {
> +        let event = new this._window.Event("devicechange");
> +        this.__DOM_IMPL__.dispatchEvent(event);

I was trying to make this API super simple and easy to synchronize the device list in client-side JS. Here is actually an one-to-one mapping of the interface we have in PresentationDeviceManager. I can make it send updated device info since we are going to change the event interface in PresentationDeviceManager.

Here should be a CustomEvent("devicechange") with following detail:
{
  type: "add" or "update" or "remote",
  deviceInfo: {id, name, type}, //latest value for add/update, last known value for remove
}

Or do you want me to create a PresentationDeviceChangeEvent in WebIDL?

@@ +44,5 @@
> +        this.__DOM_IMPL__.dispatchEvent(event);
> +        break;
> +      }
> +      case "PresentationDeviceInfoManager:GetAll:Result:Ok": {
> +        if (!msg.json) {

Done.

::: dom/webidl/PresentationDeviceInfoManager.webidl
@@ +12,5 @@
> +
> +[NavigatorProperty="mozPresentationDeviceInfos",
> + JSImplementation="@mozilla.org/presentation-device/deviceInfos;1",
> + Pref="dom.presentation.enabled",
> + CheckPermissions="settings-read settings-write"]

This API is intended to be used by System app and Settings app. Do you think it will be better to introduce another permission?
I can add:
  presentation-device-manage: {
    app: DENY_ACTION,
    trusted: DENY_ACTION,
    privileged: DENY_ACTION,
    certified: ALLOW_ACTION
  }
Do you think that makes more sense?
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #42)

> I was trying to make this API super simple and easy to synchronize the
> device list in client-side JS. Here is actually an one-to-one mapping of the
> interface we have in PresentationDeviceManager. I can make it send updated
> device info since we are going to change the event interface in
> PresentationDeviceManager.

Cool.

> Here should be a CustomEvent("devicechange") with following detail:
> {
>   type: "add" or "update" or "remote",
>   deviceInfo: {id, name, type}, //latest value for add/update, last known
> value for remove
> }

I don't feel strongly about either solution. Maybe a DOM peer would! Ask Kyle or Jonas.

> This API is intended to be used by System app and Settings app. Do you think
> it will be better to introduce another permission?
> I can add:
>   presentation-device-manage: {
>     app: DENY_ACTION,
>     trusted: DENY_ACTION,
>     privileged: DENY_ACTION,
>     certified: ALLOW_ACTION
>   }
> Do you think that makes more sense?

Yep, I think this is easier to reason about if it's a new permission. This way if we make changes to the settings one there will be no impact on the presentation one.
Comment on attachment 8523112 [details] [diff] [review]
Part 3, presentation-prompt-b2g.patch

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

::: b2g/components/B2GPresentationDevicePrompt.js
@@ +30,5 @@
> +  // nsIPresentationDevicePrompt
> +  promptDeviceSelection: function(request) {
> +    let self = this;
> +    let requestId = Cc["@mozilla.org/uuid-generator;1"]
> +                  .getService(Ci.nsIUUIDGenerator).generateUUID().toString();

Done.

@@ +62,5 @@
> +      type: "presentation-select-device",
> +      origin: request.origin,
> +      requestURL: request.requestURL,
> +      id: requestId,
> +    };

Yes, I don't think we need to pass the same information since we already have an API for it.

@@ +83,5 @@
> +    return null;
> +  },
> +};
> +
> +//module initialization

Done.

::: b2g/components/test/mochitest/test_presentation_device_prompt.html
@@ +70,5 @@
> +  });
> +}
> +
> +function testSelectedNotExisted() {
> +  info('test selected device is not existed');

Done.

@@ +78,5 @@
> +      ok(true, 'receive user prompt for device selection');
> +      let response = {
> +        id: detail.id,
> +        type: 'presentation-select-deny',
> +        deviceId: undefined, // simulate device Id that is not existed

Done.
Comment on attachment 8525007 [details] [diff] [review]
Part 1, presentation-device-management.patch, v2.2

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

::: dom/presentation/interfaces/nsIPresentationDevice.idl
@@ +14,5 @@
> +interface nsIPresentationDeviceEventListener : nsISupports
> +{
> +  /*
> +   * Callback while the remote device is requesting to start a presentation session.
> +   * @param url The URL requested to open by remote device. app:// is allowed.

Done.

@@ +45,5 @@
> +  attribute nsIPresentationDeviceEventListener listener;
> +
> +  /*
> +   * Establish a transport channel to this device.
> +   * @param url The URL requested to open by remote device. app:// is allowed.

Done.

::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
@@ +57,5 @@
> +   * Send a binary message to remote endpiont. |onBinaryMessage| should
> +   * be invoked on remote endpoint.
> +   * @param msg The binary message to send.
> +   */
> +  void postBinaryMessage(in ACString msg);

According to [1] ACString is safe from null character. We've also used the ACString in WebSocketChannel.sendBinaryMsg().
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsACString
update according to review comment, hold review because the discussion on socket transport interface is not finalized yet.
Attachment #8525007 - Attachment is obsolete: true
update according to review comment, use CustomEvent for devicechange event.
Attachment #8525012 - Attachment is obsolete: true
Attachment #8525012 - Flags: review?(khuey)
Attachment #8527228 - Flags: review?(khuey)
Attachment #8527228 - Flags: review?(fabrice)
update according to review comment.
Attachment #8523112 - Attachment is obsolete: true
Attachment #8527229 - Flags: review?(fabrice)
Attachment #8527229 - Flags: review?(fabrice) → review+
Comment on attachment 8527228 [details] [diff] [review]
Part 2, device-info-api.patch, v2

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

My next revision will cover following updates.

::: dom/base/test/test_hasFeature.html
@@ +43,5 @@
>      { name: "Navigator.mozTCPSocket", enabled: pref("dom.mozTCPSocket.enabled") },
>      { name: "Navigator.mozInputMethod", enabled: pref("dom.mozInputMethod.enabled") },
>      { name: "Navigator.mozMobileConnections", enabled: pref("dom.mobileconnection.enabled") },
>      { name: "Navigator.getMobileIdAssertion", enabled: b2gOnly },
> +    { name: "Navigator.mozMobileConnections", enabled: pref("dom.presentation.enabled") },

Copy&paste error always happens...Will change to "Navigator.mozPresentationDeviceInfos".

::: dom/tests/mochitest/general/test_interfaces.html
@@ +881,5 @@
>      {name: "PopupBoxObject", xbl: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "PresentationDeviceInfoManager",
> +     pref: "dom.presentation.enabled",
> +     permission: ["settings-read", "settings-write"]},

Forgot to change the permission list to "presentation-device-manage".
minor update for adding nsIObserver in QueryInterface.
Attachment #8527227 - Attachment is obsolete: true
minor update for fixing test cases according to comment #49.
Attachment #8527228 - Attachment is obsolete: true
Attachment #8527228 - Flags: review?(khuey)
Attachment #8527228 - Flags: review?(fabrice)
Comment on attachment 8532607 [details] [diff] [review]
Part 2, device-info-api.patch, v2.1

Ready to be reviewed!
Attachment #8532607 - Flags: review?(khuey)
Attachment #8532607 - Flags: review?(fabrice)
Per discussion with Jonas, we really want leverage WebRTC as a communication channel between presentation endpoints. I change nsIPresentationSessionTransport to nsIPresentationControlChannel. nsIPresentationControlChannel will be the interface for exchanging offer and answer. This control channel can be closed after RTCPeerConnection is established.
We can resolve the debate on socket transport interface if we take this approach. How do you guys think?
Attachment #8523123 - Attachment is obsolete: true
Attachment #8535771 - Flags: feedback?(jonas)
Attachment #8535771 - Flags: feedback?(fabrice)
Attached file nsIPresentationControlChannel.idl (obsolete) — 
The XPIDL definition according to the architecture in comment #55.
Attachment #8536248 - Flags: feedback?(jonas)
Attachment #8536248 - Flags: feedback?(fabrice)
Comment on attachment 8532607 [details] [diff] [review]
Part 2, device-info-api.patch, v2.1

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

::: dom/presentation/PresentationDeviceInfoManager.jsm
@@ +33,5 @@
> +  init: function() {
> +    log("init");
> +    ppmm.addWeakMessageListener("PresentationDeviceInfoManager:GetAll", this);
> +    ppmm.addWeakMessageListener("PresentationDeviceInfoManager:ForceDiscovery", this);
> +    Services.obs.addObserver(this, "presentation-device-change", true);

This is a service, so it already lives until shutdown.  There's no point in doing weak references here.

::: dom/webidl/PresentationDeviceInfoManager.webidl
@@ +9,5 @@
> +  DOMString name;
> +  DOMString type;
> +};
> +
> +[NavigatorProperty="mozPresentationDeviceInfos",

Is this really meant to have the trailing s?

@@ +14,5 @@
> + JSImplementation="@mozilla.org/presentation-device/deviceInfos;1",
> + Pref="dom.presentation.enabled",
> + CheckPermissions="presentation-device-manage"]
> +interface PresentationDeviceInfoManager : EventTarget {
> +  // notify if any device udpated.

updated

::: mobile/android/installer/package-manifest.in
@@ +639,5 @@
>  @BINPATH@/components/WebappsUpdateTimer.js
>  @BINPATH@/components/DataStore.manifest
>  @BINPATH@/components/DataStoreImpl.js
>  @BINPATH@/components/dom_datastore.xpt
> +

Extra \n?
Attachment #8532607 - Flags: review?(khuey) → review+
Comment on attachment 8532607 [details] [diff] [review]
Part 2, device-info-api.patch, v2.1

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

::: dom/presentation/PresentationDeviceInfoManager.jsm
@@ +33,5 @@
> +  init: function() {
> +    log("init");
> +    ppmm.addWeakMessageListener("PresentationDeviceInfoManager:GetAll", this);
> +    ppmm.addWeakMessageListener("PresentationDeviceInfoManager:ForceDiscovery", this);
> +    Services.obs.addObserver(this, "presentation-device-change", true);

Done.

::: dom/webidl/PresentationDeviceInfoManager.webidl
@@ +9,5 @@
> +  DOMString name;
> +  DOMString type;
> +};
> +
> +[NavigatorProperty="mozPresentationDeviceInfos",

This attribute represents an aggregation of device information. I can remove it if it doesn't make sense to native English speakers.

@@ +14,5 @@
> + JSImplementation="@mozilla.org/presentation-device/deviceInfos;1",
> + Pref="dom.presentation.enabled",
> + CheckPermissions="presentation-device-manage"]
> +interface PresentationDeviceInfoManager : EventTarget {
> +  // notify if any device udpated.

Done.

::: mobile/android/installer/package-manifest.in
@@ +639,5 @@
>  @BINPATH@/components/WebappsUpdateTimer.js
>  @BINPATH@/components/DataStore.manifest
>  @BINPATH@/components/DataStoreImpl.js
>  @BINPATH@/components/dom_datastore.xpt
> +

Removed.
Attachment #8535771 - Flags: feedback?(fabrice) → feedback+
Attachment #8536248 - Attachment is patch: true
Attachment #8536248 - Attachment mime type: text/x-idl → text/plain
Attachment #8536248 - Attachment is patch: false
Comment on attachment 8536248 [details]
nsIPresentationControlChannel.idl

A couple of questions:
- what's the format of the offer and answer? Are they opaque strings?
- "All the events should be pending until listener is assigned." Is the plan to buffer events until we can dispatch them to the listener? Would it be simpler to just make sendOffer(), sendAnswer() and close() fail if there's no listener set?
Attachment #8536248 - Flags: feedback?(fabrice) → feedback+
Attachment #8532607 - Flags: review?(fabrice) → review+
(In reply to Fabrice Desré [:fabrice] from comment #59)
> Comment on attachment 8536248 [details]
> nsIPresentationControlChannel.idl
> 
> A couple of questions:
> - what's the format of the offer and answer? Are they opaque strings?
My plan is sending a JSON string that contains bootstrap information for transport channel, such as:

  { "tcp": <port number> } or { "data-channel": <SDP for DataChannel> }

Do you think it'll be better to create another XPCOM interface for this?

> - "All the events should be pending until listener is assigned." Is the plan
> to buffer events until we can dispatch them to the listener? Would it be
> simpler to just make sendOffer(), sendAnswer() and close() fail if there's
> no listener set?

The problem I try to solve is that listener on the remote endpoint will only be set asynchronously. Therefore the "onOffer" and "notifyClose" event might arrive earlier than listener being set. That's why I add this comment about buffering event . Making those functions fail before setting listener still not solve the problem.
Flags: needinfo?(fabrice)
update according to review comments, carry r+.
Attachment #8532607 - Attachment is obsolete: true
Attachment #8538153 - Flags: review+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #60)

> My plan is sending a JSON string that contains bootstrap information for
> transport channel, such as:
> 
>   { "tcp": <port number> } or { "data-channel": <SDP for DataChannel> }
> 
> Do you think it'll be better to create another XPCOM interface for this?

Well, if we ever have c++ code that has to deal with that, parsing json will be quite ugly. So I would prefer an xpcom interface or a webidl dict.


> The problem I try to solve is that listener on the remote endpoint will only
> be set asynchronously. Therefore the "onOffer" and "notifyClose" event might
> arrive earlier than listener being set. That's why I add this comment about
> buffering event . Making those functions fail before setting listener still
> not solve the problem.

ok, got it!
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #62)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #60)
> 
> > My plan is sending a JSON string that contains bootstrap information for
> > transport channel, such as:
> > 
> >   { "tcp": <port number> } or { "data-channel": <SDP for DataChannel> }
> > 
> > Do you think it'll be better to create another XPCOM interface for this?
> 
> Well, if we ever have c++ code that has to deal with that, parsing json will
> be quite ugly. So I would prefer an xpcom interface or a webidl dict.
> 
Parsing json shouldn't be so hard in c++ while using nsJSON, but I agree that having an interface will help device protocol developer understand the purpose.

I'm not sure how to pass a webidl dict in XPIDL but not end up with another opaque "jsval" parameter. I can imaging all device protocol implementation will simply stringify this parameter, which makes this approach not better than the opaque string one.

So using XPCOM interface will be a more reasonable solution to me. However we don't support optional attribute, we can only add the invalid value in the comment like this:

interface nsIPresentationChannelDescription: nsISupports
{
  // any invalid port number means TCP is not supported
  attribute uint16_t tcpPort;

  // empty string means Data Channel is not supported
  [Null(Empty)] attribute DOMString dataChannelSDP;
};

How do you think?
Flags: needinfo?(fabrice)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #63)
> (In reply to Fabrice Desré [:fabrice] from comment #62)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #60)
> > 
> > > My plan is sending a JSON string that contains bootstrap information for
> > > transport channel, such as:
> > > 
> > >   { "tcp": <port number> } or { "data-channel": <SDP for DataChannel> }
> > > 
> > > Do you think it'll be better to create another XPCOM interface for this?
> > 
> > Well, if we ever have c++ code that has to deal with that, parsing json will
> > be quite ugly. So I would prefer an xpcom interface or a webidl dict.
> > 
> Parsing json shouldn't be so hard in c++ while using nsJSON, but I agree
> that having an interface will help device protocol developer understand the
> purpose.
> 
> I'm not sure how to pass a webidl dict in XPIDL but not end up with another
> opaque "jsval" parameter. I can imaging all device protocol implementation
> will simply stringify this parameter, which makes this approach not better
> than the opaque string one.
> 
> So using XPCOM interface will be a more reasonable solution to me. However
> we don't support optional attribute, we can only add the invalid value in
> the comment like this:
> 
> interface nsIPresentationChannelDescription: nsISupports
> {
>   // any invalid port number means TCP is not supported
>   attribute uint16_t tcpPort;
> 
>   // empty string means Data Channel is not supported
>   [Null(Empty)] attribute DOMString dataChannelSDP;
> };
> 
> How do you think?
Oh, actually we also need to include IP address for TCP channel and there might be more than one IP address available. In this case we'll need to send all available IP addresses to remote endpoint and remote endpoint will iterate through all the addresses to find out the correct address for establishing TCP socket.

Therefore the interface will be like:

interface nsIPresentationChannelDescription: nsISupports
{
  // empty array means TCP is not supported
  readonly attribute nsIArray tcpAddresses;

  // any invalid port number means TCP is not supported
  readonly attribute uint16_t tcpPort;

  // empty string means Data Channel is not supported
  readonly attribute DOMString dataChannelSDP;
};
Instead of relying on the array being empty or other magic value, why not add an attribute that tells which kind of channel description we deal with?
Flags: needinfo?(fabrice)
update according to latest architecture.
Attachment #8532605 - Attachment is obsolete: true
Attachment #8536248 - Attachment is obsolete: true
Attachment #8536248 - Flags: feedback?(jonas)
Attachment #8540409 - Flags: review?(fabrice)

        Attachment #8532609 -
        Attachment is obsolete: true

        Attachment #8532610 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8538153 [details] [diff] [review]
Part 2, device-info-api.patch, v3

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

::: dom/presentation/PresentationDeviceInfoManager.js
@@ +102,5 @@
> +  },
> +
> +  getAll: function() {
> +    log("getAll");
> +    let self = this;

Will remove this useless statement before landing.

    
    

    
  
Comment on attachment 8540409 [details] [diff] [review]
Part 1, presentation-device-management.patch, v4

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

lgtm, thanks!
Can you confirm that this code only runs in the parent process and that the ipc is managed by the webidl api implementation?

::: dom/presentation/interfaces/nsIPresentationControlChannel.idl
@@ +10,5 @@
> +[scriptable, uuid(ae318e05-2a4e-4f85-95c0-e8b191ad812c)]
> +interface nsIPresentationChannelDescription: nsISupports
> +{
> +  const unsigned short TYPE_TCP = 1;
> +  const unsigned short TYEP_DATACHANNEL = 2;

typo: should be TYPE_DATACHANNEL

@@ +15,5 @@
> +
> +  // Type of transport channel.
> +  readonly attribute uint8_t type;
> +
> +  // Address for TCP channel.

Addresses since this is an array

@@ +16,5 @@
> +  // Type of transport channel.
> +  readonly attribute uint8_t type;
> +
> +  // Address for TCP channel.
> +  // Should only be used while type == TCP.

s/TCP/TYPE_TCP also below

::: dom/presentation/interfaces/nsIPresentationSessionRequest.idl
@@ +20,5 @@
> +{
> +  // The device which requesting the presentation session.
> +  readonly attribute nsIPresentationDevice device;
> +
> +  // The URL requested to open by remote device. app:// is allowed.

nit: remove the app:// part.

        Attachment #8540409 -
        Flags: review?(fabrice) → review+

    
    

    
  
(In reply to Fabrice Desré [:fabrice] from comment #69)
> 
> lgtm, thanks!
> Can you confirm that this code only runs in the parent process and that the
> ipc is managed by the webidl api implementation?
> 
Yes, the device management should only runs on chrome process. I've put several |MOZ_ASSERT(NS_IsMainThread())| in this patch.

    
    

    
  
Comment on attachment 8538153 [details] [diff] [review]
Part 2, device-info-api.patch, v3

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

::: dom/presentation/PresentationDeviceInfoManager.js
@@ +102,5 @@
> +  },
> +
> +  getAll: function() {
> +    log("getAll");
> +    let self = this;

Actually, this line is necessary. mis-read my own code. LOL

    
    

    
  


  
update according to review comment, carry r+.

        Attachment #8540409 -
        Attachment is obsolete: true

        Attachment #8547416 -
        Flags: review+

    
    

    
  


  
rebase to latest m-c, carry r+.

        Attachment #8538153 -
        Attachment is obsolete: true

        Attachment #8547417 -
        Flags: review+

    
    

    
  


  
rebase to latest m-c, carry r+.

        Attachment #8527229 -
        Attachment is obsolete: true

        Attachment #8547421 -
        Flags: review+

    
    

    
  
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #75)
> try result:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fda4af51995d

Mac build fail due to "@RESPATH@" introduced in Bug 1096494. For Firefox build I should use @RESPATH@ instead of @BINPATH@ in package-manifest.in.

    
    

    
  


  
update according to comment #76, carry r+.

        Attachment #8547416 -
        Attachment is obsolete: true

        Attachment #8547896 -
        Flags: review+

    
    

    
  


  
update according to comment #76, carry r+.

        Attachment #8547417 -
        Attachment is obsolete: true

        Attachment #8547898 -
        Flags: review+

    
    

    
  
try looks good now, ready to land.
Keywords: checkin-needed

        Attachment #8540410 -
        Attachment is obsolete: true
Depends on: 1121566

    
    

    
  
Comment on attachment 8540410 [details] [diff] [review]
[demo-only] Part 4, demo-device-protocol.patch

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

Though this attachment is obsoleted, I've tried this patch and find some error.
FWIW I list them below.

::: dom/presentation/provider/DemoProvider.js
@@ +298,5 @@
> +  sendOffer: function(offer) {
> +    let offerInit = this._convertToJson(offer);
> +    let offerInit = {
> +       type: offer.type,
> +       tcpAddress

Here's a duplicate for offerInit and a wrong JSON format here.

@@ +309,5 @@
> +    }
> +    presentationServer.sendOffer(this._device, this._presentationId, offerInit);
> +  },
> +  sendAnswer: function(answer) {
> +    let answerInit == this._convertToJson(answer);

s/==/=/

    
    

    
  
Comment on attachment 8535771 [details]
PresentationDevice Architecture.png

I don't really know the code here well enough to have an opinion. But it seems like this moved forward fine without me which is great :)

        Attachment #8535771 -
        Flags: feedback?(jonas)
Depends on: 1351089
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: