1069230 - [Stingray] Presentation API webidl implementation

Status: RESOLVED FIXED

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

Description

[Kilik Kuo [:kikuo] (inactive)]

Considering the interactions in user story between primary screen & 2nd screen.
Intent to implement Presentaion API [1] draft version.
And based on the discussion from mail thread [2] & [3]

[1] http://webscreens.github.io/presentation-api/
[2] http://lists.w3.org/Archives/Public/public-webscreens/2014Aug/0003.html
[3] https://groups.google.com/forum/#!topic/mozilla.dev.webapi/Rjvu-cBJ1TY

Comment 1

[Marco Chen [:mchen]]

Kilik is working on it a while.

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP] PresentationAPI WebIDL Implemetation w/o (1. Socketchannel 2. Test case)

Hi all,

This patch is basically according to current PresentationAPI PR [1], and discussion in Second Screen Presentation Community Group [2].

Though there are still many open issues under discussion, such as 
returning Promise object for startSession & joinSession instead of PresentationSession object. We will keep going on.

Here's is a version with returning PresentationSession object first, and 
will be modified once the conclusion is getting clear. 

[1] http://www.w3.org/2014/secondscreen/presentation-api/20140721/
[2] http://lists.w3.org/Archives/Public/public-webscreens/

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [PresentationAPI] Content_Chrome Architecture (Controlling UA).svg

This is the architecture right now.
Our goal is to fit it in both desktop browser & b2g.
Classes with background in white will be implemented in Bug 1080474.
And I'll try to integrate them soon.

This is my first time implementing WebAPI, lot of things to learn.
Any suggestions will be greatly appreciated : )
Thanks.

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP]_PresentationAPI_Implementation

Separate WebIDL files & Implementation in different patches.

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP]_PresentationAPI_WebIDL.diff

Separate WebIDL and Implementation in 2 patches.

There're 2 open issues I'd like to address.

1) PresentationSeesion v.s. Promise
In this [WIP] patch, currently returning PresentationSession object for startSession/joinSession. After going through the discussion on CG, I believe that returning Promise would be more easier for web developers (it's also possible to provide mete-data back when the promise is resolved/rejected).
So, I guess I'll do a modification soon changing it to Promise.

2) Message passing API
The goal is to be RTCDataChannel-like, but only to be a subset of it. Since we'd like to make it more simple for web usages and backward compatible in v1.
Jonas has provided a reveal as follows
======================================
interface PresentationAPISocketChannelWithATwist {
  void send((DOMString or Blob or ArrayBuffer or ArrayBufferView) data);
  void close ();

  attribute BinaryType binaryType;
  readonly attribute unsigned long bufferedAmount;
  readonly attribute ?? readyState; // unclear which one we should follow.
                                    // [kilik] Maybe hide this one, and use the original PresentationSessionState ?
  attribute EventHandler onmessage;
  attribute EventHandler onopen;
  attribute EventHandler onerror;
  attribute EventHandler onclose;
};
======================================
But it is still not merged into current draft report yet.
The interface in this patch is currently aligned to PR.

And I believe attributes/methods in PresentationAPISocketChannelWithATwist should be integrated into PresentationSession because it's not necessary to expose another socketchannel object to web developers, right ?

Maybe I could apply the modification mentioned above into my next WIP patch, and ask the feedback from CG.
I'm wondering if I'm not misunderstanding the spirit of the design :)

Would you please give me some suggestions, Jonas ?
Thanks

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP]_Part1_WebIDL_1026.diff

1. Using Promise as the type of returning object for StartSession & JoinSession.
2. Add the PresentationSocketChannel interface (Not integrated yet)

Since there's no conclusion yet in current WG draft[1] regarding the signature of postMessage (only string message or include binary data). 
For FxOS, I'm wondering if it is acceptable to add a partial interface like below for capability, and only enable it under certain condition ? 
//=========================================
partial interface PresentationSession {
  readonly attribute PresentationSocketChannel socketchannel;
};

interface PresentationSocketChannel // Defined in the patch file.
//=========================================

Hello Jonas,
Sorry to bother you, but it would be great if you could give me any suggestions or concerns about these WebIDL, thanks  


[1] http://webscreens.github.io/presentation-api/

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP]_Part2_Implementation_1026.diff

1. Integrating device selecting prompt mechanism with S.C.'s [WIP] pathc in Bug 1080474
2. Returning Promise object for StartSession / JoinSession
3. Add PresentationSocketChannelInternal Implementation for PresentatoinSessionInternal in chrome process.

TODO :
1. Wrap stuff for JoinSession in PresentationManager into a runnable.
2. Implement PresentationSocketChannel in content process.
3. Handle PresentationSessionRequest from Remote
4. Fixing minor logic.

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Content_Chrome Architecture (Controlling UA)_1026.png

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP] PresentationAPI_WebIDL(NavigatorPresentation/PresentationSession/PresentationSocketChannel)

An attribute PresentationSocketChannel in partial interface PresentationSession was added.

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP]_PresentationAPI_Implementation

TODO : 
1) PresentationSocketChannel
   - Implementation in content process.
   - Integrate with nsIPresentationDeviceTransport.idl in S.C. patch in chrome process.
2) Launch APP mechanism in Presenting UA.
3) Test cases.

Comment 11

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

Comment on attachment 8512525 [details] [diff] [review]
[WIP]_PresentationAPI_Implementation

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

Nice work! I just go through the header files, IPDL, and some important cpp. Here are some of my suggestions.

::: content/base/src/nsGkAtomList.h
@@ +669,5 @@
>  GK_ATOM(onanimationstart, "onanimationstart")
>  GK_ATOM(onantennaavailablechange, "onantennaavailablechange")
>  GK_ATOM(onAppCommand, "onAppCommand")
>  GK_ATOM(onattributechanged, "onattributechanged")
> +GK_ATOM(onavailablechange, "onavailablechange")

Put it after |onaudioprocess| if you want to keep the alphabet order.

::: dom/ipc/PBrowser.ipdl
@@ +318,5 @@
> +    /**
> +     */
> +    PPresentationSession(nsCString aURL,
> +    					 nsCString aSessionId,
> +    					 nsCString aDeviceId);

I suppose |PNavigatorPresentation| and |PPresentationSession| can also be created from parent side for the receiver case.

::: dom/presentation/NavigatorPresentation.cpp
@@ +37,5 @@
> +NavigatorPresentation::NavigatorPresentation(nsPIDOMWindow* aWindow)
> +  : DOMEventTargetHelper(aWindow)
> +  , mWindow(aWindow)
> +{
> +  SetIsDOMBinding();

You don't need to do |SetIsDOMBinding| anymore, see https://bugzilla.mozilla.org/show_bug.cgi?id=1078744.

::: dom/presentation/NavigatorPresentation.h
@@ +31,5 @@
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(NavigatorPresentation, DOMEventTargetHelper)
> +  NavigatorPresentation(nsPIDOMWindow* aWindow);
> +  void Init();

nit: use following coding style in here and below.

void
Init();

@@ +54,5 @@
> +private:
> +  IPresentationManager* GetIPM();
> +  void CreateSessionForRemoteScreen();
> +
> +private:

nit: merge these public/private section together.

@@ +56,5 @@
> +  void CreateSessionForRemoteScreen();
> +
> +private:
> +  ~NavigatorPresentation();
> +  nsCOMPtr<nsPIDOMWindow> mWindow;

DOMEventTargetHelper already stores the window object. You can use |GetOwner()| instead of keeping the reference by yourself.

@@ +57,5 @@
> +
> +private:
> +  ~NavigatorPresentation();
> +  nsCOMPtr<nsPIDOMWindow> mWindow;
> +  nsAutoPtr<IPresentationManager> mIPM;

|mManager| should be a better name.

@@ +60,5 @@
> +  nsCOMPtr<nsPIDOMWindow> mWindow;
> +  nsAutoPtr<IPresentationManager> mIPM;
> +
> +  // Only be assigned on remote screen.
> +  nsRefPtr<PresentationSession> mSession;

Does it mean only one session can be created in a page? That doesn't sound a correct assumption to me.

::: dom/presentation/PresentationCommon.h
@@ +3,5 @@
> +
> +#include "mozilla/dom/PresentationSessionBinding.h"
> +#include "mozilla/Observer.h"
> +
> +#define BEGIN_PRESENTATION_NAMESPACE \

I'm not a big fan of using macro on namespace declaration.

::: dom/presentation/PresentationManager.cpp
@@ +140,5 @@
> +}
> +
> +nsresult
> +PresentationManager::HandleCreateSessionRequest(nsRefPtr<PresentationSessionRequest> &aReq)
> +{

Remember that launching app/page is handled differently on different platform (here I mean Desktop/Fennec/B2G). Be sure your implementation here can adapt to all platforms.
On B2G, you'll have to delegate the app/page opening request to SystemApp.

::: dom/presentation/PresentationManager.h
@@ +83,5 @@
> +    nsCString mURL;
> +    nsCString mSessionId;
> +  };
> +
> +  friend class PresentationFindSessionRunnable;

I thought inner class can already accesses the private scope of outer class?

::: dom/presentation/PresentationSocketChannelInternal.cpp
@@ +36,5 @@
> +                                               bool aIsBinary)
> +{
> +  MOZ_ASSERT(mPSTransport);
> +  // TODO : Passing aMsg/aIsBinary to Transport
> +  nsresult rv = mPSTransport->PostMessage();

Remember to tell me what kind of interface you want for |postMessage|. :)

@@ +44,5 @@
> +  return rv;
> +}
> +
> +NS_IMETHODIMP
> +PresentationSocketChannelInternal::OnMessage()

Remember to tell me what kind of interface you want for |onMessage|. :)

::: dom/presentation/PresentationSocketChannelInternal.h
@@ +11,5 @@
> +#include "PresentationCommon.h"
> +
> +BEGIN_PRESENTATION_NAMESPACE
> +
> +// TODO : Asking S.C, if the listner should include state method, like open/close?

I can add anything as long as you need it. :) I suppose you would want something like |NotifyOpened| and |NotifyClosed|?

::: dom/presentation/ipc/PNavigatorPresentation.ipdl
@@ +12,5 @@
> +  manager PBrowser;
> +child:
> +  async NotifyDeviceAvailableStatus(bool available);
> +  async NotifyDeviceSeletedStatus(nsCString origin,
> +  								  nsCString url,

nit: alignment.

::: dom/presentation/ipc/PPresentationSession.ipdl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +include protocol PBrowser;
> + 

nit: remove tailing space.

@@ +14,5 @@
> +child:
> +  async UpdateConnectionStatus(nsCString stateMsg);
> +
> +both:
> +  async MsgViaConnection(nsCString infoMsg, bool isBinary);

Name it |Message| should be enough, all the messages are via connections, aren't they? ;)

::: dom/presentation/ipc/moz.build
@@ +18,5 @@
> +    'PresentationSessionParent.cpp',
> +]
> +
> +LOCAL_INCLUDES += [
> +    '/dom/presentation',

I saw most header files under dom/presentation has been exported. You can just do |#include "mozilla/dom/XXX.h"| or |#include "mozilla/dom/presentation/XXX.h"| without adding LOCAL_INCLUDES.
Or, you can remove some internal header files, e.g. PresentationSessionInternal.h, from the export list and keep this LOCAL_INCLUDES.

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Great thanks for reviewing, S.C. !! 

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #11)
> ::: dom/ipc/PBrowser.ipdl
> @@ +318,5 @@
> > +    /**
> > +     */
> > +    PPresentationSession(nsCString aURL,
> > +    					 nsCString aSessionId,
> > +    					 nsCString aDeviceId);
> 
> I suppose |PNavigatorPresentation| and |PPresentationSession| can also be
> created from parent side for the receiver case.

Here's my mental model - 
https://drive.google.com/file/d/0BzDHy5K9HnUKU2hZQWpqVXJaWkU/view?usp=sharing

After Step 1, I'd like to create corresponding |PresentationSessionInteral/PresentationSocketChannelInteranl| first, and make sure the state being connected. Then trying to launch App or open new browser window to load the url passed in.

Now, here's the uncertain part for Step 5.
a) Let |NavigatorPresentation| do a sync ipc call to ask |PresentationManager| if it's triggered as a presenter. and then go step 6 & step 7 (There should be only little difference between controller & presenter).
So, |PNavigatorPresentation| and |PPresentationSession| will be created from child-side, and it's consistent with controller case.

b) After discussed with John Hu, at the end of step 4.2 I presume there's a way for SystemApp(or even gecko?) to obtain the iframe(new window?/launched app), so that |PresentationManager| could ask content process's |Navigator| to create |NavigatorPresentaion| through |TabParent|-|TabChild|.  Am I saying this right ?

But even in this way, I still lean toward |NavigatorPresentaion| & |PresentationSession| as the creator of |PNavigatorPresentation| & |PPresentationSession|

What do you think ?
Or I misunderstand the procedure, please correct me :) 

> 
> @@ +60,5 @@
> > +  nsCOMPtr<nsPIDOMWindow> mWindow;
> > +  nsAutoPtr<IPresentationManager> mIPM;
> > +
> > +  // Only be assigned on remote screen.
> > +  nsRefPtr<PresentationSession> mSession;
> 
> Does it mean only one session can be created in a page? That doesn't sound a
> correct assumption to me.

|mSession| should only be assigned in Presenter UA as the current presenting session. 
In Controller UA, there's an another member variable |mSessionStore| in |NavigatorPresentation| to hold all the sessions created by certain |Navigator|, and we can NOT obtain a session object by using |Navigator.presentation.session|.

> 
> ::: dom/presentation/PresentationManager.cpp
> @@ +140,5 @@
> > +}
> > +
> > +nsresult
> > +PresentationManager::HandleCreateSessionRequest(nsRefPtr<PresentationSessionRequest> &aReq)
> > +{
> 
> Remember that launching app/page is handled differently on different
> platform (here I mean Desktop/Fennec/B2G). Be sure your implementation here
> can adapt to all platforms.
> On B2G, you'll have to delegate the app/page opening request to SystemApp.

Discussed with John Hu, I think it's suitable to send a custom event to SystemApp to launch app/page, like the type 'presentation-launch' in https://github.com/rexboy7/gaia/blob/tv_demo/apps/system/js/app_window_factory.js

I have no idea about launching a tab on desktop browser or Fennec, is there any keyword that I can search for ?

> ::: dom/presentation/PresentationSocketChannelInternal.h
> @@ +11,5 @@
> > +#include "PresentationCommon.h"
> > +
> > +BEGIN_PRESENTATION_NAMESPACE
> > +
> > +// TODO : Asking S.C, if the listner should include state method, like open/close?
> 
> I can add anything as long as you need it. :) I suppose you would want
> something like |NotifyOpened| and |NotifyClosed|?
> 

Exactly :)
I guess I need |NotifyOpened| & |NotfiyClosed| first, maybe |NotifyError| if possible, THANKS ~

Comment 13

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

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #12)
> Great thanks for reviewing, S.C. !! 
> 
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #11)
> > ::: dom/ipc/PBrowser.ipdl
> > @@ +318,5 @@
> > > +    /**
> > > +     */
> > > +    PPresentationSession(nsCString aURL,
> > > +    					 nsCString aSessionId,
> > > +    					 nsCString aDeviceId);
> > 
> > I suppose |PNavigatorPresentation| and |PPresentationSession| can also be
> > created from parent side for the receiver case.
> 
> Here's my mental model - 
> https://drive.google.com/file/d/0BzDHy5K9HnUKU2hZQWpqVXJaWkU/view?usp=sharing
> 
> After Step 1, I'd like to create corresponding
> |PresentationSessionInteral/PresentationSocketChannelInteranl| first, and
> make sure the state being connected. Then trying to launch App or open new
> browser window to load the url passed in.
> 
> Now, here's the uncertain part for Step 5.
> a) Let |NavigatorPresentation| do a sync ipc call to ask
> |PresentationManager| if it's triggered as a presenter. and then go step 6 &
> step 7 (There should be only little difference between controller &
> presenter).
> So, |PNavigatorPresentation| and |PPresentationSession| will be created from
> child-side, and it's consistent with controller case.
> 
> b) After discussed with John Hu, at the end of step 4.2 I presume there's a
> way for SystemApp(or even gecko?) to obtain the iframe(new window?/launched
> app), so that |PresentationManager| could ask content process's |Navigator|
> to create |NavigatorPresentaion| through |TabParent|-|TabChild|.  Am I
> saying this right ?
> 
> But even in this way, I still lean toward |NavigatorPresentaion| &
> |PresentationSession| as the creator of |PNavigatorPresentation| &
> |PPresentationSession|
> 
> What do you think ?
> Or I misunderstand the procedure, please correct me :) 
This means we need to create the instance of NavigatorPresentation after page loading and to query |AmIPresenter| for every document, right? 
We should do lazy loading on these WebIDL object if we could, which is why I assume chrome process will trigger the creation of NavigatorPresentation/PresentationSession on required window/child process.

