Use DataChannel for app-to-app communication for each session instead of custom protocol over TCP socket.

Supporting DataChannel enables the binary message format as well.

upload my initial patch for references

WIP this patch implement |close(aReason)| (with test) and modify interface for ICE Candidate Todo: implement ICE Candidate things in TCPPresentationServer and test it

send ICE candidate through control channel

Comment on attachment 8656453 [details] [diff] [review] Part1: send ICE candidate through control channels, v1 Review of attachment 8656453 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure understand what is covered by this patch. There's nothing here to actually use the webrtc api to create and accept ICE candidates. Is that part of another patch? ::: dom/presentation/PresentationSessionInfo.cpp @@ +701,5 @@ > +PresentationResponderInfo::OnIceCandidate(const nsAString& aCandidate) > +{ > + MOZ_ASSERT(false, "Should not receive ICE candidates."); > + return NS_ERROR_FAILURE; > +} Can you explain *why* we should not receive ICE candidates here and in the Requester too? ::: dom/presentation/interfaces/nsIPresentationControlChannel.idl @@ +104,5 @@ > * Close the transport channel. > * @param reason The reason of channel close; NS_OK represents normal. > */ > void close(in nsresult reason); > }; you need to update the uuid of this interface.

(In reply to [:fabrice] Fabrice Desré from comment #5) > Comment on attachment 8656453 [details] [diff] [review] > Part1: send ICE candidate through control channels, v1 > > Review of attachment 8656453 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure understand what is covered by this patch. There's nothing here > to actually use the webrtc api to create and accept ICE candidates. Is that > part of another patch? > Yes. This bug is aim to support webrtc data-channel as a session transport in presentation api (now a plain TCP socket). And we figure out control channels should support sending not only SDP but also ICE candidates, so we have this patch. Follow-ups will use control channels to negotiate SDP and ICE candidates. > ::: dom/presentation/PresentationSessionInfo.cpp > @@ +701,5 @@ > > +PresentationResponderInfo::OnIceCandidate(const nsAString& aCandidate) > > +{ > > + MOZ_ASSERT(false, "Should not receive ICE candidates."); > > + return NS_ERROR_FAILURE; > > +} > > Can you explain *why* we should not receive ICE candidates here and in the > Requester too? > Since we're not supporting webrtc data-channel until this patch. Will work in the follow-up patches. > ::: dom/presentation/interfaces/nsIPresentationControlChannel.idl > @@ +104,5 @@ > > * Close the transport channel. > > * @param reason The reason of channel close; NS_OK represents normal. > > */ > > void close(in nsresult reason); > > }; > > you need to update the uuid of this interface. Bug 1069230 changed the uuid and interface without updating the implementation. Luckily this reminds me to remove unused method in this idl.

> > Bug 1069230 changed the uuid and interface without updating the > implementation. Luckily this reminds me to remove unused method in this idl. I found what you mentioned is nsIPresentationControlChannelListener. Sorry about that.

update uuid of |nsIPresentationControlChannelListener| in this patch.

Comment on attachment 8659711 [details] [diff] [review] Part1: send ICE candidate through control channels, v2 Review of attachment 8659711 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/presentation/provider/TCPPresentationServer.js @@ +370,5 @@ > _sendingMessageType: null, > > _sendMessage: function(aType, aJSONData, aOnThrow) { > if (!aOnThrow) { > + aOnThrow = function(e) {throw e.result;}; no need for this change

Hello smaug, This bug is to add alternative way (webrtc data channel) for session transport. Since the building process of TCP and data channel are quite different. We want to separate the builder from nsIPresentationSessionTransport, and leave session transport focusing on the data communication. Here's the proposed idls. What do you think about this refactor?

Rebase and modify by review comment

Hello smaug, Could I have your quick feedback to check if this refactor make sense or not? Thanks. TODO: modify test to adjust the refactor

Adjust the test to this refactor

Comment on attachment 8665362 [details] [diff] [review] Part2: separate builder from nsIPresentationSessionTransport, WIP v3 Sorry, I'm having hard time to understand the setup. There is nsIDataChannelPresentationSessionTransportBuilder, but how is that related to nsITCPPresentationSessionTransportBuilder? Based on the name they are supposed to do something similar but just create different kind of underlying transport, right? I would expect them to even inherit some base interface (other than nsISupports) One can get .transport synchronously from nsITC* but if I understand the nsIDataChannel* behavior correctly, one would need to use .listener there, and then its onDataChannelPresentationSessionTransport is called. Why such different ways to access transport? Could you explain the setup, please.

(In reply to Olli Pettay [:smaug] from comment #14) > Comment on attachment 8665362 [details] [diff] [review] > Part2: separate builder from nsIPresentationSessionTransport, WIP v3 > > Sorry, I'm having hard time to understand the setup. > There is PresentationControllingInfo, but how is > that related to nsITCPPresentationSessionTransportBuilder? Based on the > name they are supposed to do something similar > but just create different kind of underlying transport, right? I would > expect them to even inherit some base interface (other than nsISupports) Yes, the purpose of both builders is to create nsIPresentationSessionTransport, and the client of builders should exactly know which kind of builder it uses. But I think it's more understandable if they inherit some common interface. > One can get .transport synchronously from nsITC* but if I understand the > nsIDataChannel* behavior correctly, one would need to use .listener there, > and then its > onDataChannelPresentationSessionTransport is called. Why such different ways > to access transport? Per previous design, the duty of underlying TCP transport creation is in Presentation(Controlling/Presenting)Info, which the async operations locates. But I tend to put data channel creation in the implementation of nsIDataChannelPresentationSessionTransport. > > Could you explain the setup, please. Do you mean how to use |nsIDataChannelPresentationSessionTransportBuilder|? Here's a brief procedure, possibly changing after some implementation fitness: Controlling side: 1. |initWithWindow| and |createOffer|, send the offer to presenting side by the control channel. 2. Get the answer from presenting side by the control channel and |setAnswer|. 3. Get the local ice candidates from |nsIDataChannelPresentationSessionTransportBuilderListener.onLocalIceCandidate|, and send them to presenting side by the control channel. 4. Get the ice candidates of presenting side by control channel and |addIceCandidate|. 5. If there's a way to connect RTCDataChannel, |nsIDataChannelPresentationSessionTransportBuilderListener. onDataChannelPresentationSessionTransport| will be called. Presenting side is similar: 1. |initWithWindow| and get offer from controlling side by the control channel. 2. |setOffer|, |createAnswer| and send the answer to controlling side by the control channel. 3,4,5 are the same as controlling side. Last, how if we change the idls to this way: interface nsIPresentationSesssionTransportBuilder : nsISupports { nsIPresentationSesssionTransportBuilderListener listener; } interface nsIPresentationSesssionTransportBuilderListener : nsISupports { void onSessionTransport(in nsIPresentationSessionTransport transport; } interface nsITCPPresentationSessionTransportBuilder : nsIPresentationSesssionTransportBuilder { void init*(); ... } interface nsIDataChannelPresentationSessionTransportBuilderListener : nsISupports { void onLocalIceCandidate(in DOMString iceCandidate); } interface nsIDataChannelPresentationSessionTransportBuilder : nsIPresentationSesssionTransportBuilder { attribute nsIDataChannelPresentationSessionTransportBuilderListener dataChannelListener; void initWithWindow(in nsIDOMWindow window); DOMString createOffer(); DOMString createAnswer(); void setOffer(in DOMString offer); void setAnswer(in DOMString offer); void addIceCandidate(in DOMString iceCandidate); }; It's a little ugly to use two listeners for nsIDataChannelPresentationSessionTransportBuilder. But if I design interface nsIDataChannelPresentationSessionTransportBuilderListener : nsIPresentationSesssionTransportBuilderListener { void onLocalIceCandidate(in DOMString iceCandidate); } I can not find a way to let TCP builder with a nsIPresentationSesssionTransportBuilderListener and data channel build with a nsIDataChannelPresentationSessionTransportBuilderListener

But why we have totally different APIs for different nsI*PresentationSessionTransportBuilder interfaces? I would imagine there to be some common base API, and then backend specific differences are in backend specific interfaces. Why do we need the sync .transport in TCP case, but can live with async transport accessing in DataChannel? Couldn't both have some async/callback based setup there?

But let me think about that change suggestion.

Don't you want nsIDataChannelPresentationSessionTransportBuilderListener to inherit nsIPresentationSessionTransportBuilderListener and then nsIPresentationSessionTransportBuilder to be some base interface for nsI*PresentationSessionTransportBuilder, and that base interface would have .listener property (or perhaps setListener() method, if accessing the property isn't ever needed.)

or, should the init*() method take the listener as an argument?

(In reply to Olli Pettay [:smaug] from comment #16) > But why we have totally different APIs for different > nsI*PresentationSessionTransportBuilder interfaces? I would imagine there to > be some common base API, and then backend specific differences are in > backend specific interfaces. For current implementation, we exchange necessary information in |Presentation(Controlling/Presenting)Info|. Therefore, the builders should provide that information which are totally different. If we want to use a more common API, |nsIPresentationControlChannel| should be provided to the builders, and we can hide the information exchange to the builders. That will be a bigger refactor. > Why do we need the sync .transport in TCP case, but can live with async > transport accessing in DataChannel? Couldn't both have some async/callback > based setup there? We can have both async callback setup. If there's a bigger refactor, the idls become more cleaner: interface nsIPresentationSesssionTransportBuilderListener : nsISupports { void onSessionTransport(in nsIPresentationSessionTransport transport; } interface nsIPresentationSesssionTransportBuilder : nsISupports { void setControlChannel(in nsIPresentationControlChannel aChannel); // both void setBuilderListener(in nsIPresentationSesssionTransportBuilderListener listener); // both void setTransportCallBack(in nsIPresentationSessionTransportCallback callback); // both void setWindow(in nsIDOMWindow window); // data channel needs to set void setSocketTransport(in nsISocketTransport transport); // TCP sender void setChannelDescription(in nsIPresentationChannelDescription description); // TCP receiver void build(); // trigger nsIPresentationSesssionTransportBuilderListener->onSessionTransport }

Hi smaug, I'm cooking a refactor based on previous discussion. Could I have a quick feedback to check if this refactor makes any sense? Thanks! TODO: 1. implement |PresentationSessionTransportBuilder::Build()| for TCP session transport case 2. adjust the test

fix some nit TODO: 1. complete Builder->build 2. adjust test

Sorry for missing PresentationSessionTransportBuilder.*

I don't quite understand the setup now. nsIPresentationSessionTransportCallback callback; is in two different interfaces. And if there is some transport type specific properties in an interface, why not have another interface which contains those and that interface would inherit the base interface? Based on the comments, transport and description are such, so why not have something like interface nsIPresentationSessionTCPTransportBuilder : nsIPresentationSessionTransportBuilder { attribute nsISocketTransport transport; attribute nsIPresentationChannelDescription description; ... };

Or is nsIPresentationSessionTransportBuilder something where the underlying transport mechanism can be changed? In such case putting all the attributes to the same interface makes more sense, but I thought there would be separate nsIPresentationSessionTransportBuilder instances for different types of transport mechanisms.

But please explain why the two .callback attributes, and how nsIPresentationSessionTransportBuilder is supposed to be used (like, can the underlying data transferring mechanism change)

(In reply to Olli Pettay [:smaug] from comment #24) > I don't quite understand the setup now. > nsIPresentationSessionTransportCallback callback; is in two different > interfaces. > SessionTransportCallback is for SessionTransport. The callback is in the Builder since I make alignment with current building function[1]. The builder just set it the the session transport after the transport is created. [1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/interfaces/nsIPresentationSessionTransport.idl#43 void initWithSocketTransport(in nsISocketTransport transport, in nsIPresentationSessionTransportCallback callback); We can delay the set after |onSessionTransport| is called to avoid ambiguous. (see the example at the bottom) > And if there is some transport type specific properties in an interface, why > not have another interface which contains those and that interface would > inherit the base interface? > > Based on the comments, transport and description are such, so why not have > something like > interface nsIPresentationSessionTCPTransportBuilder : > nsIPresentationSessionTransportBuilder > { > attribute nsISocketTransport transport; > attribute nsIPresentationChannelDescription description; > ... > }; Okay, make sense. > Or is nsIPresentationSessionTransportBuilder something where the underlying > transport mechanism can be changed? In such case putting all the attributes > to the same interface makes more sense, but I thought there would be > separate nsIPresentationSessionTransportBuilder instances for different > types of transport mechanisms. > > But please explain why the two .callback attributes, and how > nsIPresentationSessionTransportBuilder is supposed to be used (like, can the > underlying data transferring mechanism change) I suppose the underlying transferring mechanism will not change after the builder is created. Therefore, I'd like to make two different builders and extract their common interface. The interface may be: interface nsIPresentationSessionTransportBuilder : nsISupports { const unsigned short TYPE_SENDER = 1; const unsigned short TYPE_RECEIVER = 2; attribute uint8_t type; attribute nsIPresentationSessionTransportBuilderListener listener; // |onSessionTranport| void build(); } interface nsIPresentationSessionTCPTransportBuilder : nsIPresentationSessionTransportBuilder { attribute nsISocketTransport transport; attribute nsIPresentationChannelDescription description; }; similar to data channel interface nsIPresentationSessionTransport : nsISupports { attribute nsIPresentationSessionTransportCallback callback; ... } The usage will be: tcpBuilder.type = TYPE_SENDER; tcpBuilder.listener = this; tcpBuilder.transport = aSocketTransport; tcpBuilder.build(); and an async onSessionTransport will be called after the |build()|; and we should set the transportCallback in the |onSessionTransport| like: function onSessionTransport(in nsIPresentationSessionTransport aTransport) { this._transport = aTransport; this._transport.callback = mCallback; // need to set here this._transport.enableDataNotification(); // start to get data notification from transport.callback } Do this make more sense?

(In reply to Junior [:junior] from comment #27) > (In reply to Olli Pettay [:smaug] from comment #24) > > I don't quite understand the setup now. > > nsIPresentationSessionTransportCallback callback; is in two different > > interfaces. > > > SessionTransportCallback is for SessionTransport. > The callback is in the Builder since I make alignment with current building > function[1]. > The builder just set it the the session transport after the transport is > created. Could we perhaps then call "callback" attributes something else, something which indicates their use. > > I suppose the underlying transferring mechanism will not change after the > builder is created. Therefore, I'd like to make two different builders and > extract their common interface. Sounds good. > interface nsIPresentationSessionTransportBuilder : nsISupports > { > const unsigned short TYPE_SENDER = 1; > const unsigned short TYPE_RECEIVER = 2; > > attribute uint8_t type; > attribute nsIPresentationSessionTransportBuilderListener listener; // > |onSessionTranport| > > void build(); > } > > interface nsIPresentationSessionTCPTransportBuilder : > nsIPresentationSessionTransportBuilder > { > attribute nsISocketTransport transport; > attribute nsIPresentationChannelDescription description; > }; similar to data channel And there would be another similar interface for non-TCP case, right? > The usage will be: > > tcpBuilder.type = TYPE_SENDER; > tcpBuilder.listener = this; > tcpBuilder.transport = aSocketTransport; > tcpBuilder.build(); Or should tcpBuilder have an init() or initTCPTransportBuilder() method taking all the needed properties to initialize it. And then various attributes could be readonly. Not sure if build() is then needed. > Do this make more sense? starting to make more sense yes :)

(In reply to Olli Pettay [:smaug] from comment #28) > (In reply to Junior [:junior] from comment #27) > > (In reply to Olli Pettay [:smaug] from comment #24) > > > I don't quite understand the setup now. > > > nsIPresentationSessionTransportCallback callback; is in two different > > > interfaces. > > > > > SessionTransportCallback is for SessionTransport. > > The callback is in the Builder since I make alignment with current building > > function[1]. > > The builder just set it the the session transport after the transport is > > created. > Could we perhaps then call "callback" attributes something else, something > which indicates their use. I'm not quite sure what this means. The "callback" is used by session transport only. Therefore we can remove this attribute from the builder I think. > > interface nsIPresentationSessionTCPTransportBuilder : > > nsIPresentationSessionTransportBuilder > > { > > attribute nsISocketTransport transport; > > attribute nsIPresentationChannelDescription description; > > }; similar to data channel > And there would be another similar interface for non-TCP case, right? > > Yes > > > > The usage will be: > > > > tcpBuilder.type = TYPE_SENDER; > > tcpBuilder.listener = this; > > tcpBuilder.transport = aSocketTransport; > > tcpBuilder.build(); > Or should tcpBuilder have an init() or initTCPTransportBuilder() method > taking all the needed properties to initialize it. > And then various attributes could be readonly. Not sure if build() is then > needed. > Yes with several ideas: 1. If we'd like to remove |build()|, maybe rename to |createTCPTransport| to trigger |onSessionTransport|? 2. So the idls would be: interface nsIPresentationSessionTransportBuilder : nsISupports { attribute nsIPresentationSessionTransportBuilderListener listener; } interface nsIPresentationSessionTCPTransportBuilder : nsIPresentationSessionTransportBuilder { void createTCPSenderTransport(in nsISocketTransport aTransport); void createTCPReceiverTransport(in nsIPresentationChannelDescription aDescription); }; interface nsIPresentationSessionDataChannelTransportBuilder : nsIPresentationSessionTransportBuilder { const unsigned short TYPE_SENDER = 1; const unsigned short TYPE_RECEIVER = 2; void createDataChannelTransport(in int aType, in nsIDOMWIndow aWindow, in nsIPresentationControlChannel aControlChannel); }; > > Do this make more sense? > starting to make more sense yes :) Glad to hear that :)

(In reply to Junior [:junior] from comment #29) > I'm not quite sure what this means. The "callback" is used by session > transport only. > Therefore we can remove this attribute from the builder I think. Ah, I misunderstood then. Yes, if there is just one callback, keeping the name is fine. > Yes with several ideas: > 1. If we'd like to remove |build()|, maybe rename to |createTCPTransport| to > trigger |onSessionTransport|? Sounds good > interface nsIPresentationSessionTransportBuilder : nsISupports > { > attribute nsIPresentationSessionTransportBuilderListener listener; could this be readonly? Can we set it in create* method implementations? But yeah, looking good.

(In reply to Olli Pettay [:smaug] from comment #30) > (In reply to Junior [:junior] from comment #29) > > > I'm not quite sure what this means. The "callback" is used by session > > transport only. > > Therefore we can remove this attribute from the builder I think. > Ah, I misunderstood then. Yes, if there is just one callback, keeping the > name is fine. > > > Yes with several ideas: > > 1. If we'd like to remove |build()|, maybe rename to |createTCPTransport| to > > trigger |onSessionTransport|? > Sounds good > > > interface nsIPresentationSessionTransportBuilder : nsISupports > > { > > attribute nsIPresentationSessionTransportBuilderListener listener; > could this be readonly? Can we set it in create* method implementations? Yes, lets do it :) > > > But yeah, looking good.

Comment on attachment 8665337 [details] [diff] [review] Part1: send ICE candidate through control channels, v3 Move to bug 1217683

This patch separate the builder from nsIPresentationSessionTransport based on the previous discussion. Hello smaug, May I have your review? Thanks.

Comment on attachment 8678032 [details] [diff] [review] Part1: separate builder from nsIPresentationSessionTransport, WIP v7 ># HG changeset patch ># User Junior Hsu <juhsu@mozilla.com> ># Date 1443082607 -28800 ># Thu Sep 24 16:16:47 2015 +0800 ># Node ID 300e0ebfbb2a675610900ccdcc10f85c724b03ba ># Parent ac52ed461f2919de19c41e171cbb5cbc35e6a828 >Bug 1148307 - Part1, separate object creator from nsIPresentationSessionTransport, r=smaug > >diff --git a/dom/presentation/PresentationSessionInfo.cpp b/dom/presentation/PresentationSessionInfo.cpp >--- a/dom/presentation/PresentationSessionInfo.cpp >+++ b/dom/presentation/PresentationSessionInfo.cpp >@@ -124,56 +124,56 @@ PresentationNetworkHelper::OnGetWifiIPAd > aIPAddress)); > return NS_OK; > } > > } // anonymous namespace > > #endif // MOZ_WIDGET_ANDROID > >-class PresentationChannelDescription final : public nsIPresentationChannelDescription >+class TCPPresentationChannelDescription final : public nsIPresentationChannelDescription > { > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIPRESENTATIONCHANNELDESCRIPTION > >- PresentationChannelDescription(const nsACString& aAddress, >+ TCPPresentationChannelDescription(const nsACString& aAddress, > uint16_t aPort) > : mAddress(aAddress) > , mPort(aPort) > { > } > > private: >- ~PresentationChannelDescription() {} >+ ~TCPPresentationChannelDescription() {} > > nsCString mAddress; > uint16_t mPort; > }; > > } // namespace dom > } // namespace mozilla > >-NS_IMPL_ISUPPORTS(PresentationChannelDescription, nsIPresentationChannelDescription) >+NS_IMPL_ISUPPORTS(TCPPresentationChannelDescription, nsIPresentationChannelDescription) > > NS_IMETHODIMP >-PresentationChannelDescription::GetType(uint8_t* aRetVal) >+TCPPresentationChannelDescription::GetType(uint8_t* aRetVal) > { > if (NS_WARN_IF(!aRetVal)) { > return NS_ERROR_INVALID_POINTER; > } > > // TODO bug 1148307 Implement PresentationSessionTransport with DataChannel. > // Only support TCP socket for now. > *aRetVal = nsIPresentationChannelDescription::TYPE_TCP; > return NS_OK; > } > > NS_IMETHODIMP >-PresentationChannelDescription::GetTcpAddress(nsIArray** aRetVal) >+TCPPresentationChannelDescription::GetTcpAddress(nsIArray** aRetVal) > { > if (NS_WARN_IF(!aRetVal)) { > return NS_ERROR_INVALID_POINTER; > } > > nsCOMPtr<nsIMutableArray> array = do_CreateInstance(NS_ARRAY_CONTRACTID); > if (NS_WARN_IF(!array)) { > return NS_ERROR_OUT_OF_MEMORY; >@@ -192,42 +192,43 @@ PresentationChannelDescription::GetTcpAd > > array->AppendElement(address, false); > array.forget(aRetVal); > > return NS_OK; > } > > NS_IMETHODIMP >-PresentationChannelDescription::GetTcpPort(uint16_t* aRetVal) >+TCPPresentationChannelDescription::GetTcpPort(uint16_t* aRetVal) > { > if (NS_WARN_IF(!aRetVal)) { > return NS_ERROR_INVALID_POINTER; > } > > *aRetVal = mPort; > return NS_OK; > } > > NS_IMETHODIMP >-PresentationChannelDescription::GetDataChannelSDP(nsAString& aDataChannelSDP) >+TCPPresentationChannelDescription::GetDataChannelSDP(nsAString& aDataChannelSDP) > { > // TODO bug 1148307 Implement PresentationSessionTransport with DataChannel. > // Only support TCP socket for now. > aDataChannelSDP.Truncate(); > return NS_OK; > } > > /* > * Implementation of PresentationSessionInfo > */ > > NS_IMPL_ISUPPORTS(PresentationSessionInfo, > nsIPresentationSessionTransportCallback, >- nsIPresentationControlChannelListener); >+ nsIPresentationControlChannelListener, >+ nsIPresentationSessionTransportBuilderListener); > > /* virtual */ nsresult > PresentationSessionInfo::Init(nsIPresentationControlChannel* aControlChannel) > { > SetControlChannel(aControlChannel); > return NS_OK; > } > >@@ -410,16 +411,40 @@ PresentationSessionInfo::NotifyData(cons > > if (NS_WARN_IF(!mListener)) { > return NS_ERROR_NOT_AVAILABLE; > } > > return mListener->NotifyMessage(mSessionId, aData); > } > >+// nsIPresentationSessionTransportBuilderListener >+NS_IMETHODIMP >+PresentationSessionInfo::OnSessionTransport(nsIPresentationSessionTransport* transport) >+{ >+ mTransport = transport; >+ if (mListener) { >+ mTransport->EnableDataNotification(); >+ } >+ >+ nsresult rv = mTransport->SetCallback(this); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+PresentationSessionInfo::OnError(nsresult reason) >+{ >+ return NS_ERROR_NOT_IMPLEMENTED; >+} >+ >+ > /* > * Implementation of PresentationControllingInfo > * > * During presentation session establishment, the sender expects the following > * after trying to establish the control channel: (The order between step 3 and > * 4 is not guaranteed.) > * 1. |Init| is called to open a socket |mServerSocket| for data transport > * channel. >@@ -560,18 +585,18 @@ PresentationControllingInfo::OnGetAddres > > // Prepare and send the offer. > int32_t port; > nsresult rv = mServerSocket->GetPort(&port); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > >- RefPtr<PresentationChannelDescription> description = >- new PresentationChannelDescription(aAddress, static_cast<uint16_t>(port)); >+ RefPtr<TCPPresentationChannelDescription> description = >+ new TCPPresentationChannelDescription(aAddress, static_cast<uint16_t>(port)); > return mControlChannel->SendOffer(description); > } > > // nsIPresentationControlChannelListener > NS_IMETHODIMP > PresentationControllingInfo::OnOffer(nsIPresentationChannelDescription* aDescription) > { > MOZ_ASSERT(false, "Sender side should not receive offer."); >@@ -628,33 +653,24 @@ PresentationControllingInfo::NotifyClose > > // nsIServerSocketListener > NS_IMETHODIMP > PresentationControllingInfo::OnSocketAccepted(nsIServerSocket* aServerSocket, > nsISocketTransport* aTransport) > { > MOZ_ASSERT(NS_IsMainThread()); > >- // Initialize |mTransport| and use |this| as the callback. >- mTransport = do_CreateInstance(PRESENTATION_SESSION_TRANSPORT_CONTRACTID); >- if (NS_WARN_IF(!mTransport)) { >+ // Initialize session transport builder and use |this| as the callback. >+ nsCOMPtr<nsIPresentationTCPSessionTransportBuilder> builder = >+ do_CreateInstance(PRESENTATION_SESSION_TRANSPORT_CONTRACTID); >+ if (NS_WARN_IF(!builder)) { > return ReplyError(NS_ERROR_DOM_OPERATION_ERR); > } > >- nsresult rv = mTransport->InitWithSocketTransport(aTransport, this); >- if (NS_WARN_IF(NS_FAILED(rv))) { >- return rv; >- } >- >- // Enable data notification if the listener has been registered. >- if (mListener) { >- return mTransport->EnableDataNotification(); >- } >- >- return NS_OK; >+ return builder->CreateTCPSenderTransport(aTransport, this); > } > > NS_IMETHODIMP > PresentationControllingInfo::OnStopListening(nsIServerSocket* aServerSocket, > nsresult aStatus) > { > MOZ_ASSERT(NS_IsMainThread()); > >@@ -729,61 +745,64 @@ PresentationPresentingInfo::Shutdown(nsr > mTimer->Cancel(); > } > > mLoadingCallback = nullptr; > mRequesterDescription = nullptr; > mPromise = nullptr; > } > >-nsresult >-PresentationPresentingInfo::InitTransportAndSendAnswer() >+// nsIPresentationSessionTransportBuilderListener >+NS_IMETHODIMP >+PresentationPresentingInfo::OnSessionTransport(nsIPresentationSessionTransport* transport) > { >- // Establish a data transport channel |mTransport| to the sender and use >- // |this| as the callback. >- mTransport = do_CreateInstance(PRESENTATION_SESSION_TRANSPORT_CONTRACTID); >- if (NS_WARN_IF(!mTransport)) { >- return NS_ERROR_NOT_AVAILABLE; >- } >- >- nsresult rv = mTransport->InitWithChannelDescription(mRequesterDescription, this); >- if (NS_WARN_IF(NS_FAILED(rv))) { >- return rv; >- } >- >- // Enable data notification if the listener has been registered. >- if (mListener) { >- rv = mTransport->EnableDataNotification(); >- if (NS_WARN_IF(NS_FAILED(rv))) { >- return rv; >- } >- } >+ PresentationSessionInfo::OnSessionTransport(transport); > > // Prepare and send the answer. > // TODO bug 1148307 Implement PresentationSessionTransport with DataChannel. > // In the current implementation of |PresentationSessionTransport|, > // |GetSelfAddress| cannot return the real info when it's initialized via > // |InitWithChannelDescription|. Yet this deficiency only affects the channel > // description for the answer, which is not actually checked at requester side. > nsCOMPtr<nsINetAddr> selfAddr; >- rv = mTransport->GetSelfAddress(getter_AddRefs(selfAddr)); >+ nsresult rv = mTransport->GetSelfAddress(getter_AddRefs(selfAddr)); > NS_WARN_IF(NS_FAILED(rv)); > > nsCString address; > uint16_t port = 0; > if (NS_SUCCEEDED(rv)) { > selfAddr->GetAddress(address); > selfAddr->GetPort(&port); > } > nsCOMPtr<nsIPresentationChannelDescription> description = >- new PresentationChannelDescription(address, port); >+ new TCPPresentationChannelDescription(address, port); > > return mControlChannel->SendAnswer(description); > } > >+NS_IMETHODIMP >+PresentationPresentingInfo::OnError(nsresult reason) >+{ >+ return PresentationSessionInfo::OnError(reason); >+} >+ >+nsresult >+PresentationPresentingInfo::InitTransportAndSendAnswer() >+{ >+ // Establish a data transport channel |mTransport| to the sender and use >+ // |this| as the callback. >+ nsCOMPtr<nsIPresentationTCPSessionTransportBuilder> builder = >+ do_CreateInstance(PRESENTATION_SESSION_TRANSPORT_CONTRACTID); >+ if (NS_WARN_IF(!builder)) { >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ >+ return builder->CreateTCPReceiverTransport(mRequesterDescription, this); >+} >+ > nsresult > PresentationPresentingInfo::UntrackFromService() > { > // Remove the OOP responding info (if it has never been used). > if (mContentParent) { > NS_WARN_IF(!static_cast<ContentParent*>(mContentParent.get())->SendNotifyPresentationReceiverCleanUp(mSessionId)); > } > >diff --git a/dom/presentation/PresentationSessionInfo.h b/dom/presentation/PresentationSessionInfo.h >--- a/dom/presentation/PresentationSessionInfo.h >+++ b/dom/presentation/PresentationSessionInfo.h >@@ -22,20 +22,22 @@ > #include "nsString.h" > #include "PresentationCallbacks.h" > > namespace mozilla { > namespace dom { > > class PresentationSessionInfo : public nsIPresentationSessionTransportCallback > , public nsIPresentationControlChannelListener >+ , public nsIPresentationSessionTransportBuilderListener > { > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIPRESENTATIONSESSIONTRANSPORTCALLBACK >+ NS_DECL_NSIPRESENTATIONSESSIONTRANSPORTBUILDERLISTENER > > PresentationSessionInfo(const nsAString& aUrl, > const nsAString& aSessionId, > nsIPresentationServiceCallback* aCallback) > : mUrl(aUrl) > , mSessionId(aSessionId) > , mIsResponderReady(false) > , mIsTransportReady(false) >@@ -178,16 +180,17 @@ private: > // Session info with presenting browsing context (receiver side) behaviors. > class PresentationPresentingInfo final : public PresentationSessionInfo > , public PromiseNativeHandler > , public nsITimerCallback > { > public: > NS_DECL_ISUPPORTS_INHERITED > NS_DECL_NSIPRESENTATIONCONTROLCHANNELLISTENER >+ NS_DECL_NSIPRESENTATIONSESSIONTRANSPORTBUILDERLISTENER > NS_DECL_NSITIMERCALLBACK > > PresentationPresentingInfo(const nsAString& aUrl, > const nsAString& aSessionId, > nsIPresentationDevice* aDevice) > : PresentationSessionInfo(aUrl, aSessionId, nullptr) > { > MOZ_ASSERT(aDevice); >diff --git a/dom/presentation/PresentationSessionTransport.cpp b/dom/presentation/PresentationSessionTransport.cpp >--- a/dom/presentation/PresentationSessionTransport.cpp >+++ b/dom/presentation/PresentationSessionTransport.cpp >@@ -11,16 +11,17 @@ > #include "nsIMutableArray.h" > #include "nsIOutputStream.h" > #include "nsIPresentationControlChannel.h" > #include "nsIScriptableInputStream.h" > #include "nsISocketTransport.h" > #include "nsISocketTransportService.h" > #include "nsISupportsPrimitives.h" > #include "nsNetUtil.h" >+#include "nsQueryObject.h" > #include "nsServiceManagerUtils.h" > #include "nsStreamUtils.h" > #include "nsThreadUtils.h" > #include "PresentationSessionTransport.h" > > #define BUFFER_SIZE 65536 > > using namespace mozilla; >@@ -53,74 +54,80 @@ NS_IMETHODIMP > CopierCallbacks::OnStopRequest(nsIRequest* aRequest, nsISupports* aContext, nsresult aStatus) > { > mOwner->NotifyCopyComplete(aStatus); > return NS_OK; > } > > NS_IMPL_ISUPPORTS(PresentationSessionTransport, > nsIPresentationSessionTransport, >+ nsIPresentationTCPSessionTransportBuilder, > nsITransportEventSink, > nsIInputStreamCallback, > nsIStreamListener, > nsIRequestObserver) > > PresentationSessionTransport::PresentationSessionTransport() >- : mReadyState(CLOSED) >+ : mReadyState(ReadyState::CLOSED) > , mAsyncCopierActive(false) > , mCloseStatus(NS_OK) > , mDataNotificationEnabled(false) > { > } > > PresentationSessionTransport::~PresentationSessionTransport() > { > } > > NS_IMETHODIMP >-PresentationSessionTransport::InitWithSocketTransport(nsISocketTransport* aTransport, >- nsIPresentationSessionTransportCallback* aCallback) >+PresentationSessionTransport::CreateTCPSenderTransport(nsISocketTransport* aTransport, >+ nsIPresentationSessionTransportBuilderListener* aListener) > { >- if (NS_WARN_IF(!aCallback)) { >- return NS_ERROR_INVALID_ARG; >- } >- mCallback = aCallback; >- > if (NS_WARN_IF(!aTransport)) { > return NS_ERROR_INVALID_ARG; > } > mTransport = aTransport; > >+ if (NS_WARN_IF(!aListener)) { >+ return NS_ERROR_INVALID_ARG; >+ } >+ mListener = aListener; >+ > nsresult rv = CreateStream(); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > >- SetReadyState(OPEN); >+ SetReadyState(ReadyState::OPEN); > > if (IsReadyToNotifyData()) { > return CreateInputStreamPump(); > } > >+ mType = nsIPresentationSessionTransportBuilder::TYPE_SENDER; >+ >+ nsCOMPtr<nsIPresentationSessionTransport> sessionTransport = do_QueryObject(this); >+ mListener->OnSessionTransport(sessionTransport); >+ > return NS_OK; > } > > NS_IMETHODIMP >-PresentationSessionTransport::InitWithChannelDescription(nsIPresentationChannelDescription* aDescription, >- nsIPresentationSessionTransportCallback* aCallback) >+PresentationSessionTransport::CreateTCPReceiverTransport(nsIPresentationChannelDescription* aDescription, >+ nsIPresentationSessionTransportBuilderListener* aListener) > { >- if (NS_WARN_IF(!aCallback)) { >- return NS_ERROR_INVALID_ARG; >- } >- mCallback = aCallback; >- > if (NS_WARN_IF(!aDescription)) { > return NS_ERROR_INVALID_ARG; > } > >+ if (NS_WARN_IF(!aListener)) { >+ return NS_ERROR_INVALID_ARG; >+ } >+ mListener = aListener; >+ > uint16_t serverPort; > nsresult rv = aDescription->GetTcpPort(&serverPort); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > nsCOMPtr<nsIArray> serverHosts; > rv = aDescription->GetTcpAddress(getter_AddRefs(serverHosts)); >@@ -139,17 +146,17 @@ PresentationSessionTransport::InitWithCh > } > > nsAutoCString serverHost; > supportStr->GetData(serverHost); > if (serverHost.IsEmpty()) { > return NS_ERROR_INVALID_ARG; > } > >- SetReadyState(CONNECTING); >+ SetReadyState(ReadyState::CONNECTING); > > nsCOMPtr<nsISocketTransportService> sts = > do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID); > if (NS_WARN_IF(!sts)) { > return NS_ERROR_NOT_AVAILABLE; > } > rv = sts->CreateTransport(nullptr, 0, serverHost, serverPort, nullptr, > getter_AddRefs(mTransport)); >@@ -162,16 +169,21 @@ PresentationSessionTransport::InitWithCh > > mTransport->SetEventSink(this, mainThread); > > rv = CreateStream(); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > >+ mType = nsIPresentationSessionTransportBuilder::TYPE_RECEIVER; >+ >+ nsCOMPtr<nsIPresentationSessionTransport> sessionTransport = do_QueryObject(this); >+ mListener->OnSessionTransport(sessionTransport); >+ > return NS_OK; > } > > nsresult > PresentationSessionTransport::CreateStream() > { > nsresult rv = mTransport->OpenInputStream(0, 0, 0, getter_AddRefs(mSocketInputStream)); > if (NS_WARN_IF(NS_FAILED(rv))) { >@@ -275,16 +287,47 @@ PresentationSessionTransport::EnableData > > if (IsReadyToNotifyData()) { > return CreateInputStreamPump(); > } > > return NS_OK; > } > >+// nsIPresentationSessionTransportBuilderListener >+NS_IMETHODIMP >+PresentationSessionTransport::GetType(uint8_t* aType) >+{ >+ *aType = mType; >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+PresentationSessionTransport::GetListener(nsIPresentationSessionTransportBuilderListener** aListener) >+{ >+ nsCOMPtr<nsIPresentationSessionTransportBuilderListener> listener = mListener; >+ mListener.forget(aListener); >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+PresentationSessionTransport::GetBuilder(nsIPresentationSessionTransportBuilder** aBuilder) >+{ >+ nsCOMPtr<nsIPresentationSessionTransportBuilder> builder = mBuilder; >+ builder.forget(aBuilder); >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+PresentationSessionTransport::SetBuilder(nsIPresentationSessionTransportBuilder* aBuilder) >+{ >+ mBuilder = aBuilder; >+ return NS_OK; >+} >+ > NS_IMETHODIMP > PresentationSessionTransport::GetCallback(nsIPresentationSessionTransportCallback** aCallback) > { > nsCOMPtr<nsIPresentationSessionTransportCallback> callback = mCallback; > callback.forget(aCallback); > return NS_OK; > } > >@@ -318,64 +361,64 @@ PresentationSessionTransport::EnsureCopy > } > > void > PresentationSessionTransport::NotifyCopyComplete(nsresult aStatus) > { > mAsyncCopierActive = false; > mMultiplexStream->RemoveStream(0); > if (NS_WARN_IF(NS_FAILED(aStatus))) { >- if (mReadyState != CLOSED) { >+ if (mReadyState != ReadyState::CLOSED) { > mCloseStatus = aStatus; >- SetReadyState(CLOSED); >+ SetReadyState(ReadyState::CLOSED); > } > return; > } > > uint32_t count; > nsresult rv = mMultiplexStream->GetCount(&count); > if (NS_WARN_IF(NS_FAILED(rv))) { > return; > } > > if (count) { > EnsureCopying(); > return; > } > >- if (mReadyState == CLOSING) { >+ if (mReadyState == ReadyState::CLOSING) { > mSocketOutputStream->Close(); > mCloseStatus = NS_OK; >- SetReadyState(CLOSED); >+ SetReadyState(ReadyState::CLOSED); > } > } > > NS_IMETHODIMP > PresentationSessionTransport::Send(nsIInputStream* aData) > { >- if (NS_WARN_IF(mReadyState != OPEN)) { >+ if (NS_WARN_IF(mReadyState != ReadyState::OPEN)) { > return NS_ERROR_DOM_INVALID_STATE_ERR; > } > > mMultiplexStream->AppendStream(aData); > > EnsureCopying(); > > return NS_OK; > } > > NS_IMETHODIMP > PresentationSessionTransport::Close(nsresult aReason) > { >- if (mReadyState == CLOSED || mReadyState == CLOSING) { >+ if (mReadyState == ReadyState::CLOSED || mReadyState == ReadyState::CLOSING) { > return NS_OK; > } > > mCloseStatus = aReason; >- SetReadyState(CLOSING); >+ SetReadyState(ReadyState::CLOSING); > > uint32_t count = 0; > mMultiplexStream->GetCount(&count); > if (!count) { > mSocketOutputStream->Close(); > } > > mSocketInputStream->Close(); >@@ -384,20 +427,20 @@ PresentationSessionTransport::Close(nsre > return NS_OK; > } > > void > PresentationSessionTransport::SetReadyState(ReadyState aReadyState) > { > mReadyState = aReadyState; > >- if (mReadyState == OPEN && mCallback) { >+ if (mReadyState == ReadyState::OPEN && mCallback) { > // Notify the transport channel is ready. > NS_WARN_IF(NS_FAILED(mCallback->NotifyTransportReady())); >- } else if (mReadyState == CLOSED && mCallback) { >+ } else if (mReadyState == ReadyState::CLOSED && mCallback) { > // Notify the transport channel has been shut down. > NS_WARN_IF(NS_FAILED(mCallback->NotifyTransportClosed(mCloseStatus))); > } > } > > // nsITransportEventSink > NS_IMETHODIMP > PresentationSessionTransport::OnTransportStatus(nsITransport* aTransport, >@@ -406,17 +449,17 @@ PresentationSessionTransport::OnTranspor > int64_t aProgressMax) > { > MOZ_ASSERT(NS_IsMainThread()); > > if (aStatus != NS_NET_STATUS_CONNECTED_TO) { > return NS_OK; > } > >- SetReadyState(OPEN); >+ SetReadyState(ReadyState::OPEN); > > if (IsReadyToNotifyData()) { > return CreateInputStreamPump(); > } > > return NS_OK; > } > >@@ -425,19 +468,19 @@ NS_IMETHODIMP > PresentationSessionTransport::OnInputStreamReady(nsIAsyncInputStream* aStream) > { > MOZ_ASSERT(NS_IsMainThread()); > > // Only used for detecting if the connection was refused. > uint64_t dummy; > nsresult rv = aStream->Available(&dummy); > if (NS_WARN_IF(NS_FAILED(rv))) { >- if (mReadyState != CLOSED) { >+ if (mReadyState != ReadyState::CLOSED) { > mCloseStatus = NS_ERROR_CONNECTION_REFUSED; >- SetReadyState(CLOSED); >+ SetReadyState(ReadyState::CLOSED); > } > } > > return NS_OK; > } > > // nsIRequestObserver > NS_IMETHODIMP >@@ -467,19 +510,19 @@ PresentationSessionTransport::OnStopRequ > // If we have some buffered output still, and status is not an error, the > // other side has done a half-close, but we don't want to be in the close > // state until we are done sending everything that was buffered. We also > // don't want to call |NotifyTransportClosed| yet. > return NS_OK; > } > > // We call this even if there is no error. >- if (mReadyState != CLOSED) { >+ if (mReadyState != ReadyState::CLOSED) { > mCloseStatus = aStatusCode; >- SetReadyState(CLOSED); >+ SetReadyState(ReadyState::CLOSED); > } > return NS_OK; > } > >+%{C++ >+#define PRESENTATION_SESSION_TRANSPORT_BUILDER_CONTRACTID \ >+ "@mozilla.org/presentation/presentationsessiontransportbuilder;1" >+%} This for nsIPresentationTCPSessionTransportBuilder only, right? Perhaps use tcp somewhere in the name > /* > * App-to-App transport channel for the presentation session. > */ > [scriptable, uuid(5d23ea5f-a7e5-4cf0-8fa5-6b0abd106bf2)] > interface nsIPresentationSessionTransport : nsISupports you need to update uuid > { >+ attribute nsIPresentationSessionTransportBuilder builder; It is not clear to me what sets .builder property. Should the create* (which I'd call init*) methods take nsIPresentationSessionTransport object as a param and update .builder or what? >+[scriptable, uuid(8131c4e0-3a8c-4bc1-a92a-8431473d2fe8)] >+interface nsIPresentationDataChannelSessionTransportBuilder : nsIPresentationSessionTransportBuilder >+{ >+ readonly attribute nsIDOMWindow window; >+ readonly attribute nsIPresentationControlChannel channel; >+ >+ /* >+ * The following creation function will trigger |listener.onSessionTransport| >+ * if the session transport is successfully created, |listener.onError| if >+ * some error occurs during creating session transport. >+ */ >+ void createDataChannelTransport(in uint8_t aType, >+ in nsIDOMWindow aWindow, >+ in nsIPresentationControlChannel aControlChannel, >+ in nsIPresentationSessionTransportBuilderListener aListener); >+}; Actually, can we call these create* methods init*, since the methods don't return anything. So, could you explain the nsIPresentationSessionTransport.builder property use? At which point will it be set and by who? And who then uses it?

(In reply to Olli Pettay [:smaug] from comment #34) > Comment on attachment 8678032 [details] [diff] [review] > Part1: separate builder from nsIPresentationSessionTransport, WIP v7 > > >+%{C++ > >+#define PRESENTATION_SESSION_TRANSPORT_BUILDER_CONTRACTID \ > >+ "@mozilla.org/presentation/presentationsessiontransportbuilder;1" > >+%} > This for nsIPresentationTCPSessionTransportBuilder only, right? > Perhaps use tcp somewhere in the name > > Yes, TCP only. Let me rename it > > { > >+ attribute nsIPresentationSessionTransportBuilder builder; > It is not clear to me what sets .builder property. > Should the create* (which I'd call init*) methods take > nsIPresentationSessionTransport object > as a param and update .builder or what? > > Reply at the bottom > > > >+[scriptable, uuid(8131c4e0-3a8c-4bc1-a92a-8431473d2fe8)] > >+interface nsIPresentationDataChannelSessionTransportBuilder : nsIPresentationSessionTransportBuilder > >+{ > >+ readonly attribute nsIDOMWindow window; > >+ readonly attribute nsIPresentationControlChannel channel; > >+ > >+ /* > >+ * The following creation function will trigger |listener.onSessionTransport| > >+ * if the session transport is successfully created, |listener.onError| if > >+ * some error occurs during creating session transport. > >+ */ > >+ void createDataChannelTransport(in uint8_t aType, > >+ in nsIDOMWindow aWindow, > >+ in nsIPresentationControlChannel aControlChannel, > >+ in nsIPresentationSessionTransportBuilderListener aListener); > >+}; > > Actually, can we call these create* methods init*, since the methods don't > return anything. > > They actually create the transport and pass to the caller through the async |listener.onSessionTransport|. Rename is fine to me, but I'm not sure which is better, |create*| or |init*|. > > So, could you explain the nsIPresentationSessionTransport.builder property > use? > At which point will it be set and by who? And who then uses it? That's for getting the builder's attributes. The builder should set itself to the transport, and transport uses the builder's attributes. However if we pass all the attributes through the creation function, we can try to remove it.

(In reply to Junior [:junior] from comment #35) > (In reply to Olli Pettay [:smaug] from comment #34) > > Comment on attachment 8678032 [details] [diff] [review] > > Part1: separate builder from nsIPresentationSessionTransport, WIP v7 > > > > >+%{C++ > > >+#define PRESENTATION_SESSION_TRANSPORT_BUILDER_CONTRACTID \ > > >+ "@mozilla.org/presentation/presentationsessiontransportbuilder;1" > > >+%} > > This for nsIPresentationTCPSessionTransportBuilder only, right? > > Perhaps use tcp somewhere in the name > > > > > > Yes, TCP only. Let me rename it > For TCP case, builder and the socket transport are in the same class |nsIPresentationSocketTransport|. Ideally they should be separated in two classes. However, refactoring this takes time and is not highly related to this bug.

(In reply to Junior [:junior] from comment #36) > (In reply to Junior [:junior] from comment #35) > > (In reply to Olli Pettay [:smaug] from comment #34) > > > Comment on attachment 8678032 [details] [diff] [review] > > > Part1: separate builder from nsIPresentationSessionTransport, WIP v7 > > > > > > >+%{C++ > > > >+#define PRESENTATION_SESSION_TRANSPORT_BUILDER_CONTRACTID \ > > > >+ "@mozilla.org/presentation/presentationsessiontransportbuilder;1" > > > >+%} > > > This for nsIPresentationTCPSessionTransportBuilder only, right? > > > Perhaps use tcp somewhere in the name > > > > > > > > > > Yes, TCP only. Let me rename it > > > > For TCP case, builder and the socket transport are in the same class > |nsIPresentationSocketTransport|. Ideally they should be separated in two > classes. > However, refactoring this takes time and is not highly related to this bug. s/nsIPresentationSocketTransport/PresentationSocketTransport/

Current patch

Comment on attachment 8679366 [details] [diff] [review] Part1: separate builder from nsIPresentationSessionTransport, WIP v8 ok, you removed .builder, except from PresentationSessionChromeScript.js This is fine, well PRESENTATION_SESSION_TRANSPORT_CONTRACTID should be renamed. I'm still not quite sure about "create*". "init*" or perhaps even "build*" could work better. They are after all in *Builder interfaces. buildTCPSenderTransport(...); and so. That doesn't sounds too bad. But we can change the name later too if you want to keep "create".

(In reply to Olli Pettay [:smaug] from comment #39) > Comment on attachment 8679366 [details] [diff] [review] > Part1: separate builder from nsIPresentationSessionTransport, WIP v8 > > ok, you removed .builder, except from PresentationSessionChromeScript.js My bad, that should be removed. > This is fine, well PRESENTATION_SESSION_TRANSPORT_CONTRACTID should be > renamed. If so, maybe filename PresentationSessionTransport.cpp should be renamed, too. possibly rename to PresentationTCPSessionTransport.cpp > > I'm still not quite sure about "create*". "init*" or perhaps even "build*" > could work better. They are after all in *Builder interfaces. > buildTCPSenderTransport(...); and so. That doesn't sounds too bad. > But we can change the name later too if you want to keep "create". I guess I rename to "build*" first since |build| implies time-consuming and async? Rename now is not painful.

Modify by previous discussion. Still break the test :(

Finally we pass the test. Lots of minor modifications.

Comment on attachment 8681178 [details] [diff] [review] Part1: separate builder from nsIPresentationSessionTransport, WIP v10 >+ mType = nsIPresentationSessionTransportBuilder::TYPE_SENDER; >+ >+ nsCOMPtr<nsIPresentationSessionTransport> sessionTransport = do_QueryObject(this); >+ mListener->OnSessionTransport(sessionTransport); ... >+ nsCOMPtr<nsIPresentationSessionTransport> sessionTransport = do_QueryObject(this); >+ mListener->OnSessionTransport(sessionTransport); >+ So I would expect us to call OnSessionTransport asynchronously. NS_DispatchToCurrentThread( NS_NewRunnableMethodWithArgs<nsIPresentationSessionTransport*>([this, class, method, args...])); might work here. Feel free to change in a follow up patch. > // Output stream machinery > nsCOMPtr<nsIMultiplexInputStream> mMultiplexStream; > nsCOMPtr<nsIAsyncStreamCopier> mMultiplexStreamCopier; > > nsCOMPtr<nsIPresentationSessionTransportCallback> mCallback; >+ nsCOMPtr<nsIPresentationSessionTransportBuilderListener> mListener; Shouldn't PresentationSessionTransport::OnStopRequest and/or PresentationSessionTransport::Close set mListener to null, so that we'd leak less likely. >+interface nsIPresentationSessionTransportBuilder : nsISupports >+{ >+ const unsigned short TYPE_SENDER = 1; >+ const unsigned short TYPE_RECEIVER = 2; >+ >+ readonly attribute uint8_t type; >+ readonly attribute nsIPresentationSessionTransportBuilderListener listener; (btw, I wonder if we actually need to expose this .listener attribute, or can it be some internal thing. But no need to change) >+interface nsIPresentationTCPSessionTransportBuilder : nsIPresentationSessionTransportBuilder >+{ >+ /** >+ * The following creation functions will trigger |listener.onSessionTransport| >+ * if the session transport is successfully built, |listener.onError| if some >+ * error occurs during building session transport. >+ */ >+ void buildTCPSenderTransport(in nsISocketTransport aTransport, >+ in nsIPresentationSessionTransportBuilderListener aListener); fix indentation of the latter param >+ >+ void buildTCPReceiverTransport(in nsIPresentationChannelDescription aDescription, >+ in nsIPresentationSessionTransportBuilderListener aListener); ditto >+ void buildDataChannelTransport(in uint8_t aType, >+ in nsIDOMWindow aWindow, >+ in nsIPresentationControlChannel aControlChannel, >+ in nsIPresentationSessionTransportBuilderListener aListener); fix indentation of the last 3 arguments, so that they are aligned with the first argument. >+ set builder(builder) { >+ this._buider = builder; >+ }, >+ get builder() { >+ return this; >+ }, I don't think we need builder getter/setter. (and it would be odd getter to return always 'this', even if one had set builder to some other value) >@@ -1162,17 +1162,17 @@ static const mozilla::Module::CIDEntry k > { &kNS_MOBILE_CONNECTION_SERVICE_CID, false, NULL, nsIMobileConnectionServiceConstructor }, > { &kNS_VOICEMAIL_SERVICE_CID, false, nullptr, nsIVoicemailServiceConstructor }, > { &kFAKE_TV_SERVICE_CID, false, nullptr, FakeTVServiceConstructor }, > { &kTV_TUNER_DATA_CID, false, nullptr, TVTunerDataConstructor }, > { &kTV_CHANNEL_DATA_CID, false, nullptr, TVChannelDataConstructor }, > { &kTV_PROGRAM_DATA_CID, false, nullptr, TVProgramDataConstructor }, > { &kPRESENTATION_SERVICE_CID, false, nullptr, nsIPresentationServiceConstructor }, > { &kPRESENTATION_DEVICE_MANAGER_CID, false, nullptr, PresentationDeviceManagerConstructor }, >- { &kPRESENTATION_SESSION_TRANSPORT_CID, false, nullptr, PresentationSessionTransportConstructor }, >+ { &kPRESENTATION_TCP_SESSION_TRANSPORT_CID, false, nullptr, PresentationSessionTransportConstructor }, Ah, ok, NS_GENERIC_FACTORY_CONSTRUCTOR(PresentationSessionTransport) doesn't get TCP in its name yet.

> >+interface nsIPresentationSessionTransportBuilder : nsISupports > >+{ > >+ const unsigned short TYPE_SENDER = 1; > >+ const unsigned short TYPE_RECEIVER = 2; > >+ > >+ readonly attribute uint8_t type; > >+ readonly attribute nsIPresentationSessionTransportBuilderListener listener; > (btw, I wonder if we actually need to expose this .listener attribute, or > can it be some internal thing. But no need to change) > I'll remove it. The original idea is to let transport get those attributes, but now we pass by builder* functions. > > >+ { &kPRESENTATION_TCP_SESSION_TRANSPORT_CID, false, nullptr, PresentationSessionTransportConstructor }, > Ah, ok, NS_GENERIC_FACTORY_CONSTRUCTOR(PresentationSessionTransport) > doesn't get TCP in its name yet. Okay, it's a big work but let me do it :) It should rename the file name, all the member function and scoped things...

Modified by comment 43, carry r+. Thanks smaug!

Fix some nits, carry r+

I guess it would be better if we change the parameter from inputStream to DOMString since that's may be more sync with the webidl. And RTCDataChannel can directly deal with DOMString.

Exclude unrelated file

Comment on attachment 8684126 [details] [diff] [review] Part2: let session transport send DOM string, v2 >+++ b/dom/presentation/interfaces/nsIPresentationService.idl update uuid >+++ b/dom/presentation/interfaces/nsIPresentationSessionTransport.idl update uuid > PresentationIPCService::SendSessionMessage(const nsAString& aSessionId, >- nsIInputStream* aStream) >+ const nsAString& aData) > { > MOZ_ASSERT(!aSessionId.IsEmpty()); >- MOZ_ASSERT(aStream); >+ MOZ_ASSERT(!aData.IsEmpty()); > >+ /* > mozilla::ipc::OptionalInputStreamParams stream; > nsTArray<mozilla::ipc::FileDescriptor> fds; > SerializeInputStream(aStream, stream, fds); > MOZ_ASSERT(fds.IsEmpty()); >- >- return SendRequest(nullptr, SendSessionMessageRequest(nsAutoString(aSessionId), stream)); >+ */ Remove the commented out block of code > send: function(data) { >- var binaryStream = Cc["@mozilla.org/binaryinputstream;1"]. >- createInstance(Ci.nsIBinaryInputStream); >- binaryStream.setInputStream(data); >- var message = binaryStream.readBytes(binaryStream.available()); >- sendAsyncMessage('message-sent', message); >+ sendAsyncMessage('message-sent', data); Hmm, do we want to send data or string? Since if data, using nsACString everywhere would work better. But looks like the spec uses DOMString (or what we have unimplemented ArrayBuffer etc.) and ArrayBuffer etc would need rather different handling anyway, so DOMString is fine.

modify by reviewer's comments, carry r+

Hello Jan-Ivar, We're trying to use RTCDataChannel to implement the app-to-app transport channel in presentation API. That is, we'll use a |nsIPresentationControlChannel| (currently a plain TCP socket which is created in advance) to exchange SDP in |nsIPresentationDataChannelSessionTransportBuilder|, and the builder will generate a |nsIPresentationSessionTransport| which underlying transport is RTCDataChannel. Please refer to the latest idls in Comment 50 for |nsIPresentationDataChannelSessionTransportBuilder| and |nsIPresentationSessionTransport|. During the patch cooking, I'd like to touch base with the reviewer in advance. Are you the right person to review this patch or could you suggest one? Thanks a lot!

Refer builder.idl to Comment 46 and refer sessionTransport.idl to this attachment

Sure, either me or jesup should be able to review RTCDataChannel uses.

Hello jib, This WIP patch implements happy case. Could I have your feedback first?

Comment on attachment 8688389 [details] [diff] [review] Part3: implement session transport with DataChannel, WIP v1 Review of attachment 8688389 [details] [diff] [review]: ----------------------------------------------------------------- I had to familiarize myself with the Presentation api, so please excuse my lack of full grasp of all the objects involved and choices made. Some comments, nits and questions, but overall this looks good! ::: dom/presentation/PresentationDataChannelSessionTransport.js @@ +9,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +function log(aMsg) { > + dump("-*- PresentationDataChannelSessionTransport.js : " + aMsg + "\n"); > +} I assume this is temporary? @@ +46,5 @@ > + > +PresentationTransportBuilder.prototype = { > + classID: PRESENTATIONTRANSPORTBUILDER_CID, > + contractID: PRESENTATIONTRANSPORTBUILDER_CONTRACTID, > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder, Any reason not to use webidl for new things? @@ +49,5 @@ > + contractID: PRESENTATIONTRANSPORTBUILDER_CONTRACTID, > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder, > + Ci.nsIPresentationControlChannelListener]), > + > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { What calls this? It looks like this api was added in your other patch here, and I don't see any callers. Also, are we building xpcom interfaces just for internal use? If so, then that seems unusual to me, and I wonder why, but again I'm new to this area, so there may be reasons I'm missing. @@ +58,5 @@ > + this._controlChannel.listener = this; > + this._listener = aListener; > + > + // XXX need to determine parameters > + this._pc = new this._window.RTCPeerConnection(); What are the network requirements for this? Unless this is for LAN only you'll need a STUN server at minimum. e.g. { iceServers: [{ urls: "stun:stun.l.google.com:19302" }]}; and possibly even a TURN server if this is something that needs to work in general between all machines all over the internet. @@ +64,5 @@ > + let self = this; > + this._pc.onicecandidate = function(aEvent) { > + if (aEvent.candidate) { > + self._controlChannel.sendIceCandidate(JSON.stringify(aEvent.candidate)); > + } Nit: I should divulge that I'm a huge fan of using modern JS wherever possible, and avoiding workarounds like 'self'. e.g. this._pc.onicecandidate = event => event.candidate && this._controlChannel.sendIceCandidate(JSON.stringify(event.candidate)); Applies throughout. @@ +72,5 @@ > + // TODO Handle Timeout > + log("onnegotiationneeded with type " + self._type); > + self._pc.createOffer().then(function(aOffer) { > + log("setLocalDescription"); > + return self._pc.setLocalDescription(aOffer); Same point, but I find WebRTC code in particular much easier to read when arrow functions are used, and again they solve the 'self' problem. E.g. https://jsfiddle.net/jib1/f5y48hcd/ So if this is at all possible to do here, I recommend adopting it. @@ +83,5 @@ > + > + switch (this._type) { > + case Ci.nsIPresentationSessionTransportBuilder.TYPE_SENDER: > + // XXX need to determine parameters > + this._dataChannel = this._pc.createDataChannel("presentationAPI"); You're using default settings for data channels here, which may very well be what you want. Just want to point out the multiple options: http://w3c.github.io/webrtc-pc/#idl-def-RTCDataChannelInit @@ +108,5 @@ > + this._sessionTransport.initWithDataChannel(this._dataChannel); > + > + let self = this; > + > + this._dataChannel.onopen = function() { Depending on how production-ready this needs to be (I'm not sure what the failure characteristics are of the presentation API, or how things are shut down), there may be additional considerations. E.g. If you want to be able to detect the other side closing, this can be done using pc.onclose. See this hangup demo for an example (open in two tabs): https://jsfiddle.net/jib1/cef91vdg/ The example also handles closing and re-opening multiple times cleanly, which requires code to close and re-open peerconnections. Dunno if this applies here. @@ +114,5 @@ > + self._window.setTimeout(function () { > + self._listener.onSessionTransport(self._sessionTransport); > + self._sessionTransport.callback.notifyTransportReady(); > + > + self._cleanup(); Here you call cleanup on the next cycle after data channel opens, which seems wrong. @@ +123,5 @@ > + _cleanup: function() { > + this._window = null; > + this._controlChannel = null; > + this._listener = null; > + this._pc = null; Always call this._pc.close() before dropping all _pc references, otherwise the peer connection remains active for ~30 seconds until it gets garbage collected.

Thanks for your quick feedback! needinfo just to make sure you receive my update. (In reply to Jan-Ivar Bruaroey [:jib] from comment #55) > Comment on attachment 8688389 [details] [diff] [review] > Part3: implement session transport with DataChannel, WIP v1 > > Review of attachment 8688389 [details] [diff] [review]: > ----------------------------------------------------------------- > > I had to familiarize myself with the Presentation api, so please excuse my > lack of full grasp of all the objects involved and choices made. > > Some comments, nits and questions, but overall this looks good! > > ::: dom/presentation/PresentationDataChannelSessionTransport.js > @@ +9,5 @@ > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > + > > +function log(aMsg) { > > + dump("-*- PresentationDataChannelSessionTransport.js : " + aMsg + "\n"); > > +} > > I assume this is temporary? Yes, would be commented in the final version. > > @@ +46,5 @@ > > + > > +PresentationTransportBuilder.prototype = { > > + classID: PRESENTATIONTRANSPORTBUILDER_CID, > > + contractID: PRESENTATIONTRANSPORTBUILDER_CONTRACTID, > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder, > > Any reason not to use webidl for new things? Presentation API is a whole story from discovery, launch apps to delivery data. Currently we port mdns to discovery devices, thus coming a |MulticastDNSDeviceProvider|. And we also automatically launch the appropriate app and provide a session transport (currently TCP. Will move to RTCDataChannel in this patch) to enable app-to-app communication. Moreover, web content can easily accomplish the _presentation_ scenario. On the other hand, Presentation API also support 1-UA case such as HDMI and wifi-direct. That's another discovery mechanism (handle by system app) and session transport (IPC). That's not the RTCDataChannel's role. > > @@ +49,5 @@ > > + contractID: PRESENTATIONTRANSPORTBUILDER_CONTRACTID, > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder, > > + Ci.nsIPresentationControlChannelListener]), > > + > > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { > > What calls this? It looks like this api was added in your other patch here, > and I don't see any callers. > Yes, that will be implemented in the next patch. Current implementation of session transport is TCP, so we separate the builder from session transport in Part 1 patch. Presentation(Presenting|Controlling)Info will use the builder. The usage is like: builder.buildDataChannelTransport(...) this.onSessionTransport = sessionTransport => { sessionTransport.callback = this; ... } And all the SDP negotiation will be done in the |buildDataChannelTransport|. > Also, are we building xpcom interfaces just for internal use? If so, then > that seems unusual to me, and I wonder why, but again I'm new to this area, > so there may be reasons I'm missing. > Yes the idls are not expected to used by add-on. I just use idl to do some cross languages stuff, and wish not to change previous codes too much. > @@ +58,5 @@ > > + this._controlChannel.listener = this; > > + this._listener = aListener; > > + > > + // XXX need to determine parameters > > + this._pc = new this._window.RTCPeerConnection(); > > What are the network requirements for this? > > Unless this is for LAN only you'll need a STUN server at minimum. e.g. { > iceServers: [{ urls: "stun:stun.l.google.com:19302" }]}; and possibly even a > TURN server if this is something that needs to work in general between all > machines all over the internet. > That's a tough decision since currently we only have mdns device provider, which provides the device in the same sub-net. But you're right, maybe we will have a |SIPDeviceProvider| in the future and a STUN/TURN is needed. moreover, the |*Provider.idl| could be port via add-on. So we should have a comprehensive image about the story. I'll think more about this. > @@ +64,5 @@ > > + let self = this; > > + this._pc.onicecandidate = function(aEvent) { > > + if (aEvent.candidate) { > > + self._controlChannel.sendIceCandidate(JSON.stringify(aEvent.candidate)); > > + } > > Nit: I should divulge that I'm a huge fan of using modern JS wherever > possible, and avoiding workarounds like 'self'. e.g. > > this._pc.onicecandidate = event => event.candidate && > > this._controlChannel.sendIceCandidate(JSON.stringify(event.candidate)); > > Applies throughout. > Okay, I'm not a big fan of 'self', either. > @@ +83,5 @@ > > + > > + switch (this._type) { > > + case Ci.nsIPresentationSessionTransportBuilder.TYPE_SENDER: > > + // XXX need to determine parameters > > + this._dataChannel = this._pc.createDataChannel("presentationAPI"); > > You're using default settings for data channels here, which may very well be > what you want. Just want to point out the multiple options: > > http://w3c.github.io/webrtc-pc/#idl-def-RTCDataChannelInit > I'd like to use a TCP-like transport, so default settings might be good. I'll check all the options. > @@ +108,5 @@ > > + this._sessionTransport.initWithDataChannel(this._dataChannel); > > + > > + let self = this; > > + > > + this._dataChannel.onopen = function() { > > Depending on how production-ready this needs to be (I'm not sure what the > failure characteristics are of the presentation API, or how things are shut > down), there may be additional considerations. > > E.g. If you want to be able to detect the other side closing, this can be > done using pc.onclose. onclose and other callbacks are in PresentationTransport.initWithDataChannel. But I should check if we have close and re-open scenario. Also, onerror might be called before onopen, thus I need to handle it here. > See this hangup demo for an example (open in two tabs): > https://jsfiddle.net/jib1/cef91vdg/ > > The example also handles closing and re-opening multiple times cleanly, > which requires code to close and re-open peerconnections. > > Dunno if this applies here. > > @@ +114,5 @@ > > + self._window.setTimeout(function () { > > + self._listener.onSessionTransport(self._sessionTransport); > > + self._sessionTransport.callback.notifyTransportReady(); > > + > > + self._cleanup(); > > Here you call cleanup on the next cycle after data channel opens, which > seems wrong. > The dataChannelSessionTransport would pass to the listener and should be cached there. I'll cook more comment about this. > @@ +123,5 @@ > > + _cleanup: function() { > > + this._window = null; > > + this._controlChannel = null; > > + this._listener = null; > > + this._pc = null; > > Always call this._pc.close() before dropping all _pc references, otherwise > the peer connection remains active for ~30 seconds until it gets garbage > collected. Will add it.

(In reply to Junior [:junior] from comment #56) > > > +function log(aMsg) { > > > + dump("-*- PresentationDataChannelSessionTransport.js : " + aMsg + "\n"); > > > +} > > > > I assume this is temporary? > > Yes, would be commented in the final version. Or even removed I hope. > Presentation API is a whole story from discovery, Thanks for explaining. It is helpful. > The usage is like: > builder.buildDataChannelTransport(...) > this.onSessionTransport = sessionTransport => { > sessionTransport.callback = this; > ... > } This looks like js... > Yes the idls are not expected to used by add-on. I just use idl to do some > cross languages stuff, and wish not to change previous codes too much. But if buildDataChannelTransport is in JS, and the caller is JS, why not call it directly instead of through an xpcom interface? > > @@ +114,5 @@ > > > + self._window.setTimeout(function () { > > > + self._listener.onSessionTransport(self._sessionTransport); > > > + self._sessionTransport.callback.notifyTransportReady(); > > > + > > > + self._cleanup(); > > > > Here you call cleanup on the next cycle after data channel opens, which > > seems wrong. > > The dataChannelSessionTransport would pass to the listener and should be > cached there. I'll cook more comment about this. I still don't understand. Why close the connection on open? An RTCDataChannel cannot survive without the RTCPeerConnection, and _cleanup() kills the pc (or at least it will once you change it to call this._pc.close()). > > @@ +123,5 @@ > > > + _cleanup: function() { > > > + this._window = null; > > > + this._controlChannel = null; > > > + this._listener = null; > > > + this._pc = null; > > > > Always call this._pc.close() before dropping all _pc references, otherwise > > the peer connection remains active for ~30 seconds until it gets garbage > > collected. I suspect it's the fact that you're not calling _pc.close() here that saves you above. > Will add it.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #57) > (In reply to Junior [:junior] from comment #56) > > > > +function log(aMsg) { > > > > + dump("-*- PresentationDataChannelSessionTransport.js : " + aMsg + "\n"); > > > > +} > > > > > > I assume this is temporary? > > > > Yes, would be commented in the final version. > > Or even removed I hope. I won't insist, but presentation API just began integration test last month. It's helpful to let it easily to debug. > > > Presentation API is a whole story from discovery, > > Thanks for explaining. It is helpful. > > > The usage is like: > > builder.buildDataChannelTransport(...) > > this.onSessionTransport = sessionTransport => { > > sessionTransport.callback = this; > > ... > > } > > This looks like js... > > > Yes the idls are not expected to used by add-on. I just use idl to do some > > cross languages stuff, and wish not to change previous codes too much. > > But if buildDataChannelTransport is in JS, and the caller is JS, why not > call it directly instead of through an xpcom interface? I'm sorry to mislead you here. The caller will be in PresentationSessionInfo.cpp in next patch. We have a |buildTCP(Sender|Receiver)Transport| example in Part1 attachment for your information. > > > > @@ +114,5 @@ > > > > + self._window.setTimeout(function () { > > > > + self._listener.onSessionTransport(self._sessionTransport); > > > > + self._sessionTransport.callback.notifyTransportReady(); > > > > + > > > > + self._cleanup(); > > > > > > Here you call cleanup on the next cycle after data channel opens, which > > > seems wrong. > > > > The dataChannelSessionTransport would pass to the listener and should be > > cached there. I'll cook more comment about this. > > I still don't understand. Why close the connection on open? An > RTCDataChannel cannot survive without the RTCPeerConnection, and _cleanup() > kills the pc (or at least it will once you change it to call > this._pc.close()). Okay, I have a big misunderstanding. My test doesn't remind me this :( I guess my test would fail when |this._pc.closed()| is added. > > > > @@ +123,5 @@ > > > > + _cleanup: function() { > > > > + this._window = null; > > > > + this._controlChannel = null; > > > > + this._listener = null; > > > > + this._pc = null; > > > > > > Always call this._pc.close() before dropping all _pc references, otherwise > > > the peer connection remains active for ~30 seconds until it gets garbage > > > collected. > > I suspect it's the fact that you're not calling _pc.close() here that saves > you above. > Ah-ha, I guess yes :) Thanks for the important notice > > Will add it.

(In reply to Junior [:junior] from comment #58) > It's helpful to let it easily to debug. Sure np.

One question about |addIceCandidate|: I see some example which |addIceCandidate| after |setRemoteDescription|; Let the candidates be queued if |setRemoteDescription| has not been called. Is it necessary to do this now? And I file bug 1227053 as a follow-up for |iceServers|.

(In reply to Junior [:junior] from comment #60) > Let the candidates be queued if |setRemoteDescription| has not been called. > Is it necessary to do this now? No this is not necessary, provided your signaling channel guarantees the order of offer/answer-messages vs. candidate-messages, AND - On the offer side you send the offer at the latest in the immediate setLocalDescription success callback. - On the answer side, send the answer at the latest in the immediate setLocalDescription success callback. This is important because the ICE agent starts firing onicecandidate immediately after. On the signaling wire you always want: offer, candidate, candidate, candidate never candidate, offer, candidate, candidate A common mistake is to have too many .then() in-directions here causing signaling to get out of order. Parts of our mochitest harness suffered from this actually (Bug 1155856).

(In reply to Jan-Ivar Bruaroey [:jib] from comment #61) > (In reply to Junior [:junior] from comment #60) > > Let the candidates be queued if |setRemoteDescription| has not been called. > > Is it necessary to do this now? > > No this is not necessary, provided your signaling channel guarantees the > order of offer/answer-messages vs. candidate-messages, AND > > - On the offer side you send the offer at the latest in the immediate > setLocalDescription success callback. > - On the answer side, send the answer at the latest in the immediate > setLocalDescription success callback. > > This is important because the ICE agent starts firing onicecandidate > immediately after. > > On the signaling wire you always want: > > offer, candidate, candidate, candidate > > never > > candidate, offer, candidate, candidate > > A common mistake is to have too many .then() in-directions here causing > signaling to get out of order. Parts of our mochitest harness suffered from > this actually (Bug 1155856). Okay, luckily the patch follows the rule and the underlying signaling channel is TCP which is ordered. However, I should emphasis this as a comment for |nsIPresentationControlChannel|

Comment on attachment 8690760 [details] [diff] [review] Part3: implement session transport with DataChannel, v2 Review of attachment 8690760 [details] [diff] [review]: ----------------------------------------------------------------- The overwritten dataChannel.onopen and the missing error arguments need to be fixed. I also think the ownership model needs to be cleaned up some. ::: dom/presentation/PresentationDataChannelSessionTransport.js @@ +9,5 @@ > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > + > +function log(aMsg) { > + // dump("-*- PresentationDataChannelSessionTransport.js : " + aMsg + "\n"); Maybe add a comment with details on plans for removing this eventually, since JS logging is not free? @@ +52,5 @@ > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder, > + Ci.nsIPresentationControlChannelListener, > + Ci.nsITimerCallback]), > + > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { Why the split into two interfaces, PresentationTransport and PresentationTransportBuilder? The latter seems like an elaborate closure, and the ownership model is confusing to me (more on this below). @@ +53,5 @@ > + Ci.nsIPresentationControlChannelListener, > + Ci.nsITimerCallback]), > + > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { > + if (this._type || this._window || this._controlChannel || this._listener) { nit: AFAICT all are set or none are set, so testing only one of these seems sufficient. @@ +55,5 @@ > + > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { > + if (this._type || this._window || this._controlChannel || this._listener) { > + log("buildDataChannelTransport has started. Bail out."); > + return; nit: Do only programming errors lead here? If so, maybe throw an error instead, since aListener would remain unused and would likely confuse code later if the call were to succeed, making the bug hard to find. @@ +76,5 @@ > + log("onnegotiationneeded with type " + this._type); > + this._peerConnection.createOffer() > + .then(aOffer => this._peerConnection.setLocalDescription(aOffer)) > + .then(() => this._controlChannel > + .sendOffer(new PresentationDataChannelDescription(this._peerConnection.localDescription))) nit: needs wordwrap somehow @@ +77,5 @@ > + this._peerConnection.createOffer() > + .then(aOffer => this._peerConnection.setLocalDescription(aOffer)) > + .then(() => this._controlChannel > + .sendOffer(new PresentationDataChannelDescription(this._peerConnection.localDescription))) > + .catch(() => this._reportError()); Missing error argument: e => this._reportError(e). Applies throughout. @@ +91,5 @@ > + this._peerConnection.ondatachannel = aEvent => { > + this._dataChannel = aEvent.channel; > + this._hookDataChannel(); > + } > + break; I think we should throw if type is neither TYPE_SENDER nor TYPE_RECEIVER here. Once we do that, we can easily inline this._hookDataChannel() here since it's only called once and only from here. Seems cleaner. @@ +97,5 @@ > + > + let timeout; > + try { > + timeout = Services.prefs.getIntPref("presentation.receiver.loading.timeout"); > + } catch (e) { Is try-catch really needed on getIntPref? I see little precedence in mxr. Can we just initialize it to 10000 and skip try/catch? @@ +99,5 @@ > + try { > + timeout = Services.prefs.getIntPref("presentation.receiver.loading.timeout"); > + } catch (e) { > + // This happens if the pref doesn't exist, so we have a default value. > + timeout = 10000; A 10 second timeout might cause intermittents on some of our slowest builders, if there are any automated tests for this. Even outside of testing this value seems quite aggressive, given that the ICE agent can take up to ~20 seconds to exhaust all options (even though usually things connect much sooner). We see this e.g. on systems with VPN interfaces take this long to time out. @@ +102,5 @@ > + // This happens if the pref doesn't exist, so we have a default value. > + timeout = 10000; > + } > + > + // The timer is to check if the negotaion finishes on time. typo @@ +107,5 @@ > + this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > + this._timer.initWithCallback(this, timeout, this._timer.TYPE_ONE_SHOT); > + }, > + > + notify: function(timer) { nit: timer arg is unused @@ +116,5 @@ > + }, > + > + _reportError: function(aError) { > + log("report Error " + aError.name + ":" + aError.message); > + this._listener.onError(Cr.NS_ERROR_FAILURE); Why replace aError with Cr.NS_ERROR_FAILURE? Seems less informative. Applies throughout. @@ +120,5 @@ > + this._listener.onError(Cr.NS_ERROR_FAILURE); > + this._cleanup(); > + }, > + > + _hookDataChannel: function() { This function does more than its name implies. Suggest inlining it into buildDataChannelTransport. @@ +124,5 @@ > + _hookDataChannel: function() { > + this._sessionTransport = new PresentationTransport(); > + this._sessionTransport.init(this._peerConnection, this._dataChannel); > + > + this._dataChannel.onopen = () => { Who owns peerConnection and dataChannel at this point? The builder or the sessionTransport? Example confusion: this._sessionTransport.init() above has just set dataChannel.onopen to its function, only to be overwritten here by our function (unlike addEventListener, there can only be one function assigned to dataChannel.onopen at a time). I think there either needs to be a clearer handoff or a merge of the code. @@ +130,5 @@ > + this._isTransportReady = true; > + this._timer.cancel(); > + this._timer = null; > + > + this._window.setTimeout(() => { Why is setTimeout needed here? @@ +148,5 @@ > + > + _cleanup: function() { > + if (!this._isTransportReady) { > + this._peerConnection.close(); > + this._dataChannel.close(); A cleanup function that doesn't clean up everything seems confusing to me. Ownership of the peerConnection and its dataChannel seems to depend on this._isTransportReady here, but if we no longer own these objects, why still have pointers to them? I worry this gray handoff makes it hard to reason about what can still influence these objects (more on this below). @@ +167,5 @@ > + }, > + > + // nsIPresentationControlChannelListener > + onOffer: function(aOffer) { > + log("onOffer: " + aOffer.dataChannelSDP + " with type " + this._type); E.g. what if this._isTransportReady is true here? @@ +203,5 @@ > + > + notifyClosed: function(aReason) { > + log("notifyClosed reason: " + aReason); > + if (!this._isTransportReady) { > + this._listener.onError(Cr.NS_ERROR_FAILURE); When is notifyClosed called? Are we certain we don't want to do anything in response if this._isTransportReady == true?

Sorry for the wrong format for Comment 64 :( I'd like to obsolete it and make a correct one.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #63) > Comment on attachment 8690760 [details] [diff] [review] > Part3: implement session transport with DataChannel, v2 > > Review of attachment 8690760 [details] [diff] [review]: > ----------------------------------------------------------------- > > The overwritten dataChannel.onopen and the missing error arguments need to > be fixed. I also think the ownership model needs to be cleaned up some. > Thanks for your detailed review and hope this comments could clarify these issues. > ::: dom/presentation/PresentationDataChannelSessionTransport.js > @@ +9,5 @@ > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Cu.import("resource://gre/modules/Services.jsm"); > > + > > +function log(aMsg) { > > + // dump("-*- PresentationDataChannelSessionTransport.js : " + aMsg + "\n"); > > Maybe add a comment with details on plans for removing this eventually, > since JS logging is not free? Okay, would plan to remove and file a bug to follow-up. Also comments would be added > > @@ +52,5 @@ > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder, > > + Ci.nsIPresentationControlChannelListener, > > + Ci.nsITimerCallback]), > > + > > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { > > Why the split into two interfaces, PresentationTransport and > PresentationTransportBuilder? The latter seems like an elaborate closure, > and the ownership model is confusing to me (more on this below). > The reason to split two interfaces is to _reuse_ those codes with the PresentationSessionTransport. We already have TCP for transport and throughout all the implementation of presentaiton API. On the other hand, building process of TCP and RTCDataChannel are different. We can not have a common interface of the two builders. Ownership model is explained below. > @@ +53,5 @@ > > + Ci.nsIPresentationControlChannelListener, > > + Ci.nsITimerCallback]), > > + > > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { > > + if (this._type || this._window || this._controlChannel || this._listener) { > > nit: AFAICT all are set or none are set, so testing only one of these seems > sufficient. > also time-concerned? It works for me but just confirm the intention. > @@ +55,5 @@ > > + > > + buildDataChannelTransport: function(aType, aWindow, aControlChannel, aListener) { > > + if (this._type || this._window || this._controlChannel || this._listener) { > > + log("buildDataChannelTransport has started. Bail out."); > > + return; > > nit: Do only programming errors lead here? If so, maybe throw an error > instead, since aListener would remain unused and would likely confuse code > later if the call were to succeed, making the bug hard to find. > Yes that's only made by programmer. I'll make it throw. > @@ +76,5 @@ > > + log("onnegotiationneeded with type " + this._type); > > + this._peerConnection.createOffer() > > + .then(aOffer => this._peerConnection.setLocalDescription(aOffer)) > > + .then(() => this._controlChannel > > + .sendOffer(new PresentationDataChannelDescription(this._peerConnection.localDescription))) > > nit: needs wordwrap somehow > Sorry for a silly question. Does it mean such as .then(() => { let offer = new PresentationDataChannelDescription(this._peerConnection.localDescription); this._controlChannel.sendOffer(offer); }).catch((e) => this._reportError(e)); > @@ +91,5 @@ > > + this._peerConnection.ondatachannel = aEvent => { > > + this._dataChannel = aEvent.channel; > > + this._hookDataChannel(); > > + } > > + break; > > I think we should throw if type is neither TYPE_SENDER nor TYPE_RECEIVER > here. > > Once we do that, we can easily inline this._hookDataChannel() here since > it's only called once and only from here. Seems cleaner. > The _hookDataChennel is called in two place. Another is called six lines previously, which is the time createDataChannel But throw might be a good idea. > @@ +97,5 @@ > > + > > + let timeout; > > + try { > > + timeout = Services.prefs.getIntPref("presentation.receiver.loading.timeout"); > > + } catch (e) { > > Is try-catch really needed on getIntPref? I see little precedence in mxr. > Can we just initialize it to 10000 and skip try/catch? > I'll make some experiment and feedback here later. > @@ +99,5 @@ > > + try { > > + timeout = Services.prefs.getIntPref("presentation.receiver.loading.timeout"); > > + } catch (e) { > > + // This happens if the pref doesn't exist, so we have a default value. > > + timeout = 10000; > > A 10 second timeout might cause intermittents on some of our slowest > builders, if there are any automated tests for this. > > Even outside of testing this value seems quite aggressive, given that the > ICE agent can take up to ~20 seconds to exhaust all options (even though > usually things connect much sooner). We see this e.g. on systems with VPN > interfaces take this long to time out. > Current use case is under the same subnet so I'd like to: 1. Keep 10 seconds 2. Custom to 30 seconds in the test 3. File a follow-up bug to let device provider custom the time duration. > @@ +116,5 @@ > > + }, > > + > > + _reportError: function(aError) { > > + log("report Error " + aError.name + ":" + aError.message); > > + this._listener.onError(Cr.NS_ERROR_FAILURE); > > Why replace aError with Cr.NS_ERROR_FAILURE? Seems less informative. Applies > throughout. > Does we have a nsresult here? Since I'm not sure if |this._listener.onError(aError)| works or not > @@ +120,5 @@ > > + this._listener.onError(Cr.NS_ERROR_FAILURE); > > + this._cleanup(); > > + }, > > + > > + _hookDataChannel: function() { > > This function does more than its name implies. Suggest inlining it into > buildDataChannelTransport. > Since we have two callers, I'd like to rename to |_setupDataChannel| first > @@ +124,5 @@ > > + _hookDataChannel: function() { > > + this._sessionTransport = new PresentationTransport(); > > + this._sessionTransport.init(this._peerConnection, this._dataChannel); > > + > > + this._dataChannel.onopen = () => { > > Who owns peerConnection and dataChannel at this point? The builder or the > sessionTransport? > > Example confusion: this._sessionTransport.init() above has just set > dataChannel.onopen to its function, only to be overwritten here by our > function (unlike addEventListener, there can only be one function assigned > to dataChannel.onopen at a time). > > I think there either needs to be a clearer handoff or a merge of the code. > I should create the session transport in |onopen| and handoff peerConnection and dataChannel to the session transport. > @@ +130,5 @@ > > + this._isTransportReady = true; > > + this._timer.cancel(); > > + this._timer = null; > > + > > + this._window.setTimeout(() => { > > Why is setTimeout needed here? I should create the session transport in |onopen| and handoff peerConnection and dataChannel to the session transport. > > @@ +148,5 @@ > > + > > + _cleanup: function() { > > + if (!this._isTransportReady) { > > + this._peerConnection.close(); > > + this._dataChannel.close(); > > A cleanup function that doesn't clean up everything seems confusing to me. > > Ownership of the peerConnection and its dataChannel seems to depend on > this._isTransportReady here, but if we no longer own these objects, why > still have pointers to them? I worry this gray handoff makes it hard to > reason about what can still influence these objects (more on this below). > Once |_cleanup| is called, the builder loose everything. If |this.isTransportReady == true|, peer connection and data channel are hand-offed to session transport in advance. Otherwise, the builder is failed to make a RTCDataChannel and clean up everything. > @@ +167,5 @@ > > + }, > > + > > + // nsIPresentationControlChannelListener > > + onOffer: function(aOffer) { > > + log("onOffer: " + aOffer.dataChannelSDP + " with type " + this._type); > > E.g. what if this._isTransportReady is true here? > should throw. I'll guard all the incoming message from this._controlChannel (signaling channel) > @@ +203,5 @@ > > + > > + notifyClosed: function(aReason) { > > + log("notifyClosed reason: " + aReason); > > + if (!this._isTransportReady) { > > + this._listener.onError(Cr.NS_ERROR_FAILURE); > > When is notifyClosed called? Are we certain we don't want to do anything in > response if this._isTransportReady == true? on(Offer|Answer|IceCandidate), notify(Opened|Closed) These five methods are from |nsIPresentationControlChannelListener|. |notifyClosed| is called when |this._controlChannel|(signaling channel) is closed. |this._isTransportReady == true| means that the signaling channel finished its task. But this reminds me that the time |this._isTransportReady == true| is a little earlier than the signaling channel finishes its task.

(In reply to Junior [:junior] from comment #66) > The reason to split two interfaces is to _reuse_ those codes with the > PresentationSessionTransport. OK makes sense. > > nit: AFAICT all are set or none are set, so testing only one of these seems > > sufficient. > > > also time-concerned? It works for me but just confirm the intention. Intention is to remove dead code. > > nit: needs wordwrap somehow > > > Sorry for a silly question. Does it mean such as > > .then(() => { > let offer = new > PresentationDataChannelDescription(this._peerConnection.localDescription); > this._controlChannel.sendOffer(offer); > }).catch((e) => this._reportError(e)); Does sendOffer return a promise? Looks like no, so that's ok then. Like you I'm a fan of the simpler syntax without {} when possible (I find the implicit return much safer when dealing with promise chains), but whatever works. > > I think we should throw if type is neither TYPE_SENDER nor TYPE_RECEIVER > > here. > > > > Once we do that, we can easily inline this._hookDataChannel() here since > > it's only called once and only from here. Seems cleaner. > > > The _hookDataChennel is called in two place. Another is called six lines > previously, which is the time createDataChannel You call it at the tail of both case statements, so once we have a default: throw... in the switch() then those two calls can be refactored out to a single call right after the switch(), making it a candidate for inlining. > But throw might be a good idea. > Current use case is under the same subnet so I'd like to: > 1. Keep 10 seconds > 2. Custom to 30 seconds in the test > 3. File a follow-up bug to let device provider custom the time duration. Up to you. > Does we have a nsresult here? Since I'm not sure if > |this._listener.onError(aError)| works or not I don't know the established pattern here, just observing that unless this is a call from content, then passing real errors up seems preferable when it comes time to debug. > I should create the session transport in |onopen| and handoff peerConnection > and dataChannel to the session transport. That sounds good. > Once |_cleanup| is called, the builder loose everything. I'd like to see the builder null out all references then rather than rely on a boolean. > should throw. > I'll guard all the incoming message from this._controlChannel (signaling > channel) Asserts are expensive in JS, so if we null out the references instead then things should naturally throw here I think.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #67) > (In reply to Junior [:junior] from comment #66) > > > > I think we should throw if type is neither TYPE_SENDER nor TYPE_RECEIVER > > > here. > > > > > > Once we do that, we can easily inline this._hookDataChannel() here since > > > it's only called once and only from here. Seems cleaner. > > > > > The _hookDataChennel is called in two place. Another is called six lines > > previously, which is the time createDataChannel > > You call it at the tail of both case statements, so once we have a default: > throw... in the switch() then those two calls can be refactored out to a > single call right after the switch(), making it a candidate for inlining. We can not refactor. One is a direct function call and another is in a callback function(|this._peerConnection.ondatachannel|.) > > > Does we have a nsresult here? Since I'm not sure if > > |this._listener.onError(aError)| works or not > > I don't know the established pattern here, just observing that unless this > is a call from content, then passing real errors up seems preferable when it > comes time to debug. > The thing is the errors thrown by WebAPI are |DOMException|, but the argument passed to |this._listener.onError| is nsresult. AFAIK we don't have a mapping function to do something like |this._listener.onError(TO_NSRESULT(aError))|. That's the reason I replace with NS_ERROR_FAILURE. > > Once |_cleanup| is called, the builder loose everything. > > I'd like to see the builder null out all references then rather than rely on > a boolean. > In all case, the builder null out all the references. The difference made by |this._isTransportReady| is to disable _peerConnection and _dataChannel or not. If |this._isTransportReady == true|, all needed references would pass to |this._sessionTransport| and those RTC functions should keep working. Otherwise, if |_cleanup| is called when |this._isTransportReady == false|, it indicates something wrong and we will disable the functionality of RTC. The builder null out all the reference no matter which cases.

(In reply to Junior [:junior] from comment #66) > (In reply to Jan-Ivar Bruaroey [:jib] from comment #63) > > Comment on attachment 8690760 [details] [diff] [review] > > @@ +97,5 @@ > > > + > > > + let timeout; > > > + try { > > > + timeout = Services.prefs.getIntPref("presentation.receiver.loading.timeout"); > > > + } catch (e) { > > > > Is try-catch really needed on getIntPref? I see little precedence in mxr. > > Can we just initialize it to 10000 and skip try/catch? > > If we haven't set the pref, |getIntPref| would throw. I guess we need to set the pref in |all.js| if we want to skip try/catch.

I'm not sure if it is possible the |onicecandidate| is triggered after the datachannel is established.

(In reply to Junior [:junior] (ooo 12/2-12/4) from comment #68) > We can not refactor. My mistake, I misread the code there. > > I'd like to see the builder null out all references then rather than rely on > > a boolean. > > In all case, the builder null out all the references. The difference made by > |this._isTransportReady| is to disable _peerConnection and _dataChannel or > not. What I meant is I think the code would be clearer with an explicit handoff: // Handoff the ownership of _peerConnection and _dataChannel this._sessionTransport.init(this._peerConnection, this._dataChannel); this._peerConnection = this._dataChannel = null; and then in _cleanup: if (this._peerConnection) this._peerConnection.close(); This way no boolean is needed, reducing complexity. > If |this._isTransportReady == true|, all needed references would pass to > |this._sessionTransport| and those RTC functions should keep working. Any other places this._isTransportReady is used, it seems this._sessionTransport can be used instead.

Comment on attachment 8692465 [details] [diff] [review] Part3: implement session transport with DataChannel, v3 Review of attachment 8692465 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, except I'm concerned that any error returning the answer is no longer treated as fatal, due to _isControlChannelNeeded (new to this patch). If you could explain the thinking there that would be great. ::: dom/presentation/PresentationDataChannelSessionTransport.js @@ +76,5 @@ > + // TODO bug 1227053 set iceServers from |nsIPresentationDevice| > + this._peerConnection = new this._window.RTCPeerConnection(); > + > + // |this._controlChannel == null| will throw since the control channal is > + // abnormally closed. typo @@ +86,5 @@ > + this._peerConnection.createOffer() > + .then(aOffer => this._peerConnection.setLocalDescription(aOffer)) > + .then(() => this._controlChannel > + .sendOffer(new PresentationDataChannelDescription(this._peerConnection.localDescription))) > + .catch((e) => this._reportError(e)); nit: () not needed around e. Applies throughout. @@ +98,5 @@ > + > + case Ci.nsIPresentationSessionTransportBuilder.TYPE_RECEIVER: > + this._peerConnection.ondatachannel = aEvent => { > + this._dataChannel = aEvent.channel; > + this._setupDataChannel(); Why not this._setDataChannel(aEvent.channel); ? @@ +133,5 @@ > + }, > + > + _setupDataChannel: function() { > + this._dataChannel.onopen = () => { > + log("data channel is onopen, notify the listener, type " + this._type); data channel is open @@ +139,5 @@ > + > + // Handoff the ownership of _peerConnection and _dataChannel to > + // _sessionTransport > + this._sessionTransport = new PresentationTransport(); > + this._sessionTransport.init(this._peerConnection, this._dataChannel); see comment 71 @@ +153,5 @@ > + } > + }, > + > + _cleanup: function(aReason) { > + if (aReason === Cr.NS_OK && !this._isTransportReady) { can't happen AFAICT, thus this is dead code @@ +157,5 @@ > + if (aReason === Cr.NS_OK && !this._isTransportReady) { > + aReason = Cr.NS_ERROR_FAILURE; > + } > + > + if (aReason !== Cr.NS_OK) { In JavaScript, == is preferred to === [1]. Applies throughout. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators @@ +167,5 @@ > + this._peerConnection = null; > + this._dataChannel = null; > + > + this._type = null; > + this._window = null; Why bother clearing all references? Wouldn't it be unusual for anyone to hold on to this builder-object when it can't be reused? @@ +169,5 @@ > + > + this._type = null; > + this._window = null; > + this._controlChannel.close(aReason); > + this._controlChannel = null; Is _cleanup guaranteed to only be called once? If not, this will call null.close() the second time and throw. Not a big deal, but might create noise. @@ +199,5 @@ > + .then(aAnswer => this._peerConnection.setLocalDescription(aAnswer)) > + .then(() => { > + this._isControlChannelNeeded = false; > + this._controlChannel > + .sendAnswer(new PresentationDataChannelDescription(this._peerConnection.localDescription)) Does sendAnswer guarantee that the message is sent on the wire before it returns? If not, then with this latest patch, any error while sending the answer would seem to be ignored in notifyClosed below, because _isControlChannelNeeded == false. @@ +240,5 @@ > +}; > + > + > +function PresentationTransport() { > + this._messageQueue = null; I prefer the invariant this._messageQueue = []; @@ +274,5 @@ > + if (!this._enableDataNotification || !this._callback) { > + log("queue message"); > + > + if (!this._messageQueue){ > + this._messageQueue = new Array(); Unnecessary if we initialize it to []. @@ +302,5 @@ > + return this._callback; > + }, > + > + set callback(aCallback) { > + this._callback = aCallback; What should happen if callback is set after enableDataNotification() is called? Should it receive the cached messages? @@ +317,5 @@ > + return; > + } > + this._enableDataNotification = true; > + > + if (this._messageQueue) { unnecessary once initialized to []. @@ +319,5 @@ > + this._enableDataNotification = true; > + > + if (this._messageQueue) { > + while (this._messageQueue.length > 0) { > + let data = this._messageQueue.shift(); shift is slow. I recommend this._messageQueue.forEach(data => this._callback.notifyData(data)); @@ +320,5 @@ > + > + if (this._messageQueue) { > + while (this._messageQueue.length > 0) { > + let data = this._messageQueue.shift(); > + this._callback.notifyData(data); what should happen if !this._callback ? @@ +322,5 @@ > + while (this._messageQueue.length > 0) { > + let data = this._messageQueue.shift(); > + this._callback.notifyData(data); > + } > + this._messageQueue = null; this._messageQueue = []; @@ +337,5 @@ > + this._dataChannel = null; > + this._peerConnection.close(); > + this._peerConnection = null; > + this._callback = null; > + this._messageQueue = null; What does clearing references accomplish when transports aren't reusable?

(In reply to Junior [:junior] (ooo 12/2-12/4) from comment #70) > I'm not sure if it is possible the |onicecandidate| is triggered after the > datachannel is established. Yes I believe it is possible since ICE candidate collection can take up to 20 seconds to time out, so I believe these can keep trickling in from the peer even after a connection has been established. I'm not 100% sure, since it's a bit unusual to terminate the signaling (control) channel. Usually, it is left open in case any changes cause a re-negotiation to be needed.

> I'm not 100% sure Confirmed.

TODO: 1. get the DOMWindow in Session*Info 2. deal with IPC 3. test

(In reply to Jan-Ivar Bruaroey [:jib] from comment #72) > Comment on attachment 8692465 [details] [diff] [review] > Part3: implement session transport with DataChannel, v3 > > Review of attachment 8692465 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +167,5 @@ > > + this._peerConnection = null; > > + this._dataChannel = null; > > + > > + this._type = null; > > + this._window = null; > > Why bother clearing all references? Wouldn't it be unusual for anyone to > hold on to this builder-object when it can't be reused? > The builder is supposed _not_ to be reused. Null-out all references since I'm afraid of circular reference. > @@ +169,5 @@ > > + > > + this._type = null; > > + this._window = null; > > + this._controlChannel.close(aReason); > > + this._controlChannel = null; > > Is _cleanup guaranteed to only be called once? If not, this will call > null.close() the second time and throw. Not a big deal, but might create > noise. > Will check if (this._controlChannel) > @@ +199,5 @@ > > + .then(aAnswer => this._peerConnection.setLocalDescription(aAnswer)) > > + .then(() => { > > + this._isControlChannelNeeded = false; > > + this._controlChannel > > + .sendAnswer(new PresentationDataChannelDescription(this._peerConnection.localDescription)) > > Does sendAnswer guarantee that the message is sent on the wire before it > returns? > > If not, then with this latest patch, any error while sending the answer > would seem to be ignored in notifyClosed below, because > _isControlChannelNeeded == false. > |sendAnswer| is async. I should think more about it. > @@ +302,5 @@ > > + return this._callback; > > + }, > > + > > + set callback(aCallback) { > > + this._callback = aCallback; > > What should happen if callback is set after enableDataNotification() is > called? Should it receive the cached messages? > |callback| should be set once the session transport is established. Thus I'll make it throw if |callback| is not set in |enableDataNotification| > > @@ +337,5 @@ > > + this._dataChannel = null; > > + this._peerConnection.close(); > > + this._peerConnection = null; > > + this._callback = null; > > + this._messageQueue = null; > > What does clearing references accomplish when transports aren't reusable? To avoid circular reference. > Yes I believe it is possible since ICE candidate collection can take up to > 20 seconds to time out, so I believe these can keep trickling in from the > peer even after a connection has been established. > > I'm not 100% sure, since it's a bit unusual to terminate the signaling > (control) channel. Usually, it is left open in case any changes cause a > re-negotiation to be needed. By offline discussion, we will keep current implementation that close channel once the datachannel is established. The reason is that better ICE candidates usually come earlier.

Modify by reviewer's comment, carry r+ TODO: deal with |sendAnswer| and |_isControlChannelNeeded| problem

> > @@ +199,5 @@ > > > + .then(aAnswer => this._peerConnection.setLocalDescription(aAnswer)) > > > + .then(() => { > > > + this._isControlChannelNeeded = false; > > > + this._controlChannel > > > + .sendAnswer(new PresentationDataChannelDescription(this._peerConnection.localDescription)) > > > > Does sendAnswer guarantee that the message is sent on the wire before it > > returns? > > > > If not, then with this latest patch, any error while sending the answer > > would seem to be ignored in notifyClosed below, because > > _isControlChannelNeeded == false. > > > > |sendAnswer| is async. I should think more about it. > If |sendAnswer| has error, the argument of |notifyClosed| should be NS_ERROR_XXX. (triggered by onStopRequest) Will let it check the argument. If all the check are passed, we have a final timer guard.

Handle |sendAnswer| error, carry r+

Since control channel is maintained by b2g process, and data channel should be open in content process. We need a nsIPCControlChannel as a proxy for |buildDataChannel(nsIPresentationControlChannel...)|

Hello smaug, This patch implements the in-process part. Could I have your feedback before the whole patch is done? Thanks!

Sorry about delay. Trying to get to this tomorrow. (and if I'm not fast enough always giving feedback, feel free to send email to remind me.)

Is PresentationPresentingInfo::OnSessionTransport called always only on main thread? Preferences::GetBool("dom.presentation.session_transport.data_channel.enable") is main thread only. Though, I don't quite understand the pref check there. Shouldn't we somehow ask from transport object what kind of handling it wants. Not rely on prefs there. Or do we have mRequesterDescription already set there so that we could ask it what kind transport is happening? Nit, +PresentationService::GetWindowIdBySessionId(const nsAString& aSessionId, + uint64_t *aWindowId) align param

(In reply to Olli Pettay [:smaug] from comment #83) > Is PresentationPresentingInfo::OnSessionTransport called always only on main > thread? > Preferences::GetBool("dom.presentation.session_transport.data_channel. > enable") is main thread only. > > Though, I don't quite understand the pref check there. > Shouldn't we somehow ask from transport object what kind of handling it > wants. Not rely on prefs there. Or do we have mRequesterDescription already > set there so that we could ask it what kind > transport is happening? > My bad, the transport type of presenter should be determined once |onOffer| is called. The mRequesterDescription indicates the transport type. No pref should be check here. btw, in-process case, onSessionTransport is called by javascript callback oop case, onSessionTransport should be called by non PBackground IPC. Both should be main thread in the b2g-process.

Check the transport type not by pref. This patch also contains some non-working ipdls. I still need time to clarify the flow of IPC. Please ignore those changes :)

Fix test break. This patch is still not able to handle oop case.

Since the oop case is complicated, I'd like to defer it to the next patch and review this first.

Comment on attachment 8708299 [details] [diff] [review] Part4: use data channel in substitution for TCP session transport (in-process), WIP v5 > PresentationControllingInfo::NotifyOpened() > { > MOZ_ASSERT(NS_IsMainThread()); >- return GetAddress(); >+ >+ if (!Preferences::GetBool("dom.presentation.session_transport.data_channel.enable")) { >+ // Build TCP session transport >+ return GetAddress(); >+ } This looks odd. Why are we doing anything base on prefs. Shouldn't we be able to detect what kind of connection was opened and based on that do something. Like, couldn't the caller of NotifyOpened pass the type or something? > nsresult > PresentationPresentingInfo::InitTransportAndSendAnswer() ... >+ if (type == nsIPresentationChannelDescription::TYPE_TCP) { >+ // Establish a data transport channel |mTransport| to the sender and use >+ // |this| as the callback. >+ nsCOMPtr<nsIPresentationTCPSessionTransportBuilder> builder = >+ do_CreateInstance(PRESENTATION_TCP_SESSION_TRANSPORT_CONTRACTID); >+ if (NS_WARN_IF(!builder)) { >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ >+ mBuilder = builder; >+ mTransportType = nsIPresentationChannelDescription::TYPE_TCP; >+ return builder->BuildTCPReceiverTransport(mRequesterDescription, this); >+ } else if (type == nsIPresentationChannelDescription::TYPE_DATACHANNEL) { Nit, 'else' after 'return'. You could just drop else and have if (type == nsIPresentationChannelDescription::TYPE_DATACHANNEL) { ... >+ } This ends if (type == nsIPresentationChannelDescription::TYPE_DATACHANNEL) Would be perhaps good to have } else { MOZ_ASSERT(false, "Unknown nsIPresentationChannelDescription type!"); } > // Session info with controlling browsing context (sender side) behaviors. > class PresentationControllingInfo final : public PresentationSessionInfo > , public nsIServerSocketListener > { > public: > NS_DECL_ISUPPORTS_INHERITED >@@ -172,16 +175,28 @@ private: > > void Shutdown(nsresult aReason) override; > > nsresult GetAddress(); > > nsresult OnGetAddress(const nsACString& aAddress); > > nsCOMPtr<nsIServerSocket> mServerSocket; >+ >+protected: >+ bool IsSessionReady() This should be marked as 'overridden', right? > > // Session info with presenting browsing context (receiver side) behaviors. > class PresentationPresentingInfo final : public PresentationSessionInfo > , public PromiseNativeHandler > , public nsITimerCallback > { > public: >@@ -231,14 +246,20 @@ private: > RefPtr<PresentationResponderLoadingCallback> mLoadingCallback; > nsCOMPtr<nsITimer> mTimer; > nsCOMPtr<nsIPresentationChannelDescription> mRequesterDescription; > RefPtr<Promise> mPromise; > > // The content parent communicating with the content process which the OOP > // receiver page belongs to. > nsCOMPtr<nsIContentParent> mContentParent; >+ >+protected: >+ bool IsSessionReady() This should be marked as 'overridden' >+[scriptable, uuid(de42b741-5619-4650-b961-c2cebb572c95)] > interface nsIPresentationService : nsISupports > { > /* > * Start a new presentation session and display a prompt box which asks users > * to select a device. > * > * @param url: The url of presenting page. > * @param sessionId: An ID to identify presentation session. > * @param origin: The url of requesting page. >+ * @param windowId: The window ID of sender's window So when it is ok to pass 0 here? > * @param callback: Invoke the callback when the operation is completed. > * NotifySuccess() is called with |id| if a session is > * established successfully with the selected device. > * Otherwise, NotifyError() is called with a error message. > */ > void startSession(in DOMString url, > in DOMString sessionId, > in DOMString origin, >+ in uint64_t windowId, unsigned long long would be a bit more .idl-y style. > in nsIPresentationServiceCallback callback); >+ /* >+ * The windowId for building RTCDataChannel session transport >+ */ >+ void getWindowIdBySessionId(in DOMString sessionId, >+ out uint64_t windowId); This should be just unsigned long long getwindowIdBySessionId(in DOMString sessionId); >+NS_IMETHODIMP >+PresentationIPCService::GetWindowIdBySessionId(const nsAString& aSessionId, >+ uint64_t *aWindowId) Nit, uint64_t* aWindowId > PresentationRequestParent::DoRequest(const StartSessionRequest& aRequest) > { > MOZ_ASSERT(mService); > return mService->StartSession(aRequest.url(), aRequest.sessionId(), >- aRequest.origin(), this); >+ aRequest.origin(), 0, this); I wish I understood when to pass 0 to StartSession and when some valid WindowID >+++ b/dom/presentation/tests/mochitest/PresentationSessionChromeScript.js ... >+function setTimeout(callback, delay) { >+ let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); >+ timer.initWithCallback({ notify: callback }, delay, >+ Ci.nsITimer.TYPE_ONE_SHOT); >+ return timer; >+} I learned recently that we have Timer.jsm which gives this function. Perhaps you could reuse that? > const mockedChannelDescription = { > QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationChannelDescription]), >- type: 1, >+ get type() { >+ if (Services.prefs.getBoolPref("dom.presentation.session_transport.data_channel.enable")) { >+ return 2; >+ } >+ return 1; >+ }, I think you could return Ci.nsIPresentationSessionTransportBuilder.TYPE_DATACHANNEL (which is 2) and Ci.nsIPresentationSessionTransportBuilder.TYPE_TCP (which is 1) here. >+ var promise = new Promise(function(aResolve, aReject) { >+ document.body.appendChild(iframe); >+ >+ aResolve(iframe); >+ }); >+ obs.notifyObservers(promise, 'setup-request-promise', null); can't recall why setup-request-promise takes a promise and not just some object which has iframe property, or perhaps just iframe itself. But looks like the existing tests do that already, so fine, no need to change. +SimpleTest.expectAssertions(0, 5); Which assertions are you seeing? And why? I realize the same thing is also in other presentation tests, but we should fix those assertions. mostly rs+ for the tests, but I'd like to know which assertions you're seeing. But r- mainly because I don't understand when to pass 0 as window id and when some valid id and also because it feels of if transport type is controlled by a pref usage somewhere deep in the implementation and not in the level the presentation API is first used. (or even better would be to support both transport types all the time.) So, mostly some clarifications to this, please.

(In reply to Olli Pettay [:smaug] from comment #88) > > PresentationControllingInfo::NotifyOpened() > > { > > MOZ_ASSERT(NS_IsMainThread()); > >- return GetAddress(); > >+ > >+ if (!Preferences::GetBool("dom.presentation.session_transport.data_channel.enable")) { > >+ // Build TCP session transport > >+ return GetAddress(); > >+ } > This looks odd. Why are we doing anything base on prefs. > Shouldn't we be able to detect what kind of connection was opened and based > on that do something. > Like, couldn't the caller of NotifyOpened pass the type or something? We're not use prefs for passing parameter. The pref is pre-set and never changed after initialized. On the other hand, this is the only place rely on the pref, and we need to. Not like presenter, presenters can rely on the message from controller. Here is where the controllers determine using TCP or RTCDataChannel. We do support both TCP and RTCDataChannel. But the transport type can not selected through webAPI since W3C doesn't specify it. We keep the flexibility to let an agent's default transport type, thus leading a prefs control. > > > >+ } > This ends if (type == nsIPresentationChannelDescription::TYPE_DATACHANNEL) > Would be perhaps good to have > } else { > MOZ_ASSERT(false, "Unknown nsIPresentationChannelDescription type!"); > } > The end of the if is with a return, causing a 'else after a return'. I'll add the MOZ_ASSERT only. > > >+[scriptable, uuid(de42b741-5619-4650-b961-c2cebb572c95)] > > interface nsIPresentationService : nsISupports > > { > > /* > > * Start a new presentation session and display a prompt box which asks users > > * to select a device. > > * > > * @param url: The url of presenting page. > > * @param sessionId: An ID to identify presentation session. > > * @param origin: The url of requesting page. > >+ * @param windowId: The window ID of sender's window > So when it is ok to pass 0 here? > > > PresentationRequestParent::DoRequest(const StartSessionRequest& aRequest) > > { > > MOZ_ASSERT(mService); > > return mService->StartSession(aRequest.url(), aRequest.sessionId(), > >- aRequest.origin(), this); > >+ aRequest.origin(), 0, this); > I wish I understood when to pass 0 to StartSession and when some valid > WindowID > Here I follow the previous logic. https://dxr.mozilla.org/mozilla-central/source/dom/presentation/ipc/PresentationParent.cpp#221 If the window is not handle by this process, just pass 0 since we can not handle the window in different process. It seems non-intuitive and I'll add comment in both places. > > +SimpleTest.expectAssertions(0, 5); > Which assertions are you seeing? And why? I realize the same thing is also > in other presentation tests, but we should fix those assertions. > My bad, I have doubted whether we need it and should have tested them. There's no assertions and the test looks green. I'll remove them.

rebase and modify by reviewer's comment. please see some clarification in comment 89.

Comment on attachment 8709308 [details] [diff] [review] Part4: use data channel in substitution for TCP session transport (in-process) v6 >+ // Only track the info when an actual window ID, which would never be 0, is >+ // provided (for an in-process sender page). >+ if (aWindowId != 0) { >+ mRespondingSessionIds.Put(aWindowId, new nsAutoString(aSessionId)); Oh, don't store nsAutoString here, but just nsString. > PresentationControllingInfo::NotifyOpened() > { > MOZ_ASSERT(NS_IsMainThread()); >- return GetAddress(); >+ >+ if (!Preferences::GetBool("dom.presentation.session_transport.data_channel.enable")) { >+ // Build TCP session transport >+ return GetAddress(); >+ } I still don't quite understand the pref based transport selection here. TCPPresentationServer.js is the one which calls notifyOpened, and it calls it regardless of what the pref is. And TCPPresentationServer is about tcp transportation, right? >+ if (aWindowId != 0) { >+ mRespondingSessionIds.Put(aWindowId, new nsAutoString(aSessionId)); Also here, use nsString. nsAutoString is in general for stack usage only. (I noticed the existing mRespondingSessionIds usage has nsAutoString, but nsString should be fine everywhere) I think the pref usage is still a bit unclear, but that could be changed in a followup patch.

> > PresentationControllingInfo::NotifyOpened() > > { > > MOZ_ASSERT(NS_IsMainThread()); > >- return GetAddress(); > >+ > >+ if (!Preferences::GetBool("dom.presentation.session_transport.data_channel.enable")) { > >+ // Build TCP session transport > >+ return GetAddress(); > >+ } > I still don't quite understand the pref based transport selection here. > TCPPresentationServer.js is the one which calls notifyOpened, and it calls > it regardless of what the pref is. > And TCPPresentationServer is about tcp transportation, right? > Several ambiguous things here: 1. TCPPresentationServer is a bad name, which is responds to handle control channel setup procedure. Moreover, the protocol of control channel is TCP. So it's nothing to do with the protocol of session transport. A rename bug is filed as bug 1228266, such as PresentationTCPControlChannelHandler 2. |NotifyOpened| is _not_ notifying the listener to open the session transport. |NotifyOpened| is notifying the listener that the control channel is opened and the listener could use the control channel to do the signaling stuff. > >+ if (aWindowId != 0) { > >+ mRespondingSessionIds.Put(aWindowId, new nsAutoString(aSessionId)); > Also here, use nsString. > nsAutoString is in general for stack usage only. > (I noticed the existing mRespondingSessionIds usage has nsAutoString, but > nsString should be fine everywhere) I'll modify them and check accross the dom/presentation

modify by reviewer's comment, carry r+

I'd like to request for a feedback since it's much more complicated for OOP case. The tough situation result from the opposite mechanism of IPC. For TCP session transport case, the session transport build-up procedure is in *chrome* process, where the control channel is. In this case, all the negotiation is in chrome process, and the final result is popped to child process after all the negotiation is done. Moreover, the call back of session transport is popped to child process by ipdl. interface nsIPresentationSessionTransportCallback : nsISupports { void notifyTransportReady(); void notifyTransportClosed(in nsresult reason); void notifyData(in ACString data); }; However, Data Channel Session Transport build-up procedure is in *child* process, but control channel is still in chrome process. Therefore, we need to back and froth between the two process during the negotiation. And the call back of session transport is also different. Since all the transport stuff is maintained in child process, the call back should be popped to user through webidl and noticed back to chrome process. This patch hooks the builder to PresentationParent, and next patch is still WIP.

This patch is still in WIP and far from workable. I need bunch of time to concentrate on it. My idea is let the negotiation logic in PresentationControlChannelChild, and hook the session transport to PresentationIPCService. Therefore, control channel IPC will be isolated from previous files. Here's the proposed logic: ControlChannelChild create the real builder Using control channel: Builder-> Child (as a Proxy Channel) --(via IPC)--> Parent (has the hand-offed channel) control channel callback is the opposite : Parent get the call back --(via IPC)--> Child --> Builder (as a Control Channel Listener)

A non-build-break version

implements interfaces

Mock factory in content process

I'm horribly late here, since the ? is on feedback and not on review and my review queue has been long enough to prevent focusing on feedback queue.

(In reply to Olli Pettay [:smaug] from comment #99) > I'm horribly late here, since the ? is on feedback and not on review and my > review queue has been long enough to prevent focusing on feedback queue. I aim to come out a whole patch this or next week. Target milestone is landing on early April. Is it possible to have a quick feedback to check if the scaffold is good, Or allocate some bandwidth when the patch is done?

Almost done for oop sender except semantic of terminate. TODO: make oop receiver work, test PPresentationControlChannel and make error handling solid

Comment on attachment 8716191 [details] [diff] [review] Part5-1: use data channel in substitution for TCP session transport (oop) v1 I wish the code had some assertions checking the process type. That would ease reading the code. >+ // Store the handlers based on session ID for building a OOP data channel >+ // Transport. >+ nsRefPtrHashtable<nsStringHashKey, >+ nsIPresentationTransportBuilderHandler> mTransportBuilderHandlers; So is this for OOP case only. Then the code accessing this hashtable should, if possible, have some assertions ensuring we're in such context. >- nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder> builder = >- do_CreateInstance("@mozilla.org/presentation/datachanneltransportbuilder;1"); >+ nsPIDOMWindow* window = nullptr; > >- if (NS_WARN_IF(!builder)) { >+ // in-process case. |mBuilder| in OOP case is set before. Where is that set? This is hard to follow, so at least need good comments. I assume this refers to the SetBuilder call in RegisterTransportBuilder, but it isn't clear from the code to see RegisterTransportBuilder is for OOP only. And where is that called? >@@ -842,35 +853,49 @@ PresentationPresentingInfo::InitTranspor ... > if (type == nsIPresentationChannelDescription::TYPE_DATACHANNEL) { >- nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder> builder = >- do_CreateInstance("@mozilla.org/presentation/datachanneltransportbuilder;1"); >+ // in-process case. |mBuilder| in OOP case is set before. >+ nsPIDOMWindow* window = nullptr; >+ if (!mBuilder) { >+ nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder> builder = >+ do_CreateInstance("@mozilla.org/presentation/datachanneltransportbuilder;1"); Hmm, so this is non-OOP case too. Needs better comment, but it is unclear to me why nsIPresentationDataChannelSessionTransportBuilder for non-OOP can be created in two different places. Confusing and duplicating the logic. > nsCOMPtr<nsIPresentationService> mService; >- nsTArray<nsString> mSessionIds; >+ nsTArray<nsString> mHasListenerSessionIds; >+ nsTArray<nsString> mHasHandlerSessionIds; These two arrays sound a bit odd. And it isn't clear what they mean. I mean "Handler" and "Listener" have so similar meaning that these need some comments. Also, PresentationParent::RecvRegisterSessionHandler seems to add something to mHasListenerSessionIds (the method is called *Handler, but array *Listener*) but PresentationParent::RecvRegisterTransportBuilderHandler adds to mHasHandlerSessionIds. So I'd like some good documentation somewhere about the setup. I wish there was some ascii art somewhere in the code to explain the presentation API implementation ;) (or non-ascii in MDN)

Sorry, that might not have been too useful feedback, but trying to understand the setup still.

(In reply to Olli Pettay [:smaug] from comment #102) > Comment on attachment 8716191 [details] [diff] [review] > Part5-1: use data channel in substitution for TCP session transport (oop) v1 > > I wish the code had some assertions checking the process type. That would > ease reading the code. I follow the previous convention for the process. Generally class names indicates which process those codes locate at. PresentationIPC* and *Child are content process. WebAPI implementation class depends on the process of caller. And others are at chrome process. > >+ // Store the handlers based on session ID for building a OOP data channel > >+ // Transport. > >+ nsRefPtrHashtable<nsStringHashKey, > >+ nsIPresentationTransportBuilderHandler> mTransportBuilderHandlers; > So is this for OOP case only. Then the code accessing this hashtable should, > if possible, have some assertions ensuring > we're in such context. > It's still in chrome process and try to hook an IPC actor to build transport in the content process. However, it's a dead code here I'll remove it :( my bad here. Now I hook the IPC parent to PresentationSessionInfo as a mBuilder. Like comment 94, comment 95, when an agent notices we need to build a data channel at content process, it will send |RegisterTransportBuilderHandler| through ipdl to notify chrome process: "if you want to build a transport, use the handler to build it to content process" The way to do it is set mBuilder = PresentationParent to info. > >- nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder> builder = > >- do_CreateInstance("@mozilla.org/presentation/datachanneltransportbuilder;1"); > >+ nsPIDOMWindow* window = nullptr; > > > >- if (NS_WARN_IF(!builder)) { > >+ // in-process case. |mBuilder| in OOP case is set before. > Where is that set? This is hard to follow, so at least need good comments. > I assume this refers to the SetBuilder call in RegisterTransportBuilder, but > it isn't clear from the > code to see RegisterTransportBuilder is for OOP only. > And where is that called? Mentioned previously. Moreover, the initial call point is |SendRegisterTransportBuilderHandler| in PresentationIPCService::StartSession (sender) and PresentationIPCService::NotifyReceiverReady (receiver) I'll make detailed comment > > > >@@ -842,35 +853,49 @@ PresentationPresentingInfo::InitTranspor > ... > > if (type == nsIPresentationChannelDescription::TYPE_DATACHANNEL) { > >- nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder> builder = > >- do_CreateInstance("@mozilla.org/presentation/datachanneltransportbuilder;1"); > >+ // in-process case. |mBuilder| in OOP case is set before. > >+ nsPIDOMWindow* window = nullptr; > >+ if (!mBuilder) { > >+ nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder> builder = > >+ do_CreateInstance("@mozilla.org/presentation/datachanneltransportbuilder;1"); > Hmm, so this is non-OOP case too. Needs better comment, but it is unclear to > me > why nsIPresentationDataChannelSessionTransportBuilder for non-OOP can be > created in two different places. > Confusing and duplicating the logic. > I'm not sure what you mean two places for non-OOP case. I move the creation logic to |if (!mBuilder)| block not dup another creation. > > nsCOMPtr<nsIPresentationService> mService; > >- nsTArray<nsString> mSessionIds; > >+ nsTArray<nsString> mHasListenerSessionIds; > >+ nsTArray<nsString> mHasHandlerSessionIds; > These two arrays sound a bit odd. And it isn't clear what they mean. > I mean "Handler" and "Listener" have so similar meaning that these need some > comments. > Also, PresentationParent::RecvRegisterSessionHandler seems to add something > to mHasListenerSessionIds > (the method is called *Handler, but array *Listener*) but > PresentationParent::RecvRegisterTransportBuilderHandler > adds to mHasHandlerSessionIds. > So I'd like some good documentation somewhere about the setup. I agree with you that the names are really ambiguous. Will make comment here. > I wish there was some ascii art somewhere in the code to explain the > presentation API implementation ;) > (or non-ascii in MDN) IMO presentation API really needs lots of implementation document since it's complexity. It's maintainable but the bar to contribute is a little high. However, the code structure is not review granted. I'd like to do some quick comment in the code and do the rest of document after it becomes more stable.

Make sender work. File bug 1258978 for some warning

Since lots of build-break for Part 5-1 since rebase, I'd like to merge these two patches. Haven't modify those issue address in Comment 102.

> I'm not sure what you mean two places for non-OOP case. I mean in PresentationControllingInfo::NotifyOpene... and PresentationPresentingInfo::InitTranspor... but now that I look at that, it is old code and I got lost with different Presentation*Info classes.

We are cooking a wiki now and some of the answer is moved to there https://wiki.mozilla.org/WebAPI/PresentationAPI:CoreService

Modify be comment 102; mainly rename and add comments. We plan to enrich necessary information to the wiki https://wiki.mozilla.org/WebAPI/PresentationAPI:CoreService TODO: make oop receiver work, test PPresentationControlChannel and make error handling solid

(In reply to Junior [:junior] from comment #108) > We are cooking a wiki now and some of the answer is moved to there > https://wiki.mozilla.org/WebAPI/PresentationAPI:CoreService That is great!

Might be nice to have a picture of a setup showing the case when there is controller and receiver (OOP or cross-device) and how data flows between them.

Comment on attachment 8735415 [details] [diff] [review] Part5: use data channel in substitution for TCP session transport (oop) WIP v2 >+class DCPresentationChannelDescription final : public nsIPresentationChannelDescription Perhaps worth to document that DC means DataChannel >+/** >+ * Generally transport is maintained by the chrome process. However, data >+ * channel should be live with the DOM , which implies RTCDataChannel in an OOP >+ * page should be establish in the content process. >+ * >+ * In OOP data channel transport case, |mBuilder| is hooked when the content >+ * process is ready to build a data channel transport through an IPC call >+ * |SendRegisterTransportBuilderHandler|, trigger by: >+ * 1. PresentationIPCService::StartSession (sender) is "sender" here same as "controller" >+ // The following three arrays track the keys of registered >+ // nsIPresentationSessionListener/TransportBuilder/RespondingListener >+ // respectively in the |PresentationService|. All the listener/builder will >+ // be unregistered if the actor is destroyed. >+ nsTArray<nsString> mHasSessionListenerSessionIds; >+ nsTArray<nsString> mHasTransportBuilderSessionIds; > nsTArray<uint64_t> mWindowIds; So why the last array is called mWindowIds when it is about "RespondingListener"? Could mWindowsIds be renamed to be more consistent with those other arrays?

rebase and carry r+

Rebase and move some nit from next patch to this. Carry r+

> >+/** > >+ * Generally transport is maintained by the chrome process. However, data > >+ * channel should be live with the DOM , which implies RTCDataChannel in an OOP > >+ * page should be establish in the content process. > >+ * > >+ * In OOP data channel transport case, |mBuilder| is hooked when the content > >+ * process is ready to build a data channel transport through an IPC call > >+ * |SendRegisterTransportBuilderHandler|, trigger by: > >+ * 1. PresentationIPCService::StartSession (sender) > is "sender" here same as "controller" Yes, I'll try to use consistent term like "controller" instead of "sender", "presenter" instead of "receiver"

Modify by reviewer's comment

leakcheck fail in comment 120, push a revised version in Comment 121

Seems fine in Comment 121, need a patch to turn pref off

Per offline discussion, we'll land these r+'ed patches and set pref off, thus needing this patch. The presenter will not try to establish a data channel session transport as the pref off. I guess we need a follow-up bug to let controller know its offer is rejected.

Comment on attachment 8739369 [details] [diff] [review] Part5: pref-off data channel transport Assuming this code is used only in the main thread, r+

Rebased and carry r+

Rebased and carry r+

Rebased and carry r+

Rebased and carry r+

Comment on attachment 8739879 [details] [diff] [review] Part3: implement session transport with DataChannel v6 Edit Description

Modify by reviewer's comment, carry r+

the mac build failed in the try run with: Error: /home/worker/workspace/build/src/browser/installer/package-manifest.in:585: Missing file(s): /Nightly.app/Contents/MacOS/components/PresentationDataChannelSessionTransport.js 04:02:30 INFO - Error: /home/worker/workspace/build/src/browser/installer/package-manifest.in:586: Missing file(s): /Nightly.app/Contents/MacOS/components/PresentationDataChannelSessionTransport.manifest 04:02:31 INFO - Traceback (most recent call last): 04:02:31 INFO - is this fixed ?

(In reply to Carsten Book [:Tomcat] from comment #135) > the mac build failed in the try run with: > > Error: > /home/worker/workspace/build/src/browser/installer/package-manifest.in:585: > Missing file(s): > /Nightly.app/Contents/MacOS/components/ > PresentationDataChannelSessionTransport.js > > 04:02:30 INFO - Error: > /home/worker/workspace/build/src/browser/installer/package-manifest.in:586: > Missing file(s): > /Nightly.app/Contents/MacOS/components/ > PresentationDataChannelSessionTransport.manifest > > 04:02:31 INFO - Traceback (most recent call last): > > 04:02:31 INFO - > > is this fixed ? It's depends on Bug 1059460 since the message have the same model from the title of this bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17dfb6404d37 https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe4cc7062d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/820f92f08f69 https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb658c3e3f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/4441e42e5bec

landed!

Backed out for OS X build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/6004772c0cf42aefff66ed2213bb07ec53fee761 https://hg.mozilla.org/integration/mozilla-inbound/rev/721b3076b9656e03904cd56acb619a45979f1d7a https://hg.mozilla.org/integration/mozilla-inbound/rev/699007f8060a82578d0f19afbe18093387e4c92a https://hg.mozilla.org/integration/mozilla-inbound/rev/51d3abb0441daebe5afd70422b465390eb94247a https://hg.mozilla.org/integration/mozilla-inbound/rev/8e36a279195ded147cfff012881513e566704f55 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=25506877&repo=mozilla-inbound 03:14:46 INFO - /builds/slave/m-in-m64-000000000000000000000/build/src/browser/installer/package-manifest.in ../../dist ../../dist/universal/firefox \ 03:14:52 INFO - Error: /builds/slave/m-in-m64-000000000000000000000/build/src/browser/installer/package-manifest.in:585: Missing file(s): /Nightly.app/Contents/MacOS/components/PresentationDataChannelSessionTransport.js 03:14:52 INFO - Error: /builds/slave/m-in-m64-000000000000000000000/build/src/browser/installer/package-manifest.in:586: Missing file(s): /Nightly.app/Contents/MacOS/components/PresentationDataChannelSessionTransport.manifest 03:15:17 INFO - Traceback (most recent call last): 03:15:17 INFO - File "/builds/slave/m-in-m64-000000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 414, in <module> 03:15:17 INFO - main() 03:15:17 INFO - File "/builds/slave/m-in-m64-000000000000000000000/build/src/toolkit/mozapps/installer/packager.py", line 366, in main 03:15:17 INFO - copier.add(mozpath.join(respath, 'removed-files'), removals) 03:15:17 INFO - File "/tools/python/lib/python2.7/contextlib.py", line 24, in __exit__ 03:15:17 INFO - self.gen.next() 03:15:17 INFO - File "/builds/slave/m-in-m64-000000000000000000000/build/src/python/mozbuild/mozpack/errors.py", line 131, in accumulate 03:15:17 INFO - raise AccumulatedErrors() 03:15:17 INFO - mozpack.errors.AccumulatedErrors 03:15:17 INFO - make[3]: *** [stage-package] Error 1 03:15:17 INFO - make[2]: *** [postflight_all] Error 2 03:15:17 INFO - make[1]: *** [realbuild] Error 2 03:15:17 INFO - make: *** [build] Error 2

Fix the bustage

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c014e781672 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee4c84bfbf69 https://hg.mozilla.org/integration/mozilla-inbound/rev/0db5c9b171c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a1ad6c114e https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7802ca6841

https://hg.mozilla.org/mozilla-central/rev/9c014e781672 https://hg.mozilla.org/mozilla-central/rev/ee4c84bfbf69 https://hg.mozilla.org/mozilla-central/rev/0db5c9b171c7 https://hg.mozilla.org/mozilla-central/rev/e0a1ad6c114e https://hg.mozilla.org/mozilla-central/rev/ce7802ca6841