Data channels currently rely on the window objects, which means that standalone code is not able to make use of the implementation in mozilla-central.

Beyond Alan's use case above, being able to run datachannels without a window would also allow us to expand the signaling_unittests to run the datachannel SDP handling through its paces. This is currently a pretty big hole in our unit testing.

Better would be to have a window (or fake one up) for unit tests. Also, unit tests are mostly useful for testing things that aren't visible or aren't tweakable in mochitests from JS. DataChannels is pretty exposed to JS unless you want to do packet-level SCTP tests (purposeful drops and protocol failures/violations).

(In reply to Randell Jesup [:jesup] from comment #2) > Better would be to have a window (or fake one up) for unit tests. Except that we also want to use this for non-unittest scenarios that have nothing to do with a browser. > Also, > unit tests are mostly useful for testing things that aren't visible or > aren't tweakable in mochitests from JS. I don't really agree with this philosophically. My objective is to have C++ unit tests for everything PC and below.

(In reply to Randell Jesup [:jesup] from comment #2) > DataChannels is pretty exposed to > JS unless you want to do packet-level SCTP tests (purposeful drops and > protocol failures/violations). To be clear, I'm not saying we need this in order to unit test datachannels (although that is also true) -- I want to be able to test sipcc's handling of datachannel-related SDP, and I'd like to be able to do it without loading up a full-blown browser.

So I did some hacking on the plane... This patch is pretty ugly, but it builds. I haven't tried running all the tests yet. This patch splits nsDOMDataChannel into a DataChannelBase class, and nsDOMDataChannel which does all the DOM stuff and has a DataChannelBase member that it defers to. There's a lot of redundant code in here, I'm not sure if that's really the best way to do things.

This passes the dom/media mochitests and the signalling C++ tests.

I made this a little less bad. Talking to ekr today, he suggested that we might alternately just #ifdef MOZILLA_INTERNAL_API the hell out of nsDOMDataChannel, which is a plausible alternative.

Comment on attachment 813747 [details] [diff] [review] hack nsDOMDataChannel Review of attachment 813747 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - Try build? ::: content/base/src/DataChannelBase.cpp @@ +7,5 @@ > +#include "DataChannelBase.h" > + > +#ifdef MOZ_LOGGING > +#define FORCE_PR_LOG > +#endif Not sure why the logging is set up this way... @@ +142,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +DataChannelBase::GetBinaryType(nsAString & aBinaryType) nit: fix spaces around & @@ +265,5 @@ > + "Unknown state in DataChannelBase::Send"); > + > + int32_t sent; > + if (aMsgStream) { > + sent = mDataChannel->SendBinaryStream(aMsgStream, aMsgLength); Please file a bug to remove msglength from SendBinaryStream() (and from DataChannelBase::Send()) ::: content/base/src/DataChannelBase.h @@ +42,5 @@ > + enum DataChannelBinaryType { > + DC_BINARY_TYPE_ARRAYBUFFER, > + DC_BINARY_TYPE_BLOB, > + }; > + DataChannelBinaryType DCBinaryType() const trivial nit: add blank line after enum def

Comment on attachment 813747 [details] [diff] [review] hack nsDOMDataChannel Review of attachment 813747 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/DataChannelBase.h @@ +12,5 @@ > +#include "nsIDOMDataChannel.h" > + > +class nsIDOMBlob; > +namespace mozilla { > + class ErrorResult; Should not be indented @@ +15,5 @@ > +namespace mozilla { > + class ErrorResult; > +} > + > +class DataChannelBase : public nsIDOMDataChannel New content code should be in the mozilla::dom namespace. @@ +21,5 @@ > +public: > + DataChannelBase(already_AddRefed<mozilla::DataChannel> aDataChannel, > + mozilla::DataChannelListener* aListener) > + : mDataChannel(aDataChannel), > + mBinaryType(DC_BINARY_TYPE_BLOB) { { on the next line @@ +24,5 @@ > + : mDataChannel(aDataChannel), > + mBinaryType(DC_BINARY_TYPE_BLOB) { > + MOZ_ASSERT(mDataChannel); > + if (aListener) > + mDataChannel->SetListener(aListener, nullptr); {} @@ +72,5 @@ > +protected: > + void Send(nsIInputStream* aMsgStream, const nsACString& aMsgString, > + uint32_t aMsgLength, bool aIsBinary, mozilla::ErrorResult& aRv); > + > + // Owning reference Clearly ::: content/base/src/moz.build @@ +36,5 @@ > ] > > if CONFIG['MOZ_WEBRTC']: > EXPORTS += [ > + 'DataChannelBase.h', Export as mozilla/dom/DataChannelBase.h ::: content/base/src/nsDOMDataChannel.h @@ +6,5 @@ > > #ifndef nsDOMDataChannel_h > #define nsDOMDataChannel_h > > +#include "DataChannelBase.h" mozilla/dom/ ::: media/mtransport/standalone/Makefile.in @@ +7,5 @@ > ifeq (WINNT,$(OS_TARGET)) > VISIBILITY_FLAGS = > endif > > +DEFINES = \ Move to moz.build, please. Why do we need those, though? Should we define those globally? ::: media/mtransport/standalone/moz.build @@ +16,5 @@ > > LIBRARY_NAME = 'mtransport_s' > > LOCAL_INCLUDES = [ > + '$(DEPTH)/dist/include/mozilla/net', Please don't do this. If you need something there, include it as mozilla/net/Foo.h ::: netwerk/sctp/datachannel/DataChannel.cpp @@ +78,5 @@ > > +#ifndef MOZILLA_INTERNAL_API > + > +inline nsresult > +NS_ReadInputStreamToString(nsIInputStream *aInputStream, *, & to the left @@ +83,5 @@ > + nsACString &aDest, > + uint32_t aCount) > +{ > + if (!aDest.SetLength(aCount)) > + return NS_ERROR_OUT_OF_MEMORY; {}, but check if this can happen @@ +85,5 @@ > +{ > + if (!aDest.SetLength(aCount)) > + return NS_ERROR_OUT_OF_MEMORY; > + void* dest = aDest.BeginWriting(); > + return NS_ReadInputStreamToBuffer(aInputStream, &dest, aCount); Two-space indentation

I know the patch here already has r+, but I had more plane time so I went back and tried out the #ifdef solution. I think it's more palatable in the end.

I missed a few #ifdefs in the PeerConnection code. I think I'd like to land this patch instead since it's less churn.

Comment on attachment 815530 [details] [diff] [review] ifdef nsDOMDataChannel to work with external linkage Review of attachment 815530 [details] [diff] [review]: ----------------------------------------------------------------- r+ (in that this patch should work and cleans up a few things), but I'll note that while this allows abr's wish to write datachannel signaling unittests in C++, I think it's of only marginal help in making the DataChannel implementation 'liftable' to create node.js DataChannel support and would not support ack's original request. So before checking this in, we need to decide if we want to go this route or another one. I'll say that anything which helps to enable people to build DataChannels into non-browsers is a win for the feature as a whole. ::: content/base/src/nsDOMDataChannel.cpp @@ +276,2 @@ > NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); > +#endif This is (to quote ekr) sad-making.... though it's merely a side-effect of the general problem of C++ unit tests not running their tests on mainthread. @@ +442,5 @@ > MOZ_ASSERT(NS_IsMainThread()); > return DoOnMessageAvailable(aMessage, false); > +#else > + return NS_OK; > +#endif This "works" as in compiles, but makes an nsDOMDataChannel object pretty pointless and untestable. ::: media/mtransport/standalone/moz.build @@ +32,5 @@ > '/media/mtransport/third_party/nrappkit/src/registry', > '/media/mtransport/third_party/nrappkit/src/share', > '/media/mtransport/third_party/nrappkit/src/stats', > '/media/mtransport/third_party/nrappkit/src/util/libekr', > + '/media/webrtc/trunk/third_party/libjingle/source/', Remove libjingle, it went away long ago

::: content/base/src/nsDOMDataChannel.cpp @@ +276,2 @@ > NS_ABORT_IF_FALSE(NS_IsMainThread(), "Not running on main thread"); > +#endif I'm currently having this problem; No clear workaround. I'm trying to refactor things so that I have a "main thread". Thoughts?

(In reply to Randell Jesup [:jesup] from comment #12) > Comment on attachment 815530 [details] [diff] [review] > ifdef nsDOMDataChannel to work with external linkage > > Review of attachment 815530 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ (in that this patch should work and cleans up a few things), but I'll > note that while this allows abr's wish to write datachannel signaling > unittests in C++, I think it's of only marginal help in making the > DataChannel implementation 'liftable' to create node.js DataChannel support > and would not support ack's original request. So before checking this in, > we need to decide if we want to go this route or another one. I'll say that > anything which helps to enable people to build DataChannels into > non-browsers is a win for the feature as a whole. Can you expound on this? This patch is pretty finely targeted at simply allowing the datachannel impl to be built without MOZILLA_INTERNAL_API. > This "works" as in compiles, but makes an nsDOMDataChannel object pretty > pointless and untestable. Right, I have a followup patch that adds a very simple observer API to nsIDOMDataChannel. I figured this patch was ugly enough without introducing that.

Ted -- Is it useful to land these patches for the standalone or elsewhere?

This was originally in support of node-webrtc, but ack wound up using non-Gecko code there, so I don't care about this anymore.