Or, I just came up with an idea, you can trigger the |AmIPresenter| only if onpresent event handler has been specified. You certainly don't need the pass the session object if no code is trying to use it.
> 
> > 
> > @@ +60,5 @@
> > > +  nsCOMPtr<nsPIDOMWindow> mWindow;
> > > +  nsAutoPtr<IPresentationManager> mIPM;
> > > +
> > > +  // Only be assigned on remote screen.
> > > +  nsRefPtr<PresentationSession> mSession;
> > 
> > Does it mean only one session can be created in a page? That doesn't sound a
> > correct assumption to me.
> 
> |mSession| should only be assigned in Presenter UA as the current presenting
> session. 
> In Controller UA, there's an another member variable |mSessionStore| in
> |NavigatorPresentation| to hold all the sessions created by certain
> |Navigator|, and we can NOT obtain a session object by using
> |Navigator.presentation.session|.
> 
Make sense, thanks for the explanation!
> > 
> > ::: dom/presentation/PresentationManager.cpp
> > @@ +140,5 @@
> > > +}
> > > +
> > > +nsresult
> > > +PresentationManager::HandleCreateSessionRequest(nsRefPtr<PresentationSessionRequest> &aReq)
> > > +{
> > 
> > Remember that launching app/page is handled differently on different
> > platform (here I mean Desktop/Fennec/B2G). Be sure your implementation here
> > can adapt to all platforms.
> > On B2G, you'll have to delegate the app/page opening request to SystemApp.
> 
> Discussed with John Hu, I think it's suitable to send a custom event to
> SystemApp to launch app/page, like the type 'presentation-launch' in
> https://github.com/rexboy7/gaia/blob/tv_demo/apps/system/js/
> app_window_factory.js
> 
> I have no idea about launching a tab on desktop browser or Fennec, is there
> any keyword that I can search for ?
Maybe you need a |nsIPresentationLaunchPrompt| to hind all the details and provide the implementation for B2G as the proof-of-concept. (with follow-up bugs for desktop/fennec)
For B2G implementation, you can reference to my previous gecko hack for demo ( http://people.mozilla.org/~schien/presentation-demo/gecko.patch, search for "presentation-launch")
> 
> > ::: dom/presentation/PresentationSocketChannelInternal.h
> > @@ +11,5 @@
> > > +#include "PresentationCommon.h"
> > > +
> > > +BEGIN_PRESENTATION_NAMESPACE
> > > +
> > > +// TODO : Asking S.C, if the listner should include state method, like open/close?
> > 
> > I can add anything as long as you need it. :) I suppose you would want
> > something like |NotifyOpened| and |NotifyClosed|?
> > 
> 
> Exactly :)
> I guess I need |NotifyOpened| & |NotfiyClosed| first, maybe |NotifyError| if
> possible, THANKS ~
|NotifyClosed| should be triggered if error happened in the session transport instance. Do you plan to have different handling between the normal case and error case? Does |NotifyClosed(nsresult)| suit your needs?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8512505 [details] [diff] [review]
[WIP] PresentationAPI_WebIDL(NavigatorPresentation/PresentationSession/PresentationSocketChannel)

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

::: dom/webidl/NavigatorPresentation.webidl
@@ +5,5 @@
> +  Promise<PresentationSession> startSession(DOMString url, DOMString? sessionId);
> +  [Throws]
> +  Promise<PresentationSession> joinSession(DOMString url, DOMString? sessionId);
> +  
> +  attribute EventHandler onavailablechange;

We need to also add a "readonly attribute boolean available" here. So that you can see the current state without having to wait for an event.

::: dom/webidl/PresentationSession.webidl
@@ +6,5 @@
> +  attribute EventHandler onstatechange;
> +  attribute EventHandler onmessage;
> +  readonly attribute DOMString? id;
> +  void postMessage(DOMString message);
> +  void close();

Given that 'send', 'close' and 'onmessage' live on PresentationSocketChannel, we can remove 'postMessage', 'close' and 'onmessage' from here.

@@ +11,5 @@
> +};
> +
> +partial interface PresentationSession {
> +  [Pref="dom.presentation.socketchannel.enabled", AvailableIn="CertifiedApps"]
> +  readonly attribute PresentationSocketChannel channel;

Nit: No need to create a separate partial interface here. Just add 'channel' to the main interface above.

Comment 15

[Kilik Kuo [:kikuo] (inactive)]

Attachment: PresentationAPI WebIDL

Addressed issues and carry r+ from Comment 14.

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP] PresentationAPI Implementation (Controlling UA)

TODO : 
a) Sending blob stream.

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [WIP] PresentationAPI Implementation (Presenting UA)

TODO : 
a) Send blob stream.
b) Need discuss with S.C., we might need a parameter |aIsBinary| in |nsIPresentationSessionTransport| to identify the type of string.

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Controlling UA] Architecture diagram_1110.png

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Presenting UA] Session Request Sequence_1110.png

Comment 20

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

Comment on attachment 8520370 [details] [diff] [review]
PresentationAPI WebIDL

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

::: dom/webidl/NavigatorPresentation.webidl
@@ +1,1 @@
> +[Pref="dom.presentation.enabled", AvailableIn="CertifiedApps"]

Do we want to expose this API to certified apps only? I thought this API is going to available on web page as well, just pref on in Firefox OS first.

Comment 21

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

ni? Jonas and @kilik about comment #20.

Comment 22

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

In bug 1080474 comment #22, @fabrice suggests to make |session.postMessage()| more like |window.postMessage()|, which means we can pass transferable objects directly. In below is the comparison between current approach and @fabrice's sugesstion.

[Current approach]
session.channel is more like WebSocket, which only allows only string and binary message transferring. Application need to do object serialization by themselves, but it'll be easier to do cross-browser interoperability.

