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

Kilik is working on it a while.

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/

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.

Separate WebIDL files & Implementation in different patches.

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

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/

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.

An attribute PresentationSocketChannel in partial interface PresentationSession was added.

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 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.

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 ~

(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 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.

Addressed issues and carry r+ from Comment 14.

TODO : a) Sending blob stream.

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 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.

ni? Jonas and @kilik about comment #20.

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).

(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.

(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).

(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 ?

(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 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?

(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.

Copyright added & modified from |AvailabeIn="CertifiedApps"| to |AvailableIn="PrivilegedApps"|. Carry r+ from comment 14.

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/

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.

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 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 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].

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

Rebase patch according to controller.patch in Comment 35

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).

Use template class |DebugOnly| for some warning checks

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 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

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

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.

(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 :)

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 ;)

(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

(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. :)

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

(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'.

(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'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?

(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.

(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?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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. ;)

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.

Oh, also .onopen, .onclose and .onerror

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

(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)?

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.

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 ?

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.

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

Also, rename PresentationSocketChannel as PresentationChannel.

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 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

(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.

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?

(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.

Update: * Merge PresentationChannel and PresentationSession

(I'll try to review this tomorrow.)

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.

(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?

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.

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.

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.

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

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

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

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

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)

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

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

(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?

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 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)

(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

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>

(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.

Update based on previous comments.

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.

(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 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.)

Updating based on r+ comments.

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.

Updating based on previous comments.

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

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?

(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?

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

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.

(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.)

Minor updates based on r+ comments.

Minor updates based on r+ comments.

(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 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.

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?

(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?

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

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.

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 on attachment 8603239 [details] [diff] [review] Part 5 - Establish session (receiver), v2 (and expose that API only if the app has the right permission)

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?

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.

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

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?

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.

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.

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 :)

(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

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?

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.

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

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.

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.

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 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 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 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 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.

Updating based on r+ comments.

Minor updates based on r+ comments.

Minor updates based on r+ comments.

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

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