[@fabrice's suggestion]
session.postMessage(transferable) will make session object more like a remote iframe. Application don't need to care about object serialization, but I think it'll be hard to postMessage work across different browser (I don't know if there is any canonical format for serialization/deserilaztion on arbitrary DOM/JS object).

Comment 23

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #20)
> Comment on attachment 8520370 [details] [diff] [review]
> PresentationAPI WebIDL
> 
> Review of attachment 8520370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/NavigatorPresentation.webidl
> @@ +1,1 @@
> > +[Pref="dom.presentation.enabled", AvailableIn="CertifiedApps"]
> 
> Do we want to expose this API to certified apps only? I thought this API is
> going to available on web page as well, just pref on in Firefox OS first.

I got a chance to discuss this with Jonas today, here are the road map we agreed with:

1. For now, we should make Presentation API available for packaged apps on Firefox OS because the API spec is still unstable. Therefore, the WebIDL should use Pref + AvailableIn="PrivilegedApps".

2. When spec is stable, we can make it available to web pages on Firefox OS. |AvailableIn="PrivilegedApps"| can be removed in this stage.

3. When our implementation is stable, we can make it available to all platform. Preference |dom.presentation.enabled| can be default on for all platform.

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #22)
> In bug 1080474 comment #22, @fabrice suggests to make
> |session.postMessage()| more like |window.postMessage()|, which means we can
> pass transferable objects directly. In below is the comparison between
> current approach and @fabrice's sugesstion.
> 
> [Current approach]
> session.channel is more like WebSocket, which only allows only string and
> binary message transferring. Application need to do object serialization by
> themselves, but it'll be easier to do cross-browser interoperability.
> 

The reason we chose implementing a subset of WebSocket-Like message passing API in v1 is that we'd like there to be interface compatibility for implementing peer-to-peer communication between USER AGENTS. e.g. WebRTC is a likely mechanism.(But the RTCDataChannel interface is too WebRTC specific). 
In this way, it could be easier to modify interfaces to be a RTCDataChannel-like API in v2, and not to worry about if v1 API becomes popular, because v2 API's backward-compatible.

And no matter current approach or Fabrice's suggestion, the object serialization is always be done by gecko, right?  The difference is that one is done by Application's gecko, another is by b2g's gecko, am I understanding correctly ? And this reminds me that one thing I have not implemented yet (sending blob from |PresentationSocketChannel| to |PresentationSocketChannelInternal| through ipc)

I'm not sure that if it's suitable to use transferable object in this Web API.
Using transferable object to pass object cross browsers would be fine, but is it able to cross user agents ? I don't know.

> [@fabrice's suggestion]
> session.postMessage(transferable) will make session object more like a
> remote iframe. Application don't need to care about object serialization,
> but I think it'll be hard to postMessage work across different browser (I
> don't know if there is any canonical format for serialization/deserilaztion
> on arbitrary DOM/JS object).

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #23)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #20)
> > Comment on attachment 8520370 [details] [diff] [review]
> > PresentationAPI WebIDL
> > 
> > Review of attachment 8520370 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/NavigatorPresentation.webidl
> > @@ +1,1 @@
> > > +[Pref="dom.presentation.enabled", AvailableIn="CertifiedApps"]
> > 
> > Do we want to expose this API to certified apps only? I thought this API is
> > going to available on web page as well, just pref on in Firefox OS first.
> 
> I got a chance to discuss this with Jonas today, here are the road map we
> agreed with:
> 
> 1. For now, we should make Presentation API available for packaged apps on
> Firefox OS because the API spec is still unstable. Therefore, the WebIDL
> should use Pref + AvailableIn="PrivilegedApps".
> 
> 2. When spec is stable, we can make it available to web pages on Firefox OS.
> |AvailableIn="PrivilegedApps"| can be removed in this stage.
> 
> 3. When our implementation is stable, we can make it available to all
> platform. Preference |dom.presentation.enabled| can be default on for all
> platform.

Cool ! THANKS! S.C. & Jonas.
One question, the reason lowering down the right from CertifiedApps to PrevilegeApps is that you wanna promote this API to more developers ?

Comment 26

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

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #24)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #22)
> > In bug 1080474 comment #22, @fabrice suggests to make
> > |session.postMessage()| more like |window.postMessage()|, which means we can
> > pass transferable objects directly. In below is the comparison between
> > current approach and @fabrice's sugesstion.
> > 
> > [Current approach]
> > session.channel is more like WebSocket, which only allows only string and
> > binary message transferring. Application need to do object serialization by
> > themselves, but it'll be easier to do cross-browser interoperability.
> > 
> 
> The reason we chose implementing a subset of WebSocket-Like message passing
> API in v1 is that we'd like there to be interface compatibility for
> implementing peer-to-peer communication between USER AGENTS. e.g. WebRTC is
> a likely mechanism.(But the RTCDataChannel interface is too WebRTC
> specific). 
> In this way, it could be easier to modify interfaces to be a
> RTCDataChannel-like API in v2, and not to worry about if v1 API becomes
> popular, because v2 API's backward-compatible.
> 
> And no matter current approach or Fabrice's suggestion, the object
> serialization is always be done by gecko, right?  The difference is that one
> is done by Application's gecko, another is by b2g's gecko, am I
> understanding correctly ? And this reminds me that one thing I have not
> implemented yet (sending blob from |PresentationSocketChannel| to
> |PresentationSocketChannelInternal| through ipc)
Blob and typed array are still very primitive because they represents binary data in stream or array format. The object serialization I mentioned is mainly focused on general JS/DOM object serialization. e.g. |var obj = { a: 1 };| or |var element = document.getElementById('some-id');|. JSON is a common JS object serialization format however it only works for plain JS object (which means function and prototype chain can not be transferred).

In current WebSocket-like API, you'll need to do serialization/deserialization in client JS code like this example:

//sender
var obj = { a: 1 };
var msg = JSON.stringify(obj); //serialization
session.channel.send(msg);

//receiver
session.onmessage = function(msg) {
  var obj = JSON.parse(msg); //deserialization
}
> 
> I'm not sure that if it's suitable to use transferable object in this Web
> API.
> Using transferable object to pass object cross browsers would be fine, but
> is it able to cross user agents ? I don't know.
I'm not sure what you mean "cross user agents" here. There must be one layer to translate object into text/binary format in client JS/API implementation/device protocol, and it's related to how we design the API.
Device protocol might exchanging any possible message format between endpoints but those messages are not exposed out side the device protocol. It should not affect the WebIDL/XPIDL that we are discussing.
> 
> > [@fabrice's suggestion]
> > session.postMessage(transferable) will make session object more like a
> > remote iframe. Application don't need to care about object serialization,
> > but I think it'll be hard to postMessage work across different browser (I
> > don't know if there is any canonical format for serialization/deserilaztion
> > on arbitrary DOM/JS object).

Comment 27

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

Comment on attachment 8520370 [details] [diff] [review]
PresentationAPI WebIDL

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

All the WebIDLs should contain copyright statement and API reference. Don't forget to add it before landing.

::: dom/webidl/PresentationSocketChannel.webidl
@@ +8,5 @@
> +  void send (Blob data);
> +  [Throws]
> +  void send (ArrayBuffer data);
> +  [Throws]
> +  void send (ArrayBufferView data);

You can use |void send((DOMString or Blob or ArrayBuffer or ArrayBufferView) data);| to avoid redundant declaration.

@@ +13,5 @@
> +
> +  void close ();
> +
> +  attribute PresentationBinaryType binaryType;
> +  readonly attribute unsigned long bufferedAmount;

|bufferedAmount| is more like a TCP-specific attribute. I'll suggest to remove this attribute because PresentationSocketChannel should be protocol neutral.

@@ +19,5 @@
> +  const unsigned short CONNECTING = 0;
> +  const unsigned short OPEN = 1;
> +  const unsigned short CLOSING = 2;
> +  const unsigned short CLOSED = 3;
> +  readonly attribute unsigned short readyState;

Maybe use enum for readyState?

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #26)
> (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #24)
> Blob and typed array are still very primitive because they represents binary
> data in stream or array format. The object serialization I mentioned is
> mainly focused on general JS/DOM object serialization. e.g. |var obj = { a:
> 1 };| or |var element = document.getElementById('some-id');|. JSON is a
> common JS object serialization format however it only works for plain JS
> object (which means function and prototype chain can not be transferred).
> 
> In current WebSocket-like API, you'll need to do
> serialization/deserialization in client JS code like this example:
> 
> //sender
> var obj = { a: 1 };
> var msg = JSON.stringify(obj); //serialization
> session.channel.send(msg);
> 
> //receiver
> session.onmessage = function(msg) {
>   var obj = JSON.parse(msg); //deserialization
> }

@S.C. Thanks for your explanation :)
For application cross-browser interoperability, I think, for now(considering the implementation complexity and unstable spec), providing basic type of message passing method is enough, application could do serialization/deserialzation for their own purpose. Although it would definitely be great for web developers if they don't need to do serialization/deserialization by themselves.

> I'm not sure what you mean "cross user agents" here. There must be one layer
> to translate object into text/binary format in client JS/API
> implementation/device protocol, and it's related to how we design the API.
> Device protocol might exchanging any possible message format between
> endpoints but those messages are not exposed out side the device protocol.
> It should not affect the WebIDL/XPIDL that we are discussing.

Oh, the "cross UAs" means communicating between Controlling UA and Presenting UA.
I suppose there should be one(at least) layer in a UA doing translation. 
I think we're saying the same thing, and explained it in different p.o.v : )
Yes, it should not affect the WebIDL/XPIDL discussed above.

Regarding the object translation layer, I lean toward doing it in WebAPI implementation, and keep the transferring data format as simple(few) as possible between WebAPI Implementation and device protocal.(Although there might be other data transferring formats for device protocal itself)
i.e.
JS ===>(a)  WebAPI Implementation ===>(b) Device Protocal
1. Do object translation in (a)
2. I'd like to keep the format passing through (b) as basic as possible. 
3. Maybe/Maybe not another translation will be performed inside device protocal for communicating to other user agent ? (Is this assumption correct ?)

Thanks S.C. It's really nice to discuss with you. Makes me understand the whole system more.

Comment 29

[Kilik Kuo [:kikuo] (inactive)]

Attachment: webidl.patch

Copyright added & modified from |AvailabeIn="CertifiedApps"| to |AvailableIn="PrivilegedApps"|.

Carry r+ from comment 14.

Comment 30

[Kilik Kuo [:kikuo] (inactive)]

Attachment: controller.patch

Hi Jonas,

I've implemented our PresentationAPI controller UA logic based on the discussed WebIDL and the Spec draft [1].
Though there're still some parts unimplemented yet in message-passing method, e.g. the implementation of passing binary stream. But I'd like it be treated as a follow up issue.

Would you please help review this patch and give me suggestions ? Thanks.

[1] http://w3c.github.io/presentation-api/

Comment 31

[Kilik Kuo [:kikuo] (inactive)]

Attachment: presenter.patch

The Presentation API implementation on presenting UA.
Basically it's triggered after |PresentationManager| receiving the |PRESENTATION_SESSION_REQUEST_TOPIC|.
And the idea is to launch App after |PresentationSessionInternal| & |PresentationSocketChannelInternal| is created and connected in chrome process.

Comment 32

[Kilik Kuo [:kikuo] (inactive)]

Attachment: testcase.patch

By setting pref via |SpecialPowers.pushPrefEnv|
[["dom.presentation.enabled", true],
["dom.ignore_webidl_scope_checks", true],
["dom.presentation.test.enabled", true],
["dom.presentation.test.stage", 0]]

stage 0 : to test available event.
stage 1 : To test StartSession(w/ or w/o session Id), and JoinSession API flow.
stage 2 & 3 : To test close session on controller or presenter

Comment 33

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

Comment on attachment 8527265 [details] [diff] [review]
controller.patch

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

A quick note about how to use the "presentation-device-change" event, haven't look at other part.

::: dom/presentation/PresentationManager.cpp
@@ +255,5 @@
> +{
> +  nsresult rv = NS_OK;
> +  if (!strcmp(aTopic, PRESENTATION_DEVICE_CHANGE_TOPIC)) {
> +    bool available = !NS_strcmp(aData,
> +        MOZ_UTF16("dom.presentation.available-devices"))? true : false;

The way to detect device availability should like this:

nsCOMPtr<nsIPresentationDeviceManager> deviceManager =
  do_GetService(PRESENTATION_DEVICE_MANAGER_CONTRACTID);

bool available;
deviceManager->GetDeviceAvailable(&available);

Comment 34

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

Comment on attachment 8527267 [details] [diff] [review]
testcase.patch

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

You can dynamically generate device while running test case instead of introducing FakePresentationStuff at compile time. Go check test_presentation_device_info_permission.html and PresentationDeviceInfoChromeScript.js in Bug 1080474 Attachment 8527228 [details] [diff].

Comment 35

[Kilik Kuo [:kikuo] (inactive)]

Attachment: controller.patch

Use correct way to obtain information for device availability according to Comment 33.

Comment 36

[Kilik Kuo [:kikuo] (inactive)]

Attachment: presenter.patch

Rebase patch according to controller.patch in Comment 35

Comment 37

[Kilik Kuo [:kikuo] (inactive)]

Attachment: controller.patch

Some modification from Gina Yeh's suggestions, thanks to Gina :)
1. Use template class |DebugOnly| for some warning check.
2. Let |PresentationSocketChannelInternal| inherits from |nsIPresentationSessionTransportListener| only. (Because |nsISupports| is already inherited by |nsIPresentationSessionTransportListener|)
3. Remove a utility function |IPresentationManager* GetIPresentationManager| in PresentationUtil.h (Only be used in one place).

Comment 38

[Kilik Kuo [:kikuo] (inactive)]

Attachment: presenter.patch

Use template class |DebugOnly| for some warning checks

Comment 39

[Kilik Kuo [:kikuo] (inactive)]

Attachment: testcase.patch

1. Make FakePresentationStuff.{h,cpp} only be compiled in debug build.
 ==> use |#ifdef DEBUG| for several code blocks
 ==> use |if CONFIG['MOZ_DEBUG']| in moz.build

Comment 40

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8531549 [details] [diff] [review]
controller.patch

Hi Smaug,

Could you please help review these patches ? 
The PresentationAPI specification [1] is still not firmed yet (status:editor's draft).
And I've implemented it based on a WebIDL draft which is discussed previously in [2] & [3].

And it's kinda experimental API for now, and we'd like to enable it in 3 phases.
1. Make these API available for packaged apps (APIs are limited by Pref + AvailableIn="PrivilegedApps") on FxOS since the API speci is still unstable. 
2. Spec becomes stable, make these APIs available to web pages on FxOS, so |AvailableIn="PrivilegedApps"| will be removed.
3. Our implementation is stable, make these APIs available to all platforms. |dom.presentaion.enabled| could be on by defualt for all platforms.

Any feedbacks and suggestions are appreciated.
Thanks :) 

[1] http://w3c.github.io/presentation-api/
[2] http://lists.w3.org/Archives/Public/public-webscreens/2014Sep/subject.html
[3] https://groups.google.com/forum/#!searchin/mozilla.dev.webapi/PresentationAPI/mozilla.dev.webapi/Rjvu-cBJ1TY/qt0zEok7in8J

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

How soon do you need the reviews. The patches aren't exactly small, so reviewing them may take some time.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8527264 [details] [diff] [review]
webidl.patch

>+[Pref="dom.presentation.enabled", AvailableIn="PrivilegedApps"]
>+interface NavigatorPresentation : EventTarget {
>+
>+  [Throws]
>+  Promise<PresentationSession> startSession(DOMString url, DOMString? sessionId);
>+  [Throws]
>+  Promise<PresentationSession> joinSession(DOMString url, DOMString? sessionId);
>+
>+  attribute EventHandler onavailablechange;
>+  readonly attribute PresentationSession? session;
>+
>+  readonly attribute boolean available;
Why we need this 'available' and same property in the event?


>+[Pref="dom.presentation.enabled", AvailableIn="PrivilegedApps"]
>+interface PresentationSocketChannel : EventTarget {
This is odd name. Why "Socket", why not just PresentationChannel?

It is also not clear to me why we need PresentationSocketChannel at all.
Why not just PresentationSession or PresentationChannel.



Jonas, I see you've been involved with API design. Want to explain why the 
rather complicated setup.

Comment 43

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #41)
> How soon do you need the reviews. The patches aren't exactly small, so
> reviewing them may take some time.

If it goes smoothly, we're aiming for making these patches landed before the end of Jan. 2015.
Thanks for spending time reviewing :)

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Kilik, do you know the answer to 
"It is also not clear to me why we need PresentationSocketChannel at all.
Why not just PresentationSession or PresentationChannel."?
Like, we couldn't we merge those those two interfaces we have now to one?

I'd like to understand the API before reviewing its implementation ;)

Comment 45

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #44)
> Kilik, do you know the answer to 
> "It is also not clear to me why we need PresentationSocketChannel at all.
> Why not just PresentationSession or PresentationChannel."?
> Like, we couldn't we merge those those two interfaces we have now to one?
> 
> I'd like to understand the API before reviewing its implementation ;)

Hi Smaug,

1. For the |readonly attribute boolean available| in webidl.
The idea is to let the status could be retrieved without waiting for an AvailableEvent, because the event is only triggered every time when there's device available from an unavailable status, and vice versa. 

2. For the interface |PresentationSocketChannel|.
The reason to expose another interface which is only a subset of 
RTCDataChannel is that in this way we could simply change it to be an RTCDataChannel in v2 and its could be backward-compatible. Even if the v1 API is getting popular, any codes that uses v1 API would continue to work in UAs that only implement v2 API.
And the idea came from this discussion [1].

[1] http://lists.w3.org/Archives/Public/public-webscreens/2014Aug/0028.html

Comment 46

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #43)
> (In reply to Olli Pettay [:smaug] from comment #41)
> > How soon do you need the reviews. The patches aren't exactly small, so
> > reviewing them may take some time.
> 
> If it goes smoothly, we're aiming for making these patches landed before the
> end of Jan. 2015.
> Thanks for spending time reviewing :)

I've confirmed with marco. We'd better to land these patches in mid-January 2015.

Smaug, can you reserve/arrange some time for this bug? Please let me know if you don't have time to do so. :)

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Sure, I can review this (probably this week). Just want to understand the API first ;)

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #45)
> 1. For the |readonly attribute boolean available| in webidl.
> The idea is to let the status could be retrieved without waiting for an
> AvailableEvent, because the event is only triggered every time when there's
> device available from an unavailable status, and vice versa. 

Well why you need the available attribute on the event object?
Wouldn't it be enough to just have it NavigatorPresentation and
the event to dispatch would be then just a simple Event, and in an eventlistener one would do
if (event.target.available) (and not if (event.available))

> 
> 2. For the interface |PresentationSocketChannel|.
> The reason to expose another interface which is only a subset of 
> RTCDataChannel is that in this way we could simply change it to be an
> RTCDataChannel in v2 and its could be backward-compatible.
(backwards-compatible-ish, depending on whether someone is playing with the prototype objects)

> Even if the v1
> API is getting popular, any codes that uses v1 API would continue to work in
> UAs that only implement v2 API.

Why no have PresentationSession to have all the properties you need, and then in the future remove them
and make PresentationSession to inherit RTCDataChannel.

Though, in that case I would call the interface PresentationChannel, not PresentationSession.



PresentationSocketChannel sounds anyhow rather odd. Why the 'Socket' in it. RTCDataChannel doesn't have 'Socket'.

Comment 49

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Olli Pettay [:smaug] from comment #48)
> (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #45)
> > 1. For the |readonly attribute boolean available| in webidl.
> > The idea is to let the status could be retrieved without waiting for an
> > AvailableEvent, because the event is only triggered every time when there's
> > device available from an unavailable status, and vice versa. 
> 
> Well why you need the available attribute on the event object?
> Wouldn't it be enough to just have it NavigatorPresentation and
> the event to dispatch would be then just a simple Event, and in an
> eventlistener one would do
> if (event.target.available) (and not if (event.available))
> 

I see and I think it's quite reasonable. We can make AvailableChangeEvent a simple event without any attribute.

Comment 50

[Gina Yeh [:gyeh] [:ginayeh]]

I'd like to add one more detail regarding to the attribute |available| in AvailableChangeEvent. I think Kilik is trying to follow the spec proposed by W3C WG (http://w3c.github.io/presentation-api/#availablechangeevent). Well, I also agree with smaug's suggestion in comment 48.

So, should we follow the spec (although it's still in an early stage) or implement the right way for us and then contribute our comments back to W3C WG?

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #50)
> So, should we follow the spec (although it's still in an early stage) or
> implement the right way for us and then contribute our comments back to W3C
> WG?

The latter.  Especially when there are no compatibility concerns, we should not blindly follow the spec.

Comment 52

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Olli Pettay [:smaug] from comment #48)
> > 2. For the interface |PresentationSocketChannel|.
> > The reason to expose another interface which is only a subset of 
> > RTCDataChannel is that in this way we could simply change it to be an
> > RTCDataChannel in v2 and its could be backward-compatible.
> (backwards-compatible-ish, depending on whether someone is playing with the
> prototype objects)
> 
> > Even if the v1
> > API is getting popular, any codes that uses v1 API would continue to work in
> > UAs that only implement v2 API.
> 
> Why no have PresentationSession to have all the properties you need, and
> then in the future remove them
> and make PresentationSession to inherit RTCDataChannel.

It's doable and either way is fine with me from technical point of view. Let's wait for Jonas's feedback.

> Though, in that case I would call the interface PresentationChannel, not
> PresentationSession.
> 
> 
> 
> PresentationSocketChannel sounds anyhow rather odd. Why the 'Socket' in it.
> RTCDataChannel doesn't have 'Socket'.

I found that the original interface name proposed by Jonas is "PresentationAPISocketChannelWithATwist", and he also added a comment for this naming:

(http://lists.w3.org/Archives/Public/public-webscreens/2014Aug/0028.html)
interface PresentationAPISocketChannelWithATwist {
  // The twist is that it's both a socket and a channel.
  .....
}



smaug, does it answer your question?

Comment 53

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #51)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #50)
> > So, should we follow the spec (although it's still in an early stage) or
> > implement the right way for us and then contribute our comments back to W3C
> > WG?
> 
> The latter.  Especially when there are no compatibility concerns, we should
> not blindly follow the spec.

I see. Thanks for the prompt reply, Kyle. ;)

Comment 54

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should use a separate interface for the communication "channel" (or "socket") part of the API.

That way the object could switch to becoming a full RTCDataChannel in the future if that is the direction we choose to go.


So for now I think we should use the subset of RTCDataChannel which is for sending and receiving messages. I.e.  .send(), .onmessage, .readyState, .bufferedAmount and .close(). This is also the subset which is compatible with WebSocket, which is intentional.

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, also .onopen, .onclose and .onerror

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I guess also .binaryType. Even though that's a horrible design.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #54)
> I think we should use a separate interface for the communication "channel"
> (or "socket") part of the API.
> 
> That way the object could switch to becoming a full RTCDataChannel in the
> future if that is the direction we choose to go.
> 
Well we could still just inherit RTCDataChannel, no?

So still wondering why the rather complicated API.


Or are you for some reason against extending RTCDataChannel (in the future)?

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, I hadn't thought of extending RTCDataChannel in the future... 

I guess that might be doable. However RTCDataChannel, and WebRTC, is fairly complicated and I don't know where it's going to head in the future. So I would worry that subclassing might cause conflicts with future properties or other semantics of the API. But I can certainly check with the WebRTC developers.

Comment 59

[Kilik Kuo [:kikuo] (inactive)]

Hi Gina,

Since I'm going to pay more attention on media (MSE) related issues in the next few weeks. (maybe months...)
I'm afraid of not being able to continue the review & polishing procedure in quick response.
Wondering that would you like to take this bug from now ?

Comment 60

[Gina Yeh [:gyeh] [:ginayeh]]

Sure! Thanks you for taking time on the implementation in the past few months. I can take it and will focus on this bug in the following weeks.

Comment 61

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Presentation API and WebIDL bindings

Updates:
* Keep AvailableChangeEvent as a simple event without any attribute. (Thus, remove 'AvaiilableChangeEvent.webidl')
* Add implementation of WebIDL bindings.

Comment 62

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Presentation API and WebIDL bindings

Also, rename PresentationSocketChannel as PresentationChannel.

Comment 63

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8540044 [details] [diff] [review]
Patch 1(v1): Presentation API and WebIDL bindings

Update:
* 'AvailableChangeEvent.webidl' is removed
* Rename as 'PresentationChannel'

Jonas, only a few changes are made on the previous API. Can you take some time on it? Thanks.


smaug, thanks for your feedback on the API, and I've updated those WebIDL. Would you mind starting review from the API and WebIDL bindings? A few TODOs are marked in the patch and they will be implemented in the following patches. Please let me know if you have any concerns or any questions regarding to the API. Thanks.

Comment 64

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Presentation API and WebIDL bindings

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8540086 [details] [diff] [review]
Patch 1(v1): Presentation API and WebIDL bindings

I would still merge PresentationSession and PresentationChannel.
We're not using RTCDataChannel here, and if we do in the future, we could extend it.
And starting to use RTCDataChannel in any way would be backwards incompatible change.


Both the implementation and API would be simpler if there was just PresentationChannel and no PresentationSession

>+NavigatorPresentation::StartSession(const nsAString& aUrl,
>+                                    const nsAString& aSessionId,
>+                                    ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
>+  MOZ_ASSERT(global);
Please don't assert here but throw an exception using ErrorResult

Comment 66

[Marco Chen [:mchen]]

(In reply to Olli Pettay [:smaug] from comment #65)
> Comment on attachment 8540086 [details] [diff] [review]
> Patch 1(v1): Presentation API and WebIDL bindings
> 
> I would still merge PresentationSession and PresentationChannel.
> We're not using RTCDataChannel here, and if we do in the future, we could
> extend it.
> And starting to use RTCDataChannel in any way would be backwards
> incompatible change.

Could we consider that the state machines of PresentationSession and PresentationChannel are two different ones?

For example, 
  1. App1 on UA1 already created one Session to App2 on UA2 and started to work.
  2. For some reasons, the backend of Channel is disconnected. (ex: network congestion caused timeout)
     (according to spec didn't specify the backend of Channel there might be another reasons for any possible backends)
  3. Then UA1 can automatically try to recover the Channel.
  4. Before UA1 re-establish the Channel, state of Session could be still "connected" but state of channel would be "closed" -> "connecting". 

This can give programmer and user more information/logic about this is just a temporary channel lost (no necessary to re-invoke requestSession(...)) or this session is really closed by remote side.

My two cent.

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Do we really have a session "connected" if we have network issues so that channel is closed?

The draft spec is still rather vague about this all.
Maybe there is some clarification somewhere in the mailing list?

Comment 68

[Marco Chen [:mchen]]

(In reply to Olli Pettay [:smaug] from comment #67)
> Do we really have a session "connected" if we have network issues so that
> channel is closed?
> 
> The draft spec is still rather vague about this all.
> Maybe there is some clarification somewhere in the mailing list?

I can't find relevant discussion about my point so maybe we could keep your way first.

Comment 69

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Presentation API and WebIDL bindings

Update:
* Merge PresentationChannel and PresentationSession

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

(I'll try to review this tomorrow.)

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8541097 [details] [diff] [review]
Patch 1(v2): Presentation API and WebIDL bindings

>+NS_IMPL_CYCLE_COLLECTION_CLASS(NavigatorPresentation)
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(NavigatorPresentation, DOMEventTargetHelper)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(NavigatorPresentation, DOMEventTargetHelper)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
So nothing is added to cycle collection. These shouldn't be needed, nor the relevant macro in the class declaration


>+
>+NS_IMPL_ADDREF_INHERITED(NavigatorPresentation, DOMEventTargetHelper)
>+NS_IMPL_RELEASE_INHERITED(NavigatorPresentation, DOMEventTargetHelper)
>+
>+// QueryInterface implementation for NavigatorPresentation
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(NavigatorPresentation)
>+NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
ditto here, but about idl interfaces. You shouldn't need to have NS_DECL_ISUPPORTS_INHERITED

>+already_AddRefed<Promise>
>+NavigatorPresentation::StartSession(const nsAString& aUrl,
>+                                    const nsAString& aSessionId,
>+                                    ErrorResult& aRv)
>+{
>+  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetOwner());
>+  if (NS_WARN_IF(!global)) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return nullptr;
>+  }
>+
>+  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return nullptr;
>+  }
>+
>+  // TODO Resolve/reject the promise.
So obviously this needs to be fixed. Will that happen in a followup patch?

>+  nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return nullptr;
>+  }
>+
>+  // TODO Resolve/reject the promise.
ditto

>+NS_IMPL_ADDREF_INHERITED(PresentationSession, DOMEventTargetHelper)
>+NS_IMPL_RELEASE_INHERITED(PresentationSession, DOMEventTargetHelper)
>+
>+// QueryInterface implementation for PresentationSession
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(PresentationSession)
>+NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
You shouldn't need to have NS_DECL_ISUPPORTS_INHERITED for this class either.



Those fixed this should be fine, but obviously there will be followup patches.

Comment 72

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #49)
> (In reply to Olli Pettay [:smaug] from comment #48)
> > (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #45)
> > > 1. For the |readonly attribute boolean available| in webidl.
> > > The idea is to let the status could be retrieved without waiting for an
> > > AvailableEvent, because the event is only triggered every time when there's
> > > device available from an unavailable status, and vice versa. 
> > 
> > Well why you need the available attribute on the event object?
> > Wouldn't it be enough to just have it NavigatorPresentation and
> > the event to dispatch would be then just a simple Event, and in an
> > eventlistener one would do
> > if (event.target.available) (and not if (event.available))
> > 
> 
> I see and I think it's quite reasonable. We can make AvailableChangeEvent a
> simple event without any attribute.

I found changes in Presentation API, and one of concerns is related to AvailableChangeEvent. They'd like to remove attribute available and put the actual information into the event. The main concern of this change is to optimize power consumption for network discovery. Here's what I found on the latest spec:


> (The rationale of putting the actual boolean information into a property of the event e.available is to 
> allow the implementation to optimize power consumption for network discovery of remote wireless screens. If 
> this information was provided in a globally accessible flag, the network discovery could never be suspended 
> for keeping the flag up to date.) 

http://w3c.github.io/presentation-api/#starting-new-presentations-or-manually-connecting-to-existing-presentations


Thus, I'd like to remove attribute available from NavigatorPresentation and add it back to AvailableChangeEvent. What do you think, smaug?

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

So if we don't have .available in NavigatorPresentation, how would one then check
the current state. I mean, if presentation is available, and there aren't any changes happening, 
the app couldn't know the presentation is available.
See also comment 14.

I would just let UA to update .available in NavigatorPresentation when it decides that
the state changes and at that moment also the event would be dispatched. But it would be up to the UA
to update the flag, so it could easily suspend network discovery.

Comment 74

[Anton Vayvod]

Hi there, I'm following the bug with lots of interest and glad to see the progress being made.

Regarding the available property, there's this issue: https://github.com/w3c/presentation-api/issues/11
and the corresponding conversation around it: http://lists.w3.org/Archives/Public/public-webscreens/2014Sep/0017.html in the Working Group.

Feel free to chime in with feedback and comments.

Comment 75

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks for providing the information, Anton.

We're working on a draft which is expected to be simple but useful. We also follow the discussion in the WG, and will contribute back if we have any feedbacks/comments.

Comment 76

[Gina Yeh [:gyeh] [:ginayeh]]

Sorry for the late update. I'm going to attach our latest version of mozPresentation API.

Comment 77

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Presentation.webidl

Smaug, I would greatly appreciate it if you kindly give me some feedback on this.

Comment 78

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: WebIDL

All Presentation API-related WebIDLs are compacted into a patch. Please check this one. Thanks.

Comment 79

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: WebIDL

Sorry for the spam! The last one cannot be reviewed.

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8579878 [details] [diff] [review]
WebIDL

The API starts to look rather good.

But this is missing the changes to Navigator.webidl?

Why is the API different to http://www.w3.org/2014/secondscreen/presentation-api/20140721/#navigatorpresentation? 
Why resumeSession and not joinSession? Why more 
Are we proposing this API to the WG?

In the draft spec presentationID can be null. (DOMString?), and it is not optional. I guess that is a bug in the spec since it then talks about 
"presentationId, an optional identifier for the presentation "

Per spec readonly attribute DOMString id; should be nullable.

The spec reuses MessageEvent. Why couldn't we. (why PresentationMessageEvent)

Comment 81

[Gina Yeh [:gyeh] [:ginayeh]]

Thank you for taking time on the review.


(In reply to Olli Pettay [:smaug] from comment #80)
> But this is missing the changes to Navigator.webidl?

Yes. It's missing. I'll add them in the next patch.


> Why is the API different to
> http://www.w3.org/2014/secondscreen/presentation-api/20140721/
> #navigatorpresentation? 
> Why resumeSession and not joinSession? Why more 

Two main reasons for replacing joinSession with resumeSession.

1) In current implementation, we only support users to "resume" the sessions which had been established before. If the id is unrecognized, the request is rejected.

2) joinSession is more like for multi-player use cases. In multi-player use cases, someone can "join" a session, and all participants exchange messages via the session. However, we haven't supported multipleplayer use cases yet.

Does it make sense to you?


> Are we proposing this API to the WG?

Yes. We'll have a F2F meeting in May, and we'll also provide some feedbacks to the WG based on our API.


> In the draft spec presentationID can be null. (DOMString?), and it is not
> optional. I guess that is a bug in the spec since it then talks about 
> "presentationId, an optional identifier for the presentation "
> 
> Per spec readonly attribute DOMString id; should be nullable.

Hmm. I think it's a bug in the spec, and that's why I change it to non-nullable here. Let me open an issue for that.


> The spec reuses MessageEvent. Why couldn't we. (why PresentationMessageEvent)

Since MessageEvent is only exposed on a particular set of global interfaces [1], we cannot dispatch MessageEvent from PresentationSession. Any suggestions? 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MessageEvent.webidl?from=MessageEvent.webidl&case=true#11

Comment 82

[Olli Pettay [:smaug][[e-mail]]]

I don't understand the comment about MessageEvent. You can dispatch it just fine to PresentationSession.

Comment 83

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 1: WebIDL Bindings.

Comment 84

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 2: nsIPresentationService.

Comment 85

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 3: PresentationParent and PresentationChild.

Comment 86

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 4: nsIPresentationListener.

Comment 87

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 5: StartSession.

Comment 88

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 6: Handle session request.

Comment 89

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 7: Send mozChromeEvent to launch App and handle mozContentEvent when the App is launched.

Comment 90

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 8: Sync IPC for creating mozPresentation.session on receiver side.

Comment 91

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: The rest of implementation (WIP).

Comment 92

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Part 2: nsIPresentationService.

Comment 93

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Olli Pettay [:smaug] from comment #82)
> I don't understand the comment about MessageEvent. You can dispatch it just
> fine to PresentationSession.

We tried to do so, but something weired happens.

Sender calls startSession() and then a promise is resolved with an instance of PresentationSession, and we add event listener on the instance of PresentationSession. However, when Receiver posts a message, the instance of PresentationSession doesn't get the MessageEvent. I guess it's related to "[Exposed]" attribute in the WebIDL, but I might be wrong. Any thoughts?

Comment 94

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v1

I'm taking the bug since Gina has left. Per comment 93 (no MessageEvent is received when using |AddEventListener('message', ...)| instead of |onmessage=...|), so far we still use a new PresentationMessageEvent instead of the existent MessageEvent.

Comment 95

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8591581 [details] [diff] [review]
Part 1 - WebIDL Bindings, v1

>+presentation::Presentation*
Hmm, Presentation should be in mozilla::dom namespace


>+Navigator::GetMozPresentation(ErrorResult& aRv)
Drop Moz prefix. We're trying to not add more moz-prefixed stuff, and given that this API is being standardized there
really shouldn't be any need of the prefix.


>+'Presentation': {
>+    'nativeType': 'mozilla::dom::presentation::Presentation',
>+},
>+
>+'PresentationSession': {
>+    'nativeType': 'mozilla::dom::presentation::PresentationSession',
>+},

So if there wasn't presentation namespace, you wouldn't need it.
And the generic rule is to not add more namespaces.


>+Presentation::Presentation(nsPIDOMWindow* aWindow)
>+  : DOMEventTargetHelper(aWindow)
>+  , mAvailable(false)
>+  , mSession(nullptr)
no need to initialize mSession which is an nsRefPtr


>+partial interface Navigator {
>+  [Throws, Pref="dom.presentation.enabled", AvailableIn="PrivilegedApps"]
>+  readonly attribute Presentation? mozPresentation;
so this would be without moz prefix.

So, I would still drop PresentationMessageEvent.
What is the issue with MessageEvent? Some issue with dispatching events?


(but yeah, looking good otherwise)

Comment 96

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #95)
> Comment on attachment 8591581 [details] [diff] [review]
> Part 1 - WebIDL Bindings, v1
>
> So, I would still drop PresentationMessageEvent.
> What is the issue with MessageEvent? Some issue with dispatching events?
> 
We found an issue that MessageEvent might not be dispatched to the web content under some circumstances. It works fine if we register a listener by using

session.onmessage = function() {...};

However, somehow MessageEvent doesn't appear dispatched if we use the following way to register.

session.addEventListener('message', function() {...});

Gina guessed "[Exposed]" attribute in MessageEvent WebIDL [1] might restrict its accessibility and cause the issue, but couldn't say for sure. So we create PresentationMessageEvent as a workaround.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MessageEvent.webidl?from=MessageEvent.webidl&case=true#11

Comment 97

[Olli Pettay [:smaug][[e-mail]]]

That doesn't really make sense. onmessage = function() {} effectively just a shorthand for
addEventListener("message", function() {}).

(And Exposed shouldn't matter here)

Do you have the code showing the dispatching of the event and the code showing how addEventListener is used?

The following seems to work with whatever event interface (MessageEvent, or Event, or UIEvent or...)
data:text/html,<script>window.onmessage = function(e) { document.write("onfoo: " + e.type + " ");}; window.addEventListener("message", function(e) { document.write("AddEventListener: " + e.type);}); window.dispatchEvent(new MessageEvent("message"));</script>

Comment 98

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #97)
> That doesn't really make sense. onmessage = function() {} effectively just a
> shorthand for
> addEventListener("message", function() {}).
> 
> (And Exposed shouldn't matter here)
> 
> Do you have the code showing the dispatching of the event and the code
> showing how addEventListener is used?
We found the issue on B2G build in late Jan when preparing the MWC demo with presentation API. Yet I just verified with a mochitest against the latest Gecko, and surprisingly the issue appears no longer reproducible. (Perhaps it was introduced by a regression or got fixed in the past couple months.) So I'll follow the comment to drop PresentationMessageEvent and reuse existent MessageEvent in the next revision.

Comment 99

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v2

Update based on previous comments.

Comment 100

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8593290 [details] [diff] [review]
Part 1 - WebIDL Bindings, v2

Why would this be available only in PrivilegedApps? But we can change that behavior later.

Comment 101

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #100)
> Why would this be available only in PrivilegedApps? But we can change that
> behavior later.

Comment 23 mentions the road map. The "AvailableIn" constraint only applies at the initial stage and will eventually be removed.

Comment 102

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Presentation service and listeners, v1

Comment 103

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8593877 [details] [diff] [review]
Part 2 - Presentation service and listeners, v1

>+PresentationService::PresentationService()
>+  : mSessionInfo(128)
What is this magical 128?
Please define some constant which explain what the default value is

>+PresentationService::NotifyAvailableChange(bool aAvailable)
>+{
>+  NS_WARN_IF(!mListeners.Length());
>+  nsTObserverArray<nsCOMPtr<nsIPresentationListener> >::ForwardIterator iter(mListeners);
>+  while (iter.HasMore()) {
>+    nsIPresentationListener* listener = iter.GetNext();
>+    NS_WARN_IF(NS_FAILED(listener->NotifyAvailableChange(aAvailable)));
listener should be probably nsCOMPtr<nsIPresentationListener> so that if 
NotifyAvailableChange does something unusual, we don't end up dealing with deleted objects here.

>+PresentationService::RegisterListener(nsIPresentationListener* aListener)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  mListeners.AppendElement(aListener);
Should we check that mListeners doesn't already contain aListener?
I think so, so that it would behave like AddEventListener and other similar methods.

>+class PresentationService final : public nsIPresentationService
>+                                , public nsIObserver
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBSERVER
>+  NS_DECL_NSIPRESENTATIONSERVICE
>+
>+  PresentationService();
>+  bool Init();
>+
>+private:
>+  ~PresentationService();
>+  void NotifyAvailableChange(bool aAvailable);
>+
>+  nsClassHashtable<nsStringHashKey, PresentationSessionInfo> mSessionInfo;
>+  nsTObserverArray<nsCOMPtr<nsIPresentationListener> > mListeners;
Just need to remember that since this is a service, listeners must be removed manually when needed.
Otherwise we'll have leaks.

>+%{C++
>+template<typename T> struct already_AddRefed;
>+
>+already_AddRefed<nsIPresentationService>
>+NS_CreatePresentationService();
>+%}
Do we actually need this in the .idl?
Couldn't you move this to nsLayoutModule.cpp so that one doesn't accidentally call
NS_CreatePresentationService at random places.



(Once all the patches have got r+, I'd like to still see the all-in-one-patch to ease understanding of the big picture here.)

Comment 104

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Presentation service and listeners, v2

Updating based on r+ comments.

Comment 105

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - IPC, v1

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8594685 [details] [diff] [review]
Part 3 - IPC, v1

Looks mostly good, but PresentationChild usage is error prone.
PresentationIPCService dtor sets sPresentationChild to null, but
since PresentationIPCService is refcounted, it is easy to keep that accidentally  alive longer than the  PresentationChild (which is deleted in ContentChild::DeallocPPresentationChild). And that could lead to use of
delete objects.
So, I think ContentChild::DeallocPPresentationChild should clear sPresentationChild.

Could you also add MOZ_COUNT_CTOR/DTOR to PresentationRequestChild.

Comment 107

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - IPC, v2

Updating based on previous comments.

Comment 108

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Establish session (sender) & available changes, v1

StartSession Flow Chart:
https://docs.google.com/drawings/d/1kElQpHDHlVHBc-1HIvbd8Bhlx_hBIwwpkuUn900CJsI/edit?usp=sharing

Comment 109

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8596448 [details] [diff] [review]
Part 3 - IPC, v2

>+  MOZ_ASSERT(mService);
>+  if (mService) {
isn't useful. Either you always have mService and don't need to assert nor have
'if', or you may have null, in which case you need 'if'.

I don't still quite understand how mSessionListeners and mListeners are used, but perhaps the next patch explains that.

Have you tested the setup with multiple windows or tabs, each wanting to use some session?

Comment 110

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Sean Lin [:seanlin] from comment #108)
> StartSession Flow Chart:
> https://docs.google.com/drawings/d/1kElQpHDHlVHBc-
> 1HIvbd8Bhlx_hBIwwpkuUn900CJsI/edit?usp=sharing

Couple of nits.
- Can IPC really fail? I'd assume child process dies then.
- If user just dismisses the prompt, that shouldn't be the same as cancel. It just means user hasn't
  decided.
- I don't understand the "is the app installed" part. Is that b2g specific thing?
  Same with DOMContentLoaded.
So how does this all work in non-b2g environment?

Comment 111

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I missed that Sender/Receiver part of this...

Comment 112

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8597195 [details] [diff] [review]
Part 4 - Establish session (sender) & available changes, v1

I don't yet know how nsIPresentationDeviceManager works. Is there a patch for it somewhere?

Use nsAutoString on stack when possible, not nsString.

Why do we need to have
+  } else if (!strcmp(aTopic, "profile-after-change")) {
+    return NS_OK;
+  }
I don't see anything adding observer for profile-after-change.
Or is that because we initialize the service around that time?
If so, please add a comment.

Please be consistent with indentation. 2 spaces. (PresentationService::StartSession for example has some 4 spaces usage)



This is more like feedback+ since this depends on the stuff I haven't seen yet.
But in general, code looks good.

Comment 113

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #109)
> I don't still quite understand how mSessionListeners and mListeners are
> used, but perhaps the next patch explains that.
> 
> Have you tested the setup with multiple windows or tabs, each wanting to use
> some session?
|mListeners| are associated with presentations for available changes; whereas |mSessionListeners| are associated with sessions identified by their IDs for state changes or message notifications.

I've made some initial tests and will write more formal ones in follow-up patches.

(In reply to Olli Pettay [:smaug] from comment #110)
> Couple of nits.
> - Can IPC really fail? I'd assume child process dies then.
Will address this in follow-ups and update the diagram. Thanks.

> - If user just dismisses the prompt, that shouldn't be the same as cancel.
> It just means user hasn't decided.
I'll double check with the UX team to figure out the behavior for this.

> - I don't understand the "is the app installed" part. Is that b2g specific
> thing? Same with DOMContentLoaded.
> So how does this all work in non-b2g environment?
Yup. They are b2g specific, and may be skipped for non-b2g scenarios. Yet per comment 23, we'll only make the API available on b2g at first. So the detailed non-b2g flow will be addressed later once we're about to open it on other platforms.

(In reply to Olli Pettay [:smaug] from comment #112)
> I don't yet know how nsIPresentationDeviceManager works. Is there a patch
> for it somewhere?
It has been implemented in bug 1080474 and landed. FYI.

> I don't see anything adding observer for profile-after-change.
> Or is that because we initialize the service around that time?
> If so, please add a comment.
Will do.
(That's because we added 
  { "profile-after-change", "PresentationService", PRESENTATION_SERVICE_CONTRACTID }
 to layout categories in nsLayoutModule.cpp in patch part 2, in order to launch the service earlier.)

Comment 114

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - IPC, v3

Minor updates based on r+ comments.

Comment 115

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Establish session (sender) & available changes, v2

Minor updates based on r+ comments.

Comment 116

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Sean Lin [:seanlin] from comment #113)
> (In reply to Olli Pettay [:smaug] from comment #110)
> > - I don't understand the "is the app installed" part. Is that b2g specific
> > thing? Same with DOMContentLoaded.
> > So how does this all work in non-b2g environment?
> Yup. They are b2g specific, and may be skipped for non-b2g scenarios. Yet
> per comment 23, we'll only make the API available on b2g at first. So the
> detailed non-b2g flow will be addressed later once we're about to open it on
> other platforms.
A quick update. "is the app installed" part is b2g specific. Yet DOMContentLoaded could still be needed in non-b2g environment (i.e. open a new tab). The diagram is primarily for b2g scenarios at this moment.

Comment 117

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v1

Comment 118

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v1

Comment 119

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8597922 [details] [diff] [review]
Part 5 - Establish session (receiver), v1

> /*
>  * Implementation of PresentationResponderInfo
>+ *
>+ * During presentation session establishment, the receiver expects the following
>+ * after trying to launch the app by notifying "presentation-launch-receiver":
>+ * 1. |Observe| of |nsIObserver| is called with "presentation-receiver-launched".
>+ *    Then start listen to "DOMContentLoaded" event.
>+ * 2. |HandleEvent| of |nsIDOMEventListener| is called with "DOMContentLoaded"
>+ *    to indicate the receiver is standing by. Then notify the sender of
>+ *    "receiverReady" via the control channel.
This feels rather odd setup. Shouldn't we just have some API for the receiver side which it invokes when it is ready.

>+    // Start to listen to "DOMContentLoaded" event.
>+    nsCOMPtr<nsIFrameLoaderOwner> owner = do_QueryInterface(aSubject);
>+    if (NS_WARN_IF(!owner)) {
>+      return ReplyError(NS_ERROR_NOT_AVAILABLE);
>+    }
>+
>+    nsCOMPtr<nsIFrameLoader> frameLoader;
>+    nsresult rv = owner->GetFrameLoader(getter_AddRefs(frameLoader));
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return ReplyError(rv);
>+    }
>+    frameLoader->ActivateFrameEvent(NS_LITERAL_STRING("DOMContentLoaded"), false);
>+
>+    nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(aSubject);
>+    if (NS_WARN_IF(!target)) {
>+      return ReplyError(NS_ERROR_NOT_AVAILABLE);
>+    }
>+    rv = target->AddEventListener(NS_LITERAL_STRING("DOMContentLoaded"),
>+                                  this, false);
So an issue with this kind of setup is that the DOMContentLoaded might be for some iframe inside the relevant mozbrowser, not the main document.

Comment 120

[Olli Pettay [:smaug][[e-mail]]]

I wonder where we want to expose the receiver. Some b2g app (in which case perhaps expose the API only if some permission is enabled), but do we want to support desktop FF as an receiver too?

Comment 121

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #120)
> I wonder where we want to expose the receiver. Some b2g app (in which case
> perhaps expose the API only if some permission is enabled), but do we want
> to support desktop FF as an receiver too?

At the first step desktop FF could not be either a sender or receiver (per comment 23). But it will in the long run. Though in current implementation the API is exposed to all privileged b2g apps once preference is enabled, we may add permission control to the WebIDLs if necessary.

(In reply to Olli Pettay [:smaug] from comment #119)
> This feels rather odd setup. Shouldn't we just have some API for the
> receiver side which it invokes when it is ready.
Ni SC, who's more involved in the discussion of Presentation API spec and appears the better candidate to address this.

SC, could you share your thoughts about this?

Comment 122

[Chris Mills [:cmills]]

Adding dev-doc-needed to start tracking this. Is there a meta bug for this API?

Comment 123

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v2

Handle the DOMContentLoaded event only for the main document.

I haven't changed the logic to use an API invoked at the receiver side when it's ready. I'd like to leave this part as it is until SC's feedback about this API is available. Once we're sure to have the new one and clear about how it looks like, we may update in a follow-up patch if necessary.

Comment 124

[Olli Pettay [:smaug][[e-mail]]]

I still don't understand why DOMContentLoaded should be used for this kind of stuff.
And ActivateFrameEvent makes remote frame to listen for DOMContentLoaded, so all the nested iframes send DOMContentLoaded across IPC.
And, we don't want ActivateFrameEvent to be used here actually. Parent document shouldn't get
DOMContentLoaded which is originally dispatched to some child document.

Why can't we just have some simple API here where the page calls some method which then internally
ends up doing something similar to SendReceiverReady()?

Comment 125

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8603239 [details] [diff] [review]
Part 5 - Establish session (receiver), v2

(and expose that API only if the app has the right permission)

Comment 126

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

Sorry for the late response.

The "ReceiverReady" is actually part of the protocol for establishing app-to-app transport channel and this protocol is running on a browser-to-browser control channel. Exposing an API for that means web page needs to be involved in the browser control protocol, which is an unnecessary burden for web developer IMO. Also, the life time of browser control channel will be prolonged if the web page doesn't call corresponding API.

The usage of "DOMContentLoaded" in this patch is only for obtaining a "safe" timing to establish app-to-app transport channel. @smaug, do you have any other suggestion that can fulfill this requirement without adding WebAPI?

Comment 127

[Olli Pettay [:smaug][[e-mail]]]

I don't understand the reasoning to use DOMContentLoaded.
Why not load event? Why not something else, like first animationFrameCallback? Why not when we start loading 
the page?

Also, pages can prevent DOMContentLoaded to be dispatched (use document.write), so it isn't really 
"safe".
(and even with DOMContentLoaded, it really should be the DOMContentLoaded for the topmost document, 
not any iframe.)

What does ReceiverReady mean? A web page could just load a small shim, get DOMContentLoaded and load events and start then actually load the scripts for the application, so around the time DOMContentLoaded is dispatched, the application isn't really ready to do anything.

Comment 128

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

Thanks @smaug for pointing out "activeFrameEvent for DOMContentLoaded" is not the correct way to fulfill our requirement.

The ReceiverReady is for detecting if presentation display has successfully navigated to the designated URL (see startSession step 5.1.1 [1]). UA will continue the session transport establishment procedure after that. Application will get another event notifying that session transport has been connected, then continue the application logic. For complex web page, they can use the session transport to create application specific event/protocol for their own requirement. We don't need a specific API for that.

I have a new proposal and would like to get your feedback:

1. Sender UA will send |SessionRequest|. Receiver will then navigate to the designated URL (a browser frame will be created for Firefox OS).

2. Without waiting for any event, sender UA will initiate the session transport establishing protocol immediately, i.e. sending offer via control channel. Receiver will then create the IPC backend (PPresentation.ipdl) and monitoring the docshell loading progress in content process directly (via nsIWebProgress).

3. When docshell reports load complete, content process will notify chrome process via PPresentation.ipdl and continue the session transport establish protocol by sending answer via control channel. Sender will consider presentation display has successfully launch the URL and finish the session transport establishment. If control channel is closed before receiving answer, sender will know this request is failed.

With this approach, the "ReceiverReady" is merged with "Answer" and we don't need to monitor page loading in chrome process. @smaug, how do you think?

[1] http://w3c.github.io/presentation-api/#starting-a-presentation-session

Comment 129

[Olli Pettay [:smaug][[e-mail]]]

Why do we need " When docshell reports load complete" ? That may still not mean that the 
web application is really up and running.

Or let me ask differently - what is wrong with adding a simple API for the receiver so that
receiver can notify the sender UA whenever receiver thinks it is "ready" to start session?

Comment 130

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

One main reason:

We can request an URL that doesn't aware of the presentation API, e.g. an image URL. In this case, no body in web content will call the "sessionReady" API, thus session request is never success or fail. Sender side will not be able to know what to do next because it can never get the session object from Promise.

That's the reason I'm against adding an API for that.

Comment 131

[Olli Pettay [:smaug][[e-mail]]]

I see.
Ok, so docshell reporting load complete... I assume that would mean
effectively around the time when "load" event is dispatched to the top level window.
But should the sender/client be able to initiate presentation even earlier, like around the time
when the receiver/server has something to paint? Just thinking about the case when load takes lots of time.

Comment 132

[Olli Pettay [:smaug][[e-mail]]]

But as a first step, fine, around the time load is fired (docshell calls LoadComplete).

Thanks for explaining this, and sorry about being difficult with this stuff :)

Comment 133

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

(In reply to Olli Pettay [:smaug] from comment #131)
> I see.
> Ok, so docshell reporting load complete... I assume that would mean
> effectively around the time when "load" event is dispatched to the top level
> window.
> But should the sender/client be able to initiate presentation even earlier,
> like around the time
> when the receiver/server has something to paint? Just thinking about the
> case when load takes lots of time.
nsIWebProgress provides multi-level of state notification. [1]
The earliest state we can use is STATE_STOP for STATE_IS_REQUEST, this event is fired soon after HTTP response is received completely. In this case we'll not be affected by the long page loading time like "window.onload".

[1] https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#126

Comment 134

[Olli Pettay [:smaug][[e-mail]]]

STATE_STOP is still rather late, isn't it? Say, you're using a slow network and trying to load 
http://www.whatwg.org/specs/web-apps/current-work/
Wouldn't something like the first STATE_TRANSFERRING be closer to what is wanted here?
Or perhaps the first time the page is reflowed/painted?

Comment 135

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=ce6cfd9bad2f#3975 might
be one reasonable option, if the notification should happen around the time we're ready to paint something.

Comment 136

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Presentation service and listeners, v3

Update |nsIPresentationService| interface due to the proposal about session establishment in comment 128.
The primary change is to add |notifyReceiverReady|.

Comment 137

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - IPC, v4

Update some IPC methods due to the proposal about session establishment in comment 128.
The primary change is to add |NotifyPresentationReceiverLaunched| to PContent.ipdl, so that B2G process can ask the content process to listen to STATE_TRANSFERRING events for its DocShell.
Adding |NotifyReceiverReady| to PPresentation.ipdl.

Comment 138

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Establish session (sender) & available changes, v3

Update session establishment logic at the sender side.
The sender expects the following after trying to establish the control channel:
(The order between step 2 and 3 is not guaranteed.)
1. |Init| is called to open a socket |mServerSocket| for data transport channel and send the offer to the receiver via the control channel.
2.1 |OnSocketAccepted| of |nsIServerSocketListener| is called to indicate the data transport channel is connected. Then initialize |mTransport|.
2.2 |NotifyTransportReady| of |nsIPresentationSessionTransportCallback| is called.
3. |OnAnswer| of |nsIPresentationControlChannelListener| is called to indicate the receiver is ready. Close the control channel since it's no longer needed.
4. Once both step 2 and 3 are done, the presentation session is ready to use. So notify the listener of CONNECTED state.

Comment 139

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v3

Update session establishment logic at the receiver side.
The receiver expects the following after trying to launch the app by notifying "presentation-launch-receiver":
(The order between step 2 and 3 is not guaranteed.)
1. |Observe| of |nsIObserver| is called with "presentation-receiver-launched". Then start listen to document |STATE_TRANSFERRING| event.
2. |NotifyResponderReady| is called to indicate the receiver page is ready for presentation use.
3. |OnOffer| of |nsIPresentationControlChannelListener| is called.
4. Once both step 2 and 3 are done, establish the data transport channel and send the answer. (The control channel will be closed by the sender once it receives the answer.)
5. |NotifyTransportReady| of |nsIPresentationSessionTransportCallback| is called. The presentation session is ready to use, so notify the listener of CONNECTED state.

Comment 140

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8606901 [details] [diff] [review]
Part 2 - Presentation service and listeners, v3

I assume the following patches will change the initialization of the service a bit so that it is initialized only if explicitly allowed
(perhaps some specific permission and/or pref)

+  if (mListeners.Contains(aListener)) {
+    mListeners.AppendElement(aListener);
+  }
Should be !mListeners.Contains(aListener)

Comment 141

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8606903 [details] [diff] [review]
Part 3 - IPC, v4

> already_AddRefed<nsIPresentationService>
> NS_CreatePresentationService()
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>-  nsCOMPtr<nsIPresentationService> service = new PresentationService();
>-  return NS_WARN_IF(!static_cast<PresentationService*>(service.get())->Init()) ?
>-         nullptr : service.forget();
>+  nsCOMPtr<nsIPresentationService> service;
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    service = new mozilla::dom::PresentationIPCService();
>+  } else {
>+    service = new PresentationService();
>+    if (NS_WARN_IF(!static_cast<PresentationService*>(service.get())->Init())) {
>+      return nullptr;
>+    }
>+  }
>+
>+  return service.forget();
> }
So here we should probably check also that the process has right permissions/pref to actually use
presentation service.

>+struct PresentationErrorResponse
>+{
>+  nsresult error;
>+};
>+
>+struct PresentationSuccessResponse
>+{
>+};
>+
>+union PresentationResponse
>+{
>+  PresentationErrorResponse;
>+  PresentationSuccessResponse;
>+};
This is odd. Why not just nsresult?


>+PresentationIPCService::NotifyAvailableChange(bool aAvailable)
>+{
>+  nsTObserverArray<nsCOMPtr<nsIPresentationListener> >::ForwardIterator iter(mListeners);
s/> >/>>/


r+ if comments addressed, possibly in a new patch.

Comment 142

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8606904 [details] [diff] [review]
Part 4 - Establish session (sender) & available changes, v3

>+NS_IMETHODIMP
>+Presentation::NotifyAvailableChange(bool aAvailable)
>+{
>+  mAvailable = aAvailable;
>+
>+  PresentationAvailableEventInit init;
>+  init.mAvailable = mAvailable;
>+  nsRefPtr<PresentationAvailableEvent> event =
>+    PresentationAvailableEvent::Constructor(this,
>+                                            NS_LITERAL_STRING("availablechange"),
>+                                            init);
>+  return DispatchTrustedEvent(event);
So I think we should dispatch this async - that would be a tad safer in case some event listener does something
unexpected. Async would mean that NotifyAvailableChange callers don't need to be careful that scripts may run when called.
So, use AsyncEventDispatcher.


>+  virtual ~PresentationSessionInfo() {
{ goes to its own line 

>+    Shutdown(NS_OK);
>+  }
>+
>+  virtual void Shutdown(nsresult aReason);
>+
>+  nsresult ReplySuccess();
>+
>+  bool IsSessionReady() {
ditto, and elsewhere with some other methods



Because of many TODOs, still a bit hard to review.

Comment 143

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8606905 [details] [diff] [review]
Part 5 - Establish session (receiver), v3

>+PresentationResponderLoadingCallback::Init(nsIDocShell* aDocShell)
>+{
>+  mProgress = do_GetInterface(aDocShell);
>+  if (NS_WARN_IF(!mProgress)) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  nsCOMPtr<nsIDOMWindow> domWindow;
>+  nsresult rv = mProgress->GetDOMWindow(getter_AddRefs(domWindow));
>+  if (NS_SUCCEEDED(rv)) {
>+    // The DOM window is available, so the page is ready for presentation use.
>+    return NotifyReceiverReady();
>+  }
As far as I see, GetDOMWindow() succeeds always, except when docshell is being shutdown (because it returns the outerwindow which is bound to the docshell).
So, I guess the rest of the code in this method isn't really tested, right?



>+  bool isApp;
>+  rv = uri->SchemeIs("app", &isApp);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    ctrlChannel->Close(NS_ERROR_DOM_ABORT_ERR);
>+    return rv;
>+  }
>+  bool isHttp;
>+  rv = uri->SchemeIs("http", &isHttp);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    ctrlChannel->Close(NS_ERROR_DOM_ABORT_ERR);
>+    return rv;
>+  }
>+  bool isHttps;
>+  rv = uri->SchemeIs("https", &isHttps);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    ctrlChannel->Close(NS_ERROR_DOM_ABORT_ERR);
>+    return rv;
>+  }
>+
>+  if (NS_WARN_IF(!isApp && !isHttp && !isHttps)) {
>+    ctrlChannel->Close(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>+    return NS_OK;
>+  }
Why this limitation? Why file isn't supported, but http is?
This needs at least some comments.


But I'd like to see a new patch with that DOMWindow handling fixed.

Comment 144

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Presentation service and listeners, v4

Updating based on r+ comments.

Comment 145

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - IPC, v5

Minor updates based on r+ comments.

Comment 146

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Establish session (sender) & available changes, v4

Minor updates based on r+ comments.

Comment 147

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v4

Prevent using |GetDOMWindow()| to determine page loading state.

Comment 148

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v3

Protect Presentation API with permission "presentation".
Use a simple event instead of the specific PresentationSessionStateEvent for state changes to better fit the spec.

Comment 149

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 6 - mozChromeEvent for app launch

Hi Fabrice,

We plan to use 'presentation-launch-receiver' chrome event to open the responding app/page when the device receives a presentation request; and then listen to the 'presentation-receiver-launched' content event after it has been launched. Could you help review the change we made in shell.js?

Comment 150

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 7 - Presentation session

Implement presentation session activities.

Comment 151

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel

Implement the data transport channel over TCP for now. It will probably be refactored to use DataChannel in bug 1148307 in the future.

Comment 152

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 7 - Presentation session, v2

Implement presentation session activities.
Remove session info when content process gets killed.

Comment 153

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8610481 [details] [diff] [review]
Part 1 - WebIDL Bindings, v3

In the future when just adding something small to an already reviewed patch please provide a followup patch.

So, in the current spec it is send(), but this patch has postMessage().
Which one is the correct one? Still being discussed? Could we just use send()
as per spec, and rename later to postMessage() if needed.

I see https://github.com/w3c/presentation-api/commit/bb065d7dfaf35f992eef4d02b58917ca8f46b36d
So we should use send at least for now. So, please provide a new patch on top of this one which has send() and not postMessage()

Comment 154

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8610479 [details] [diff] [review]
Part 5 - Establish session (receiver), v4

> ContentChild::RecvNotifyPresentationReceiverLaunched(PBrowserChild* aIframe,
>                                                      const nsString& aSessionId)
> {
>-    // TODO Listen to |nsIWebProgressListener| state changes for this frame.
>+    nsCOMPtr<nsIDocShell> docShell =
>+        do_GetInterface(static_cast<TabChild*>(aIframe)->WebNavigation());
>+    NS_WARN_IF(!docShell);
>+
>+    nsCOMPtr<nsIPresentationService> service =
>+      do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+    NS_WARN_IF(!service);
>+
>+    NS_WARN_IF(NS_FAILED(static_cast<PresentationIPCService*>(service.get())->MonitorResponderLoading(aSessionId, docShell)));
> 
>     return true;
> }
Please be consistent with indentation. Not 2 and 4 spaces.
New code could use 2 spaces (I know coding style in ContentChild is a mess)


>+PresentationResponderLoadingCallback::Init(nsIDocShell* aDocShell)
>+{
>+  mProgress = do_GetInterface(aDocShell);
>+  if (NS_WARN_IF(!mProgress)) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  uint32_t busyFlags = nsIDocShell::BUSY_FLAGS_NONE;
>+  nsresult rv = aDocShell->GetBusyFlags(&busyFlags);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return rv;
>+  }
>+
>+  if ((busyFlags & nsIDocShell::BUSY_FLAGS_NONE) ||
>+      (busyFlags & nsIDocShell::BUSY_FLAGS_PAGE_LOADING)) {
>+    // The docshell has finished loading or is receiving data (|STATE_TRANSFERRING|
>+    // has already been fired), so the page is ready for presentation use.
>+    return NotifyReceiverReady();
>+  }
>+
>+  // Start to listen to document state change event |STATE_TRANSFERRING|.
>+  return mProgress->AddProgressListener(this, nsIWebProgress::NOTIFY_STATE_DOCUMENT);
>+}
So I still don't quite understand the setup, mainly because it is not clear at which point we get 
"presentation-receiver-launched". What if we are already loading but loading is slow and we don't get any 
OnStateChange calls? So I think you could check here if we already have 


>+PresentationResponderLoadingCallback::OnProgressChange(nsIWebProgress *aWebProgress,
>+                                                       nsIRequest *aRequest,
* goes with the type
Same also elsewhere

Comment 155

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8611121 [details] [diff] [review]
Part 7 - Presentation session, v2

>-  // TODO: Send input stream to the session.
>@@ -106,22 +125,166 @@ PresentationSession::PostMessage(const n
> 
>   NS_ConvertUTF16toUTF8 msgString(aData);
>   rv = stream->SetData(msgString.BeginReading(), msgString.Length());
>   if(NS_WARN_IF(NS_FAILED(rv))) {
>     aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
>     return;
>   }
> 
>-  // TODO: Send the message to the stream.
>+  nsCOMPtr<nsIPresentationService> service =
>+    do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+  if(NS_WARN_IF(!service)) {
>+    aRv.Throw(NS_ERROR_NOT_AVAILABLE);
>+    return;
>+  }
>+
>+  rv = service->PostMessage(mId, stream);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
>+  }
I think you want InvalidStateError here. NS_ERROR_DOM_INVALID_STATE_ERR in both cases.
Though the spec isn't quite clear, but InvalidStateError is the only error it mentions.



> PresentationSession::Terminate(ErrorResult& aRv)
Oh, now I realized, this should be Close() and close() in .webidl, not .terminate()



> {
>   // Closing is not allowed if the socket is not connected.
>   if (NS_WARN_IF(mState == PresentationSessionState::Terminated)) {
>     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return;
>   }
> 
>-  // TODO: Terminate the socket.
>+  nsCOMPtr<nsIPresentationService> service =
>+    do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+  if(NS_WARN_IF(!service)) {
>+    aRv.Throw(NS_ERROR_NOT_AVAILABLE);
>+    return;
>+  }
>+
>+  nsresult rv = service->Terminate(mId);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
>+  }
Per spec close() never throws anything, so I think you should remove [Throws] from the .webidl.


>+PresentationSession::NotifyMessage(const nsAString& aSessionId,
>+                                   const nsACString& aData)
>+{
>+  if (!aSessionId.Equals(mId)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  // No message should be expected when the session is not connected.
>+  if (NS_WARN_IF(mState != PresentationSessionState::Connected)) {
>+    return NS_ERROR_DOM_INVALID_STATE_ERR;
>+  }
>+
>+  // Transform the data.
>+  AutoJSAPI jsapi;
>+  if (!jsapi.Init(GetOwner())) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  JSContext* cx = jsapi.cx();
>+  JS::Rooted<JS::Value> jsData(cx);
>+  NS_ConvertUTF8toUTF16 utf16Data(aData);
>+  JSString* jsString = JS_NewUCStringCopyN(cx, utf16Data.get(), utf16Data.Length());
>+  if(NS_WARN_IF(!jsString)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  jsData = STRING_TO_JSVAL(jsString);
>+
>+  return DispatchMessageEvent(jsData);
Please use ToJSValue to convert a string to JS::Value


I'd like to see a new patch fixing the small issue on top of this patch.

Comment 156

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8610491 [details] [diff] [review]
Part 8 - Data transport channel

I think some necko peer must review this too

>+PresentationRequesterInfo::GetAddress(nsACString& aAddress)
>+{
>+#ifdef MOZ_WIDGET_GONK
>+  nsCOMPtr<nsINetworkManager> networkManager =
>+    do_GetService("@mozilla.org/network/manager;1");
>+  if (NS_WARN_IF(!networkManager)) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  nsCOMPtr<nsINetworkInterface> active;
>+  networkManager->GetActive(getter_AddRefs(active));
>+  if (NS_WARN_IF(!active)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  char16_t **ips = nullptr;
** goes with the type
>+  uint32_t *prefixs = nullptr;
so does *

>+PresentationSessionTransport::GetCallback(nsIPresentationSessionTransportCallback** aCallback)
>+{
>+  if (!mCallback) {
>+    *aCallback = nullptr;
>+    return NS_OK;
>+  }
>+
>+  *aCallback = mCallback;
>+  NS_ADDREF(*aCallback);
>+  return NS_OK;
You could just do
nsCOMPtr<nsIPresentationSessionTransportCallback> callback = mCallback;
callback.forget(aCallback);

Comment 157

[Olli Pettay [:smaug][[e-mail]]]

In other words, that wasn't a review. Some network peer should review it, at least the use
of necko APIs

Comment 158

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v4

Renaming some methods in PresentationSession based on r+ comments.

Comment 159

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v5

Minor updates based on r+ comments.

Comment 160

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 7 - Presentation session, v3

Minor updates based on r+ comments.

Comment 161

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8610491 [details] [diff] [review]
Part 8 - Data transport channel

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

Hi Jason,

Could you help review this patch for the use of necko API?

Comment 162

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel, v2

Comment 163

[[:fabrice] Fabrice Desré]

Comment on attachment 8610486 [details] [diff] [review]
Part 6 - mozChromeEvent for app launch

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

So, for many other apis we abstract the UI part into an xpcom component (see dom/interfaces/apps/nsIInterAppCommUIGlue.idl¸ dom/activities/interfaces/nsIActivityUIGlue.idl, dom/payment/interfaces/nsIPaymentUIGlue.idl) for instance.
I think it's a better design that sending observer notifications around since you don't really expect anyone else to observe that.
So something as simple as that should be enough:
interface PresentationRequestUIGlue {
  nsISupports sendRequest(url); // returns a promise that resolves to the frame
}

Comment 164

[Honza Bambas (:mayhemer)]

Comment on attachment 8612789 [details] [diff] [review]
Part 8 - Data transport channel, v2

Steeling.

Comment 165

[Honza Bambas (:mayhemer)]

Comment on attachment 8612789 [details] [diff] [review]
Part 8 - Data transport channel, v2

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

r- for an absolute lack of any comments in the new code.

Please add well written comments and resubmit.

Comment 166

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

Comment on attachment 8612170 [details] [diff] [review]
Part 1 - WebIDL Bindings, v4

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

::: dom/presentation/moz.build
@@ +16,3 @@
>  ]
>  
>  SOURCES += [

Hi Sean, can you also change this to |UNIFIED_SOURCES|? Thanks!

Comment 167

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel, v3

Adding some comments.
Besides, some comments about |nsIPresentationChannelDescription| are available at
https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationControlChannel.idl#11
And for those about |nsIPresentationSessionTransport|:
https://bugzilla.mozilla.org/attachment.cgi?id=8610471&action=diff#a/dom/presentation/interfaces/nsIPresentationSessionTransport.idl_sec1

Comment 168

[Honza Bambas (:mayhemer)]

(In reply to Sean Lin [:seanlin] from comment #167)
> Created attachment 8614554 [details] [diff] [review]
> Part 8 - Data transport channel, v3
> 
> Adding some comments.

Hmm.. Where exactly..?

Adding comments means - at the very least - to explain purpose, lifetime and whatever overview details for each new class you make.

Never mind, I'll walk through it, it will just be harder.  And honestly, I have a strong taste to not let this go through for the general lack of comments anyway.  This is not how an open source code is about to be written.  Sorry.

> Besides, some comments about |nsIPresentationChannelDescription| are
> available at
> https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/
> nsIPresentationControlChannel.idl#11
> And for those about |nsIPresentationSessionTransport|:
> https://bugzilla.mozilla.org/attachment.cgi?id=8610471&action=diff#a/dom/
> presentation/interfaces/nsIPresentationSessionTransport.idl_sec1

That might help, thanks.

Comment 169

[Honza Bambas (:mayhemer)]

Comment on attachment 8614554 [details] [diff] [review]
Part 8 - Data transport channel, v3

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

Except that I don't fully follow how this is integrated to the rest of the patches/platform (no useful comments added...) I can see some issues with PresentationSessionTransport.cpp.  Please see TCPSocket.js how a well implemented use of socket transport should look like.

r-, I'd like to a see a next version.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +90,5 @@
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  address->SetData(mAddress);
> +
> +  array->AppendElement(address, false);

The name of the method is "GetTcpAddress", not "GetTcpAddresses".  Interface comment says nothing (example of a bad comment ;)), so I'm confused if this is correct or not...  Hard to say.

BTW, the network manager returns an array of local addresses, so you might want to carry them all?  I can see you just pick the first.  Further the code is trying to use all possible IPs.

::: dom/presentation/PresentationSessionTransport.cpp
@@ +36,5 @@
> +  }
> +}
> +
> +nsresult
> +PresentationSessionTransport::Init(nsIPresentationSessionTransportCallback* aCallback)

this should be InitInternal

@@ +45,5 @@
> +  mCallback = aCallback;
> +
> +  // TODO May need to find better parameters.
> +
> +  nsresult rv = mTransport->SetEventSink(this, NS_GetCurrentThread());

how is the transport created?

@@ +151,5 @@
> +    supportStr->GetData(serverHost);
> +    rv = sts->CreateTransport(nullptr, 0, serverHost, serverPort, nullptr,
> +                              getter_AddRefs(mTransport));
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      // Try next address.

I don't think you get all possible errors here soon enough.  You must wait for the output stream to be writable first.  When it is, try to write.  When it fails first time you try writing (except the error would be WOULD_BLOCK), try the next address.  I see you don't do any SSL here (intention?)  In case of SSL socket the error is detected even later I think (I don't remember exactly, tho.)

we should have some layer over the socket transport making this easier :/  I see this struggle not the first time.  networking is hard!

@@ +192,5 @@
> +  if (NS_WARN_IF(!mTransport)) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> +  }
> +
> +  return mTransport->GetScriptableSelfAddr(aSelfAddress);;

two ;;

@@ +204,5 @@
> +  if (mBufferedData.IsEmpty()) {
> +    mBufferedData.AppendElement(aData);
> +    return SendInternal(aData);
> +  } else {
> +    mBufferedData.AppendElement(aData);

if this is called on a thread different than the socket thread, then you need a lock over the mBufferData array.

a bit better way is to have a multiplex input stream, but up to you (see TCPSocket.js).

@@ +218,5 @@
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  return NS_AsyncCopy(aData, mOutputStream, sts, NS_ASYNCCOPY_VIA_WRITESEGMENTS,
> +                      4096, SendBufferedData, this, false, false, nullptr,

who's gonna close the source?
what threads is this called on?

@@ +226,5 @@
> +void
> +PresentationSessionTransport::SendBufferedData(void* aClosure,
> +                                               nsresult aStatus)
> +{
> +  NS_WARN_IF(NS_FAILED(aStatus));

this probably means the socket is closed or the input was broken.

@@ +232,5 @@
> +  nsRefPtr<PresentationSessionTransport> sessionTransport =
> +    static_cast<PresentationSessionTransport*>(aClosure);
> +
> +  sessionTransport->mBufferedData.RemoveElementAt(0);
> +  if (!sessionTransport->mBufferedData.IsEmpty()) {

this is called on the socket thread.

@@ +242,5 @@
> +NS_IMETHODIMP
> +PresentationSessionTransport::Close(nsresult aReason)
> +{
> +  mInputStream = nullptr;
> +  mOutputStream = nullptr;

threading...

@@ +265,5 @@
> +
> +  if (aStatus == NS_NET_STATUS_CONNECTED_TO) {
> +    // Start reading the input stream.
> +    nsCOMPtr<nsIInputStreamPump> pump;
> +    nsresult rv = NS_NewInputStreamPump(getter_AddRefs(pump), mInputStream);

this overwrites your AsyncWait on the input, just what to make sure you are aware..

@@ +294,5 @@
> +  uint64_t isAvailable;
> +  nsresult rv = aStream->Available(&isAvailable);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return Close(rv);
> +  }

what is this for?  You are checking for connect or close errors?  If so, then it's not the best way.  I'll again let you see the TCPSocket.js file, all problems solved there..

@@ +344,5 @@
> +    return Close(rv);
> +  }
> +
> +  // Pass the incoming data to the listener.
> +  return mCallback->NotifyData(data);

just a note that this can be coming in chunks, is the callback implementation prepared for it?  I'm in favor leaving the code exactly as it is - to deliver in chunks.  Just make sure the target can consume it.

Comment 170

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 6 - mozChromeEvent for app launch, v2

Updating based on comment 163.

Comment 171

[[:fabrice] Fabrice Desré]

Comment on attachment 8628740 [details] [diff] [review]
Part 6 - mozChromeEvent for app launch, v2

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

Can we have tests for that?

::: b2g/components/PresentationRequestUIGlue.js
@@ +7,5 @@
> +const { interfaces: Ci, utils: Cu, classes: Cc } = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

Please use the native Promise implementation instead.

@@ +13,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",
> +                                  "resource://gre/modules/SystemAppProxy.jsm");
> +
> +function PresentationRequestUIGlue() {
> +  // This matrix is to store the session ID / deferral binding.

nit: that's not really a matrix, just a dictionnary

@@ +23,5 @@
> +  // }
> +  this._deferreds = {};
> +
> +  // Listen to the result for the opened iframe from front-end.
> +  SystemAppProxy.addEventListener ("mozPresentationContentEvent", function (aEvent) {

nit: addEventListener("mozPresentationContentEvent", aEvent => {
...
}); // no need to bind()

Comment 172

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 9 - Tests. (WIP)

Comment 173

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 6 - mozChromeEvent for app launch, v3

Adding tests for PresentationRequestUIGlue and other minor updates.

(More tests with other components at the receiver side are available in Part 9 WIP patch.)

Comment 174

[[:fabrice] Fabrice Desré]

Comment on attachment 8630385 [details] [diff] [review]
Part 6 - mozChromeEvent for app launch, v3

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

Thanks!

Comment 175

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8630385 [details] [diff] [review]
Part 6 - mozChromeEvent for app launch, v3

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

Hi smaug,

In this patch, some parts of PresentationService and PresentationSessionInfo are updated due to the introduction of the UI glue. Could you help take a look at them?

Comment 176

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Honza Bambas (:mayhemer) from comment #169)
> Comment on attachment 8614554 [details] [diff] [review]
> Part 8 - Data transport channel, v3
> 
> Review of attachment 8614554 [details] [diff] [review]:
> ::: dom/presentation/PresentationSessionInfo.cpp
> @@ +90,5 @@
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +  }
> > +  address->SetData(mAddress);
> > +
> > +  array->AppendElement(address, false);
> 
> The name of the method is "GetTcpAddress", not "GetTcpAddresses".  Interface
> comment says nothing (example of a bad comment ;)), so I'm confused if this
> is correct or not...  Hard to say.
> 
> BTW, the network manager returns an array of local addresses, so you might
> want to carry them all?  I can see you just pick the first.  Further the
> code is trying to use all possible IPs.
> 
Ultimately we'll carry all the local addresses. But it may require a more comprehensive ICE-relevant solution to verify each address is reachable for the connecting peer. For now this patch is used as an intermediate one before the session transport is migrated to Data Channel, which appears more robust for some of these concerns, in bug 1148307 in the near future. And at the first stage this API is only exposed on Firefox OS where the first IP appears enough for most scenarios. Could we leave this for now and let it fully resolved in bug 1148307?

> ::: dom/presentation/PresentationSessionTransport.cpp
> @@ +45,5 @@
> > +  mCallback = aCallback;
> > +
> > +  // TODO May need to find better parameters.
> > +
> > +  nsresult rv = mTransport->SetEventSink(this, NS_GetCurrentThread());
> 
> how is the transport created?
> 
|mTransport| is created either in |InitWithSocketTransport| or |InitWithChannelDescription|. (This method is called in those two methods after |mTransport| is initialized.)

> @@ +151,5 @@
> > +    supportStr->GetData(serverHost);
> > +    rv = sts->CreateTransport(nullptr, 0, serverHost, serverPort, nullptr,
> > +                              getter_AddRefs(mTransport));
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      // Try next address.
> 
> I don't think you get all possible errors here soon enough.  You must wait
> for the output stream to be writable first.  When it is, try to write.  When
> it fails first time you try writing (except the error would be WOULD_BLOCK),
> try the next address.  I see you don't do any SSL here (intention?)  In case
> of SSL socket the error is detected even later I think (I don't remember
> exactly, tho.)
> 
> we should have some layer over the socket transport making this easier :/  I
> see this struggle not the first time.  networking is hard!
> 
We plan to have a workable intermediate implementation of session transport for now, and then migrate it to Data Channel (with SSL) in bug 1148307 as a follow-up.

> @@ +294,5 @@
> > +  uint64_t isAvailable;
> > +  nsresult rv = aStream->Available(&isAvailable);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    return Close(rv);
> > +  }
> 
> what is this for?  You are checking for connect or close errors?  If so,
> then it's not the best way.  I'll again let you see the TCPSocket.js file,
> all problems solved there..
> 
Actually we follow the logic in TCPSocket.js for this method (|OnInputStreamReady|). Btw, since TCPSocket.js is well implemented to resolve many mentioned issues, and we plan to have a workable implementation for now, is it an acceptable idea to simply adopt TCPSocket.js here? I found there are some examples to use a relevant XPCOM instance to manipulate TCP sockets. [1][2][3]

Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket)

(Typically it seems to require a window object if initiated through WebIDL code [4]. But accessing it as an XPCOM instance appears to bypass such requirement and still has a way to use the core socket function.)

[1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/shumway/chrome/RtmpUtils.jsm#38
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/secondscreen/RokuApp.jsm#312
[3] https://dxr.mozilla.org/mozilla-central/source/dom/network/TCPSocketParentIntermediary.js#41
[4] https://dxr.mozilla.org/mozilla-central/source/dom/network/TCPSocket.js#523

Comment 177

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8630385 [details] [diff] [review]
Part 6 - mozChromeEvent for app launch, v3

>-  rv = obs->NotifyObservers(aRequest, "presentation-launch-receiver", nullptr);
>+  nsCOMPtr<nsISupports> promise;
>+  rv = glue->SendRequest(url, sessionId, getter_AddRefs(promise));
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     ctrlChannel->Close(NS_ERROR_DOM_ABORT_ERR);
>     return info->ReplyError(rv);
>   }
>+  static_cast<PresentationResponderInfo*>(info.get())->SetPromise(static_cast<Promise*>(promise.get()));
Promise has IID, so I'd prefer it you did something like
nsCOMPtr<Promise> realPromise = do_QueryInterface(promise);
static_cast<PresentationResponderInfo*>(info.get())->SetPromise(realPromise);

>+PresentationResponderInfo::ResolvedCallback(JSContext* aCx,
>+                                            JS::Handle<JS::Value> aValue)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  if (NS_WARN_IF(!aValue.isObject())) {
>+    ReplyError(NS_ERROR_NOT_AVAILABLE);
>+    return;
>+  }
>+
>+  JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
>+  if (NS_WARN_IF(!obj)) {
>+    ReplyError(NS_ERROR_NOT_AVAILABLE);
>+    return;
>+  }
>+
>+  // Start to listen to document state change event |STATE_TRANSFERRING|.
>+  HTMLIFrameElement* frame = nullptr;
>+  nsresult rv = UNWRAP_OBJECT(HTMLIFrameElement, obj, frame);
>+  if (NS_WARN_IF(!frame)) {
>+    ReplyError(NS_ERROR_NOT_AVAILABLE);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIFrameLoaderOwner> owner = do_QueryInterface((nsIFrameLoaderOwner*) frame);
C++ casting please, so static_cast.
But, HTMLIFrameElement is an nsIFrameLoaderOwner so you don't need any QIing or owner here.
HTMLIFrameElement extends nsGenericHTMLFrameElement which extends nsIFrameLoaderOwner

>+  void SetPromise(Promise* aPromise) {
nit, { goes to its own line.


>+[scriptable, uuid(faa45119-6fb5-496c-aa4c-f740177a38b5)]
>+interface nsIPresentationRequestUIGlue : nsISupports
>+{
>+  /*
>+   * This method is called to open the responding app/page when a presentation
>+   * request comes in.

Please add a comment on which side of presentation setup this is supposed to be used.
It this for the app doing the initial request, or the one showing the presentation.

Comment 178

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #177)
> Comment on attachment 8630385 [details] [diff] [review]
> Part 6 - mozChromeEvent for app launch, v3
> >+PresentationResponderInfo::ResolvedCallback(JSContext* aCx,
> >+                                            JS::Handle<JS::Value> aValue)
> >+{
> >+  MOZ_ASSERT(NS_IsMainThread());
> >+
> >+  if (NS_WARN_IF(!aValue.isObject())) {
> >+    ReplyError(NS_ERROR_NOT_AVAILABLE);
> >+    return;
> >+  }
> >+
> >+  JS::Rooted<JSObject*> obj(aCx, &aValue.toObject());
> >+  if (NS_WARN_IF(!obj)) {
> >+    ReplyError(NS_ERROR_NOT_AVAILABLE);
> >+    return;
> >+  }
> >+
> >+  // Start to listen to document state change event |STATE_TRANSFERRING|.
> >+  HTMLIFrameElement* frame = nullptr;
> >+  nsresult rv = UNWRAP_OBJECT(HTMLIFrameElement, obj, frame);
> >+  if (NS_WARN_IF(!frame)) {
> >+    ReplyError(NS_ERROR_NOT_AVAILABLE);
> >+    return;
> >+  }
> >+
> >+  nsCOMPtr<nsIFrameLoaderOwner> owner = do_QueryInterface((nsIFrameLoaderOwner*) frame);
> C++ casting please, so static_cast.
> But, HTMLIFrameElement is an nsIFrameLoaderOwner so you don't need any QIing
> or owner here.
> HTMLIFrameElement extends nsGenericHTMLFrameElement which extends
> nsIFrameLoaderOwner
> 
nsGenericHTMLFrameElement.h doesn't get listed in the EXPORTS in dom/html/moz.build. So it would result in build failures (nsGenericHTMLFrameElement.h: No such file or directory) once mozilla/dom/HTMLIFrameElement.h is included for using |static_cast| or directly |frame->GetFrameLoader(...)|. That's why I use QI as a workaround.

Comment 179

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 6 - mozChromeEvent for app launch, v4

Updating based on r+ comments.

Comment 180

[Honza Bambas (:mayhemer)]

(In reply to Sean Lin [:seanlin] from comment #176)
> (In reply to Honza Bambas (:mayhemer) from comment #169)
> > Comment on attachment 8614554 [details] [diff] [review]
> > Part 8 - Data transport channel, v3
> > 
> > Review of attachment 8614554 [details] [diff] [review]:
> > ::: dom/presentation/PresentationSessionInfo.cpp
> > @@ +90,5 @@
> > > +    return NS_ERROR_OUT_OF_MEMORY;
> > > +  }
> > > +  address->SetData(mAddress);
> > > +
> > > +  array->AppendElement(address, false);
> > 
> > The name of the method is "GetTcpAddress", not "GetTcpAddresses".  Interface
> > comment says nothing (example of a bad comment ;)), so I'm confused if this
> > is correct or not...  Hard to say.
> > 
> > BTW, the network manager returns an array of local addresses, so you might
> > want to carry them all?  I can see you just pick the first.  Further the
> > code is trying to use all possible IPs.
> > 
> Ultimately we'll carry all the local addresses. But it may require a more
> comprehensive ICE-relevant solution to verify each address is reachable for
> the connecting peer. For now this patch is used as an intermediate one
> before the session transport is migrated to Data Channel, 

Can you point me at that code? 

> which appears more
> robust for some of these concerns, in bug 1148307 in the near future. And at
> the first stage this API is only exposed on Firefox OS where the first IP
> appears enough for most scenarios. Could we leave this for now and let it
> fully resolved in bug 1148307?

Yes, we can, and we could save this comment ping-pong if you mentioned that in the patch (what you have to anyway) or at least pointed that out in a bugzilla comment ;)

> We plan to have a workable intermediate implementation of session transport
> for now, and then migrate it to Data Channel (with SSL) in bug 1148307 as a
> follow-up.

Hmm.. but this intermediate impl can then be pretty broken...

> 
> > @@ +294,5 @@
> > > +  uint64_t isAvailable;
> > > +  nsresult rv = aStream->Available(&isAvailable);
> > > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> > > +    return Close(rv);
> > > +  }
> > 
> > what is this for?  You are checking for connect or close errors?  If so,
> > then it's not the best way.  I'll again let you see the TCPSocket.js file,
> > all problems solved there..
> > 
> Actually we follow the logic in TCPSocket.js for this method
> (|OnInputStreamReady|). Btw, since TCPSocket.js is well implemented to
> resolve many mentioned issues, and we plan to have a workable implementation
> for now, is it an acceptable idea to simply adopt TCPSocket.js here? 

If you can, please do!  Building on top of an existing code is always good.

> I found
> there are some examples to use a relevant XPCOM instance to manipulate TCP
> sockets. [1][2][3]
> 
> Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsIDOMTCPSocket)

You can definitely do this.  If there are problem or you need some tweaks in TCPSocket.js and around, we can introduce them if it would make this bug simpler to fix.

Comment 181

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel, v4

Implement nsIPresentationSessionTransport on top of TCPSocket.js.

Comment 182

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 9 - Tests, v1

Comment 183

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8633974 [details] [diff] [review]
Part 9 - Tests, v1

># HG changeset patch
># User Sean Lin <[e-mail]>
># Date 1429760641 -28800
>#      Thu Apr 23 11:44:01 2015 +0800
># Node ID 87b53e3fe756ba775bc8283fe4b646130563b7da
># Parent  b4c87312cadf90f76d9ffbfdc655198a8038f3c7
>Bug 1069230 - Presentation API implementation. Part 9 - Tests.
>
>diff --git a/dom/presentation/PresentationSessionInfo.cpp b/dom/presentation/PresentationSessionInfo.cpp
>--- a/dom/presentation/PresentationSessionInfo.cpp
>+++ b/dom/presentation/PresentationSessionInfo.cpp
>@@ -511,16 +511,20 @@ PresentationRequesterInfo::OnSocketAccep
> }
> 
> NS_IMETHODIMP
> PresentationRequesterInfo::OnStopListening(nsIServerSocket* aServerSocket,
>                                            nsresult aStatus)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>+  if (aStatus == NS_BINDING_ABORTED) { // The server socket was manually closed.
>+    return NS_OK;
>+  }
>+
>   Shutdown(aStatus);
> 
>   if (!IsSessionReady()) {
>     // It happens before the session is ready. Reply the callback.
>     return ReplyError(aStatus);
>   }
> 
>   // It happens after the session is ready. Notify session state change.
>@@ -788,17 +792,17 @@ PresentationResponderInfo::ResolvedCallb
>   }
> }
> 
> void
> PresentationResponderInfo::RejectedCallback(JSContext* aCx,
>                                             JS::Handle<JS::Value> aValue)
> {
>   MOZ_ASSERT(NS_IsMainThread());
>-  NS_WARNING("The receiver page fails to become ready before timeout.");
>+  NS_WARNING("Launching the receiver page has been rejected.");
> 
>   if (mTimer) {
>     mTimer->Cancel();
>     mTimer = nullptr;
>   }
> 
>   ReplyError(NS_ERROR_DOM_ABORT_ERR);
> }
Why these changes in a patch adding tests?




May take a bit time before I can review the tests.

Comment 184

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] from comment #183)
> Comment on attachment 8633974 [details] [diff] [review]
> Part 9 - Tests, v1
> 
> ># HG changeset patch
> ># User Sean Lin <[e-mail]>
> ># Date 1429760641 -28800
> >#      Thu Apr 23 11:44:01 2015 +0800
> ># Node ID 87b53e3fe756ba775bc8283fe4b646130563b7da
> ># Parent  b4c87312cadf90f76d9ffbfdc655198a8038f3c7
> >Bug 1069230 - Presentation API implementation. Part 9 - Tests.
> >
> >diff --git a/dom/presentation/PresentationSessionInfo.cpp b/dom/presentation/PresentationSessionInfo.cpp
> >--- a/dom/presentation/PresentationSessionInfo.cpp
> >+++ b/dom/presentation/PresentationSessionInfo.cpp
> >@@ -511,16 +511,20 @@ PresentationRequesterInfo::OnSocketAccep
> > }
> > 
> > NS_IMETHODIMP
> > PresentationRequesterInfo::OnStopListening(nsIServerSocket* aServerSocket,
> >                                            nsresult aStatus)
> > {
> >   MOZ_ASSERT(NS_IsMainThread());
> > 
> >+  if (aStatus == NS_BINDING_ABORTED) { // The server socket was manually closed.
> >+    return NS_OK;
> >+  }
> >+
> >   Shutdown(aStatus);
> > 
> >   if (!IsSessionReady()) {
> >     // It happens before the session is ready. Reply the callback.
> >     return ReplyError(aStatus);
> >   }
> > 
> >   // It happens after the session is ready. Notify session state change.
> >@@ -788,17 +792,17 @@ PresentationResponderInfo::ResolvedCallb
> >   }
> > }
> > 
> > void
> > PresentationResponderInfo::RejectedCallback(JSContext* aCx,
> >                                             JS::Handle<JS::Value> aValue)
> > {
> >   MOZ_ASSERT(NS_IsMainThread());
> >-  NS_WARNING("The receiver page fails to become ready before timeout.");
> >+  NS_WARNING("Launching the receiver page has been rejected.");
> > 
> >   if (mTimer) {
> >     mTimer->Cancel();
> >     mTimer = nullptr;
> >   }
> > 
> >   ReplyError(NS_ERROR_DOM_ABORT_ERR);
> > }
> Why these changes in a patch adding tests?
> 
The change in |OnStopListening| (to handle a manual socket close more gracefully) is due to an issue discovered from a test case. (The PresentationSessionInfo may get cleaned up too early before the corresponding PresentationSession invokes |unregisterSessionListener|.) And I updated the wording of the warning message in |RejectedCallback| since it was kinda misleading and might cause potential confusion with the real place for timeout handling.

Comment 185

[Florian Bender]

Is (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #23)
> 2. When spec is stable, we can make it available to web pages on Firefox OS.
> |AvailableIn="PrivilegedApps"| can be removed in this stage.
> 
> 3. When our implementation is stable, we can make it available to all
> platform. Preference |dom.presentation.enabled| can be default on for all
> platform.

I know we're not past step 1 yet, but are there any follow-up bugs open for step 2 & 3 yet?

Comment 186

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

(In reply to Florian Bender from comment #185)
> Is (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from
> comment #23)
> > 2. When spec is stable, we can make it available to web pages on Firefox OS.
> > |AvailableIn="PrivilegedApps"| can be removed in this stage.
> > 
> > 3. When our implementation is stable, we can make it available to all
> > platform. Preference |dom.presentation.enabled| can be default on for all
> > platform.
> 
> I know we're not past step 1 yet, but are there any follow-up bugs open for
> step 2 & 3 yet?

I'm working on filing all the follow-up bugs for the future development plan. You can refer to the description first. https://wiki.mozilla.org/WebAPI/PresentationAPI#Development_Plan

Comment 187

[Honza Bambas (:mayhemer)]

Comment on attachment 8632674 [details] [diff] [review]
Part 8 - Data transport channel, v4

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

f+ only since you will have to rewrite this once again on top of bug 885982.

as far as I understand the relations here this looks good.  but I don't provide any warranty for this r+ since I don't know well enough the surrounding code.  the r+ will be mostly just for the socket integration

::: dom/presentation/PresentationDeviceInfoManager.manifest
@@ +1,5 @@
>  # PresentationDeviceInfoManager.js
>  component {1bd66bef-f643-4be3-b690-0c656353eafd} PresentationDeviceInfoManager.js
>  contract @mozilla.org/presentation-device/deviceInfo;1 {1bd66bef-f643-4be3-b690-0c656353eafd}
> +
> +# PresentationDeviceInfoManager.js

wrong file name?

::: dom/presentation/PresentationSessionInfo.cpp
@@ +365,5 @@
> +  }
> +
> +  nsRefPtr<PresentationChannelDescription> description =
> +    new PresentationChannelDescription(address, static_cast<uint16_t>(port));
> +  rv = mControlChannel->SendOffer(description);

someone who knows the PresentationRequesterInfo and mControlChannel relation should review this.

@@ +402,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  char16_t** ips = nullptr;
> +  uint32_t* prefixs = nullptr;

prefixes

@@ +423,5 @@
> +
> +  NS_Free(prefixs);
> +  NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(count, ips);
> +#else
> +  // TODO Get host IP via other platforms.

truncate aAddress!

::: dom/presentation/PresentationSessionInfo.h
@@ +144,5 @@
>    }
>  
>    void Shutdown(nsresult aReason);
>  
> +  nsresult GetAddress(nsACString& aAddress);

who is maintaining this code?  how is it possible the whole file gets into the tree w/o any comments!?

::: dom/presentation/PresentationSessionTransport.js
@@ +108,5 @@
> +  send: function(aData) {
> +    let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].
> +                       createInstance(Ci.nsIBinaryInputStream);
> +    binaryStream.setInputStream(aData);
> +    this._socket.send(binaryStream.readBytes(binaryStream.available()));

wish there were a method taking a stream :/

::: dom/presentation/interfaces/nsIPresentationSessionTransport.idl
@@ +13,2 @@
>  #define PRESENTATION_SESSION_TRANSPORT_CONTRACTID \
>    "@mozilla.org/presentation/presentationsessiontransport;1"

this should go as well, no?

::: dom/presentation/tests/xpcshell/test_presentation_session_transport.js
@@ +1,4 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

please, add a description what steps the test performs.

@@ +48,5 @@
> +
> +const serverCallback = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationSessionTransportCallback]),
> +  notifyTransportReady: function() {
> +    Assert.ok(true, "Server transport ready.");

these should be somehow checked at the end of the test run, otherwise this ok() has absolutely no meaning.  applies to more places.

Comment 188

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8633974 [details] [diff] [review]
Part 9 - Tests, v1

kikuo, any chance you could help me a bit with this review
(I've been totally overloaded with reviews for couple of days and I'm about to take some vacation). If you're too busy or don't feel comfortable reviewing this, assign the review back to me.

Comment 189

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Honza Bambas (:mayhemer) from comment #187)
> Comment on attachment 8632674 [details] [diff] [review]
> Part 8 - Data transport channel, v4
> 
> Review of attachment 8632674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ only since you will have to rewrite this once again on top of bug 885982.
>
Since MozTCPSocket and MozTCPServerSocket will be moved to WebIDL soon(?!), it doesn't appear we may keep accessing them as XPCOMs. So there seem 3 ways to rewrite this patch:

1. Follow the new C++ implementation (probably from some of the logic in patch 1 and 2 in bug 885982) and rewrite |nsIPresentationSessionTransport| in C++. It looks a more typical way but may introduce more development overhead to finish the transient implementation supposed to be replaced in bug 1148307.

2. Follow the current implementation in TCPSocket.js. It might take less overhead to rewrite in JS, but may therefore carry the (possibly performance) drawbacks from the old code supposed to be deprecated. (Not sure if it's acceptable even for the initial and transient implementation for |nsIPresentationSessionTransport|.)

3. Follow the way of rewriting RokuApp.jsm (patch 4 in bug 885982). But it hasn't been r+'d and doesn't seem complete yet (bug 885982 comment 59).

Do you have any preference or suggestions for them?

> as far as I understand the relations here this looks good.  but I don't
> provide any warranty for this r+ since I don't know well enough the
> surrounding code.  the r+ will be mostly just for the socket integration
>
It would be really appreciated even just for the socket integration. :)

Comment 190

[Honza Bambas (:mayhemer)]

I don't have any preference, it's entirely up to you.  You are working in area of code I don't own/maintain.

I think jdm is better to consult on this.  He may have ideas how to easily integrate his new TCPSocket impl.

Comment 191

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Hi Josh,

In patch 8 we use MozTCPSocket and MozTCPServerSocket as XPCOMs, and it appears no longer working after they're migrated to WebIDL in bug 885982 in the near future. So we plan to rewrite our patch and come out 3 possible ways in comment 189. Do you have any idea about how to easily integrate with your new TCPSocket impl?

Comment 192

[Josh Matthews [:jdm]]

Unless you have access to a Window object, it will be exceedingly difficult to use the new TCPSocket implementation. You may need to rewrite the code to use nsISocketTransport like the RokuApp.jsm changes instead.

Comment 193

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8633974 [details] [diff] [review]
Part 9 - Tests, v1

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

Hi Sean,

Mostly it looks good to me but with several concerns.
I'd like to discuss these with you and maybe make them better.

::: dom/presentation/tests/mochitest/PresentationSessionChromeScript.js
@@ +305,5 @@
> +}
> +
> +addMessageListener('trigger-device-add', function() {
> +  var deviceManager = Cc['@mozilla.org/presentation-device/manager;1']
> +                      .getService(Ci.nsIPresentationDeviceManager);

I presume that the deviceManager here is a singleton service as the one described in |function tearDown()| above.
Why not declare it in a global scope, or use 'let' keyword to make deviceManager limited in their scope ?

::: dom/presentation/tests/mochitest/test_presentation_receiver_start_session_timeout.html
@@ +40,5 @@
> +    });
> +
> +    gScript.addMessageListener('control-channel-closed', function controlChannelClosedHandler(aReason) {
> +      gScript.removeMessageListener('control-channel-closed', controlChannelClosedHandler);
> +      isnot(aReason, SpecialPowers.Cr.NS_OK, "The control channel is closed due to timeout.");

Is it possible here to check if the failure reason is "NS_ERROR_DOM_TIMEOUT_ERR" ?
I have no idea if the error code here is already translated several times.

::: dom/presentation/tests/mochitest/test_presentation_sender_start_session_error.html
@@ +36,5 @@
> +        ok(false, "startSession shouldn't succeed in this case.");
> +        aReject();
> +      },
> +      function(aError) {
> +        ok(true, "Error is expected when starting session: " + aError.name);

Since it targets to check the case NoAvailableDevice, I think it would be more precise to check the aError.name equals to the message string of NS_ERROR_DOM_INVALID_STATE_ERR.

Also there are other errors which may occurs while |Presentation::StartSession| is called, it would be better to check those cases too.

@@ +91,5 @@
> +
> +    gScript.addMessageListener('data-transport-initialized', function dataTransportInitializedHandler() {
> +      gScript.removeMessageListener('data-transport-initialized', dataTransportInitializedHandler);
> +      info("Data transport channel is initialized.");
> +      gScript.sendAsyncMessage('trigger-control-channel-close', SpecialPowers.Cr.NS_ERROR_ABORT);

Is there any chance that controlChannel close() is called before 'data-transport-initialized' is received ?

@@ +139,5 @@
> +
> +    gScript.addMessageListener('data-transport-initialized', function dataTransportInitializedHandler() {
> +      gScript.removeMessageListener('data-transport-initialized', dataTransportInitializedHandler);
> +      info("Data transport channel is initialized.");
> +      gScript.sendAsyncMessage('trigger-data-transport-close', SpecialPowers.Cr.NS_ERROR_UNEXPECTED);

Is there any chance that sessiontransport close() is called before sessiontransport is initialized ?

Comment 194

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #193)
> ::: dom/presentation/tests/mochitest/PresentationSessionChromeScript.js
> @@ +305,5 @@
> > +}
> > +
> > +addMessageListener('trigger-device-add', function() {
> > +  var deviceManager = Cc['@mozilla.org/presentation-device/manager;1']
> > +                      .getService(Ci.nsIPresentationDeviceManager);
> 
> I presume that the deviceManager here is a singleton service as the one
> described in |function tearDown()| above.
> Why not declare it in a global scope, or use 'let' keyword to make
> deviceManager limited in their scope ?
> 
The scope of variables declared with |var| is function-wide; whereas those declared with |let| is block-wide. In this case |deviceManager| is declared at the beginning of the callback function for |trigger-device-add|, so there should be no difference to do it with |var| or |let|.

> :::
> dom/presentation/tests/mochitest/
> test_presentation_receiver_start_session_timeout.html
> @@ +40,5 @@
> > +    });
> > +
> > +    gScript.addMessageListener('control-channel-closed', function controlChannelClosedHandler(aReason) {
> > +      gScript.removeMessageListener('control-channel-closed', controlChannelClosedHandler);
> > +      isnot(aReason, SpecialPowers.Cr.NS_OK, "The control channel is closed due to timeout.");
> 
> Is it possible here to check if the failure reason is
> "NS_ERROR_DOM_TIMEOUT_ERR" ?
> I have no idea if the error code here is already translated several times.
> 
Actually I was trying to do so. But somehow only core errors [1] get exposed to SpecialPowers.Cr. So |SpecialPowers.Cr.NS_ERROR_DOM_TIMEOUT_ERR| would result in undefined value. Though we may still try to take the non-standard internal code of NS_ERROR_DOM_TIMEOUT_ERR (0x80530017) to compare, it seems less readable and might introduce unexpected failures once the internal code tweaks someday. So I just make a loose check to only ensure it's an error as a compromise.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/ErrorList.h#4-45

> :::
> dom/presentation/tests/mochitest/
> test_presentation_sender_start_session_error.html
> @@ +36,5 @@
> > +        ok(false, "startSession shouldn't succeed in this case.");
> > +        aReject();
> > +      },
> > +      function(aError) {
> > +        ok(true, "Error is expected when starting session: " + aError.name);
> 
> Since it targets to check the case NoAvailableDevice, I think it would be
> more precise to check the aError.name equals to the message string of
> NS_ERROR_DOM_INVALID_STATE_ERR.
> 
> Also there are other errors which may occurs while
> |Presentation::StartSession| is called, it would be better to check those
> cases too.
> 
Will do.

> @@ +91,5 @@
> > +
> > +    gScript.addMessageListener('data-transport-initialized', function dataTransportInitializedHandler() {
> > +      gScript.removeMessageListener('data-transport-initialized', dataTransportInitializedHandler);
> > +      info("Data transport channel is initialized.");
> > +      gScript.sendAsyncMessage('trigger-control-channel-close', SpecialPowers.Cr.NS_ERROR_ABORT);
> 
> Is there any chance that controlChannel close() is called before
> 'data-transport-initialized' is received ?
> 
Will add a test case for this.

> @@ +139,5 @@
> > +
> > +    gScript.addMessageListener('data-transport-initialized', function dataTransportInitializedHandler() {
> > +      gScript.removeMessageListener('data-transport-initialized', dataTransportInitializedHandler);
> > +      info("Data transport channel is initialized.");
> > +      gScript.sendAsyncMessage('trigger-data-transport-close', SpecialPowers.Cr.NS_ERROR_UNEXPECTED);
> 
> Is there any chance that sessiontransport close() is called before
> sessiontransport is initialized ?
It's less likely to have this situation. The session transport instance has to be initialized, and then there becomes a way to manipulate it or listen to the subsequent close.

Comment 195

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel, v5

Follow the TCPSocket logic of r+ patch 1 & 2 in bug 885982 to rewrite the implementation of session transports.

Comment 196

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 9 - Tests, v2

Tweak some tests based on comment 193 and comment 194.

Comment 197

[Honza Bambas (:mayhemer)]

Comment on attachment 8638422 [details] [diff] [review]
Part 8 - Data transport channel, v5

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

TCPSocket.cpp/h is now in hands of jdm.  If you want to follow TCP socket please ask him for the review.  Also, I'm not at all familiar with purpose and integration of this class so someone else (a peer of the module you are adding this to) eyes are needed anyway.

There are absolutely no comments in the header file, so I really don't want to waste time repeating my self to add them and don't want to waste time trying to find out what is doing what...

Comment 198

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Sean Lin [:seanlin] from comment #194)
> > :::
> > dom/presentation/tests/mochitest/
> > test_presentation_receiver_start_session_timeout.html
> > @@ +40,5 @@
> > > +    });
> > > +
> > > +    gScript.addMessageListener('control-channel-closed', function controlChannelClosedHandler(aReason) {
> > > +      gScript.removeMessageListener('control-channel-closed', controlChannelClosedHandler);
> > > +      isnot(aReason, SpecialPowers.Cr.NS_OK, "The control channel is closed due to timeout.");
> > 
> > Is it possible here to check if the failure reason is
> > "NS_ERROR_DOM_TIMEOUT_ERR" ?
> > I have no idea if the error code here is already translated several times.
> > 
> Actually I was trying to do so. But somehow only core errors [1] get exposed
> to SpecialPowers.Cr. So |SpecialPowers.Cr.NS_ERROR_DOM_TIMEOUT_ERR| would
> result in undefined value. Though we may still try to take the non-standard
> internal code of NS_ERROR_DOM_TIMEOUT_ERR (0x80530017) to compare, it seems
> less readable and might introduce unexpected failures once the internal code
> tweaks someday. So I just make a loose check to only ensure it's an error as
> a compromise.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/xpcom/base/ErrorList.h#4-45
> 

I suggest that defining a const variable with the error code name would also be a readable way with precision, like [1],[2],[3]...etc.
NS_ERROR_DOM_TIMEOUT_ERR should be a standard error code [4][5] w/o too much concerns, so I don't think we should worry this too much.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_reopen.js#7
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/test/test_bug466751.xhtml#31
[3] https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/unit/test_setVersion_events.js#23

[4] https://dxr.mozilla.org/mozilla-central/source/xpcom/base/ErrorList.h#487
[5] http://www.w3.org/TR/dom/#dom-domexception-timeout_err 

Another thing I am curious about is that per mentioned only core errors are exposed to SpecialPower.Cr, but I found [6], I'm not quite familiar with test and don't know how to achieve that. But maybe some way out there. NS_ERROR_XPC_NOT_ENOUGH_ARGS seems not to be a core error.
Maybe S.C. could clear the fog of confusion for us :)

[6] https://dxr.mozilla.org/mozilla-central/source/dom/network/tests/test_networkstats_alarms.html#61

Comment 199

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 9 - Tests, v3

Ensure the errors are expected ones for some tests.

Comment 200

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8639666 [details] [diff] [review]
Part 9 - Tests, v3

Thanks for your work :)

Comment 201

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel, v5

Hi Josh,

Could you help review this patch (based on Honza's suggestion)? (since the latest revision is rewritten by following the TCPSocket logic of r+ patch 1 & 2 in bug 885982.)

Olli had a brief look at this patch in comment 156 and recommended someone else to review the socket integration part. The information below might help catch up some background knowledge and overviews about PresentationSessionTransport.
[1] |nsIPresentationChannelDescription|: https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationControlChannel.idl#11
[2] |nsIPresentationSessionTransport|: https://bugzilla.mozilla.org/attachment.cgi?id=8610471&action=diff#a/dom/presentation/interfaces/nsIPresentationSessionTransport.idl_sec1

Comment 202

[Josh Matthews [:jdm]]

I'll review the patch later today. Sorry about the long wait.

Comment 203

[Josh Matthews [:jdm]]

Will the code in part 8 be invoked from content processes under e10s?

Comment 204

[Josh Matthews [:jdm]]

Comment on attachment 8639706 [details] [diff] [review]
Part 8 - Data transport channel, v5

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

This looks ok to me.

::: dom/presentation/PresentationSessionTransport.cpp
@@ +266,5 @@
> +{
> +  if (!mCallback) {
> +    *aCallback = nullptr;
> +    return NS_OK;
> +  }

This check is redundant; the code below will work fine with null pointers.

@@ +290,5 @@
> +
> +  return mTransport->GetScriptableSelfAddr(aSelfAddress);
> +}
> +
> +nsresult

This return value is never checked. Let's just make this void and use NS_ENSURE_SUCCESS_VOID on the AsyncCopy call.

@@ +358,5 @@
> +  mCloseStatus = aReason;
> +  SetReadyState(CLOSING);
> +
> +  uint32_t count = 0;
> +  mMultiplexStream->GetCount(&count);

I think this will crash if we call Close before it's open...

@@ +391,5 @@
> +                                                int64_t aProgressMax)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (static_cast<uint32_t>(aStatus) != nsISocketTransport::STATUS_CONNECTED_TO) {

NS_NET_STATUS_CONNECTED_TO and remove the static_cast.

::: dom/presentation/PresentationSessionTransport.h
@@ +42,5 @@
> +class PresentationSessionTransport final : public nsIPresentationSessionTransport
> +                                         , public nsITransportEventSink
> +                                         , public nsIInputStreamCallback
> +                                         , public nsIStreamListener
> +                                         , public nsSupportsWeakReference

Is weak reference support necessary?

Comment 205

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 1 - WebIDL Bindings, v5

Merging with the latest code base.

Comment 206

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 2 - Presentation service and listeners, v5

Merging with the latest code base.

Comment 207

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 3 - IPC, v6

Merging with the latest code base.

Comment 208

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 4 - Establish session (sender) & available changes, v5

Merging with the latest code base.

Comment 209

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 5 - Establish session (receiver), v6

Merging with the latest code base.

Comment 210

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 6 - mozChromeEvent for app launch, v5

Merging with the latest code base.

Comment 211

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 7 - Presentation session, v4

Merging with the latest code base.

Comment 212

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 8 - Data transport channel, v6

Updating based on r+ comments and merging with the latest code base.

Comment 213

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Part 9 - Tests, v4

Merging with the latest code base.

Comment 214

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch (aggregated)

The overall patch aggregating all the parts as a reference.

Comment 215

[Sean Lin [:seanlin] (inactive from Mar 2016)]

The correspondent try run. FYI.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa42911b147a

Please check in the patches from part 1 to 9 sequentially. Thank you.

Comment 216

[Mounir Lamouri (:mounir)]

Comment on attachment 8644403 [details] [diff] [review]
Part 1 - WebIDL Bindings, v5

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

::: dom/webidl/Presentation.webidl
@@ +33,5 @@
> +   * - "PageLoadTimeout":      Presenting page takes too long to load.
> +   * - "DataChannelFailed":    Failed to establish data channel.
> +   */
> +  [Throws]
> +  Promise<PresentationSession> startSession(DOMString url,

The spec has changed and methods such as startSession() now live in PresentationRequest. Are you planning to implement the changes in a follow-up?

Comment 217

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/91124e7dba5f
https://hg.mozilla.org/integration/b2g-inbound/rev/6dfe03f1db49
https://hg.mozilla.org/integration/b2g-inbound/rev/ae8604b64e92
https://hg.mozilla.org/integration/b2g-inbound/rev/fd03fa83f0ae
https://hg.mozilla.org/integration/b2g-inbound/rev/e89355a01d88
https://hg.mozilla.org/integration/b2g-inbound/rev/3eb3f4dd3df8
https://hg.mozilla.org/integration/b2g-inbound/rev/da1c2259d1b6
https://hg.mozilla.org/integration/b2g-inbound/rev/e445b8e084a5
https://hg.mozilla.org/integration/b2g-inbound/rev/c4af32fef209

Comment 218

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

(In reply to Mounir Lamouri (:mounir) from comment #216)
> Comment on attachment 8644403 [details] [diff] [review]
> Part 1 - WebIDL Bindings, v5
> 
> Review of attachment 8644403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/Presentation.webidl
> @@ +33,5 @@
> > +   * - "PageLoadTimeout":      Presenting page takes too long to load.
> > +   * - "DataChannelFailed":    Failed to establish data channel.
> > +   */
> > +  [Throws]
> > +  Promise<PresentationSession> startSession(DOMString url,
> 
> The spec has changed and methods such as startSession() now live in
> PresentationRequest. Are you planning to implement the changes in a
> follow-up?

Yes we are going to catch up with the latest specs in a follow-up bug. I'll file one today.

Comment 219

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

https://hg.mozilla.org/mozilla-central/rev/91124e7dba5f
https://hg.mozilla.org/mozilla-central/rev/6dfe03f1db49
https://hg.mozilla.org/mozilla-central/rev/ae8604b64e92
https://hg.mozilla.org/mozilla-central/rev/fd03fa83f0ae
https://hg.mozilla.org/mozilla-central/rev/e89355a01d88
https://hg.mozilla.org/mozilla-central/rev/3eb3f4dd3df8
https://hg.mozilla.org/mozilla-central/rev/da1c2259d1b6
https://hg.mozilla.org/mozilla-central/rev/e445b8e084a5
https://hg.mozilla.org/mozilla-central/rev/c4af32fef209